[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 #29 from Martin Robinson <mrobinson at webkit.org>  2011-09-06 19:36:32 PST ---
(From update of attachment 106401)
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.

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