Bug 49982

Summary: Change EventHandler::dispatchMouseEvent code to use DOM traversal instead of render tree traversal
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eae, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 49977    
Bug Blocks: 48698    
Attachments:
Description Flags
Patch
none
Patch none

Description Dimitri Glazkov (Google) 2010-11-23 10:23:19 PST
Per my comment introduced in bug 49977, we should use DOM traversal in discovering first mouse-focusable node, rather than render tree.
Comment 1 Emil A Eklund 2010-12-09 14:29:48 PST
Created attachment 76122 [details]
Patch
Comment 2 Adam Barth 2010-12-09 14:32:51 PST
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?
Comment 3 Darin Adler 2010-12-09 14:34:04 PST
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.
Comment 4 WebKit Review Bot 2010-12-09 14:35:43 PST
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.
Comment 5 Darin Adler 2010-12-09 14:36:27 PST
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.
Comment 6 Emil A Eklund 2010-12-09 14:42:11 PST
Created attachment 76125 [details]
Patch
Comment 7 Emil A Eklund 2010-12-09 14:51:12 PST
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.
Comment 8 Eric Seidel (no email) 2010-12-10 00:02:04 PST
Comment on attachment 76125 [details]
Patch

Seems sane.
Comment 9 WebKit Commit Bot 2010-12-10 04:15:04 PST
Comment on attachment 76125 [details]
Patch

Clearing flags on attachment: 76125

Committed r73717: <http://trac.webkit.org/changeset/73717>
Comment 10 WebKit Commit Bot 2010-12-10 04:15:10 PST
All reviewed patches have been landed.  Closing bug.