Bug 61854 – [GTK] Implement popup menus in Webkit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It's currently unimplemented for the GTK port causing a crash when clicking on a menu list.

[Carlos Garcia Campos]

Created attachment 95583 [details]
Patch

[Carlos Garcia Campos]

Created attachment 95749 [details]
New patch

Martin suggested that we could try to reuse the code from WebCore instead of copying it, so I have moved the common parts into a new class in Webcore that is used by both WebKit1 and WebKit2.

[Martin Robinson]

Comment on attachment 95749 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95749&action=review

Nice work! This is just blocked on my understanding of the inner main loop, but in general, very nice patch.

> Source/WebCore/platform/gtk/GtkPopupMenu.cpp:78
> +    // The size calls are directly copied from gtkcombobox.c which is LGPL.

These few lines do not necessitate a licensing issue, in my opinion, so it's probably best to change this comment to something like "This approach follows the one in gtkcombobox.c." If you believe the code segment is the kind that requires relying on the original files license, we are legally obligated to include the original author's copyright in the header.

> Source/WebCore/platform/gtk/GtkPopupMenu.cpp:108
> +        // Center vertically the empty popup in the combo box area

This comment should have a period at the end.

> Source/WebCore/platform/gtk/PopupMenuGtk.cpp:73
> +            GOwnPtr<char> actionName(g_strdup_printf("popup-menu-action-%d", i));
> +            GRefPtr<GtkAction> action = adoptGRef(gtk_action_new(actionName.get(), client()->itemText(i).utf8().data(), 0, 0));
> +            g_object_set_data(G_OBJECT(action.get()), "popup-menu-action-index", GINT_TO_POINTER(i));
> +            g_signal_connect(action.get(), "activate", G_CALLBACK(menuItemActivated), this);
> +            // FIXME: Apply the PopupMenuStyle from client()->itemStyle(i)
> +            gtk_action_set_sensitive(action.get(), client()->itemIsEnabled(i));

I think that this should be spun off into a helper function. createGtkActionFromMenuItem or something like that.

> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:71
> +            GOwnPtr<char> actionName(g_strdup_printf("popup-menu-action-%d", i));
> +            GRefPtr<GtkAction> action = adoptGRef(gtk_action_new(actionName.get(), items[i].m_text.utf8().data(), items[i].m_toolTip.utf8().data(), 0));
> +            g_object_set_data(G_OBJECT(action.get()), "popup-menu-action-index", GINT_TO_POINTER(i));
> +            g_signal_connect(action.get(), "activate", G_CALLBACK(menuItemActivated), this);
> +            gtk_action_set_sensitive(action.get(), items[i].m_isEnabled);

I think I'd like to see this split off into a helper function.

> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:95
> +    GDK_THREADS_LEAVE();
> +    g_main_loop_run(m_runLoop);
> +    GDK_THREADS_ENTER();
> +
> +    g_main_loop_unref(m_runLoop);
> +    m_runLoop = 0;
> +
> +    g_signal_handler_disconnect(m_popup->platformMenu(), unmapHandler);
> +

I'm not sure I totally understand why the main loop must run here. Perhaps it could have a comment explaining the issue.

[Martin Robinson]

Comment on attachment 95749 [details]
New patch

Carlos explained to me via jabber why there must be an inner main loop. r=me with the previously mentioned changes and a comment explaining the inner main loop.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 95749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95749&action=review
> 
> Nice work! This is just blocked on my understanding of the inner main loop, but in general, very nice patch.
> 
> > Source/WebCore/platform/gtk/GtkPopupMenu.cpp:78
> > +    // The size calls are directly copied from gtkcombobox.c which is LGPL.
> 
> These few lines do not necessitate a licensing issue, in my opinion, so it's probably best to change this comment to something like "This approach follows the one in gtkcombobox.c." If you believe the code segment is the kind that requires relying on the original files license, we are legally obligated to include the original author's copyright in the header.

that's copy-pasted from existing PopupMenuGtk.cpp, I'll change the comment anyway.

> > Source/WebCore/platform/gtk/GtkPopupMenu.cpp:108
> > +        // Center vertically the empty popup in the combo box area
> 
> This comment should have a period at the end.

Ditto.

> > Source/WebCore/platform/gtk/PopupMenuGtk.cpp:73
> > +            GOwnPtr<char> actionName(g_strdup_printf("popup-menu-action-%d", i));
> > +            GRefPtr<GtkAction> action = adoptGRef(gtk_action_new(actionName.get(), client()->itemText(i).utf8().data(), 0, 0));
> > +            g_object_set_data(G_OBJECT(action.get()), "popup-menu-action-index", GINT_TO_POINTER(i));
> > +            g_signal_connect(action.get(), "activate", G_CALLBACK(menuItemActivated), this);
> > +            // FIXME: Apply the PopupMenuStyle from client()->itemStyle(i)
> > +            gtk_action_set_sensitive(action.get(), client()->itemIsEnabled(i));
> 
> I think that this should be spun off into a helper function. createGtkActionFromMenuItem or something like that.

Ok.

> > Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:71
> > +            GOwnPtr<char> actionName(g_strdup_printf("popup-menu-action-%d", i));
> > +            GRefPtr<GtkAction> action = adoptGRef(gtk_action_new(actionName.get(), items[i].m_text.utf8().data(), items[i].m_toolTip.utf8().data(), 0));
> > +            g_object_set_data(G_OBJECT(action.get()), "popup-menu-action-index", GINT_TO_POINTER(i));
> > +            g_signal_connect(action.get(), "activate", G_CALLBACK(menuItemActivated), this);
> > +            gtk_action_set_sensitive(action.get(), items[i].m_isEnabled);
> 
> I think I'd like to see this split off into a helper function.

Ok.

> > Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:95
> > +    GDK_THREADS_LEAVE();
> > +    g_main_loop_run(m_runLoop);
> > +    GDK_THREADS_ENTER();
> > +
> > +    g_main_loop_unref(m_runLoop);
> > +    m_runLoop = 0;
> > +
> > +    g_signal_handler_disconnect(m_popup->platformMenu(), unmapHandler);
> > +
> 
> I'm not sure I totally understand why the main loop must run here. Perhaps it could have a comment explaining the issue.

Sure. Thanks for the review!

[Carlos Garcia Campos]

Committed r87930: <http://trac.webkit.org/changeset/87930>