[webkit-reviews] review denied: [Bug 185814] [Win][MiniBrowser] Add MainWindow class : [Attachment 341159] Patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 185814: [Win][MiniBrowser] Add MainWindow class
https://bugs.webkit.org/show_bug.cgi?id=185814

Attachment 341159: Patch

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




--- Comment #10 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 341159
  --> https://bugs.webkit.org/attachment.cgi?id=341159
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.