Bug 113037 - removeAllEventListeners() should be called to shadow trees
Summary: removeAllEventListeners() should be called to shadow trees
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-22 03:17 PDT by Hajime Morrita
Modified: 2013-03-26 10:25 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.99 KB, patch)
2013-03-22 03:45 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (7.17 KB, patch)
2013-03-26 02:39 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2013-03-22 03:17:20 PDT
This is a followup of Bug 113035.
Comment 1 Hajime Morrita 2013-03-22 03:45:25 PDT
Created attachment 194499 [details]
Patch
Comment 2 Mike West 2013-03-22 03:57:44 PDT
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. :)
Comment 3 Hajime Morrita 2013-03-22 04:11:41 PDT
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.
Comment 4 Hajime Morrita 2013-03-26 02:39:24 PDT
Created attachment 195042 [details]
Patch
Comment 5 WebKit Review Bot 2013-03-26 08:09:31 PDT
Comment on attachment 195042 [details]
Patch

Clearing flags on attachment: 195042

Committed r146882: <http://trac.webkit.org/changeset/146882>
Comment 6 WebKit Review Bot 2013-03-26 08:09:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Ryosuke Niwa 2013-03-26 10:25:15 PDT
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.