| Summary: | [Wincairo] Add support for context menus to non-legacy minibrowser | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Stephan Szabo <stephan.szabo> | ||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | achristensen, Basuke.Suzuki, bfulgham, commit-queue, don.olmstead, ews-watchlist, lforschler, pvollan, rniwa, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Stephan Szabo
2018-06-19 13:40:37 PDT
Created attachment 343165 [details]
Add context menu functions and WM_MENUCOMMAND handler
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 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 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. Created attachment 343638 [details]
Add context menu functions and WM_MENUCOMMAND handler - updated for review comments
Created attachment 343724 [details]
Add context menu functions and WM_MENUCOMMAND handler
Added changelog, specified reviewer.
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> All reviewed patches have been landed. Closing bug. |