[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





--- Comment #11 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-06-13 01:10:19 PST ---
(In reply to comment #10)
> (From update of attachment 92211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92211&action=review
> 
> Sorry for the very late review. This looks great!

No problem :-)

> 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.

I don't remember I'll look at it again.

> > Source/WebCore/platform/gtk/ContextMenuGtk.cpp:82
> > +    GList* children = gtk_container_get_children(GTK_CONTAINER(menu));
> 
> GOwnPtr here?

Yes.

> > Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp:126
> >      GtkAction* platformAction = 0;
> 
> I think you should use GRefPtr here.

Ok.

> > 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?

I'm not sure, this code is also in WebKit1 and it's part of the code that could be moved to WebCore in a following patch, so I'll look at it after landing.

> > 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.

Ok.

> > 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.

Ok.

> > 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.

Sure.

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