[Webkit-unassigned] [Bug 78097] [GTK] Add WebKitWebView::hovered-over-element signal to WebKit2 GTK+ API

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #126042|review?                     |review-
               Flag|                            |




--- Comment #5 from Martin Robinson <mrobinson at webkit.org>  2012-02-08 08:19:53 PST ---
(From update of attachment 126042)
View in context: https://bugs.webkit.org/attachment.cgi?id=126042&action=review

This is, in general, great. There are a few minor things that I would change below.

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:38
> + * an image or a media.

A couple small suggestions for the documentation here:

I think this should be either "or a media elment" or just "or media"

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:45
> + * a link, image or media element in the coordinates of the Hit Test.

"or a media element at the coordinates"

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:47
> + * are active at the same time, for example if there's a link containing and image.

"an image"

> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:247
> +    unsigned context = WEBKIT_HIT_TEST_RESULT_CONTEXT_DOCUMENT;
> +
> +    const String& linkURL = toImpl(wkHitTestResult)->absoluteLinkURL();
> +    if (!linkURL.isEmpty())
> +        context |= WEBKIT_HIT_TEST_RESULT_CONTEXT_LINK;
> +

I probably would have just had the WebKitHitTestResult contain the WKHitTestResultRef, but this seems okay too.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:629
> +     * This signal is emitted when the mouse cursor is moved over an

"is moved" -> "moves"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:631
> +     * whether the mouse cursor is over an element, a Hit Test is performed

"whether the mouse cursor is over an element" -> "what type of element the mouse cursor is over"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:637
> +     * The signal is emitted again when the mouse is moved out of the
> +     * current element with a new @hit_test_result or %NULL if there
> +     * isn't an element at the new mouse position.

My comment below may change this.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:760
> +void webkitWebViewHoveredElement(WebKitWebView* webView, WKHitTestResultRef wkHitTestResult, unsigned modifiers)

You probably want to rename this to webkitWebViewHoveredOverElement

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:764
> +    bool isEmpty = wkHitTestResultIsEmpty(wkHitTestResult);
> +    if ((isEmpty && !priv->hoveredElementHitTestResult)

Why not send empty hit tests? The fact that the element is not one of the types of elements that a hit test specifically measures against is also interesting information. Additionally, in the future, the hit test may also contain the DOM node. Suddently applications would start getting lot of new hit tests and that could break applications unwittingly.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:768
> +        || (!isEmpty
> +            && priv->hoveredElementHitTestResult
> +            && priv->hoveredElementModifiers == modifiers
> +            && webkitHitTestResultCompare(priv->hoveredElementHitTestResult.get(), wkHitTestResult)))

Is it important to only send the first hover event? If it's because this causes a lot of CPU usage, I think we should just rate limit here than changing the behavior entirely. I dislike the idea of making this signal too different from the underlying C API signal before we know whether or not it's important.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:772
> +    priv->hoveredElementHitTestResult = !isEmpty ? adoptGRef(webkitHitTestResultCreate(wkHitTestResult)) : 0;

I think you should still send a hit test even if it's empty. Recall that the hit test may contain the DOM node one day. I also think it's consistent. An empty hit test result is still a hit test result.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:285
>      UIClientTest()
>          : m_scriptType(Alert)
>          , m_scriptDialogConfirmed(true)
> +        , m_hoveredElementModifiers(0)
>      {
>          webkit_settings_set_javascript_can_open_windows_automatically(webkit_web_view_get_settings(m_webView), TRUE);
>          g_signal_connect(m_webView, "create", G_CALLBACK(viewCreate), this);
>          g_signal_connect(m_webView, "script-alert", G_CALLBACK(scriptAlert), this);
>          g_signal_connect(m_webView, "script-confirm", G_CALLBACK(scriptConfirm), this);
>          g_signal_connect(m_webView, "script-prompt", G_CALLBACK(scriptPrompt), this);
> +        g_signal_connect(m_webView, "hovered-over-element", G_CALLBACK(hoveredOverElement), this);

I think it makes sense to just extend WebViewTest with another fixture. If you add this the name "UIClientTest" is no longer strictly accurate.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:307
> +    WebKitHitTestResult* getHoveredElementAt(int x, int y, unsigned int mouseModifiers = 0)
> +    {
> +        mouseMoveTo(x, y, mouseModifiers);
> +        g_main_loop_run(m_mainLoop);
> +        return m_hoveredElementHitTestResult.get();

Might want to call this something like moveMouseAndWaitUntilHoveringOverElement.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:382
> +    test->showInWindowAndWaitUntilMapped();

I love this. :)

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