[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