[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 #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-06-02 09:58:31 PST ---
(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!

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