Bug 49904 – [GTK] Add a signal to allow applications to handle its own context menu

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

In addition to the populate-popup signal that allows to add custom options to the default context menu, it would be useful to have a signal that allows application to handle its own context menu. We only need to provide the hit test result and the coordinates to the application. I would allow us to get rid of the enable-default-context-menu setting. See also ephy bug: https://bugzilla.gnome.org/show_bug.cgi?id=608491

[Carlos Garcia Campos]

Created attachment 74543 [details]
Add WebKitWebView::context-menu signal

Note that this change is compatible with the current code and doesn't break any application using the default context menu or its own one, since the default handler returns FALSE.

[WebKit Review Bot]

Attachment 74543 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/gtk/ChangeLog', u'WebKit/gtk/webkit/webkitwebview.cpp', u'WebKit/gtk/webkit/webkitwebview.h', u'WebKit/gtk/webkitmarshal.list']" exit_code: 1
WebKit/gtk/webkit/webkitwebview.h:130:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:131:  Extra space between gint and x  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:132:  Extra space between gint and y  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:133:  Extra space between gboolean and keyboard_mode  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:134:  Extra space between WebKitHitTestResult and *hit_test_result  [whitespace/declaration] [3]
Total errors found: 5 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 75018 [details]
Updated patch to apply cleanly in current git master

[WebKit Review Bot]

Attachment 75018 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/gtk/ChangeLog', u'WebKit/gtk/webkit/webkitwebview.cpp', u'WebKit/gtk/webkit/webkitwebview.h', u'WebKit/gtk/webkitmarshal.list']" exit_code: 1
WebKit/gtk/webkit/webkitwebview.h:143:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.h:144:  Extra space between gint and x  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:145:  Extra space between gint and y  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:146:  Extra space between gboolean and keyboard_mode  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitwebview.h:147:  Extra space between WebKitHitTestResult and *hit_test_result  [whitespace/declaration] [3]
Total errors found: 5 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 75018 [details]
Updated patch to apply cleanly in current git master

View in context: https://bugs.webkit.org/attachment.cgi?id=75018&action=review

A few comments: in general, I like the patch. Epiphany replaces the context menu completely by using a different technique, though: handling the right-clicks that should cause a context menu to appear itself, and then showing the menu it creates if the page doesn't claim the right click. That said, it was a pain to get the behaviour right, and having a separate signal could make life simpler for other applications, so +1 from me. I'd like to hear what Xan thinks, though.

> WebKit/gtk/webkit/webkitwebview.h:147
> +    gboolean                   (*context_menu)            (WebKitWebView        *web_view,
> +                                                           gint                  x,
> +                                                           gint                  y,
> +                                                           gboolean              keyboard_mode,
> +                                                           WebKitHitTestResult  *hit_test_result);

This breaks ABI, I would suggest adding the signal and avoiding the virtual method.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 75018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75018&action=review
> 
> A few comments: in general, I like the patch. Epiphany replaces the context menu completely by using a different technique, though: handling the right-clicks that should cause a context menu to appear itself, and then showing the menu it creates if the page doesn't claim the right click. That said, it was a pain to get the behaviour right, and having a separate signal could make life simpler for other applications, so +1 from me. I'd like to hear what Xan thinks, though.
> 
> > WebKit/gtk/webkit/webkitwebview.h:147
> > +    gboolean                   (*context_menu)            (WebKitWebView        *web_view,
> > +                                                           gint                  x,
> > +                                                           gint                  y,
> > +                                                           gboolean              keyboard_mode,
> > +                                                           WebKitHitTestResult  *hit_test_result);
> 
> This breaks ABI, I would suggest adding the signal and avoiding the virtual method.

I use the default handler to return FALSE when the client doesn't connect to the signal to make current clients' code compatible. We can use one fo the reserved paddings.

See also ephy bug: https://bugzilla.gnome.org/show_bug.cgi?id=608491

[Carlos Garcia Campos]

Created attachment 76383 [details]
New patch rebased to current git master

Signals returning boolean always return FALSE when no callbacks are connected, so it's safe to remove the vfunc from the class structure.

[Xan Lopez]

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

This looks good to me, so 1/2 r+. You need another reviewer to give the final r+.

