Bug 186815 - [Wincairo] Add support for context menus to non-legacy minibrowser
Summary: [Wincairo] Add support for context menus to non-legacy minibrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-19 13:40 PDT by Stephan Szabo
Modified: 2018-06-27 12:22 PDT (History)
10 users (show)

See Also:


Attachments
Add context menu functions and WM_MENUCOMMAND handler (9.29 KB, patch)
2018-06-20 11:07 PDT, Stephan Szabo
rniwa: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.73 MB, application/zip)
2018-06-20 21:29 PDT, Build Bot
no flags Details
Add context menu functions and WM_MENUCOMMAND handler - updated for review comments (9.15 KB, patch)
2018-06-26 13:59 PDT, Stephan Szabo
no flags Details | Formatted Diff | Diff
Add context menu functions and WM_MENUCOMMAND handler (10.52 KB, patch)
2018-06-27 10:38 PDT, Stephan Szabo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Szabo 2018-06-19 13:40:37 PDT
[Wincairo] Add support for context menus to non-legacy minibrowser
Comment 1 Stephan Szabo 2018-06-20 11:07:39 PDT
Created attachment 343165 [details]
Add context menu functions and WM_MENUCOMMAND handler
Comment 2 Build Bot 2018-06-20 21:28:57 PDT
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
Comment 3 Build Bot 2018-06-20 21:29:09 PDT
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
Comment 4 Ryosuke Niwa 2018-06-25 17:24:28 PDT
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.
Comment 5 Stephan Szabo 2018-06-26 13:59:15 PDT
Created attachment 343638 [details]
Add context menu functions and WM_MENUCOMMAND handler - updated for review comments
Comment 6 Stephan Szabo 2018-06-27 10:38:22 PDT
Created attachment 343724 [details]
Add context menu functions and WM_MENUCOMMAND handler

Added changelog, specified reviewer.
Comment 7 WebKit Commit Bot 2018-06-27 12:19:24 PDT
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>
Comment 8 WebKit Commit Bot 2018-06-27 12:19:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-06-27 12:22:03 PDT
<rdar://problem/41536813>