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

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Stephan Szabo]

[Wincairo] Add support for context menus to non-legacy minibrowser

[Stephan Szabo]

Created attachment 343165 [details]
Add context menu functions and WM_MENUCOMMAND handler

[EWS Watchlist]

Comment on attachment 343165 [details]
Add context menu functions and WM_MENUCOMMAND handler

Attachment 343165 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8273103

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 343212 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ryosuke Niwa]

Comment on attachment 343165 [details]
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.

[Stephan Szabo]

Created attachment 343638 [details]
Add context menu functions and WM_MENUCOMMAND handler - updated for review comments

[Stephan Szabo]

Created attachment 343724 [details]
Add context menu functions and WM_MENUCOMMAND handler

Added changelog, specified reviewer.

[WebKit Commit Bot]

Comment on attachment 343724 [details]
Add context menu functions and WM_MENUCOMMAND handler

Clearing flags on attachment: 343724

Committed r233271: <https://trac.webkit.org/changeset/233271>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41536813>