[webkit-reviews] review granted: [Bug 186815] [Wincairo] Add support for context menus to non-legacy minibrowser : [Attachment 343165] Add context menu functions and WM_MENUCOMMAND handler
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 25 17:24:28 PDT 2018
Ryosuke Niwa <rniwa at webkit.org> has granted Stephan Szabo
<stephan.szabo at sony.com>'s request for review:
Bug 186815: [Wincairo] Add support for context menus to non-legacy minibrowser
https://bugs.webkit.org/show_bug.cgi?id=186815
Attachment 343165: Add context menu functions and WM_MENUCOMMAND handler
https://bugs.webkit.org/attachment.cgi?id=343165&action=review
--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 343165
--> https://bugs.webkit.org/attachment.cgi?id=343165
Add context menu functions and WM_MENUCOMMAND handler
View in context: https://bugs.webkit.org/attachment.cgi?id=343165&action=review
> Source/WebKit/UIProcess/win/WebContextMenuProxyWin.cpp:86
> +void WebContextMenuProxyWin::populate(HMENU menu, const
Vector<WebContextMenuItemData>& items)
Looks like these can just be static local functions instead of a member.
> Source/WebKit/UIProcess/win/WebView.cpp:579
> + HMENU hMenu = reinterpret_cast<HMENU>(lParam);
> + unsigned index = static_cast<unsigned>(wParam);
Use auto on the variable declarations? There's no need to repeat the type
twice.
> Source/WebKit/UIProcess/win/WebView.cpp:581
> + MENUITEMINFO menuItemInfo = { 0 };
Hm... it's a bit odd to do this for Win32 structs.. We don't seem to do this
elsewhere.
> Source/WebKit/UIProcess/win/WebView.cpp:588
> + menuItemInfo.cch++;
Why don't we do this earlier instead of menuItemInfo.cch + 1 for Vector
allocation separately?
> Source/WebKit/UIProcess/win/WebView.cpp:591
> + ::GetMenuItemInfo(hMenu, index, true, &menuItemInfo);
I think we should use TRUE here to match the type properly.
Also, it's a bit strange that we're getting the title out of the menu item.
I may work better if we had a hash map of wID to titles in WebView itself.
It's not a big deal if we kept this code as is though.
> Source/WebKit/UIProcess/win/WebView.cpp:595
> + bool enabled = !(menuItemInfo.fState & MFS_GRAYED);
Please use MFS_DISABLED instead.
More information about the webkit-reviews
mailing list