Bug 49658

Summary: [GTK] Simplify context-menu handling code
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, gns, mrobinson, pnormand, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch to simplify context-menu handling in gtk port
mrobinson: review-
Updated patch
none
New patch to fix the build issues
none
New patch rebased to current master none

Description Carlos Garcia Campos 2010-11-17 06:17:03 PST
We can simplify the context-menu handling code by using a GtkMenuItem as PlatformMenuItemDescription instead of the custom struct we are currently using.
Comment 1 Carlos Garcia Campos 2010-11-17 06:19:22 PST
Created attachment 74103 [details]
Patch to simplify context-menu handling in gtk port
Comment 2 Martin Robinson 2010-11-17 08:39:43 PST
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.
Comment 3 Carlos Garcia Campos 2010-11-18 02:10:46 PST
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.
Comment 4 WebKit Review Bot 2010-11-18 02:14:19 PST
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 5 Martin Robinson 2010-11-18 09:16:02 PST
Comment on attachment 74218 [details]
Updated patch

Great patch.
Comment 6 WebKit Review Bot 2010-11-18 15:59:51 PST
Attachment 74218 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6169060
Comment 7 Carlos Garcia Campos 2010-11-19 00:16:40 PST
Created attachment 74361 [details]
New patch to fix the build issues

I forgot to include GOwnPtr.h
Comment 8 WebKit Review Bot 2010-11-19 00:18:30 PST
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 9 Eric Seidel (no email) 2010-11-19 05:44:59 PST
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.
Comment 10 Martin Robinson 2010-12-10 02:31:35 PST
Committed r73703: <http://trac.webkit.org/changeset/73703>
Comment 11 Philippe Normand 2010-12-10 04:04:31 PST
I think that patch broke a test:

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r73715%20(16918)/results.html
Comment 12 Martin Robinson 2010-12-10 11:10:50 PST
Committed r73746: <http://trac.webkit.org/changeset/73746>
Comment 13 WebKit Review Bot 2010-12-10 11:45:31 PST
http://trac.webkit.org/changeset/73703 might have broken GTK Linux 32-bit Debug
Comment 14 WebKit Review Bot 2010-12-10 11:45:46 PST
http://trac.webkit.org/changeset/73705 might have broken GTK Linux 32-bit Debug
Comment 15 WebKit Review Bot 2010-12-10 12:19:51 PST
http://trac.webkit.org/changeset/73746 might have broken GTK Linux 64-bit Debug
Comment 16 Eric Seidel (no email) 2010-12-10 22:28:35 PST
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).
Comment 17 Carlos Garcia Campos 2010-12-13 02:54:46 PST
Reopening to attach a new patch.
Comment 18 Carlos Garcia Campos 2010-12-13 03:00:27 PST
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.
Comment 19 WebKit Review Bot 2010-12-13 03:01:40 PST
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.
Comment 20 Martin Robinson 2010-12-14 08:04:30 PST
This style error looks like a false positive, so let's give this another whirl.
Comment 21 WebKit Commit Bot 2010-12-14 08:25:36 PST
Comment on attachment 76364 [details]
New patch rebased to current master

Clearing flags on attachment: 76364

Committed r74028: <http://trac.webkit.org/changeset/74028>
Comment 22 WebKit Commit Bot 2010-12-14 08:25:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 WebKit Review Bot 2010-12-14 09:35:27 PST
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