Bug 185814

Created attachment 340833 [details] Patch

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.

Could anyone please review this?

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

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 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.

Created attachment 341159 [details] Patch

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 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 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 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?

Created attachment 341266 [details] Patch

Created attachment 341268 [details] Patch

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 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.

Created attachment 341442 [details] Patch

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

<rdar://problem/40590683>