Bug 48720 – Customizable context menu support in WK2

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

After https://bugs.webkit.org/show_bug.cgi?id=48699 is resolved, customization from the embedding app will have to come via an injected bundle, and this bug tracks adding that mechanism.

This task in radar as <rdar://problem/8613727>

[Brady Eidson]

Created attachment 73391 [details]
Add an injected bundle client to modify the default menu.

[Darin Adler]

Comment on attachment 73391 [details]
Add an injected bundle client to modify the default menu.

View in context: https://bugs.webkit.org/attachment.cgi?id=73391&action=review

> WebCore/platform/ContextMenuItem.h:262
>          ContextMenuItem(ContextMenu* subMenu = 0);
>          ContextMenuItem(ContextMenuItemType type, ContextMenuAction action, const String& title, ContextMenu* subMenu = 0);
> +
> +        ContextMenuItem(ContextMenuItemType type, ContextMenuAction action, const String& title, bool enabled, bool checked);
> +        ContextMenuItem(ContextMenuAction action, const String& title, bool enabled, bool checked, Vector<ContextMenuItem>& subMenuItems);

The argument names type and action should be omitted.

The word “submenu” is a single word, so it should not be capitalized “subMenu”.

Since this function does not adopt the contents of the submenu items vector, it should be const Vector&, not Vector&. Similarly, the submenu argument to the old constructors should be a const ContextMenu*.

> WebCore/platform/ContextMenuItem.h:281
>          void setSubMenu(ContextMenu*);
> +        void setSubMenu(Vector<ContextMenuItem>&);

The word “submenu” is a single word, so it should not be capitalized “subMenu”.

Since this function does not adopt the contents of the submenu items vector, it should be const Vector&, not Vector&.

Similarly, the other function should take a const ContextMenu&, not a ContextMenu*.

> WebCore/platform/mac/ContextMenuItemMac.mm:82
> +    if (type == SeparatorType) {
> +        m_platformDescription = [NSMenuItem separatorItem];
> +        return;
> +    }

Should we do any assertions about action, title, enabled, and checked here?

> WebCore/platform/mac/ContextMenuItemMac.mm:93
> +ContextMenuItem::ContextMenuItem(ContextMenuAction action, const String& title, bool enabled, bool checked, Vector<ContextMenuItem>& subMenuItems)

This should share more code with the other constructor. I suggest using a helper function.

> WebCore/platform/mac/ContextMenuItemMac.mm:173
> +        [subMenu insertItem:subMenuItems[i].releasePlatformDescription() atIndex:i];

I suggest just using addItem: instead of insertItem:atIndex: here.

> WebCore/platform/mac/ContextMenuMac.mm:175
> +        PlatformMenuItemDescription platformItem = menuItemVector[i].releasePlatformDescription();
> +        [platformMenu addObject:platformItem];
> +        [platformItem release];

It seems like the release (it should be “leak”) in releasePlatformDescription is not doing you any good here. Unfortunate.

> WebCore/platform/mac/ContextMenuMac.mm:178
> +    return [platformMenu autorelease];

Isn’t PlatformMenuDescription a RetainPtr? If so, then you could just construct one and you need not autorelease.

> WebKit2/Shared/WebContextMenuItem.cpp:38
> +WebContextMenuItem::WebContextMenuItem(const WebContextMenuItemData& data)
> +    : m_webContextMenuItemData(data)
> +{
> +}
> +
> +WebContextMenuItemData* WebContextMenuItem::data()
> +{
> +    return &m_webContextMenuItemData;
> +}

These seems like they should be inline in the header. Defining them outside the class definition is OK to keep the class definition cleaner, but these can just have “inline” and be in the header.

> WebKit2/Shared/WebContextMenuItem.h:44
> +    WebContextMenuItemData* data();
> +private:

Missing blank line here.

> WebKit2/Shared/WebContextMenuItemData.cpp:85
> +        ContextMenuItem result(m_type, m_action, m_title, m_enabled, m_checked);
> +        return result;

Why not do this in a single line?

> WebKit2/Shared/WebContextMenuItemData.cpp:89
> +    Vector<ContextMenuItem> subMenuItems = coreItems(m_submenu);
> +    return ContextMenuItem(m_action, m_title, m_enabled, m_checked, subMenuItems);

