There is now a way to provide custom menu items into the page's context menu from within WebCore. I've added it for web inspector, but it is also in line with the contextmenu HTML5 proposal. I'd like to support it in Chromium port as well (currently we do not use context menu items in our context menu client at all and use renderer one's instead). This is an upstream part of the change. It goes through the menu items in ContextMenu and pushes information about these 'custom' ones into the client as a part of WebContextMenuData.
Created attachment 44476 [details] [PATCH] Proposed change (upstream part)
Attachment 44476 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/chromium/src/WebViewImpl.cpp:38: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1
Created attachment 44477 [details] [PATCH] Same with style bot happy.
style-queue ran check-webkit-style on attachment 44477 [details] without any errors.
It seems we need a better platform abstraction here. Something more akin to FloatPoint (which wraps a platform type). I'm not suggesting that you need to build such, but perhaps what you have built could be converted into something that could work for all ports in a second patch.
(In reply to comment #5) > It seems we need a better platform abstraction here. Something more akin to > FloatPoint (which wraps a platform type). I'm not suggesting that you need to > build such, but perhaps what you have built could be converted into something > that could work for all ports in a second patch. We surely do. ContextMenu(Item) is just a bunch of platform-dependent ifdefs with custom implementations. It'd be much cleaner if we used simple platform-agnostic containers under WebCore/page and used client interfaces to manage the specifics. It should not be that hard to implement: ContextMenu(Platform) needs to be moved to WebKit/(platform)/WebCoreSupport/ and should gain constructors taking pure ContextMenu as an input. Is that what you were thinking about? It would require building / testing on all platforms though :(
Comment on attachment 44477 [details] [PATCH] Same with style bot happy. > +++ b/WebKit/chromium/public/WebPopupMenuInfo.h ... > -// Describes the contents of a popup menu. > +// Describes the contents of a popup menu or custom context menu item provided from within WebCore. Perhaps this struct should be renamed to WebMenuInfo since it is no longer specific to popup menus. > struct WebPopupMenuInfo { > struct Item { > enum Type { > Option, > + CheckableOption, > Group, > Separator, > }; > WebString label; > Type type; > + unsigned action; Can you define an enumeration for action? It occurs to me that we already have WebMediaPlayerAction and performMediaPlayerAction, which could be incorporated into a more generic action enum and associated WebView method. (WebMediaPlayerAction has two fields, but those could be expressed as enum values instead.) > +++ b/WebKit/chromium/public/WebView.h ... > virtual void hideAutofillPopup() = 0; > > + // Context menu -------------------------------------------------------- > + > + virtual void executeCustomContextMenuAction(unsigned action) = 0; > > // Visited link state -------------------------------------------------- nit: WebView.h has two new lines above these delimiter comments > +++ b/WebKit/chromium/src/ContextMenuClientImpl.cpp ... > + // Filter out custom menu elements and add them into the data. > + Vector<WebPopupMenuInfo::Item> customItems; > + for (size_t i = 0; i < defaultMenu->itemCount(); ++i) { > + ContextMenuItem* inputItem = defaultMenu->itemAtIndex(i, defaultMenu->platformDescription()); > + if (inputItem->action() < ContextMenuItemBaseCustomTag || inputItem->action() >= ContextMenuItemBaseApplicationTag) > + continue; > + > + WebPopupMenuInfo::Item outputItem; > + outputItem.label = inputItem->title(); > + outputItem.enabled = inputItem->enabled(); > + outputItem.checked = inputItem->checked(); > + outputItem.action = static_cast<unsigned>(inputItem->action() - ContextMenuItemBaseCustomTag); > + switch (inputItem->type()) { > + case ActionType: > + outputItem.type = WebPopupMenuInfo::Item::Option; > + break; > + case CheckableActionType: > + outputItem.type = WebPopupMenuInfo::Item::CheckableOption; > + break; > + case SeparatorType: > + outputItem.type = WebPopupMenuInfo::Item::Separator; > + break; > + } > + customItems.append(outputItem); > + } > + > + WebVector<WebPopupMenuInfo::Item> outputItems(customItems.size()); > + for (size_t i = 0; i < customItems.size(); ++i) > + outputItems[i] = customItems[i]; > + data.customItems.swap(outputItems); nit: Please break this code out into a new function. The existing function is already way too long.
Thanks for the review! > Perhaps this struct should be renamed to WebMenuInfo since it > is no longer specific to popup menus. WebPopupMenuInfo is still used for popup only, it is just its ::Item that is now reused among popup and context menu. I'd better extract it to top-level class then? > > struct WebPopupMenuInfo { > > struct Item { > > enum Type { > > Option, > > + CheckableOption, > > Group, > > Separator, > > }; > > WebString label; > > Type type; > > + unsigned action; > > Can you define an enumeration for action? It occurs to me that we > already have WebMediaPlayerAction and performMediaPlayerAction, which > could be incorporated into a more generic action enum and associated > WebView method. The idea here is that these are custom actions defined by the ContextMenu clients. They could be dynamic or derived from the <menu> tag in HTML5 in the future. So the idea is that client passes action id to the framework and expects to receive it in the fire event. > > nit: WebView.h has two new lines above these delimiter comments > Fixing. > > nit: Please break this code out into a new function. The existing > function is already way too long. Fixing.
Created attachment 44486 [details] [PATCH] Review comments addressed.
style-queue ran check-webkit-style on attachment 44486 [details] without any errors.
Comment on attachment 44486 [details] [PATCH] Review comments addressed. OK, r=me I'm still unconvinced that we shouldn't merge WebMediaPlayerAction into this system. One nit: please use the term "perform" instead of "execute" for consistency with the performMediaPlayerAction method.
Committing to http://svn.webkit.org/repository/webkit/trunk ... C WebKit/chromium/public/WebPopupMenuInfo.h => WebKit/chromium/public/WebMenuItemInfo.h M WebCore/ChangeLog M WebCore/platform/ContextMenu.h M WebCore/platform/ContextMenuItem.h M WebCore/platform/chromium/ContextMenuChromium.cpp M WebCore/platform/chromium/ContextMenuItemChromium.cpp M WebKit/chromium/ChangeLog M WebKit/chromium/WebKit.gyp M WebKit/chromium/public/WebContextMenuData.h M WebKit/chromium/public/WebPopupMenuInfo.h M WebKit/chromium/public/WebView.h M WebKit/chromium/src/ContextMenuClientImpl.cpp M WebKit/chromium/src/ContextMenuClientImpl.h M WebKit/chromium/src/WebViewImpl.cpp M WebKit/chromium/src/WebViewImpl.h Committed r51874