Bug 49982

Created attachment 76122 [details] Patch

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 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.

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 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.

Created attachment 76125 [details] Patch

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 on attachment 76125 [details] Patch Seems sane.

Comment on attachment 76125 [details] Patch Clearing flags on attachment: 76125 Committed r73717: <http://trac.webkit.org/changeset/73717>

All reviewed patches have been landed. Closing bug.