[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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #89347|review?                     |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2011-04-13 09:14:27 PST ---
(From update of attachment 89347)
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?

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