Implement 'mouseenter' and 'mouseleave' from DOM Level 3 Events.

Implement 'mouseenter' and 'mouseleave' from DOM Level 3 Events.

This patch adds 'mouseenter' and 'mouseleave' as accepted event names in
a variety of places, and does the core of the implementation inside
Document::updateHoverActiveState. Since we're already doing all the
testing there to determine hover state (which 'mouseenter'/'mouseleave'
closely mirrors), that method now accepts a PlatformMouseEvent*
parameter, using it to directly dispatch the new events on relevant
nodes.

A bit of logic has also been added to that method to ensure that we only
generate events when an event handler is listening for them, as
performance is otherwise slightly less than awesome. This optimization
requires the addition of an EventTarget method to determine if capturing
event listeners exist (which requires the addition of an
EventListenerMap method to do the dirty work).

This patch is a lightly modified version of the mouseenter/mouseleave
WebKit patch[1] submitted by allan.jensen@digia.com (carewolf), reviewed
by David Hyatt.

[1]: http://trac.webkit.org/changeset/149173

BUG=236215
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153877

[Mike West, 2013-07-08 10:59:47]

This is a port of carewolf's WebKit patch to implement mouseenter/mouseleave.
It's a pretty straightforward copy/paste. Perhaps you'd like to take a look,
Jochen?

The tests are also copy/pasted (only formatting changes), but I don't
particularly like them. I plan to rewrite them using js-test-pre and some
reasonable assertions, but I think there's some value in copying things over
directly to ensure we have the same behavior as WebKit. I plan to submit
rewritten tests in a subsequent CL, which I'll then submit to WebKit for review.

WDYT?

[jochen (gone - plz use gerrit), 2013-07-08 15:21:22]

On 2013/07/08 10:59:47, Mike West (on paternity leave) wrote:
> This is a port of carewolf's WebKit patch to implement mouseenter/mouseleave.
> It's a pretty straightforward copy/paste. Perhaps you'd like to take a look,
> Jochen?
> 
> The tests are also copy/pasted (only formatting changes), but I don't
> particularly like them. I plan to rewrite them using js-test-pre and some
> reasonable assertions, but I think there's some value in copying things over
> directly to ensure we have the same behavior as WebKit. I plan to submit
> rewritten tests in a subsequent CL, which I'll then submit to WebKit for
review.
> 
> WDYT?

generally looks good

You should wait for the intent to ship thread to come to a conclusion before
landing

[PhistucK, 2013-07-08 17:57:25]

This is exciting.

I only have nits, sorry. :P

https://codereview.chromium.org/18836002/diff/1/LayoutTests/fast/events/mouse...
File LayoutTests/fast/events/mouseenter-mouseleave-capture.html (right):

https://codereview.chromium.org/18836002/diff/1/LayoutTests/fast/events/mouse...
LayoutTests/fast/events/mouseenter-mouseleave-capture.html:19:
document.body.addEventListener('mouseleave',logMouseEvent,true);
Perhaps add a whitespace after every comma?

https://codereview.chromium.org/18836002/diff/1/LayoutTests/fast/events/mouse...
File LayoutTests/fast/events/mouseenter-mouseleave.html (right):

https://codereview.chromium.org/18836002/diff/1/LayoutTests/fast/events/mouse...
LayoutTests/fast/events/mouseenter-mouseleave.html:37:
onMouseOver="logMouseOverOutEvent(event)"
onMouseOut="logMouseOverOutEvent(event)"
Perhaps change all of the attribute names from camelCase to lowercase? it feels
like MSDN. ;)

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:5062: // mouseenter and mouseleave events do not
bubble, so they are dispatched iff there is a capturing
s/iff/if

[esprehn, 2013-07-08 18:13:02]

You can't do the "look for listeners" optimization in one step, since a listener
for "leave" can add a listener for "enter".

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:5126: if (event && (hasCapturingMouseEnterListener
|| nodesToAddToChain[i]->hasEventListeners(eventNames().mouseenterEvent)))
This is not correct, I could add a listener inside the mouseleave handler and
you'd not fire the event because you checked too early.

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/EventListener...
File Source/core/dom/EventListenerMap.cpp (right):

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/EventListener...
Source/core/dom/EventListenerMap.cpp:83: for (unsigned j = 0; j <
vector->size(); ++j) {
const EventListenerVector* vector = find(eventType);
....

you don't need the nested loops, find() can do the work for you.

[Mike West, 2013-07-09 09:35:11]

new tests + esprehn is smart.

[Mike West, 2013-07-09 09:37:00]

Thanks for your comments. I've addressed them in the latest version of the
patch. Would you mind taking a look?

I decided to simply rewrite the tests in this pass; I'll submit them to WebKit
once this lands.

https://codereview.chromium.org/18836002/diff/1/LayoutTests/fast/events/mouse...
File LayoutTests/fast/events/mouseenter-mouseleave.html (right):

https://codereview.chromium.org/18836002/diff/1/LayoutTests/fast/events/mouse...
LayoutTests/fast/events/mouseenter-mouseleave.html:37:
onMouseOver="logMouseOverOutEvent(event)"
onMouseOut="logMouseOverOutEvent(event)"
On 2013/07/08 17:57:26, PhistucK wrote:
> Perhaps change all of the attribute names from camelCase to lowercase? it
feels
> like MSDN. ;)

