Bug 49658 – [GTK] Simplify context-menu handling code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

We can simplify the context-menu handling code by using a GtkMenuItem as PlatformMenuItemDescription instead of the custom struct we are currently using.

[Carlos Garcia Campos]

Created attachment 74103 [details]
Patch to simplify context-menu handling in gtk port

[Martin Robinson]

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.

[Carlos Garcia Campos]

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.

[WebKit Review Bot]

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.

[Martin Robinson]

Comment on attachment 74218 [details]
Updated patch

Great patch.

[WebKit Review Bot]

Attachment 74218 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6169060

[Carlos Garcia Campos]

Created attachment 74361 [details]
New patch to fix the build issues

I forgot to include GOwnPtr.h

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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.

[Martin Robinson]

Committed r73703: <http://trac.webkit.org/changeset/73703>

[Philippe Normand]

I think that patch broke a test:

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r73715%20(16918)/results.html

[Martin Robinson]

Committed r73746: <http://trac.webkit.org/changeset/73746>

[WebKit Review Bot]

http://trac.webkit.org/changeset/73703 might have broken GTK Linux 32-bit Debug

[WebKit Review Bot]

http://trac.webkit.org/changeset/73705 might have broken GTK Linux 32-bit Debug

[WebKit Review Bot]

http://trac.webkit.org/changeset/73746 might have broken GTK Linux 64-bit Debug

[Eric Seidel (no email)]

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).

[Carlos Garcia Campos]

Reopening to attach a new patch.

[Carlos Garcia Campos]

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.

[WebKit Review Bot]

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.

[Martin Robinson]

This style error looks like a false positive, so let's give this another whirl.

[WebKit Commit Bot]

Comment on attachment 76364 [details]
New patch rebased to current master

Clearing flags on attachment: 76364

Committed r74028: <http://trac.webkit.org/changeset/74028>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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