[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 #92211|review?                     |review+
               Flag|                            |




--- Comment #10 from Martin Robinson <mrobinson at webkit.org>  2011-06-10 16:22:10 PST ---
(From update of attachment 92211)
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.

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