[Webkit-unassigned] [Bug 54827] [GTK] Add context menu support for Webkit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54827





--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-04-14 02:03:34 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.