Bug 15299 – GTK: ChromeClientGtk.cpp does not implement mouseDidMoveOverElement()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Lars Lindner]

Currently the GTK widget implementation does not yet implement the mouseDidMoveOverElement() which as I understand it should generate the "hovering_over_link" signal so that the embedder can react on it.

[Lars Lindner]

Created attachment 16418 [details]
Possible implementation for ChromeClient::mouseDidMoveOverElement()

This patch is not complete as it does not cover "unhovering". I'd like to enhance it further to cover this issue too, but from the given GTK API code it's not clear to me which is the intended behaviour.

Because there is no "unhovering" signal I'd suggest a second sending of the "hovering_over_link" signal with an empty or NULL URL passed to tell the embedder that the mouse has left the link again...

Another issue with the current GTK API is that it does not indicate which parameter of "hovering_over_link" is which. The supplied patch assumes that the link title is the first and the link URL is the second.

[Adam Roben (:aroben)]

Comment on attachment 16418 [details]
Possible implementation for ChromeClient::mouseDidMoveOverElement()

I think it would be nicer to return early if url.isEmpty() is true instead of nesting the entire function inside the if.

Your patch contains tabs, which will be disallowed by our pre-commit hook. Please use indents of 4 spaces instead of tabs. See <http://webkit.org/coding/coding-style.html>

Your patch needs a ChangeLog. See <http://webkit.org/coding/contributing.html> for how to create one.

Thanks for the patch!

[Lars Lindner]

Created attachment 16459 [details]
Possible implementation for ChromeClient::mouseDidMoveOverElement()

Revision of previous patch that fixes the wrong indention and missing Changelog. I also changed the condition so that it now has an else branch that sends the signal with NULL as user data, which I think is a solution to the "unhovering" problem.

[Adam Roben (:aroben)]

Comment on attachment 16459 [details]
Possible implementation for ChromeClient::mouseDidMoveOverElement()

+    } else {
+        g_signal_emit_by_name(m_webPage, "hovering_over_link", NULL, NULL);   
+    }

As specified on <http://webkit.org/coding/coding-style.html>, please omit the braces around the single-line body of the else block. We also use 0 instead of NULL since this is C++ code.

We normally put the bug title and URL in the ChangeLog entry as well.

This looks great, though! Fix up those things and I'll r+ it.

[Lars Lindner]

Created attachment 16461 [details]
Possible implementation for ChromeClient::mouseDidMoveOverElement()

Ok. New revision removes braces from single line else block, replaces NULL with 0 and adds bug title+link to Changelog.

Thanks for the patience and the quick reviews!

[Adam Roben (:aroben)]

Comment on attachment 16461 [details]
Possible implementation for ChromeClient::mouseDidMoveOverElement()

r=me

[Holger Freyther]

Landed in r25818. Thank you!

[Cosimo Cecchi]

The committed patch makes CPU go almost 100% if connecting to the signal, as "hovering_over_link" is fired once for each pixel you move the mouse, which makes this unusable for embedding applications.
I'm not experienced with the codebase, so I can't provide patches, but the signal IMHO should be fired once, with a boolean argument set to TRUE when hovering over a link, and again once, with the argument set to FALSE when unhovering the link.

[Lars Lindner]

Sorry!!! Cosimo Cecchi is right. The patch as submitted is broken. I don't know about correct procedure but reverting sounds good.

I'd like to provide a correction but I have compilation problems on my machine. I assume it is a setup problem, but don't know how to solve it. Maybe someone can help. My compilation fails with TCSystemAlloc.cpp:

