[webkit-reviews] review denied: [Bug 78097] [GTK] Add WebKitWebView::hovered-over-element signal to WebKit2 GTK+ API : [Attachment 126042] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 78097: [GTK] Add WebKitWebView::hovered-over-element signal to WebKit2 GTK+
API
https://bugs.webkit.org/show_bug.cgi?id=78097

Attachment 126042: Patch
https://bugs.webkit.org/attachment.cgi?id=126042&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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_g
et_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. :)