[webkit-reviews] review denied: [Bug 54827] [GTK] Add context menu support for Webkit2 : [Attachment 91660] Patch updated to current git master

[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 91660: Patch updated to current git master
https://bugs.webkit.org/attachment.cgi?id=91660&action=review

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