Bug 113037 – removeAllEventListeners() should be called to shadow trees

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

This is a followup of Bug 113035.

[Hajime Morrita]

Created attachment 194499 [details]
Patch

[Mike West]

Comment on attachment 194499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194499&action=review

Two quick drive-by comments:

> Source/WebCore/ChangeLog:11
> +        No new tests. This is rather a defensive change.

If we're not currently clearing event listeners on the shadow tree, it seems like this change will produce some web-visible differences in behavior. It would be good to add a test to ensure that the behavior doesn't regress.

> Source/WebCore/dom/ElementShadow.cpp:126
> +void ElementShadow::removeAllEventListenersFromShadowTrees()

The name seems slightly redundant: I'd suggest sticking to "removeAllEventListeners" here as well, as calling this on an ElementShadow seems to already imply that we'll be removing listeners from shadow-land. :)

[Hajime Morrita]

Comment on attachment 194499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194499&action=review

Thanks for the comment, Mike.

>> Source/WebCore/ChangeLog:11
>> +        No new tests. This is rather a defensive change.
> 
> If we're not currently clearing event listeners on the shadow tree, it seems like this change will produce some web-visible differences in behavior. It would be good to add a test to ensure that the behavior doesn't regress.

Hmm.
Since the clearance happens in the destructor and there is no way to refer event target from the listener,
I have no idea how to do it for now.
i needs fresh brain. Will attack next week.

>> Source/WebCore/dom/ElementShadow.cpp:126
>> +void ElementShadow::removeAllEventListenersFromShadowTrees()
> 
> The name seems slightly redundant: I'd suggest sticking to "removeAllEventListeners" here as well, as calling this on an ElementShadow seems to already imply that we'll be removing listeners from shadow-land. :)

Makes sense. will do.

[Hajime Morrita]

Created attachment 195042 [details]
Patch

[WebKit Review Bot]

Comment on attachment 195042 [details]
Patch

Clearing flags on attachment: 195042

Committed r146882: <http://trac.webkit.org/changeset/146882>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Comment on attachment 195042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195042&action=review

> Source/WebCore/ChangeLog:3
> +        remoeveAllEventListeners() should be called to shadow trees

Nit: remoeve.