Bug 49982 – Change EventHandler::dispatchMouseEvent code to use DOM traversal instead of render tree traversal

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dimitri Glazkov (Google)]

Per my comment introduced in bug 49977, we should use DOM traversal in discovering first mouse-focusable node, rather than render tree.

[Emil A Eklund]

Created attachment 76122 [details]
Patch

[Adam Barth]

Comment on attachment 76122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76122&action=review

> WebCore/ChangeLog:12
> +	Walk up DOM/hosted tree rather than render tree.

We can't land files with tabs in them.

> WebCore/page/EventHandler.cpp:1869
>                      && m_frame->selection()->toNormalizedRange()->compareNode(n, ec) == Range::NODE_INSIDE

Can this run JavaScript?

> WebCore/page/EventHandler.cpp:1875
> +            node = node->parentOrHostNode();

Are we holding references during this walk?

[Darin Adler]

Comment on attachment 76122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76122&action=review

> WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Need to remove this or replace it with an explanation of why there are no new tests. The tools won’t allow this to be checked in with OOPS in it, and unlike the Reviewed by line, the tools won’t fill in the value for us.

> WebCore/ChangeLog:12
> +	Walk up DOM/hosted tree rather than render tree.

Tab here. Won’t be able to land a patch with a tab in it.

> WebCore/page/EventHandler.cpp:1861
> +        while (node) {
>              if (node && node->isMouseFocusable()) {

No need to check node for null twice in a row here.

[WebKit Review Bot]

Attachment 76122 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/page/EventHandler.cpp']" exit_code: 1
WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 76122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76122&action=review

>> WebCore/page/EventHandler.cpp:1875
>> +            node = node->parentOrHostNode();
> 
> Are we holding references during this walk?

Adam’s point is a good one. It’s incorrect to use the node pointer after calling JavaScript if we didn’t hold a reference to it. But that bug is not new to the new code. I can’t tell from here what is done with the node after we call break. It would be easier to read if we separated the loop searching for a mouse-focusable node from the work we do on that node.

[Emil A Eklund]

Created attachment 76125 [details]
Patch

[Emil A Eklund]

Adam,

> > WebCore/ChangeLog:12
> > +	Walk up DOM/hosted tree rather than render tree.
>
> We can't land files with tabs in them.

My bad, fixed.


> > WebCore/page/EventHandler.cpp:1869
> >                      && m_frame->selection()->toNormalizedRange()->
> >compareNode(n, ec) == Range::NODE_INSIDE
>
> Can this run JavaScript?

As far as I can tell it can not. It does update the render tree though.


> > WebCore/page/EventHandler.cpp:1875
> > +            node = node->parentOrHostNode();
>
> Are we holding references during this walk?

No, I don't see how they could become invalidated during the execution though as no dom or js interactions take place.

[Eric Seidel (no email)]

Comment on attachment 76125 [details]
Patch

Seems sane.

[WebKit Commit Bot]

Comment on attachment 76125 [details]
Patch

Clearing flags on attachment: 76125

Committed r73717: <http://trac.webkit.org/changeset/73717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.