[Martin Robinson]

The context menu signal + setting is already far too complex. I'm not a fan of adding another signal entirely. This patch is very close to handling every use case we need for context menus. 

The changes that make sense to me:
1. Have the signal carry a GList of default menu items. Users can modify the list to modify what the default signal handler uses for the menu.
2. Deprecate the existing context menu setting and signal.
3. Pass the actual DOM node instead of the hit test result. The DOM node carries more useful information and makes sense even for keyboard activation.

[Carlos Garcia Campos]

(In reply to comment #9)
> The context menu signal + setting is already far too complex. I'm not a fan of adding another signal entirely. This patch is very close to handling every use case we need for context menus. 

close? which use case is not handled? I see 4 uses cases:

- Apps that don't care at all about the context menu and want webkit to show its default context menu. In this case just enable the default context menu setting.
- Apps tha want to add just a few custom options to the context menu that are not already handled by the default context menu. In this case just enabled the default context menu setting and connect to the populate signal to add the custom options to the default menu.
- Apps that want to handle the whole menu by their own. This is the case of epiphany. It disables the default context menu and creates its own, with the options handled by the default context menu and some others. In this case connect to contex-menu signal, show the menu and return TRUE to not handle the default context menu.
- Apps that don't want any context menu at all, not even the default one. They simply connect to context-menu and return TRUE without doing anything else.

> The changes that make sense to me:
> 1. Have the signal carry a GList of default menu items. Users can modify the list to modify what the default signal handler uses for the menu.

what would be the id to know whether an item is the same you want to use? the label of the menu item? 

> 2. Deprecate the existing context menu setting and signal.

how would you handle the case of applications (like ephy) that want to handle its own context menu?You are assuming apps want a menu to fill, but some apps have their own menu like the ephy one, created with GtkUIManager using a placeholder menu item to fill. 

> 3. Pass the actual DOM node instead of the hit test result. The DOM node carries more useful information and makes sense even for keyboard activation.

[Xan Lopez]

(In reply to comment #10)
> (In reply to comment #9)
> > The context menu signal + setting is already far too complex. I'm not a fan of adding another signal entirely. This patch is very close to handling every use case we need for context menus. 
> 
> close? which use case is not handled? I see 4 uses cases:
> 

He means it's very close to handling all cases only with this signal. Obviously all possible cases are already handled with all the signals + the setting. FWIW I'm not really a fan of the GList API and I think a setting makes things easier for some common cases, but I guess that's up for debate.

[Martin Robinson]

> He means it's very close to handling all cases only with this signal. Obviously all possible cases are already handled with all the signals + the setting. FWIW I'm not really a fan of the GList API and I think a setting makes things easier for some common cases, but I guess that's up for debate.

I'm not particularly attached to passing the default menu items with a GList. What would you propose as an alternative? A GtkMenu?

[Carlos Garcia Campos]

(In reply to comment #12)
> > He means it's very close to handling all cases only with this signal. Obviously all possible cases are already handled with all the signals + the setting. FWIW I'm not really a fan of the GList API and I think a setting makes things easier for some common cases, but I guess that's up for debate.
> 
> I'm not particularly attached to passing the default menu items with a GList. What would you propose as an alternative? A GtkMenu?

The problem is not whether use a list or any other container, but the items, what would you pass? a GtkMenuItem? how can the user know that a GtkMenuItem is the same menu entry he wanted to add? Now that simplify-context menu patch was committed, menu items have a GtkAction associated, we might use a well known id for the actions and use gtk_activatable_get_related_action() and gtk_action_get_name(). We would have to document the names of the actions handled by the default context menu. This is a lot of more complex than adding a new signal.

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > The context menu signal + setting is already far too complex. I'm not a fan of adding another signal entirely. This patch is very close to handling every use case we need for context menus. 
> > 
> > close? which use case is not handled? I see 4 uses cases:
> > 
> 
> He means it's very close to handling all cases only with this signal.

I think both signals are complementary to cover different test cases. 

> Obviously all possible cases are already handled with all the signals + the setting.

Ok, sorry, I misunderstood it.

> FWIW I'm not really a fan of the GList API and I think a setting makes things easier for some common cases, but I guess that's up for debate.

Passing a list of items, using a GList or any other container, would mean the client would have to iterate the list comparing every item in the list with every item he wanted to add to know wehter to add it ro not, assuming we know how to compare the items which is another problem.

[Martin Robinson]

(In reply to comment #14)

> I think both signals are complementary to cover different test cases. 

I think API simplicity is the big advantage of having only one signal. Even more confusing is an API where the WebKitWebSettings default context menu setting only affects the old signal. All of this logic can be encapsulated by simply firing one signal. If we're going to fix the situation, why not fix it all the way?

> Passing a list of items, using a GList or any other container, would mean the client would have to iterate the list comparing every item in the list with every item he wanted to add to know wehter to add it ro not, assuming we know how to compare the items which is another problem.

Right now this is the only way to expose the default menu items to the embedder. We must expose some way for the embedder to both show the input method menu and use a non-default context menu. 

It's unfortunate that this is so ugly.  At the hackfest we briefly discussed making the order and naming follow API stability rules. If, as you say, there's an even saner way to do this with the GtkAction API, I think we should do that. Let's make the API simple and beautiful. :)

[Martin Robinson]

Actually, since I'm rallying for this, I can post a modified version of the patch tomorrow which implements what I'm mentioning.

[Carlos Garcia Campos]

(In reply to comment #15)
> (In reply to comment #14)
> 
> > I think both signals are complementary to cover different test cases. 
> 
> I think API simplicity is the big advantage of having only one signal. Even more confusing is an API where the WebKitWebSettings default context menu setting only affects the old signal. All of this logic can be encapsulated by simply firing one signal. If we're going to fix the situation, why not fix it all the way?

if the problem is the setting, we can have the 4 test cases I mentioned with the two signals without the setting. The problem is that I don't see how to implement such 4 cases with only one signal. 

> 
> > Passing a list of items, using a GList or any other container, would mean the client would have to iterate the list comparing every item in the list with every item he wanted to add to know wehter to add it ro not, assuming we know how to compare the items which is another problem.
> 
> Right now this is the only way to expose the default menu items to the embedder. We must expose some way for the embedder to both show the input method menu and use a non-default context menu. 
> 
> It's unfortunate that this is so ugly.  At the hackfest we briefly discussed making the order and naming follow API stability rules. If, as you say, there's an even saner way to do this with the GtkAction API, I think we should do that. Let's make the API simple and beautiful. :)

GtkAction has a unique id which is the name, but the API won't be simpler even less beautiful (but I admit it's a matter of taste). 
If we go with single signal I would need at least a way to disable the context menu, so that ephy can still has its own context menu and perform a hit test by its own, which is what it currently does, but disabling the context menu we would avoid doing the hit test twice. We would also need API to get the current focused node from ephy to implement the context menu when it's triggered by the keyboard.

[Carlos Garcia Campos]

(In reply to comment #16)
> Actually, since I'm rallying for this, I can post a modified version of the patch tomorrow which implements what I'm mentioning.

That would be great, if you could also update the ephy patch to make sure we can still handle the context menu in ephy with the new api:

https://bugzilla.gnome.org/show_bug.cgi?id=608491

[Martin Robinson]

Created attachment 76818 [details]
Suggested changes to the patch

[Carlos Garcia Campos]

(In reply to comment #19)
> Created an attachment (id=76818) [details]
> Suggested changes to the patch

Bz doesn't even allow me to comment the patch, so I'll copy paste my review here.

>     GtkMenu* menu = GTK_MENU(coreMenu->platformDescription());
>+    g_object_ref(menu);
>     if (!menu)
>-        return FALSE;
>+        return;

why do you need to ref the menu?, also you are calling g_object_ref(), which is not null safe, before the if (!menu).


>+    PlatformMouseEvent event(location, 
>+                             global,
>+                             RightButton,
>+                             MouseEventPressed,
>+                             0, // clickCount
>+                             false, // shift
>+                             false, // ctrl
>+                             false, // alt
>+                             false, // meta
>+                             gtk_get_current_event_time());
>+
>+    gboolean result = frame->eventHandler()->handleMousePressEvent(event);
>+    webkit_web_view_forward_context_menu_event(WEBKIT_WEB_VIEW(widget), frame, event, true);
>+    return result;

how can you know whether the popup menu will be shown or not? this function should return TRUE when a menu was activated, webkit_web_view_forward_context_menu_event has to be boolean to know whether the menu has been handled or not.

> }
> 
> #ifndef GTK_API_VERSION_2
>@@ -853,14 +853,16 @@ static gboolean webkit_web_view_button_press_event(GtkWidget* widget, GdkEventBu
>     priv->previousClickButton = event->button;
>     priv->previousClickTime = eventTime;
> 
>-    if (event->button == 3)
>-        return webkit_web_view_forward_context_menu_event(webView, PlatformMouseEvent(event));
>-
>     Frame* frame = core(webView)->mainFrame();
>     if (!frame->view())
>         return FALSE;
>-
>     gboolean result = frame->eventHandler()->handleMousePressEvent(platformEvent);
>+
>+    if (event->button == 3) {
>+        webkit_web_view_forward_context_menu_event(webView, frame, PlatformMouseEvent(event), false);
>+        return result;

the same here, depending on whether you return TRUE or FALSE the event will be propagated or not, if the menu hasn't been activated we should return FALSE.


>     /*
>+     * WebKitWebView::context-menu
>+     * @webView: the object which received the signal
>+     * @x: the X coordinate of the position where the context menu should be shown
>+     * @y: the Y coordinate of the position where the context menu should be shown
>+     * @keyboard_mode: %TRUE if the context menu was trigged using the keyboard
>+     * @hit_test_result: a #WebKitHitTestResult with the context of the current position.
>+     * @menu: the default menu for the element this context menu corresponds to
>+     *
>+     * Emitted when a context menu is about to be displayed to give the application a
>+     * chance to create and handle its own context menu. If you only want to add custom
>+     * options to the default context menu you can simply modify the @menu signal
>+     * parameter.
>+     * 
>+     * When keyboard_mode is %TRUE the given coordinates should be used to position the
>+     * popup menu, when the context menu has been triggered by a mouse event you could
>+     * either use the given coordinates or pass %NULL to the #GtkMenuPositionFunc
>+     * parameter of gtk_menu_popup() function.
>+     *
>+     * When the signal is handled and a popup menu has been created by the application,
>+     * %TRUE should be returned. If %FALSE is returned, a popup menu will appears as long
>+     * as the #WebKitWebSettings::enable-default-context-menu setting is active. If you
>+     * don't want any context menu to be shown, you can simply connect to this signal and
>+     * return %TRUE without doing anything else.
>+     *
>+     * Since: 1.3.8
>+     */
>+    webkit_web_view_signals[CONTEXT_MENU] = g_signal_new("context-menu",
>+            G_TYPE_FROM_CLASS(webViewClass),
>+            (GSignalFlags)G_SIGNAL_RUN_LAST,
>+            0,
>+            0, 0,
>+            webkit_marshal_BOOLEAN__INT_INT_BOOLEAN_OBJECT_OBJECT,
>+            G_TYPE_BOOLEAN, 5,
>+            G_TYPE_INT, G_TYPE_INT, G_TYPE_BOOLEAN,
>+            WEBKIT_TYPE_HIT_TEST_RESULT, GTK_TYPE_MENU);

The signal is still confusing to me, when the user has its own context menu, the menu parameter should be ignored, and when the user wants to add stuff to the menu, x, y, and keyboard_mode params are useless. I still see two different signals for two different use cases (create your own menu, add items to the default one). 
And we still have the problem of how the user knows whether the given menu already has the items he/she wants to add. I agree it would be useful to be able to build a context menu in the application with some options (or submenus) from webkit, like the input method or spell checker submenus, but providing a way to let the users create their own menus rather than modifying the default one.

[Martin Robinson]

(In reply to comment #20)
> (In reply to comment #19)

Thanks for your comments!

> >+    g_object_ref(menu);
> why do you need to ref the menu?, also you are calling g_object_ref(), which is not null safe, before the if (!menu).

This was left over from an earlier iteration. Removed.

> >+    gboolean result = frame->eventHandler()->handleMousePressEvent(event);
> >+    webkit_web_view_forward_context_menu_event(WEBKIT_WEB_VIEW(widget), frame, event, true);
> >+    return result;
> how can you know whether the popup menu will be shown or not? this function should return TRUE when a menu was activated, webkit_web_view_forward_context_menu_event has to be boolean to know whether the menu has been handled or not.

Fixed in both cases!

> >     /*
> >+     * WebKitWebView::context-menu
> >+     * @webView: the object which received the signal
> >+     * @x: the X coordinate of the position where the context menu should be shown
> >+     * @y: the Y coordinate of the position where the context menu should be shown
> >+     * @keyboard_mode: %TRUE if the context menu was trigged using the keyboard
> >+     * @hit_test_result: a #WebKitHitTestResult with the context of the current position.
> >+     * @menu: the default menu for the element this context menu corresponds to
> >+     *
> >+     * Emitted when a context menu is about to be displayed to give the application a
> >+     * chance to create and handle its own context menu. If you only want to add custom
> >+     * options to the default context menu you can simply modify the @menu signal
> >+     * parameter.
> >+     * 
> >+     * When keyboard_mode is %TRUE the given coordinates should be used to position the
> >+     * popup menu, when the context menu has been triggered by a mouse event you could
> >+     * either use the given coordinates or pass %NULL to the #GtkMenuPositionFunc
> >+     * parameter of gtk_menu_popup() function.
> >+     *
> >+     * When the signal is handled and a popup menu has been created by the application,
> >+     * %TRUE should be returned. If %FALSE is returned, a popup menu will appears as long
> >+     * as the #WebKitWebSettings::enable-default-context-menu setting is active. If you
> >+     * don't want any context menu to be shown, you can simply connect to this signal and
> >+     * return %TRUE without doing anything else.
> >+     *
> >+     * Since: 1.3.8
> >+     */
> >+    webkit_web_view_signals[CONTEXT_MENU] = g_signal_new("context-menu",
> >+            G_TYPE_FROM_CLASS(webViewClass),
> >+            (GSignalFlags)G_SIGNAL_RUN_LAST,
> >+            0,
> >+            0, 0,
> >+            webkit_marshal_BOOLEAN__INT_INT_BOOLEAN_OBJECT_OBJECT,
> >+            G_TYPE_BOOLEAN, 5,
> >+            G_TYPE_INT, G_TYPE_INT, G_TYPE_BOOLEAN,
> >+            WEBKIT_TYPE_HIT_TEST_RESULT, GTK_TYPE_MENU);
> 
> The signal is still confusing to me, when the user has its own context menu, the menu parameter should be ignored, and when the user wants to add stuff to the menu, x, y, and keyboard_mode params are useless. I still see two different signals for two different use cases (create your own menu, add items to the default one). 

A few points: If the user wishes to both display her own menu and re-use the default menu items, having two signals would make it very difficult (and impossible with your version, I think). I think having one signal is easily less confusing than two incompatible signals. I don't think it's true that every signal should have only one usecase. 

> And we still have the problem of how the user knows whether the given menu already has the items he/she wants to add. I agree it would be useful to be able to build a context menu in the application with some options (or submenus) from webkit, like the input method or spell checker submenus, but providing a way to let the users create their own menus rather than modifying the default one.

I agree that this practice is a bit ugly, but embedders have already been doing it for quite some. Indeed, every other port takes this approach. Further, it seems like there's a chance that we can use GtkAction to make this more sane. Epiphany is the only embedder I know that recreates the entire menu. 

In the end, I believe that having 2 or 3 new API points for managing the context menu is ugly as well, and presents the same kind of future-proofing problems.

[Martin Robinson]

Created attachment 76876 [details]
Patch with suggested fixes

[Christian Dywan]

(In reply to comment #21)
> I agree that this practice is a bit ugly, but embedders have already been doing it for quite some. Indeed, every other port takes this approach. Further, it seems like there's a chance that we can use GtkAction to make this more sane. Epiphany is the only embedder I know that recreates the entire menu. 

I don't think it is possible to equally replace the whole menu, that is why Midori still doesn't do it. It can't create 'Inspect Element', 'Open Frame in New Window' or the input method menu items in the same way. Apart from that I think it is good for consistency to share copying and link context menus instead of re-creating those.

[Martin Robinson]

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

Going to r- both of these patches until we can resolve these API discussions.

[Carlos Garcia Campos]

Created attachment 106400 [details]
New patch

After talking with Martin about this bug, we thought we could add a new class to handle the context menu and pass that new object as parameter of the new signal. Then I looked again at the patches and I realized we don't actually need a new class. Following the idea of passing the default context menu widget as a parameter to the signal, the only thing we would need to make menu modification easier is a well know name for every possible menu item, but I think that could be part fo a following patch, since it's actually another problem that we already have with the populate-popup signal. So, this patch adds a new context-menu signal that passes the default menu, a hit test result and whether it was triggered by keyboard. I've added x and y properties to WebKitHitTestResult so that we don't need to pass x, y as signal parameters, like previous patches did. The hit test result already contains the coordinates (relative ot the view's window) that the client can use when the menu is triggered by the keyboard to position the menu.

[WebKit Review Bot]

Attachment 106400 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testhittestresult.c"
Source/WebKit/gtk/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:22:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:24:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:28:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 106401 [details]
Updated patch: untabify changelog to fix style

[Carlos Garcia Campos]

I updated also the epiphany patch to use this new patch:

https://bugzilla.gnome.org/show_bug.cgi?id=608491#c6

[Martin Robinson]

Comment on attachment 106401 [details]
Updated patch: untabify changelog to fix style

View in context: https://bugs.webkit.org/attachment.cgi?id=106401&action=review

I really like this patch and I think that we should do something similar for WebKit2. I'm giving the first r+ here, but we need to get another reviewer to agree to this change.

> Source/WebKit/gtk/webkit/webkithittestresult.cpp:324
> +    targetFrame = result.targetFrame();
> +    if (targetFrame && targetFrame->view()) {
> +        // Convert document coords to widget coords.
> +        point = targetFrame->view()->contentsToWindow(result.point());
> +    } else
> +        point = result.point();
> +

Maybe this should be ASSERT(result.targetFrame())? Are there legitimate situations where targetFrame is null?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:316
> +    HitTestRequest request(HitTestRequest::Active);
> +    IntPoint point = frame->view()->windowToContents(event.pos());
> +    return frame->document()->prepareMouseEvent(request, point, event);

Nice cleanup.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:324
> +    WebKitWebSettings* settings = webkit_web_view_get_settings(webView);
> +    gboolean enableDefaultContextMenu;
> +    g_object_get(settings, "enable-default-context-menu", &enableDefaultContextMenu, NULL);

I wouldn't cache settings here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:328
> +static gboolean webkit_web_view_forward_context_menu_event(WebKitWebView* webView, const PlatformMouseEvent& event, bool keyboardMode)

keyboardMode should really be something like triggeredWithKeyboard.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:387
> +    gboolean handled;
> +    g_signal_emit(webView, webkit_web_view_signals[CONTEXT_MENU], 0, defaultMenu, hitTestResult.get(), keyboardMode, &handled);
> +    if (handled)
> +        return TRUE;

We bail out early if there's no defaultMenu below, so we probably shouldn't fire this signal if there's no defaultMenu either, right?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:389
> +    // Return now ff there's no default context menu or it's disabled by enable-default-context-menu setting.

ff -> if

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2775
> +     * @keyboard_mode: %TRUE if the context menu was triggered using the keyboard

Instead of keyboard_mode I'd prefer a name like triggered_with_keyboard.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2789
> +     * When the signal is handled and a popup menu has been created by the application,
> +     * %TRUE should be returned. Note that when the context menu is handled by the
> +     * application, the "enable-default-context-menu" setting will be ignored and

I suggest using a more active voice here: "If your application will create and display its own popup menu, %TRUE should be returned."

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2790
> +     * #WebKitWebView::populate-popup signal won't be emitted.

missing a "the" at the beginning of the line.

[Carlos Garcia Campos]

(In reply to comment #29)
> (From update of attachment 106401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106401&action=review
> 
> I really like this patch and I think that we should do something similar for WebKit2. I'm giving the first r+ here, but we need to get another reviewer to agree to this change.
> 
> > Source/WebKit/gtk/webkit/webkithittestresult.cpp:324
> > +    targetFrame = result.targetFrame();
> > +    if (targetFrame && targetFrame->view()) {
> > +        // Convert document coords to widget coords.
> > +        point = targetFrame->view()->contentsToWindow(result.point());
> > +    } else
> > +        point = result.point();
> > +
> 
> Maybe this should be ASSERT(result.targetFrame())? Are there legitimate situations where targetFrame is null?

I'm not sure, but we are also checking targetFrame->view() even when we have a targetFrame.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:316
> > +    HitTestRequest request(HitTestRequest::Active);
> > +    IntPoint point = frame->view()->windowToContents(event.pos());
> > +    return frame->document()->prepareMouseEvent(request, point, event);
> 
> Nice cleanup.
> 
> > Source/WebKit/gtk/webkit/webkitwebview.cpp:324
> > +    WebKitWebSettings* settings = webkit_web_view_get_settings(webView);
> > +    gboolean enableDefaultContextMenu;
> > +    g_object_get(settings, "enable-default-context-menu", &enableDefaultContextMenu, NULL);
> 
> I wouldn't cache settings here.

Ok.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:328
> > +static gboolean webkit_web_view_forward_context_menu_event(WebKitWebView* webView, const PlatformMouseEvent& event, bool keyboardMode)
> 
> keyboardMode should really be something like triggeredWithKeyboard.

Ok.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:387
> > +    gboolean handled;
> > +    g_signal_emit(webView, webkit_web_view_signals[CONTEXT_MENU], 0, defaultMenu, hitTestResult.get(), keyboardMode, &handled);
> > +    if (handled)
> > +        return TRUE;
> 
> We bail out early if there's no defaultMenu below, so we probably shouldn't fire this signal if there's no defaultMenu either, right?

Because populate-popup doesn't make sense without a menu, but context-menu can be used to create your own context menu ignoring whether there's a default one or not. The thing is that we are returning earlier above when context menu controller doesn't return a context menu, and I think the only situation when we have a context menu, but platform description is NULL is when it has been explicitely released with releasePlatformDescription, and that's not happening here. I'll look at this again in detail.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:389
> > +    // Return now ff there's no default context menu or it's disabled by enable-default-context-menu setting.
> 
> ff -> if

Ok.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:2775
> > +     * @keyboard_mode: %TRUE if the context menu was triggered using the keyboard
> 
> Instead of keyboard_mode I'd prefer a name like triggered_with_keyboard.

I used keyboard_mode for consistency with GTK API, see for example:

http://developer.gnome.org/gtk3/stable/GtkWidget.html#GtkWidget-query-tooltip

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:2789
> > +     * When the signal is handled and a popup menu has been created by the application,
> > +     * %TRUE should be returned. Note that when the context menu is handled by the
> > +     * application, the "enable-default-context-menu" setting will be ignored and
> 
> I suggest using a more active voice here: "If your application will create and display its own popup menu, %TRUE should be returned."

Ok.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:2790
> > +     * #WebKitWebView::populate-popup signal won't be emitted.
> 
> missing a "the" at the beginning of the line.

Ok.

Thanks for reviewing.

[Carlos Garcia Campos]

Created attachment 106556 [details]
Updated patch

Another updated patch with fixes suggested by Martin

[Martin Robinson]

(In reply to comment #30)

> > > Source/WebKit/gtk/webkit/webkitwebview.cpp:2775
> > > +     * @keyboard_mode: %TRUE if the context menu was triggered using the keyboard
> > 
> > Instead of keyboard_mode I'd prefer a name like triggered_with_keyboard.
> 
> I used keyboard_mode for consistency with GTK API, see for example:

I think maybe the GTK+ API is being a bad example here. I can't think of any issues with spending a few more characters to make the variable name clearer.

[Gustavo Noronha (kov)]

Comment on attachment 106556 [details]
Updated patch

FWIW I agree with Martin regarding the naming, we're already deviating from the signal name anyway, maybe we can steer GTK+ to that better name with the 4.0 release ;).

[Carlos Garcia Campos]

Committed r111843: <http://trac.webkit.org/changeset/111843>

[Carlos Garcia Campos]

(In reply to comment #34)
> Committed r111843: <http://trac.webkit.org/changeset/111843>

Sorry, I forgot to update the keyboard_mode, landed now:

http://trac.webkit.org/changeset/111844