I've rewritten the tests.

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:5062: // mouseenter and mouseleave events do not
bubble, so they are dispatched iff there is a capturing
On 2013/07/08 17:57:26, PhistucK wrote:
> s/iff/if

"iff" is short for "if and only if" (e.g. "necessary and sufficient"). I guess
that's not a well-known abbreviation. I'll change the text.

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:5126: if (event && (hasCapturingMouseEnterListener
|| nodesToAddToChain[i]->hasEventListeners(eventNames().mouseenterEvent)))
On 2013/07/08 18:13:02, esprehn wrote:
> This is not correct, I could add a listener inside the mouseleave handler and
> you'd not fire the event because you checked too early.

You're right; I didn't think of that case at all. I'll split up the checks, and
add a new test to cover that behavior.

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/EventListener...
File Source/core/dom/EventListenerMap.cpp (right):

https://codereview.chromium.org/18836002/diff/1/Source/core/dom/EventListener...
Source/core/dom/EventListenerMap.cpp:83: for (unsigned j = 0; j <
vector->size(); ++j) {
On 2013/07/08 18:13:02, esprehn wrote:
> const EventListenerVector* vector = find(eventType);
> ....
> 
> you don't need the nested loops, find() can do the work for you.

m_entries is a pair<AtomicString, Vector>. I don't think I can ask the vector to
compare against the first entry in the pair with find().

[PhistucK, 2013-07-09 09:42:33]

>
https://codereview.chromium.org/18836002/diff/1/LayoutTests/fast/events/mouse...
> File LayoutTests/fast/events/mouseenter-mouseleave.html (right):
> 
>
https://codereview.chromium.org/18836002/diff/1/LayoutTests/fast/events/mouse...
> LayoutTests/fast/events/mouseenter-mouseleave.html:37:
> onMouseOver="logMouseOverOutEvent(event)"
> onMouseOut="logMouseOverOutEvent(event)"
> On 2013/07/08 17:57:26, PhistucK wrote:
> > Perhaps change all of the attribute names from camelCase to lowercase? it
> feels
> > like MSDN. ;)
> 
> I've rewritten the tests.

Now we have lost coverage for their HTML attribute counterparts. Was that
unnecessary and intentional?

[esprehn, 2013-07-10 02:13:56]

LGTM, please add attribute tests before landing. It'd be nice if you simplified
containsCapturing() or even contains() too.

I'm not a huge fan of all this stuff inside hover update now, but we can
refactor all that later. This should really be a series of steps, not one huge
function.

https://codereview.chromium.org/18836002/diff/12001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/18836002/diff/12001/Source/core/dom/Document....
Source/core/dom/Document.h:258: DEFINE_ATTRIBUTE_EVENT_LISTENER(mouseleave);
You need tests for this. You had them and then lost it.

https://codereview.chromium.org/18836002/diff/12001/Source/core/dom/EventList...
File Source/core/dom/EventListenerMap.cpp (right):

https://codereview.chromium.org/18836002/diff/12001/Source/core/dom/EventList...
Source/core/dom/EventListenerMap.cpp:87: }
This is identical to:

const EventListenerVector* vector = find(eventType);
for (unsigned j = 0; j < vector->size(); ++j) {
  if (vector[j].useCapture)
     return true;
}
return false;

similarly contains() is identical to:

return find(eventType);

You don't need to simplify contains() in this patch though.

[Mike West, 2013-07-10 06:49:35]

On 2013/07/10 02:13:56, esprehn wrote:
> I'm not a huge fan of all this stuff inside hover update now, but we can
> refactor all that later. This should really be a series of steps, not one huge
> function.

How would you like it to look? I'm happy to put together followup refactorings.
 
> https://codereview.chromium.org/18836002/diff/12001/Source/core/dom/Document.h
> File Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/18836002/diff/12001/Source/core/dom/Document....
> Source/core/dom/Document.h:258: DEFINE_ATTRIBUTE_EVENT_LISTENER(mouseleave);
> You need tests for this. You had them and then lost it.

Added an inline attribute test.

>
https://codereview.chromium.org/18836002/diff/12001/Source/core/dom/EventList...
> Source/core/dom/EventListenerMap.cpp:87: }
> similarly contains() is identical to:
> 
> return find(eventType);

Ah! EventListenerMap::find! Sorry, I was focused on Vector::find. :)

I'll update both of these in a followup CL. Thanks for the tip.

[jochen (gone - plz use gerrit), 2013-07-10 06:59:06]

lgtm

[commit-bot: I haz the power, 2013-07-10 07:37:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/18836002/21001

[commit-bot: I haz the power, 2013-07-10 10:08:35]

Change committed as 153877