Bug 185814 - [Win][MiniBrowser] Add MainWindow class
Summary: [Win][MiniBrowser] Add MainWindow class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on: 185969
Blocks: 184770
  Show dependency treegraph
 
Reported: 2018-05-21 00:31 PDT by Fujii Hironori
Modified: 2018-05-27 20:50 PDT (History)
6 users (show)

See Also:


Attachments
Patch (21.82 KB, patch)
2018-05-21 01:01 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (28.28 KB, patch)
2018-05-23 19:28 PDT, Fujii Hironori
Hironori.Fujii: review-
Details | Formatted Diff | Diff
Patch (17.71 KB, patch)
2018-05-25 03:01 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (25.14 KB, patch)
2018-05-25 03:13 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (25.51 KB, patch)
2018-05-27 20:47 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2018-05-21 00:31:35 PDT
[Win][MiniBrowser] Add MainWindow class

This is a sub task of Bug 184770.
Comment 1 Fujii Hironori 2018-05-21 01:01:30 PDT
Created attachment 340833 [details]
Patch
Comment 2 Build Bot 2018-05-21 01:09:33 PDT
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.
Comment 3 Fujii Hironori 2018-05-21 22:17:32 PDT
Could anyone please review this?
Comment 4 Fujii Hironori 2018-05-22 18:27:44 PDT
Hi Per, do you have a time to review this patch?
Comment 5 Per Arne Vollan 2018-05-23 07:55:58 PDT
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.
Comment 6 Fujii Hironori 2018-05-23 18:23:30 PDT
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.
Comment 7 Fujii Hironori 2018-05-23 19:28:33 PDT
Created attachment 341159 [details]
Patch
Comment 8 Build Bot 2018-05-23 19:30:19 PDT
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.
Comment 9 Per Arne Vollan 2018-05-24 14:46:15 PDT
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.
Comment 10 Fujii Hironori 2018-05-24 17:46:06 PDT
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.
Comment 11 Fujii Hironori 2018-05-25 02:57:02 PDT
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?
Comment 12 Fujii Hironori 2018-05-25 03:01:58 PDT
Created attachment 341266 [details]
Patch
Comment 13 Fujii Hironori 2018-05-25 03:13:40 PDT
Created attachment 341268 [details]
Patch
Comment 14 Per Arne Vollan 2018-05-25 07:35:58 PDT
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.
Comment 15 Fujii Hironori 2018-05-25 16:59:48 PDT
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.
Comment 16 Fujii Hironori 2018-05-27 20:47:21 PDT
Created attachment 341442 [details]
Patch
Comment 17 Fujii Hironori 2018-05-27 20:49:35 PDT
Committed r232234: <https://trac.webkit.org/changeset/232234>
Comment 18 Radar WebKit Bug Importer 2018-05-27 20:50:22 PDT
<rdar://problem/40590683>