Bug 185814 – [Win][MiniBrowser] Add MainWindow class

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fujii Hironori]

[Win][MiniBrowser] Add MainWindow class

This is a sub task of Bug 184770.

[Fujii Hironori]

Created attachment 340833 [details]
Patch

[EWS Watchlist]

Attachment 340833 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/MainWindow.cpp:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/MainWindow.cpp:27:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Fujii Hironori]

Could anyone please review this?

[Fujii Hironori]

Hi Per, do you have a time to review this patch?

[Per Arne Vollan]

Comment on attachment 340833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340833&action=review

> Tools/MiniBrowser/win/Common.cpp:74
> +MainWindow* gMainWindow = nullptr;

I think we can use std::unique_ptr<MainWindow> here.

> Tools/MiniBrowser/win/Common.cpp:95
> +void MainWindow::resizeSubViews()

Could this method be moved into MainWindow.cpp?

> Tools/MiniBrowser/win/Common.cpp:389
> +LRESULT CALLBACK MainWindow::WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)

Ditto.

> Tools/MiniBrowser/win/MainWindow.h:32
> +class MainWindow {

Perhaps we could consider renaming MainWindow to MiniBrowser, and renaming MiniBrowser to BrowserWindow? If it makes sense, this can be done in a follow-up patch.

> Tools/MiniBrowser/win/MainWindow.h:52
> +#define URLBAR_HEIGHT  24
> +#define CONTROLBUTTON_WIDTH 24

I think these defines could be moved into MainWindow.cpp.

[Fujii Hironori]

Comment on attachment 340833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340833&action=review

Thank you very much for the view.

>> Tools/MiniBrowser/win/Common.cpp:74
>> +MainWindow* gMainWindow = nullptr;
> 
> I think we can use std::unique_ptr<MainWindow> here.

I have a plan to support multiple windows properly.
I will bind the lifetime of MainWindow to its window by using GWLP_USERDATA.
And, the instance of MainWindow will be destructed on WM_CLOSE.

>> Tools/MiniBrowser/win/Common.cpp:95
>> +void MainWindow::resizeSubViews()
> 
> Could this method be moved into MainWindow.cpp?

Will do in this patch.

>> Tools/MiniBrowser/win/Common.cpp:389
>> +LRESULT CALLBACK MainWindow::WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> 
> Ditto.

Will do in this patch.

>> Tools/MiniBrowser/win/MainWindow.h:32
>> +class MainWindow {
> 
> Perhaps we could consider renaming MainWindow to MiniBrowser, and renaming MiniBrowser to BrowserWindow? If it makes sense, this can be done in a follow-up patch.

I will rename MiniBrowser to WK1BrowserWindow (bug 184770 comment 13).
IMHO, renaming MainWindow to MiniBrowser does not sound so good.
Because I have a plan to support multiple window, then multiple instances of MainWindow will be created by selecting menu items "Create WK1 Window" and "Create WK2 Window".
I think the class named 'MiniBrowser' gives the false impression of it's a singleton to people.

>> Tools/MiniBrowser/win/MainWindow.h:52
>> +#define CONTROLBUTTON_WIDTH 24
> 
> I think these defines could be moved into MainWindow.cpp.

You are right. But, I can't do it at the moment because it is used in Common.cpp yet.
I will do it in follow-up changes.

[Fujii Hironori]

Created attachment 341159 [details]
Patch

[EWS Watchlist]

Attachment 341159 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/MainWindow.cpp:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/MiniBrowser/win/MainWindow.cpp:27:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Comment on attachment 341159 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341159&action=review

> Tools/MiniBrowser/win/MainWindow.cpp:40
> +TCHAR szTitle[MAX_LOADSTRING]; // The title bar text
> +TCHAR szWindowClass[MAX_LOADSTRING]; // the main window class name

I think these can be data members of the MainWindow class.

> Tools/MiniBrowser/win/MainWindow.cpp:58
> +void PrintView(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam);
> +bool ToggleMenuItem(HWND hWnd, UINT menuID);

Does it make sense to move these functions into the MainWindow class?

> Tools/MiniBrowser/win/MainWindow.h:32
> +class MainWindow {

What do you think about renaming this class to MiniBrowserWindow?

> Tools/MiniBrowser/win/MainWindow.h:36
> +    void resizeSubViews();

It seems this method can be moved into the private section.

> Tools/MiniBrowser/win/MainWindow.h:38
> +    HWND hwnd() { return m_hMainWnd; }
> +    MiniBrowser* browserWindow() { return m_browserWindow.get(); }

Can the const modifier be added to these methods?

> Tools/MiniBrowser/win/MainWindow.h:41
> +    static LRESULT CALLBACK WndProc(HWND, UINT, WPARAM, LPARAM);
> +    static ATOM registerClass(HINSTANCE hInstance);

Should these methods also be moved into the private section? I think you can call registerClass from the init() method.

> Tools/MiniBrowser/win/MainWindow.h:47
> +    HWND m_hMainWnd;
> +    HWND m_hURLBarWnd;
> +    HWND m_hBackButtonWnd;
> +    HWND m_hForwardButtonWnd;

These members can be initialized here, I think.

[Fujii Hironori]

Comment on attachment 341159 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341159&action=review

>> Tools/MiniBrowser/win/MainWindow.cpp:40
>> +TCHAR szWindowClass[MAX_LOADSTRING]; // the main window class name
> 
> I think these can be data members of the MainWindow class.

Agreed. Will do.

>> Tools/MiniBrowser/win/MainWindow.cpp:58
>> +bool ToggleMenuItem(HWND hWnd, UINT menuID);
> 
> Does it make sense to move these functions into the MainWindow class?

I have a plan to make PrintView private in a follow-up patch.
PrintView is private in the original patch (Bug 184770).

>> Tools/MiniBrowser/win/MainWindow.h:32
>> +class MainWindow {
> 
> What do you think about renaming this class to MiniBrowserWindow?

Agreed. Will do.

>> Tools/MiniBrowser/win/MainWindow.h:36
>> +    void resizeSubViews();
> 
> It seems this method can be moved into the private section.

I have a plan to make resizeSubViews private in a follow-up patch.
resizeSubViews was private in the original patch (Bug 184770).

>> Tools/MiniBrowser/win/MainWindow.h:38
>> +    MiniBrowser* browserWindow() { return m_browserWindow.get(); }
> 
> Can the const modifier be added to these methods?

Agreed. Will do.

>> Tools/MiniBrowser/win/MainWindow.h:41
>> +    static ATOM registerClass(HINSTANCE hInstance);
> 
> Should these methods also be moved into the private section? I think you can call registerClass from the init() method.

Agreed. Will do.

>> Tools/MiniBrowser/win/MainWindow.h:47
>> +    HWND m_hForwardButtonWnd;
> 
> These members can be initialized here, I think.

Agreed. Will do.

[Fujii Hironori]

Comment on attachment 341159 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341159&action=review

>>> Tools/MiniBrowser/win/MainWindow.cpp:58
>>> +bool ToggleMenuItem(HWND hWnd, UINT menuID);
>> 
>> Does it make sense to move these functions into the MainWindow class?
> 
> I have a plan to make PrintView private in a follow-up patch.
> PrintView is private in the original patch (Bug 184770).

Sorry. I'm wrong.
I renamed PrintView to MiniBrowser::print because PrintView uses WK1 API. (Bug 184770)
ToggleMenuItem is also using WK1 API. I will rethink which file it should move to in follow-up patch.

>>> Tools/MiniBrowser/win/MainWindow.h:32
>>> +class MainWindow {
>> 
>> What do you think about renaming this class to MiniBrowserWindow?
> 
> Agreed. Will do.

I will rename MiniBrowser to WK1BrowserWindow and add BrowserWindow abstruct class. (Bug 184770)
WK1BrowserWindow, BrowserWindow and MiniBrowserWindow.
Doesn't it cause any confusion?

[Fujii Hironori]

Created attachment 341266 [details]
Patch

[Fujii Hironori]

Created attachment 341268 [details]
Patch

[Per Arne Vollan]

Comment on attachment 341268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341268&action=review

R=me.

> Tools/MiniBrowser/win/MainWindow.cpp:36
> +#define URLBAR_HEIGHT  24
> +#define CONTROLBUTTON_WIDTH 24

I think I would prefer to make these constexpr inside the resizeSubViews method.

> Tools/MiniBrowser/win/MainWindow.cpp:52
> +LRESULT CALLBACK EditProc(HWND, UINT, WPARAM, LPARAM);
> +LRESULT CALLBACK BackButtonProc(HWND, UINT, WPARAM, LPARAM);
> +LRESULT CALLBACK ForwardButtonProc(HWND, UINT, WPARAM, LPARAM);
> +LRESULT CALLBACK ReloadButtonProc(HWND, UINT, WPARAM, LPARAM);
> +INT_PTR CALLBACK About(HWND, UINT, WPARAM, LPARAM);
> +INT_PTR CALLBACK CustomUserAgent(HWND, UINT, WPARAM, LPARAM);
> +INT_PTR CALLBACK Caches(HWND, UINT, WPARAM, LPARAM);
> +INT_PTR CALLBACK AuthDialogProc(HWND, UINT, WPARAM, LPARAM);

Could these functions also be static members of the MainWindow class?

> Tools/MiniBrowser/win/MainWindow.cpp:62
> +    LoadString(hInst, IDC_MINIBROWSER, buff, length);

The second parameter of LoadString should be 'id', I believe.

[Fujii Hironori]

Comment on attachment 341268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341268&action=review

Thank you very much for r+, Per.

>> Tools/MiniBrowser/win/MainWindow.cpp:36
>> +#define CONTROLBUTTON_WIDTH 24
> 
> I think I would prefer to make these constexpr inside the resizeSubViews method.

Will do.

>> Tools/MiniBrowser/win/MainWindow.cpp:52
>> +INT_PTR CALLBACK AuthDialogProc(HWND, UINT, WPARAM, LPARAM);
> 
> Could these functions also be static members of the MainWindow class?

The main purpose of this bug is splitting the patch of Bug 184770. I will do it in follow-up patches.

>> Tools/MiniBrowser/win/MainWindow.cpp:62
>> +    LoadString(hInst, IDC_MINIBROWSER, buff, length);
> 
> The second parameter of LoadString should be 'id', I believe.

Good catch. Thanks. Will fix.

[Fujii Hironori]

Created attachment 341442 [details]
Patch

[Fujii Hironori]

Committed r232234: <https://trac.webkit.org/changeset/232234>

[Radar WebKit Bug Importer]

<rdar://problem/40590683>