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

Wed Feb 8 23:27:03 PST 2012

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

--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-02-08 23:27:03 PST ---
(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.

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