It's currently unimplemented for the GTK port causing a crash when clicking on a menu list.
Created attachment 95583 [details] Patch
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.
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.
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.
(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!
Committed r87930: <http://trac.webkit.org/changeset/87930>