[Webkit-unassigned] [Bug 61854] [GTK] Implement popup menus in Webkit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61854





--- Comment #3 from Martin Robinson <mrobinson at webkit.org>  2011-06-02 08:30:27 PST ---
(From update of attachment 95749)
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.

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