Bug 32277 – Chromium: support custom WebCore context menu items in Chromium port.

• Component: WebKit API
• Status: RESOLVED FIXED

[Pavel Feldman]

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.

[Pavel Feldman]

Created attachment 44476 [details]
[PATCH] Proposed change (upstream part)

[WebKit Review Bot]

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

[Pavel Feldman]

Created attachment 44477 [details]
[PATCH] Same with style bot happy.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44477 [details] without any errors.

[Eric Seidel (no email)]

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.

[Pavel Feldman]

(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 :(

[Darin Fisher (:fishd, Google)]

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.

[Pavel Feldman]

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.

[Pavel Feldman]

Created attachment 44486 [details]
[PATCH] Review comments addressed.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44486 [details] without any errors.

[Darin Fisher (:fishd, Google)]

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.

[Pavel Feldman]

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