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.
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.
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!
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.
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.
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!
Comment on attachment 16461 [details] Possible implementation for ChromeClient::mouseDidMoveOverElement() r=me
Landed in r25818. Thank you!
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.
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.
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.
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.
Reopening due to issues mentioned in comment 8.
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.
Created attachment 16502 [details] proposed fix Fixed patch.
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.
Landed in r26020.
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.
Comment on attachment 16502 [details] proposed fix Clearing the review flag....
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.
(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.
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.
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.
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.
Comment on attachment 16572 [details] Fix signal not emitted when hovering on links in succession - update2 r=me. Thanks for fixing it up!
Landed in r26590.