Bug 78097 – [GTK] Add WebKitWebView::mouse-target-changed signal to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Implement mouseDidMoveOverElement UI client callback and emit the view signal passing a HitTestResult.

[Carlos Garcia Campos]

Created attachment 126042 [details]
Patch

Implements the API discussed in the mailing list.

[WebKit Review Bot]

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

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

WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:266:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.h"
Total errors found: 1 in 21 files


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

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

(In reply to comment #2)
> Attachment 126042 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1
> 
> WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
> Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:266:  Use 0 instead of NULL.  [readability/null] [5]

This is a g_object_new()

[Martin Robinson]

Comment on attachment 126042 [details]
Patch

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. :)

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 126042 [details])
> 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"

Ok.

> > 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"

Ok

> > 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"

Oops

> > 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.

In that case we couldn't return const char * in public methods, we want to cache the whole HitTestResult, so we don't need the keep the C one.

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

Ok.

> > 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"

Ok.

> > 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

Right, I missed this one when I renamed hovered-element as hovered-over-element.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:764
> > +    bool isEmpty = wkHitTestResultIsEmpty(wkHitTestResult);
> > +    if ((isEmpty && !priv->hoveredElementHitTestResult)
> 
> Why not send empty hit tests?

We do send empty hit tests, as NULL. But we don't send them if previous one was empty too. 

> 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. 

In that case (which is unlikely to happen) wkHitTestResultIsEmpty won't return true, because it will contain a node. The same will happen if we add more info to HitTestResult in the C API, like whether it's over a selection or editable content.

> Suddently applications would start getting lot of new hit tests and that could break applications unwittingly.

I don't think so, apps should check the context of the hit test before using it, see the minibrowser example, that uses this signal to show the url of hovered link sin the status bar. 

> > 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?

Emitting the signal a lot of times with the same hit test result means apps would need to check whether it's the same or not.

> If it's because this causes a lot of CPU usage,

It's not only because the CPU usage, it makes the API easier to use. The signal is called hovered-over-element, I woulnd't expect it to be emitted twice for the same element. This the same behaviour of the webkit1 hovering-link signal, fwiw. 

> I think we should just rate limit here than changing the behavior entirely.

what behaviour are we changing?

>  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.

I think it's important, try to implement the url hovering in minibrowser if this signal is emitted a lot of times for the same link. I dislike the idea of making this signal too different from the WebKit1 equivalent :-P

> > 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.

In that case it won't be empty.

>  I also think it's consistent. An empty hit test result is still a hit test result.

NULL is an empty hit test result. I don't want apps checking all the possible values of a hit test result object and all of the returning NULL.

> > 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.

mouseDidMoveOverElement is a callback of the UI Client. 

> > 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.

It returns the hovered element at the given position, but I don't care about private names in unit tests, so I'll change it if you think it's important.

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

I knew it :-)

[Martin Robinson]

