Bug 94107 – Refactoring: move EventHandler::targetNode into HitTestResult

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Iain Merrick]

Refactoring: move EventHandler::targetNode into HitTestResult

[Iain Merrick]

Created attachment 158566 [details]
Patch

[Iain Merrick]

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.

[Adam Barth]

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.

[Iain Merrick]

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.

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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.

[Iain Merrick]

(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;"?

[Dimitri Glazkov (Google)]

Comment on attachment 158566 [details]
Patch

I am actually just totally confused about what this code does. Let me dig up some history...

[Iain Merrick]

(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. :)

[Dimitri Glazkov (Google)]

First off, it looks like you're reverting the change that Alice made: https://bugs.webkit.org/show_bug.cgi?id=57921

[Dimitri Glazkov (Google)]

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?

[Iain Merrick]

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

[Iain Merrick]

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

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

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.

[Ryosuke Niwa]

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.

[WebKit Review Bot]

Comment on attachment 158566 [details]
Patch

Clearing flags on attachment: 158566

Committed r125715: <http://trac.webkit.org/changeset/125715>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.