Bug 15299 - GTK: ChromeClientGtk.cpp does not implement mouseDidMoveOverElement()
Summary: GTK: ChromeClientGtk.cpp does not implement mouseDidMoveOverElement()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-09-27 12:07 PDT by Lars Lindner
Modified: 2007-10-14 05:08 PDT (History)
4 users (show)

See Also:


Attachments
Possible implementation for ChromeClient::mouseDidMoveOverElement() (1.10 KB, patch)
2007-09-27 12:13 PDT, Lars Lindner
aroben: review-
Details | Formatted Diff | Diff
Possible implementation for ChromeClient::mouseDidMoveOverElement() (1.73 KB, patch)
2007-09-29 13:05 PDT, Lars Lindner
aroben: review-
Details | Formatted Diff | Diff
Possible implementation for ChromeClient::mouseDidMoveOverElement() (1.84 KB, patch)
2007-09-29 16:57 PDT, Lars Lindner
aroben: review+
Details | Formatted Diff | Diff
Patch avoiding unnecessary signal generation (1.67 KB, patch)
2007-10-01 19:38 PDT, Lars Lindner
no flags Details | Formatted Diff | Diff
proposed fix using boolean property (5.30 KB, patch)
2007-10-02 03:29 PDT, Cosimo Cecchi
mrowe: review-
Details | Formatted Diff | Diff
proposed fix (2.64 KB, patch)
2007-10-02 05:33 PDT, Cosimo Cecchi
no flags Details | Formatted Diff | Diff
Fix signal not emitted when hovering on links in succession (2.61 KB, patch)
2007-10-05 05:03 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
Fix signal not emitted when hovering on links in succession - update (2.60 KB, patch)
2007-10-06 16:42 PDT, Jan Alonzo
aroben: review-
Details | Formatted Diff | Diff
Fix signal not emitted when hovering on links in succession - update2 (2.60 KB, patch)
2007-10-07 00:55 PDT, Jan Alonzo
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Lindner 2007-09-27 12:07:38 PDT
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.
Comment 1 Lars Lindner 2007-09-27 12:13:31 PDT
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 2 Adam Roben (:aroben) 2007-09-28 12:06:53 PDT
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!
Comment 3 Lars Lindner 2007-09-29 13:05:57 PDT
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 4 Adam Roben (:aroben) 2007-09-29 15:28:32 PDT
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.
Comment 5 Lars Lindner 2007-09-29 16:57:07 PDT
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 6 Adam Roben (:aroben) 2007-09-29 17:29:10 PDT
Comment on attachment 16461 [details]
Possible implementation for ChromeClient::mouseDidMoveOverElement()

r=me
Comment 7 Holger Freyther 2007-10-01 14:53:37 PDT
Landed in r25818. Thank you!
Comment 8 Cosimo Cecchi 2007-10-01 16:12:08 PDT
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.
Comment 9 Lars Lindner 2007-10-01 18:22:59 PDT
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.
Comment 10 Lars Lindner 2007-10-01 19:38:57 PDT
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.
Comment 11 Cosimo Cecchi 2007-10-02 03:29:07 PDT
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.
Comment 12 Mark Rowe (bdash) 2007-10-02 04:21:44 PDT
Reopening due to issues mentioned in comment 8.
Comment 13 Mark Rowe (bdash) 2007-10-02 04:37:42 PDT
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.
Comment 14 Cosimo Cecchi 2007-10-02 05:33:07 PDT
Created attachment 16502 [details]
proposed fix

Fixed patch.
Comment 15 Mark Rowe (bdash) 2007-10-02 05:36:03 PDT
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.
Comment 16 Holger Freyther 2007-10-03 09:41:13 PDT
Landed in r26020.
Comment 17 Jan Alonzo 2007-10-05 05:03:04 PDT
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 18 Holger Freyther 2007-10-06 02:41:00 PDT
Comment on attachment 16502 [details]
proposed fix

Clearing the review flag....
Comment 19 Holger Freyther 2007-10-06 02:42:41 PDT
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.
Comment 20 Holger Freyther 2007-10-06 02:47:46 PDT
(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.
Comment 21 Jan Alonzo 2007-10-06 16:42:39 PDT
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 22 Adam Roben (:aroben) 2007-10-06 23:26:23 PDT
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.
Comment 23 Jan Alonzo 2007-10-07 00:55:28 PDT
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 24 Adam Roben (:aroben) 2007-10-07 01:28:56 PDT
Comment on attachment 16572 [details]
Fix signal not emitted when hovering on links in succession - update2

r=me. Thanks for fixing it up!
Comment 25 Mark Rowe (bdash) 2007-10-14 05:08:11 PDT
Landed in r26590.