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

Wed Feb 8 10:43:14 PST 2012

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

--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-02-08 10:43:14 PST ---
(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 :-)

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