Bug 54827 – [GTK] Add context menu support for Webkit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alejandro G. Castro]

We still need to add the context menu support for Webkit2.

[Carlos Garcia Campos]

Created attachment 89347 [details]
Patch

This patch adds support for context menus. Not all options exposed by the context menu work in this moment, since they have to be implemented in minibrowser and WebKitWebView. This patch depends on bug #57944.

[Martin Robinson]

Comment on attachment 89347 [details]
Patch

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

Great work. I have a few minor quibbles, but very minor.

> Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:208
> +    return action ? String::fromUTF8(gtk_action_get_label(action)) : String();

Nice.

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:37
> +#define WEBKIT_CONTEXT_MENU_ACTION "webkit-context-menu"

Please use a static const instead of a define and use WebKit naming style.

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:116
> +    case WebCore::ContextMenuItemTagCopyImageUrlToClipboard:

This shouldn't be GTK_STOCK_COPY?

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:154
> +            GtkAction* action = 0;
> +            GOwnPtr<char> actionName(g_strdup_printf("context-menu-action-%d", item.action()));
> +
> +            if (item.type() == WebCore::CheckableActionType) {
> +                action = GTK_ACTION(gtk_toggle_action_new(actionName.get(), item.title().utf8().data(), 0, gtkStockIDFromContextMenuAction(item.action())));
> +                gtk_toggle_action_set_active(GTK_TOGGLE_ACTION(action), item.checked());
> +            } else
> +                action = gtk_action_new(actionName.get(), item.title().utf8().data(), 0, gtkStockIDFromContextMenuAction(item.action()));
> +
> +            gtk_action_set_sensitive(action, item.enabled());

Would you consider splitting this out into a helper? Something like getActionForWebContextMenuItem. You could use early returns and it would make this for loop easier to read.

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:160
> +            g_object_unref(action);

Perhaps a GRefPtr?

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:179
> +    GtkMenu* menu = GTK_MENU(gtk_menu_new());
> +    populateMenu(menu, items);

I think it would simplify things to change populateMenu to createGtkMenu and have it responsible for creating the GtkMenu itself. You could change this to:

GtkMenu* menu = createGtkMenu(items);

and simplify the submenu case above.

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:180
> +    webkitWebViewBaseShowContextMenu(WEBKIT_WEB_VIEW_BASE(m_webView), menu, position);

I'd much rather have the logic for showing the menu be in this class? Do you have a particular reason for having it in the WebView GObject?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 89347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89347&action=review
> 
> Great work. I have a few minor quibbles, but very minor.
> 
> > Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:208
> > +    return action ? String::fromUTF8(gtk_action_get_label(action)) : String();
> 
> Nice.
> 
> > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:37
> > +#define WEBKIT_CONTEXT_MENU_ACTION "webkit-context-menu"
> 
> Please use a static const instead of a define and use WebKit naming style.

Ok, this was copied from Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp, do you want me to change that one too? or do I file another bug report for that?

> > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:116
> > +    case WebCore::ContextMenuItemTagCopyImageUrlToClipboard:
> 
> This shouldn't be GTK_STOCK_COPY?

No idea, it was copied from the same file. 

> > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:154
> > +            GtkAction* action = 0;
> > +            GOwnPtr<char> actionName(g_strdup_printf("context-menu-action-%d", item.action()));
> > +
> > +            if (item.type() == WebCore::CheckableActionType) {
> > +                action = GTK_ACTION(gtk_toggle_action_new(actionName.get(), item.title().utf8().data(), 0, gtkStockIDFromContextMenuAction(item.action())));
> > +                gtk_toggle_action_set_active(GTK_TOGGLE_ACTION(action), item.checked());
> > +            } else
> > +                action = gtk_action_new(actionName.get(), item.title().utf8().data(), 0, gtkStockIDFromContextMenuAction(item.action()));
> > +
> > +            gtk_action_set_sensitive(action, item.enabled());
> 
> Would you consider splitting this out into a helper? Something like getActionForWebContextMenuItem. You could use early returns and it would make this for loop easier to read.

Ok.

> > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:160
> > +            g_object_unref(action);
> 
> Perhaps a GRefPtr?

Yes.

> > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:179
> > +    GtkMenu* menu = GTK_MENU(gtk_menu_new());
> > +    populateMenu(menu, items);
> 
> I think it would simplify things to change populateMenu to createGtkMenu and have it responsible for creating the GtkMenu itself. You could change this to:
> 
> GtkMenu* menu = createGtkMenu(items);
> 
> and simplify the submenu case above.

hmm, I'll try that way.

> > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:180
> > +    webkitWebViewBaseShowContextMenu(WEBKIT_WEB_VIEW_BASE(m_webView), menu, position);
> 
> I'd much rather have the logic for showing the menu be in this class? Do you have a particular reason for having it in the WebView GObject?

Yes, we don't implement GtkWidget::popup-menu signal yet, but we could use the same code like we currently do in webkit1.

[Carlos Garcia Campos]

Created attachment 89762 [details]
Updated pach according to review

[Martin Robinson]

Comment on attachment 89762 [details]
Updated pach according to review

Looks okay, but going to remove the review flag until we resolve the dependency as changes there will most certainly make this one obsolete.

[Carlos Garcia Campos]

Created attachment 91660 [details]
Patch updated to current git master

[WebKit Review Bot]

Attachment 91660 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:347:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 91660 [details]
Patch updated to current git master

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

Very nice change. We should no longer duplicate code between WebCore/WebKit1 and WebKit2 though. Do you mind finding a way to avoid it. One way is to build a helper class which is used by both WebCore for WebKit1 and WebKit2.

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:175
> +        const WebContextMenuItemData& item = items.at(i);
> +        GtkWidget* menuItem;
> +
> +        if (item.type() == WebCore::SeparatorType)
> +            menuItem = gtk_separator_menu_item_new();
> +        else {
> +            GRefPtr<GtkAction> action = getActionForWebContextMenuItem(item);
> +            g_signal_connect(action.get(), "activate", G_CALLBACK(contextMenuItemActivatedCallback), m_page);
> +
> +            menuItem = gtk_action_create_menu_item(action.get());
> +
> +            if (item.type() == WebCore::SubmenuType) {
> +                GtkMenu* subMenu = createGtkMenu(item.submenu());
> +                gtk_menu_item_set_submenu(GTK_MENU_ITEM(menuItem), GTK_WIDGET(subMenu));
> +            }
> +        }
> +
> +        gtk_menu_shell_append(GTK_MENU_SHELL(menu), menuItem);

This could should probably be a helper function like convertContextMenuItemDataToGtkMenuItem. That would allow early returns and make the code a little cleaner, I think.

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:187
> +    GtkMenu* menu = createGtkMenu(items);
> +    webkitWebViewBaseShowContextMenu(WEBKIT_WEB_VIEW_BASE(m_webView), menu, position);

No need to cache the menu as a local variable here.

[Carlos Garcia Campos]

Created attachment 92211 [details]
New patch

We don't actually need any new class, we can use the existing ContextMenu and ContextMenuItem WebCore classes. I've implemented the missing methods in both classes and used from webkit2 code.

[Martin Robinson]

Comment on attachment 92211 [details]
New patch

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

Sorry for the very late review. This looks great!

The only major change I'd like to see before landing is that all the code in Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp move to the WebContextMenuProxyGtk including having the WebContextMenuProxyGtk store the lastPopupPosition.

If this isn't possible or doesn't make sense for some other reason, let's talk about it when we're both around.

> Source/WebCore/platform/gtk/ContextMenuGtk.cpp:82
> +    GList* children = gtk_container_get_children(GTK_CONTAINER(menu));

GOwnPtr here?

> Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:126
>      GtkAction* platformAction = 0;

I think you should use GRefPtr here.

> Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:129
>          platformAction = GTK_ACTION(gtk_toggle_action_new(actionName.get(), title.utf8().data(), 0, gtkStockIDFromContextMenuAction(action)));

You'll need adoptPtr here.

> Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:132
>          platformAction = gtk_action_new(actionName.get(), title.utf8().data(), 0, gtkStockIDFromContextMenuAction(action));

