[webkit-reviews] review granted: [Bug 54827] [GTK] Add context menu support for Webkit2 : [Attachment 92211] New patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted 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 92211: New patch
https://bugs.webkit.org/attachment.cgi?id=92211&action=review

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