[Webkit-unassigned] [Bug 186815] [Wincairo] Add support for context menus to non-legacy minibrowser

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=186815

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #343165|review?                     |review+
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180626/b8ccac5a/attachment.html>