(In reply to comment #6)

> > 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. 
> 
> In that case (which is unlikely to happen) wkHitTestResultIsEmpty won't return true, because it will contain a node. The same will happen if we add more info to HitTestResult in the C API, like whether it's over a selection or editable content.

I think it's important to send the HitTest even when it's empty. This doesn't make the logic in embedders any more complicated and protects against poorly written applications that assume the HitTest isn't NULL. In general, I think we should avoid forcing NULL checks in embedders when it makes sense.

Another thing is that this "feels" wrong to me. What does it mean when a hit test returns a NULL result? The idea that a hit test returns a result that says "I didn't hit anything" seems more sensible to me.  I think perhaps you are thinking of the HitTestResult as "items found by the hit test," whereas I think of it more like GAsyncResult.

> > Is it important to only send the first hover event?
> 
> Emitting the signal a lot of times with the same hit test result means apps would need to check whether it's the same or not.
> 
> > If it's because this causes a lot of CPU usage,
> 
> It's not only because the CPU usage, it makes the API easier to use. The signal is called hovered-over-element, I woulnd't expect it to be emitted twice for the same element. This the same behaviour of the webkit1 hovering-link signal, fwiw. 

I'm this case, I think this particular change (only emitting the signal once per element) is probably fine in the end. As long as a new HitTest is always sent when you stop hovering on an element, this seems safe.

> > I think we should just rate limit here than changing the behavior entirely.
> what behaviour are we changing?

We're changing the behavior from the WebKit2 C API. Part of the reason I'm so picky about some of this stuff is that we are going under the assumption that the C API could possibly be removed. If the C API is removed, we'll need to rewrite the WebKitTestRunner against the GObject API. If that happens, we may not be able to properly run all the tests if the API isn't rich enough. In this case, I think you've convinced me that it's a fairly safe change though.

> >  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.
> 
> I think it's important, try to implement the url hovering in minibrowser if this signal is emitted a lot of times for the same link. I dislike the idea of making this signal too different from the WebKit1 equivalent :-P

In WebKit1 it seems that the signal is emmitted every time ChromeClient::mouseDidMoveOverElement is called, so wouldn't this be a change? In any case, either behavior is probably fine here.

> >  I also think it's consistent. An empty hit test result is still a hit test result.
> 
> NULL is an empty hit test result. I don't want apps checking all the possible values of a hit test result object and all of the returning NULL.

Why not add a webkit_hit_test_is_empty method then? The apps can also check if context is 0. I think this is safer than having this value be null.

> > I think it makes sense to just extend WebViewTest with another fixture. If you add this the name "UIClientTest" is no longer strictly accurate.
> 
> mouseDidMoveOverElement is a callback of the UI Client. 

You're absolutely right.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > > 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. 
> > 
> > In that case (which is unlikely to happen) wkHitTestResultIsEmpty won't return true, because it will contain a node. The same will happen if we add more info to HitTestResult in the C API, like whether it's over a selection or editable content.
> 
> I think it's important to send the HitTest even when it's empty. This doesn't make the logic in embedders any more complicated and protects against poorly written applications that assume the HitTest isn't NULL.

It's documented and includes the allow none tag.

> In general, I think we should avoid forcing NULL checks in embedders when it makes sense.

I think the opposite, we already return NULL in a lot of places.

> Another thing is that this "feels" wrong to me. What does it mean when a hit test returns a NULL result?

This is not this case, the user didn't perform a hit test. A hit test can never return NULL, because at least WEBKIT_HIT_TEST_RESULT_CONTEXT_DOCUMENT is always true. This case is different, the signal is called hovered-over-element, no hit-test-performed.

> The idea that a hit test returns a result that says "I didn't hit anything" seems more sensible to me.  I think perhaps you are thinking of the HitTestResult as "items found by the hit test," whereas I think of it more like GAsyncResult.

Yes, because that's the case. In GAsyncResult the user asks for it. When we add the hit test api, a hit test operation will never return NULL. In this case we are only interested in the elements and context at the current mouse position, and if there's nothing interesting we just pass NULL. We have documented that a hit test is performed to get the information, the user is not expected to use this signal to perform hot tests. For that use case we don't need a new signal at all, just connect to motion-notify and do a hit test for the motion coordinates (when we add the api).

> > > Is it important to only send the first hover event?
> > 
> > Emitting the signal a lot of times with the same hit test result means apps would need to check whether it's the same or not.
> > 
> > > If it's because this causes a lot of CPU usage,
> > 
> > It's not only because the CPU usage, it makes the API easier to use. The signal is called hovered-over-element, I woulnd't expect it to be emitted twice for the same element. This the same behaviour of the webkit1 hovering-link signal, fwiw. 
> 
> I'm this case, I think this particular change (only emitting the signal once per element) is probably fine in the end.

Qt does the same fwiw.

> As long as a new HitTest is always sent when you stop hovering on an element, this seems safe.

Exactly, that's what the NULL hit test is for, it allows applications to reset the status bar, for example when mouse is moved out of a link.

> > > I think we should just rate limit here than changing the behavior entirely.
> > what behaviour are we changing?
> 
> We're changing the behavior from the WebKit2 C API. Part of the reason I'm so picky about some of this stuff is that we are going under the assumption that the C API could possibly be removed.

What I understood when I talked to apple guys was that the C API is not going to be removed, but replaces by a C++ one.

> If the C API is removed, we'll need to rewrite the WebKitTestRunner against the GObject API. If that happens, we may not be able to properly run all the tests if the API isn't rich enough. In this case, I think you've convinced me that it's a fairly safe change though.

In this case you can just connect to motion-notify and do a hit test if that's what WTR needs.

> > >  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.
> > 
> > I think it's important, try to implement the url hovering in minibrowser if this signal is emitted a lot of times for the same link. I dislike the idea of making this signal too different from the WebKit1 equivalent :-P
> 
> In WebKit1 it seems that the signal is emmitted every time ChromeClient::mouseDidMoveOverElement is called, so wouldn't this be a change?

bool isLink = hit.isLiveLink();
    if (isLink) {
        KURL url = hit.absoluteLinkURL();
        if (!url.isEmpty() && url != m_hoveredLinkURL) {
            TextDirection dir;
            CString titleString = hit.title(dir).utf8();
            CString urlString = url.string().utf8();
            g_signal_emit_by_name(m_webView, "hovering-over-link", titleString.data(), urlString.data());
            m_hoveredLinkURL = url;
        }
    } else if (!isLink && !m_hoveredLinkURL.isEmpty()) {
        g_signal_emit_by_name(m_webView, "hovering-over-link", 0, 0);
        m_hoveredLinkURL = KURL();
    }

It does the same, if url is not empty and is not the same than the previous one, signal is emitted and new url is saved, if it's not a link or there's no previous one, the signal is emitted with NULL.

> In any case, either behavior is probably fine here.

Great.

> > >  I also think it's consistent. An empty hit test result is still a hit test result.
> > 
> > NULL is an empty hit test result. I don't want apps checking all the possible values of a hit test result object and all of the returning NULL.
> 
> Why not add a webkit_hit_test_is_empty method then? The apps can also check if context is 0. I think this is safer than having this value be null.

Because from the API pov a hit test result is never empty, WEBKIT_HIT_TEST_RESULT_CONTEXT_DOCUMENT is always present in context.

> > > I think it makes sense to just extend WebViewTest with another fixture. If you add this the name "UIClientTest" is no longer strictly accurate.
> > 
> > mouseDidMoveOverElement is a callback of the UI Client. 
> 
> You're absolutely right.

Maybe we can move that fixture to a new file TestUIClient if we see it grows too much.

[Carlos Garcia Campos]

Created attachment 126250 [details]
Updated patch

[WebKit Review Bot]

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

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

WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:266:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.h"
Total errors found: 1 in 21 files


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

[Carlos Garcia Campos]

Created attachment 126297 [details]
Updated patch

Renamed the signal as mouse-target-changed as suggested by Martin so that it makes sense to pass empty hit test results to the callback.

[WebKit Review Bot]

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

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

WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.cpp:266:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitHitTestResult.h"
Total errors found: 1 in 21 files


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

[Carlos Garcia Campos]

Committed r107250: <http://trac.webkit.org/changeset/107250>