../../JavaScriptCore/wtf/TCSystemAlloc.cpp:89: error: expected constructor, destructor, or type conversion before ‘(’ token
../../JavaScriptCore/wtf/TCSystemAlloc.cpp:92: error: expected constructor, destructor, or type conversion before ‘(’ token
../../JavaScriptCore/wtf/TCSystemAlloc.cpp: In function ‘void* TryDevMem(size_t, size_t)’:
../../JavaScriptCore/wtf/TCSystemAlloc.cpp:240: error: ‘FLAGS_malloc_devmem_start’ was not declared in this scope
../../JavaScriptCore/wtf/TCSystemAlloc.cpp:252: error: ‘FLAGS_malloc_devmem_start’ was not declared in this scope
../../JavaScriptCore/wtf/TCSystemAlloc.cpp:253: error: ‘FLAGS_malloc_devmem_limit’ was not declared in this scope
[...]

I also see a lot of

### icu-config: Can't find /usr/lib32/libicuuc.so - ICU prefix is wrong.
###      Try the --prefix= or --exec-prefix= options 
### icu-config: Exitting.

during the compilation. Which confuses me a bit because I run on AMD64.

[Lars Lindner]

Created attachment 16491 [details]
Patch avoiding unnecessary signal generation

Cosimo: Could you please try again using this patch (applies against r25818)? After retesting the old one I found it caused 15% CPU load for me (when continuously moving the mouse) and this new one lowers this to 3-5%.

BTW I solved my compilation problem by copying the 64bit libicu* libraries to /usr/lib32.

[Cosimo Cecchi]

Created attachment 16500 [details]
proposed fix using boolean property

Attached patch is similar to last one posted by Lars, but uses boolean property to set visibility of the link uri. Works fine for me on my WebKit installation.

[Mark Rowe (bdash)]

Reopening due to issues mentioned in comment 8.

[Mark Rowe (bdash)]

Comment on attachment 16500 [details]
proposed fix using boolean property

Marking review- after a discussion with Cosimo in #webkit.  The API change is going to be split out into a separate patch, and the bug fix updated to following the style guidelines.

[Cosimo Cecchi]

Created attachment 16502 [details]
proposed fix

Fixed patch.

[Mark Rowe (bdash)]

Comment on attachment 16502 [details]
proposed fix

r=me.  Two tabs have snuck into this patch, so whomever lands it will need to fix those first.

[Holger Freyther]

Landed in r26020.

[Jan Alonzo]

Created attachment 16542 [details]
Fix signal not emitted when hovering on links in succession

Hi! There is still a problem with the previous patch where the signal doesn't get emitted if you hover on links in succession (i.e. you hover from one link to another). The attached patch should fix this issue.

[Holger Freyther]

Comment on attachment 16502 [details]
proposed fix

Clearing the review flag....

[Holger Freyther]

Reopening the bug. I wonder why I was too shy to ask this question what will happen if two links are next to each other.

[Holger Freyther]

(In reply to comment #17)
> Created an attachment (id=16542) [edit]
> Fix signal not emitted when hovering on links in succession

+    , m_hoveredLink(0)

KURL will be automatically initialized. So this is not needed/wanted. I wonder what will happen in the following case:

Currently you get a signal with NULL, NULL when leaving a link, when you switch from one to another URL you will not get such a signal. Is that a semantic issue? Do we at least want to document this? I also wonder if we need m_didSendLinkSignal at all. We can just compare the URLs.

[Jan Alonzo]

Created attachment 16569 [details]
Fix signal not emitted when hovering on links in succession - update

> (In reply to comment #17)
> +    , m_hoveredLink(0)
> 
> KURL will be automatically initialized. So this is not needed/wanted. I wonder
> what will happen in the following case:

Fixed in the updated patch.

> Currently you get a signal with NULL, NULL when leaving a link, when you switch
> from one to another URL you will not get such a signal. Is that a semantic
> issue? Do we at least want to document this?

As long as we document it, it shouldn't be an issue. I thought the signal was just a hint that we hovered out on a link. Any suggestions?

> I also wonder if we need
> m_didSendLinkSignal at all. We can just compare the URLs.

The updated patch removes m_didSendLinkSignal and just checks for differences between current and previously hovered link.

[Adam Roben (:aroben)]

Comment on attachment 16569 [details]
Fix signal not emitted when hovering on links in succession - update

+        if (!url.isEmpty() && (url != m_hoveredLink || m_hoveredLink.isEmpty())) {

The m_hoveredLink.isEmpty() part of this check is superfluous. If m_hoveredLink.isEmpty() will return true, then so will url != m_hoveredLink (since we already checked !url.isEmpty()).

+        m_hoveredLink = "";

It would be better to say:

m_hoveredLink = KURL();

I think it would be good to rename m_hoveredLink to m_hoveredLinkURL.

r- so we can get that redundant check in the if statement out of here.

[Jan Alonzo]

Created attachment 16572 [details]
Fix signal not emitted when hovering on links in succession - update2

(In reply to comment #22)
> m_hoveredLink.isEmpty())) {
> 
> The m_hoveredLink.isEmpty() part of this check is superfluous. If
> m_hoveredLink.isEmpty() will return true, then so will url != m_hoveredLink
> (since we already checked !url.isEmpty()).

Hi! Yeah you're right. Fixed in the updated patch.

> +        m_hoveredLink = "";
> 
> It would be better to say:
> 
> m_hoveredLink = KURL();

Fixed.
 
> I think it would be good to rename m_hoveredLink to m_hoveredLinkURL.

Fixed.

> r- so we can get that redundant check in the if statement out of here.

Done. Thank you.

[Adam Roben (:aroben)]

Comment on attachment 16572 [details]
Fix signal not emitted when hovering on links in succession - update2

r=me. Thanks for fixing it up!

[Mark Rowe (bdash)]

Landed in r26590.