[webkit-reviews] review denied: [Bug 54827] [GTK] Add context menu support for Webkit2 : [Attachment 89347] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 54827: [GTK] Add context menu support for Webkit2
https://bugs.webkit.org/show_bug.cgi?id=54827

Attachment 89347: Patch
https://bugs.webkit.org/attachment.cgi?id=89347&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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?