| Summary: | Refactoring: move EventHandler::targetNode into HitTestResult | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Iain Merrick <husky> | ||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, aboxhall, allan.jensen, dglazkov, eric, husky, rniwa, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Iain Merrick
2012-08-15 07:17:55 PDT
Created attachment 158566 [details]
Patch
Unfortunately had to modify lots of platform-specific files. Relying on EWS to catch any mistakes. There are several places where we call targetNode() repeatedly; seems like it'd be worth calling it once and caching the result. I have *not* done so in this patch but let me know if you'd like me to. Comment on attachment 158566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158566&action=review > Source/WebCore/rendering/HitTestResult.cpp:808 > + Node* node = innerNode(); > + if (!node) > + return 0; > + if (node->inDocument()) > + return node; > + > + Element* element = node->parentElement(); > + if (element && element->inDocument()) > + return element; I'm trying to understanding how this is different from innerNode. In the context of an event, it makes sense to speak about the targetNode of an event, but outside the context of an event the word "target" doesn't mean that much. Specifically, how can a node not be inDocument but its parentElement be inDocument? > Source/WebCore/rendering/HitTestResult.h:180 > + Node* targetNode() const; I would move this function next to innerNode() so that it's clear that there is a choice between the two. Comment on attachment 158566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158566&action=review >> Source/WebCore/rendering/HitTestResult.cpp:808 >> + return element; > > I'm trying to understanding how this is different from innerNode. In the context of an event, it makes sense to speak about the targetNode of an event, but outside the context of an event the word "target" doesn't mean that much. > > Specifically, how can a node not be inDocument but its parentElement be inDocument? That's a great question, and I don't know! I'm trying to understand this code by teasing apart the bits that look unnecessarily tangled-up. Does inDocument() refer to the shadow DOM? >> Source/WebCore/rendering/HitTestResult.h:180 >> + Node* targetNode() const; > > I would move this function next to innerNode() so that it's clear that there is a choice between the two. Good idea, will do. (In reply to comment #4) > (From update of attachment 158566 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158566&action=review > > >> Source/WebCore/rendering/HitTestResult.cpp:808 > >> + return element; > > > > I'm trying to understanding how this is different from innerNode. In the context of an event, it makes sense to speak about the targetNode of an event, but outside the context of an event the word "target" doesn't mean that much. > > > > Specifically, how can a node not be inDocument but its parentElement be inDocument? > > That's a great question, and I don't know! I'm trying to understand this code by teasing apart the bits that look unnecessarily tangled-up. > > Does inDocument() refer to the shadow DOM? No. It's a bit on a node that tracks whether a node is in a document tree (shadow DOM elements are considered in-document for this purpose). It answer the question: is the node floating in space, or can you reach the document node by traversing its ancestor? > > >> Source/WebCore/rendering/HitTestResult.h:180 > >> + Node* targetNode() const; > > > > I would move this function next to innerNode() so that it's clear that there is a choice between the two. > > Good idea, will do. Comment on attachment 158566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158566&action=review >>>> Source/WebCore/rendering/HitTestResult.cpp:808 >>>> + return element; >>> >>> I'm trying to understanding how this is different from innerNode. In the context of an event, it makes sense to speak about the targetNode of an event, but outside the context of an event the word "target" doesn't mean that much. >>> >>> Specifically, how can a node not be inDocument but its parentElement be inDocument? >> >> That's a great question, and I don't know! I'm trying to understand this code by teasing apart the bits that look unnecessarily tangled-up. >> >> Does inDocument() refer to the shadow DOM? > > No. It's a bit on a node that tracks whether a node is in a document tree (shadow DOM elements are considered in-document for this purpose). It answer the question: is the node floating in space, or can you reach the document node by traversing its ancestor? Ah, the difference is that the innerNode can be a non-Element for hit-testing purposes. For events, it has to be an element. Comment on attachment 158566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158566&action=review >>>>> Source/WebCore/rendering/HitTestResult.cpp:808 >>>>> + return element; >>>> >>>> I'm trying to understanding how this is different from innerNode. In the context of an event, it makes sense to speak about the targetNode of an event, but outside the context of an event the word "target" doesn't mean that much. >>>> >>>> Specifically, how can a node not be inDocument but its parentElement be inDocument? >>> >>> That's a great question, and I don't know! I'm trying to understand this code by teasing apart the bits that look unnecessarily tangled-up. >>> >>> Does inDocument() refer to the shadow DOM? >> >> No. It's a bit on a node that tracks whether a node is in a document tree (shadow DOM elements are considered in-document for this purpose). It answer the question: is the node floating in space, or can you reach the document node by traversing its ancestor? > > Ah, the difference is that the innerNode can be a non-Element for hit-testing purposes. For events, it has to be an element. I don't know why we're checking for element->inDocument there though. I think we should just zap that. (In reply to comment #7) > > Ah, the difference is that the innerNode can be a non-Element for hit-testing purposes. For events, it has to be an element. Could we give this a better name, then? innerElement? > I don't know why we're checking for element->inDocument there though. I think we should just zap that. So just "return element ? element : node;"? Comment on attachment 158566 [details]
Patch
I am actually just totally confused about what this code does. Let me dig up some history...
(In reply to comment #9) > (From update of attachment 158566 [details]) > I am actually just totally confused about what this code does. Let me dig up some history... Glad it's not just me. Thanks for your patience. :) First off, it looks like you're reverting the change that Alice made: https://bugs.webkit.org/show_bug.cgi?id=57921 The archeology sheds no light. I think we should do this: 1) work on this patch 2) file a bug to refurbish targetNode function and fix it. Also -- why exactly do you need targetNode? (In reply to comment #11) > First off, it looks like you're reverting the change that Alice made: https://bugs.webkit.org/show_bug.cgi?id=57921 Hmm, true. I'd argue that this is a different way of doing the same thing (moving the method from MouseEventWithHitTestResults -> HitTestResult, instead of MouseEventWithHitTestResults -> EventHandler); and now that WebFrameImpl is calling this same method, HitTestResult is a more natural location. Please feel free to push back if you think this refactoring isn't worthwhile. But it looks to me (coming to this code fresh and not fully understanding it yet) like EventHandler has accumulated some cruft. (In reply to comment #12) > The archeology sheds no light. I think we should do this: > > 1) work on this patch > 2) file a bug to refurbish targetNode function and fix it. > > Also -- why exactly do you need targetNode? My main focus here is unforking some Android-specific relating to text selection -- see https://bugs.webkit.org/show_bug.cgi?id=93998 I figured it was worth trying to clean up the campground a little at the same time. (Our Android changes are a bit hacky and need cleaning up anyway.) (In reply to comment #13) > (In reply to comment #11) > > First off, it looks like you're reverting the change that Alice made: https://bugs.webkit.org/show_bug.cgi?id=57921 > > Hmm, true. I'd argue that this is a different way of doing the same thing (moving the method from MouseEventWithHitTestResults -> HitTestResult, instead of MouseEventWithHitTestResults -> EventHandler); and now that WebFrameImpl is calling this same method, HitTestResult is a more natural location. > > Please feel free to push back if you think this refactoring isn't worthwhile. But it looks to me (coming to this code fresh and not fully understanding it yet) like EventHandler has accumulated some cruft. No, I think this makes sense. Comment on attachment 158566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158566&action=review I am ok with this. Ryosuke, any comments before we land? > Source/WebCore/rendering/HitTestResult.cpp:798 > +Node* HitTestResult::targetNode() const Let's file a separate bug to refurbish this. I don't think "targetNode" is a descriptive name. The whole point of this function is to preferring an attached node when the inner node is detached from the document. To make it even more ad-hoc, it only moves up one level in the tree. The function name should at least reflect that. Comment on attachment 158566 [details] Patch Clearing flags on attachment: 158566 Committed r125715: <http://trac.webkit.org/changeset/125715> All reviewed patches have been landed. Closing bug. |