Bug 57921

Created attachment 88370 [details] Patch

Comment on attachment 88370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88370&action=review Ugh... so each port implements a part of EventHandler? That's so ugly. We'd definitely need to do a lot of refactoring here. r=me provided you revert the change to make subframeForHitTestResult a member of EventHandler or explain why that's needed. > Source/WebCore/page/EventHandler.cpp:1099 > -Frame* subframeForHitTestResult(const MouseEventWithHitTestResults& hitTestResult) > +Frame* EventHandler::subframeForHitTestResult(const MouseEventWithHitTestResults& hitTestResult) Why does this function need to be a member of EventHandler? > Source/WebCore/page/EventHandler.h:271 > + static Frame* subframeForHitTestResult(const MouseEventWithHitTestResults&); Again, I don't think this function needs to be a member function.

(In reply to comment #2) > (From update of attachment 88370 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88370&action=review > > Ugh... so each port implements a part of EventHandler? That's so ugly. We'd definitely need to do a lot of refactoring here. r=me provided you revert the change to make subframeForHitTestResult a member of EventHandler or explain why that's needed. > > > Source/WebCore/page/EventHandler.cpp:1099 > > -Frame* subframeForHitTestResult(const MouseEventWithHitTestResults& hitTestResult) > > +Frame* EventHandler::subframeForHitTestResult(const MouseEventWithHitTestResults& hitTestResult) > > Why does this function need to be a member of EventHandler? So that it can access the private member targetNode() function. > > Source/WebCore/page/EventHandler.h:271 > > + static Frame* subframeForHitTestResult(const MouseEventWithHitTestResults&); > > Again, I don't think this function needs to be a member function. As above.

(In reply to comment #3) > (In reply to comment #2) > > > Source/WebCore/page/EventHandler.cpp:1099 > > > -Frame* subframeForHitTestResult(const MouseEventWithHitTestResults& hitTestResult) > > > +Frame* EventHandler::subframeForHitTestResult(const MouseEventWithHitTestResults& hitTestResult) > > > > Why does this function need to be a member of EventHandler? > > So that it can access the private member targetNode() function. Okay, that makes sense.

Created attachment 88378 [details] Patch

Attachment 88378 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8344110

The commit-queue encountered the following flaky tests while processing attachment 88378 [details]: security/block-test.html bug 55741 (authors: beidson@apple.com, mrowe@apple.com, and sam@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 88378 [details] Patch Rejecting attachment 88378 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 1 Last 500 characters of output: 74cbc631664ef88 r83034 = 567fc0865ad481eb5c6b83bd71522332eca82b49 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc A LayoutTests/svg/text/tspan-getComputedTextLength.svg A LayoutTests/svg/text/tspan-getComputedTextLength-expected.txt M LayoutTests/ChangeLog r83035 = 2b0df32696eedac166f4d1a38770490a906d0d6c (refs/remotes/origin/master) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://queues.webkit.org/results/8346119

Created attachment 88400 [details] Patch

Attachment 88400 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8346162

Comment on attachment 88400 [details] Patch r- seems like you aren't updating event handler for all ports.

Created attachment 88585 [details] Patch

Comment on attachment 88585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88585&action=review > Source/WebCore/page/EventHandler.cpp:1304 > +Node* EventHandler::targetNode(const MouseEventWithHitTestResults& event) > +{ > + return targetNode(event.hitTestResult()); > +} > + > +Node* EventHandler::targetNode(const HitTestResult& hitTestResult) Oh my second thought, targetNode isn't really a descriptive name. How about nodePreferringAttached or attachedNodeOrInnerNode?

Comment on attachment 88585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88585&action=review >> Source/WebCore/page/EventHandler.cpp:1304 >> +Node* EventHandler::targetNode(const HitTestResult& hitTestResult) > > Oh my second thought, targetNode isn't really a descriptive name. How about nodePreferringAttached or attachedNodeOrInnerNode? For now, I'd say r+ since I can't come up with a better name. > Source/WebCore/page/EventHandler.cpp:1314 > + Element* element = node->parentElement(); > + if (element && element->inDocument()) > + return element; What if parent element was also detached? I don't think this covers all the cases. > Source/WebCore/page/EventHandler.cpp:1316 > + return node; It seems really odd to "prefer" attached node yet we also return unattached innerNode.

Comment on attachment 88585 [details] Patch Clearing flags on attachment: 88585 Committed r83153: <http://trac.webkit.org/changeset/83153>

All reviewed patches have been landed. Closing bug.