And here as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:335
> +static IntPoint globalPointForClientPoint(GdkWindow* window, const IntPoint& clientPoint)
> +{
> +    int x, y;
> +    gdk_window_get_origin(window, &x, &y);
> +    return clientPoint + IntSize(x, y);
> +}

Could you remove this and simply use convertWidgetRectToScreenRect from WebCore/platform/GtkUtilities.h?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:340
> +static void popupMenuPositionFunction(GtkMenu* menu, gint* x, gint* y, gboolean* pushIn, gpointer userData)
> +{
> +    WebKitWebViewBase* view = WEBKIT_WEB_VIEW_BASE(userData);
> +    WebKitWebViewBasePrivate* priv = view->priv;

I wish we could share more of this with WebKit1. :(

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:364
> +    priv->lastPopupPosition = globalPointForClientPoint(gtk_widget_get_window(GTK_WIDGET(webViewBase)), position);

I'd say just use webViewBase->priv->lastPopupPosition here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:366
> +    gtk_menu_popup(menu, 0, 0, &popupMenuPositionFunction, webViewBase, 3, gtk_get_current_event_time());

3 is a magic number, so you should drop a quick line of documentation explaining it.

> Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39
> +static const char* contextMenuActionId = "webkit-context-menu-action";

Mind prefixing this with g like gContextMenuActionId? I've been doing that lately for globals.

[Carlos Garcia Campos]

(In reply to comment #10)
> (From update of attachment 92211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92211&action=review
> 
> Sorry for the very late review. This looks great!

No problem :-)

> The only major change I'd like to see before landing is that all the code in Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp move to the WebContextMenuProxyGtk including having the WebContextMenuProxyGtk store the lastPopupPosition.
> 
> If this isn't possible or doesn't make sense for some other reason, let's talk about it when we're both around.

I don't remember I'll look at it again.

> > Source/WebCore/platform/gtk/ContextMenuGtk.cpp:82
> > +    GList* children = gtk_container_get_children(GTK_CONTAINER(menu));
> 
> GOwnPtr here?

Yes.

> > Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:126
> >      GtkAction* platformAction = 0;
> 
> I think you should use GRefPtr here.

Ok.

> > Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:129
> >          platformAction = GTK_ACTION(gtk_toggle_action_new(actionName.get(), title.utf8().data(), 0, gtkStockIDFromContextMenuAction(action)));
> 
> You'll need adoptPtr here.
> 
> > Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:132
> >          platformAction = gtk_action_new(actionName.get(), title.utf8().data(), 0, gtkStockIDFromContextMenuAction(action));
> 
> And here as well.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:335
> > +static IntPoint globalPointForClientPoint(GdkWindow* window, const IntPoint& clientPoint)
> > +{
> > +    int x, y;
> > +    gdk_window_get_origin(window, &x, &y);
> > +    return clientPoint + IntSize(x, y);
> > +}
> 
> Could you remove this and simply use convertWidgetRectToScreenRect from WebCore/platform/GtkUtilities.h?

I'm not sure, this code is also in WebKit1 and it's part of the code that could be moved to WebCore in a following patch, so I'll look at it after landing.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:340
> > +static void popupMenuPositionFunction(GtkMenu* menu, gint* x, gint* y, gboolean* pushIn, gpointer userData)
> > +{
> > +    WebKitWebViewBase* view = WEBKIT_WEB_VIEW_BASE(userData);
> > +    WebKitWebViewBasePrivate* priv = view->priv;
> 
> I wish we could share more of this with WebKit1. :(
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:364
> > +    priv->lastPopupPosition = globalPointForClientPoint(gtk_widget_get_window(GTK_WIDGET(webViewBase)), position);
> 
> I'd say just use webViewBase->priv->lastPopupPosition here.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:366
> > +    gtk_menu_popup(menu, 0, 0, &popupMenuPositionFunction, webViewBase, 3, gtk_get_current_event_time());
> 
> 3 is a magic number, so you should drop a quick line of documentation explaining it.

Ok.

> > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39
> > +static const char* contextMenuActionId = "webkit-context-menu-action";
> 
> Mind prefixing this with g like gContextMenuActionId? I've been doing that lately for globals.

Sure.

[Carlos Garcia Campos]

Committed r88634: <http://trac.webkit.org/changeset/88634>