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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49904





--- Comment #30 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-09-07 00:00:06 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.