We can simplify the context-menu handling code by using a GtkMenuItem as PlatformMenuItemDescription instead of the custom struct we are currently using.
Created attachment 74103 [details] Patch to simplify context-menu handling in gtk port
Comment on attachment 74103 [details] Patch to simplify context-menu handling in gtk port View in context: https://bugs.webkit.org/attachment.cgi?id=74103&action=review Great patch! I have two small tweaks to suggest. > WebCore/platform/ContextMenuItem.h:188 > + typedef GtkMenuItem* PlatformMenuItemDescription; I think we should make this a PlatformRefPtr<GtkMenuItem>(...). > WebCore/platform/gtk/ContextMenuItemGtk.cpp:118 > + : m_platformDescription(GTK_MENU_ITEM(g_object_ref(item))) It would remove the need to call g_object_ref here. > WebCore/platform/gtk/ContextMenuItemGtk.cpp:135 > + GOwnPtr<char>actionName(g_strdup_printf("context-menu-action-%d", action)); Missing a space here between <char> and actionName. > WebCore/platform/gtk/ContextMenuItemGtk.cpp:156 > + if (m_platformDescription) > + g_object_unref(m_platformDescription); A smart pointer also removes the need for this code.
Created attachment 74218 [details] Updated patch I had to add PlatformRefPtr::leakRef() to implement PlatformMenuItemDescription ContextMenuItem::releasePlatformDescription() since we want to return the object with the sink reference to be consumed by the parent widget.
Attachment 74218 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/PlatformRefPtr.h', u'WebCore/ChangeLog', u'WebCore/platform/ContextMenuItem.h', u'WebCore/platform/gtk/ContextMenuGtk.cpp', u'WebCore/platform/gtk/ContextMenuItemGtk.cpp']" exit_code: 1 JavaScriptCore/wtf/PlatformRefPtr.h:78: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 74218 [details] Updated patch Great patch.
Attachment 74218 [details] did not build on gtk: Build output: http://queues.webkit.org/results/6169060
Created attachment 74361 [details] New patch to fix the build issues I forgot to include GOwnPtr.h
Attachment 74361 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/PlatformRefPtr.h', u'WebCore/ChangeLog', u'WebCore/platform/ContextMenuItem.h', u'WebCore/platform/gtk/ContextMenuGtk.cpp', u'WebCore/platform/gtk/ContextMenuItemGtk.cpp']" exit_code: 1 JavaScriptCore/wtf/PlatformRefPtr.h:78: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 74218 [details] Updated patch Cleared Martin Robinson's review+ from obsolete attachment 74218 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Committed r73703: <http://trac.webkit.org/changeset/73703>
I think that patch broke a test: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r73715%20(16918)/results.html
Committed r73746: <http://trac.webkit.org/changeset/73746>
http://trac.webkit.org/changeset/73703 might have broken GTK Linux 32-bit Debug
http://trac.webkit.org/changeset/73705 might have broken GTK Linux 32-bit Debug
http://trac.webkit.org/changeset/73746 might have broken GTK Linux 64-bit Debug
Comment on attachment 74361 [details] New patch to fix the build issues Cleared review? from attachment 74361 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Reopening to attach a new patch.
Created attachment 76364 [details] New patch rebased to current master Rebased to current git master and fixing the crash in editing/selection/empty-cell-right-click.html. It also improves the code that connects the activate signal for the menu items. gtk_container_get_children() uses gtk_container_foreach() to build the list of items, so we are iterating twice, one to build the list and another one to connect the signal. Using gtk_container_foreach() directly we iterate only once and we avoid creating/destroying the GList. It also makes easier to connect the acivate signal for submenu items.
Attachment 76364 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/PlatformRefPtr.h', u'WebCore/ChangeLog', u'WebCore/platform/ContextMenuItem.h', u'WebCore/platform/gtk/ContextMenuGtk.cpp', u'WebCore/platform/gtk/ContextMenuItemGtk.cpp', u'WebKit/gtk/ChangeLog', u'WebKit/gtk/webkit/webkitwebview.cpp']" exit_code: 1 JavaScriptCore/wtf/PlatformRefPtr.h:78: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
This style error looks like a false positive, so let's give this another whirl.
Comment on attachment 76364 [details] New patch rebased to current master Clearing flags on attachment: 76364 Committed r74028: <http://trac.webkit.org/changeset/74028>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/74028 might have broken GTK Linux 64-bit Debug The following tests are not passing: editing/selection/extend-by-character-002.html