Why not do this in a single line?

> WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.cpp:74
> +    RefPtr<ImmutableArray> array = adoptRef(toImpl(newMenuWK));

I suggest putting this line closer to the getContextMenuFromDefaultMenu line because we don’t want the adopt to ever get separated from the out argument.

> WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.cpp:78
> +    for (size_t i = 0; i < array->size(); ++i) {

The repeated calls to array->size() are kinda slow. You should change this to get it only once.

> WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.cpp:85
> +        newMenu.append(*(item->data()));

No need for the parentheses here. You should just leave them out.

> WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.h:34
> +    class HitTestResult;

No reason to declare this. It’s not used in this header.

> WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.h:39
> +class ImmutableArray;

No reason to declare this. It’s not used in this header.

> WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:44
> +    if (wkClient && wkClient->version)
> +        return;

Is this really right? If someone passes a newer version of WKBundlePageContextMenuClient we want to ignore it entirely? That doesn’t seem quite right to me. I guess all the other functions use that pattern. I’ll ask Sam about this.

> WebKit2/WebProcess/WebCoreSupport/WebContextMenuClient.cpp:51
> +    Vector<ContextMenuItem> coreItemVector = coreItems(newMenu);
> +    return platformMenuDescription(coreItemVector);

This should be done in one line.

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 73391 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73391&action=review
> 
> > WebCore/platform/ContextMenuItem.h:262
> >          ContextMenuItem(ContextMenu* subMenu = 0);
> >          ContextMenuItem(ContextMenuItemType type, ContextMenuAction action, const String& title, ContextMenu* subMenu = 0);
> > +
> > +        ContextMenuItem(ContextMenuItemType type, ContextMenuAction action, const String& title, bool enabled, bool checked);
> > +        ContextMenuItem(ContextMenuAction action, const String& title, bool enabled, bool checked, Vector<ContextMenuItem>& subMenuItems);
>  ...
> Since this function does not adopt the contents of the submenu items vector, it should be const Vector&, not Vector&. Similarly, the submenu argument to the old constructors should be a const ContextMenu*.

And...

> > WebCore/platform/ContextMenuItem.h:281
> >          void setSubMenu(ContextMenu*);
> > +        void setSubMenu(Vector<ContextMenuItem>&);
> ... 
> Since this function does not adopt the contents of the submenu items vector, it should be const Vector&, not Vector&.

Unfortunately, to get the platform item out of a ContextMenuItem is a non-const operation.  


> 
> Similarly, the other function should take a const ContextMenu&, not a ContextMenu*.
> 
> > WebCore/platform/mac/ContextMenuItemMac.mm:82
> > +    if (type == SeparatorType) {
> > +        m_platformDescription = [NSMenuItem separatorItem];
> > +        return;
> > +    }
> 
> Should we do any assertions about action, title, enabled, and checked here?

It's technically legal to have them here, even though they'll be lost on Mac.

> 
> > WebCore/platform/mac/ContextMenuItemMac.mm:93
> > +ContextMenuItem::ContextMenuItem(ContextMenuAction action, const String& title, bool enabled, bool checked, Vector<ContextMenuItem>& subMenuItems)
> 
> This should share more code with the other constructor. I suggest using a helper function.

Will do.

> 
> > WebCore/platform/mac/ContextMenuItemMac.mm:173
> > +        [subMenu insertItem:subMenuItems[i].releasePlatformDescription() atIndex:i];
> 
> I suggest just using addItem: instead of insertItem:atIndex: here.

Done

> > WebCore/platform/mac/ContextMenuMac.mm:175
> > +        PlatformMenuItemDescription platformItem = menuItemVector[i].releasePlatformDescription();
> > +        [platformMenu addObject:platformItem];
> > +        [platformItem release];
> 
> It seems like the release (it should be “leak”) in releasePlatformDescription is not doing you any good here. Unfortunate.

I agree.

> > WebCore/platform/mac/ContextMenuMac.mm:178
> > +    return [platformMenu autorelease];
> 
> Isn’t PlatformMenuDescription a RetainPtr? If so, then you could just construct one and you need not autorelease.

Nope:
typedef NSMutableArray* PlatformMenuDescription;

> 
> > WebKit2/Shared/WebContextMenuItem.cpp:38
> > +WebContextMenuItem::WebContextMenuItem(const WebContextMenuItemData& data)
> > +    : m_webContextMenuItemData(data)
> > +{
> > +}
> > +
> > +WebContextMenuItemData* WebContextMenuItem::data()
> > +{
> > +    return &m_webContextMenuItemData;
> > +}
> 
> These seems like they should be inline in the header. Defining them outside the class definition is OK to keep the class definition cleaner, but these can just have “inline” and be in the header.

I agree on data(), but have precogg'ed that the constructor will be more complex soon and I'll leave it in the .cpp file.
 
> > WebKit2/Shared/WebContextMenuItemData.cpp:85
> > +        ContextMenuItem result(m_type, m_action, m_title, m_enabled, m_checked);
> > +        return result;
> 
> Why not do this in a single line?

Because of the design of WebCore ContextMenuItems and their PlatformDescription nonsense, we have to work with non-const objects.

> > WebKit2/Shared/WebContextMenuItemData.cpp:89
> > +    Vector<ContextMenuItem> subMenuItems = coreItems(m_submenu);
> > +    return ContextMenuItem(m_action, m_title, m_enabled, m_checked, subMenuItems);
> 
> Why not do this in a single line?

Ditto

> > WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.cpp:74
> > +    RefPtr<ImmutableArray> array = adoptRef(toImpl(newMenuWK));
> 
> I suggest putting this line closer to the getContextMenuFromDefaultMenu line because we don’t want the adopt to ever get separated from the out argument.

Done

> > WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.cpp:78
> > +    for (size_t i = 0; i < array->size(); ++i) {
> 
> The repeated calls to array->size() are kinda slow. You should change this to get it only once.

Done
 
> > WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.cpp:85
> > +        newMenu.append(*(item->data()));
> 
> No need for the parentheses here. You should just leave them out.

Done

> > WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.h:34
> > +    class HitTestResult;
> 
> No reason to declare this. It’s not used in this header.
> 
> > WebKit2/WebProcess/InjectedBundle/InjectedBundlePageContextMenuClient.h:39
> > +class ImmutableArray;
> 
> No reason to declare this. It’s not used in this header.

Correct and correct.
 
> > WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:44
> > +    if (wkClient && wkClient->version)
> > +        return;
> 
> Is this really right? If someone passes a newer version of WKBundlePageContextMenuClient we want to ignore it entirely? That doesn’t seem quite right to me. I guess all the other functions use that pattern. I’ll ask Sam about this.

He's the best to answer :)

> > WebKit2/WebProcess/WebCoreSupport/WebContextMenuClient.cpp:51
> > +    Vector<ContextMenuItem> coreItemVector = coreItems(newMenu);
> > +    return platformMenuDescription(coreItemVector);
> 
> This should be done in one line.

See previous non-const answer  :(  

Thanks for the review!

[Brady Eidson]

http://trac.webkit.org/changeset/71682

[WebKit Review Bot]

http://trac.webkit.org/changeset/71682 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
fast/events/tabindex-focus-blur-all.html

[Brady Eidson]

Patch that adds a UIProcess client and changes the injected bundle client to give a user-data out param is forthcoming.

[Brady Eidson]

Created attachment 73800 [details]
Patch v1 (without ChangeLogs)

Patch.

I know there's no ChangeLogs here - I won't land without them.  Just trying to get the patch up before I head home for the night.

[Sam Weinig]

Comment on attachment 73800 [details]
Patch v1 (without ChangeLogs)

View in context: https://bugs.webkit.org/attachment.cgi?id=73800&action=review

> WebKit2/Shared/API/c/WKContextMenuItem.cpp:66
> +    ImmutableArray* array = toImpl(submenuItems);
> +    size_t size = array->size();
> +    
> +    Vector<WebContextMenuItemData> webSubmenu;
> +    webSubmenu.reserveCapacity(size);
> +    
> +    for (size_t i = 0; i < size; ++i) {
> +        WebContextMenuItem* item = array->at<WebContextMenuItem>(i);
> +        if (item)
> +            webSubmenu.append(*item->data());
> +    }
> +    
> +    return toAPI(WebContextMenuItem::create(WebContextMenuItemData(ContextMenuItemTagNoAction, toImpl(title)->string(), enabled, webSubmenu)).leakRef());

This should all be in WebContextMenuItem.h/cpp instead of in the API wrapper.

> WebKit2/Shared/API/c/WKContextMenuItem.cpp:72
> +    static WKContextMenuItemRef separatorItem = toAPI(WebContextMenuItem::create(WebContextMenuItemData(SeparatorType, ContextMenuItemTagNoAction, String(), true, false)).leakRef());
> +    return separatorItem;

This static should be too.

> WebKit2/Shared/API/c/WKContextMenuItem.cpp:117
> +    WebContextMenuItemData* data = toImpl(itemRef)->data();
> +    if (data->type() != SubmenuType)
> +        return 0;
> +    
> +    const Vector<WebContextMenuItemData>& submenuVector(data->submenu());
> +    unsigned size = submenuVector.size();
> +    
> +    RefPtr<MutableArray> submenu = MutableArray::create();
> +    submenu->reserveCapacity(size);
> +    
> +    for (unsigned i = 0; i < size; ++i) {
> +        RefPtr<WebContextMenuItem> item = WebContextMenuItem::create(submenuVector[i]);
> +        submenu->append(item.get());
> +    }
> +    
> +    return toAPI(submenu.release().leakRef());

Same here.

The WKArrayRef should probably be created by first generating a Vector<RefPtr<WebContextMenuItem> > and then using ImmutableArray::adopt().

> WebKit2/Shared/API/c/WKContextMenuItem.h:28
> +#ifndef WKContextMenuItemTags_h
> +#define WKContextMenuItemTags_h
> +

This header guard seems wrong.

> WebKit2/Shared/API/c/WKContextMenuItem.h:54
> +#endif /* WKContextMenuItemTags_h */

This one too.

> WebKit2/Shared/API/c/WKSharedAPICast.h:413
> +        return action;

This should probably have an explicit cast to make it clear what it is happening.

> WebKit2/UIProcess/WebAPIContextMenuClient.h:31
> +#include <wtf/Vector.h>

Can we avoid #including <wtf/Vector.h> here in favor of <wtf/Forward.h>

> WebKit2/UIProcess/WebAPIContextMenuClient.h:40
> +class WebAPIContextMenuClient : public APIClient<WKPageContextMenuClient> {
> +public:

I don't like the API in this class name, can we replace it with Page?

> WebKit2/UIProcess/WebPageProxy.cpp:1193
> +    RefPtr<APIObject> userData;
> +    WebContextUserMessageDecoder messageDecoder(userData, pageNamespace()->context());
> +    if (!arguments->decode(messageDecoder))
> +        return;
> +        

Probably best to do this as the first thing in this function.

> WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:28
> +#include "ImmutableDictionary.h"

Why is this change needed?

[Eric Seidel (no email)]

Comment on attachment 73391 [details]
Add an injected bundle client to modify the default menu.

Cleared Darin Adler's review+ from obsolete attachment 73391 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Brady Eidson]

(In reply to comment #8)
> (From update of attachment 73800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73800&action=review
> 
> > WebKit2/UIProcess/WebAPIContextMenuClient.h:31
> > +#include <wtf/Vector.h>
> 
> Can we avoid #including <wtf/Vector.h> here in favor of <wtf/Forward.h>

Sadly no.
 
> > WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:28
> > +#include "ImmutableDictionary.h"
> 
> Why is this change needed?

It's not.

New patch coming.

[Brady Eidson]

Created attachment 73912 [details]
v2 - Incorporated Sam's feedback

[Sam Weinig]

Comment on attachment 73912 [details]
v2 - Incorporated Sam's feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=73912&action=review

> WebKit2/WebKit2.xcodeproj/project.pbxproj:197
> +		5111CB02128E223B002203F7 /* untitled.h in Headers */ = {isa = PBXBuildFile; fileRef = 5111CB01128E223B002203F7 /* untitled.h */; };

What is this change about?

> WebKit2/WebKit2.xcodeproj/project.pbxproj:763
> +		5111CB01128E223B002203F7 /* untitled.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = untitled.h; sourceTree = "<group>"; };

What is this change about?

[Brady Eidson]

Landed in r72024

Can close this one now - will file new bugs for any more followup