Detach Event Listener Properties before moving Node.

Detach Event Listener Properties before moving Node.

In preparation for storing EventListenerProperties on a per-frame basis,
this CL makes sure old properties are cleared before moving the node
iterators between the frameHosts.

BUG=

Review-Url: https://codereview.chromium.org/2585353002
Cr-Commit-Position: refs/heads/master@{#443920}
Committed: https://chromium.googlesource.com/chromium/src/+/95a1418e25fe0ef90aa7eced41f73e3e7db1f22c

[wjmaclean, 2016-12-19 18:42:18]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 18:42:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 20:45:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 20:45:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wjmaclean, 2016-12-19 20:52:40]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 20:53:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 22:13:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 22:13:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wjmaclean, 2017-01-03 19:26:00]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 19:26:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 20:43:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 20:43:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wjmaclean, 2017-01-04 16:04:12]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 16:04:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2017-01-04 17:43:25]

Description was changed from

==========
Detach Event Listener Properties before moving Node.

In preparation for storing EventListenerProperties on a per-frame basis,
this CL makes sure old properties are cleared before moving the node
iterators between the frameHosts.

BUG=
==========

to

==========
Detach Event Listener Properties before moving Node.

In preparation for storing EventListenerProperties on a per-frame basis,
this CL makes sure old properties are cleared before moving the node
iterators between the frameHosts.

BUG=
==========

[wjmaclean, 2017-01-04 17:43:25]

wjmaclean@chromium.org changed reviewers:
+ bokan@chromium.org

[wjmaclean, 2017-01-04 17:44:36]

bokan@ - this is the last of the CLs we started before the holidays. (I'll
remove/reword the TODO statement before landing ... rewording suggestions
welcome!)

[commit-bot: I haz the power, 2017-01-04 18:49:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 18:49:31]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-01-04 23:33:26]

bokan@chromium.org changed reviewers:
+ dtapuska@chromium.org

[bokan, 2017-01-04 23:33:27]

looks like a nice clean up overall. I'm just a little confused as to why we need
to pass an oldDocument rather than using Node::document. If you can find out why
and either document it prominently, or better yet, remove the oldDocument param
I'll stamp.

+dtapuska as FYI

https://codereview.chromium.org/2585353002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp (right):

https://codereview.chromium.org/2585353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp:140: // oldDocument in
the call to node.willMoveToNewDocument().
This was the first question I had above, why can't we just use node.document()
since "willMove" implies it hasn't moved yet. The fact that we pass in an
|oldDocument| to this method on TreeScopeAdapter seems suspect to me.

[wjmaclean, 2017-01-04 23:41:30]

On 2017/01/04 23:33:27, bokan wrote:
> looks like a nice clean up overall. I'm just a little confused as to why we
need
> to pass an oldDocument rather than using Node::document. If you can find out
why
> and either document it prominently, or better yet, remove the oldDocument
param
> I'll stamp.
> 
> +dtapuska as FYI
> 
>
https://codereview.chromium.org/2585353002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp (right):
> 
>
https://codereview.chromium.org/2585353002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp:140: // oldDocument in
> the call to node.willMoveToNewDocument().
> This was the first question I had above, why can't we just use node.document()
> since "willMove" implies it hasn't moved yet. The fact that we pass in an
> |oldDocument| to this method on TreeScopeAdapter seems suspect to me.

I've sent tkent@ and dominicc@ an e-mai lto try and get some more insight.
However, I have observed (in tests at least) that oldDocument and
node.document() (prior to the move) do indeed differ at times. I'll update here
once I know more.

[wjmaclean, 2017-01-04 23:52:49]

On 2017/01/04 23:41:30, wjmaclean wrote:
> 
> I've sent tkent@ and dominicc@ an e-mai lto try and get some more insight.
> However, I have observed (in tests at least) that oldDocument and
> node.document() (prior to the move) do indeed differ at times. I'll update
here
> once I know more.

I suppose if it *was* safe to just use node.document(), then the function
TreeScopeAdopter::moveNodeToNewDocument() wouldn't need oldDocument as an input
parameter ...

[bokan, 2017-01-05 14:08:44]

On 2017/01/04 23:52:49, wjmaclean wrote:
> On 2017/01/04 23:41:30, wjmaclean wrote:
> > 
> > I've sent tkent@ and dominicc@ an e-mai lto try and get some more insight.
> > However, I have observed (in tests at least) that oldDocument and
> > node.document() (prior to the move) do indeed differ at times. I'll update
> here
> > once I know more.
> 
> I suppose if it *was* safe to just use node.document(), then the function
> TreeScopeAdopter::moveNodeToNewDocument() wouldn't need oldDocument as an
input
> parameter ...

[bokan, 2017-01-05 14:09:26]

On 2017/01/04 23:52:49, wjmaclean wrote:
> On 2017/01/04 23:41:30, wjmaclean wrote:
> > 
> > I've sent tkent@ and dominicc@ an e-mai lto try and get some more insight.
> > However, I have observed (in tests at least) that oldDocument and
> > node.document() (prior to the move) do indeed differ at times. I'll update
> here
> > once I know more.
> 
> I suppose if it *was* safe to just use node.document(), then the function
> TreeScopeAdopter::moveNodeToNewDocument() wouldn't need oldDocument as an
input
> parameter ...

Right, I'm just confused as to why that's the case and if we could clarify the
reason in code.

[wjmaclean, 2017-01-05 14:19:28]

On 2017/01/05 14:09:26, bokan wrote:
> 
> Right, I'm just confused as to why that's the case and if we could clarify the
> reason in code.

Indeed ... me too. I'll update when I have an answer.

[wjmaclean, 2017-01-12 15:26:14]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 15:26:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2017-01-12 16:35:59]

On 2017/01/05 14:19:28, wjmaclean wrote:
> On 2017/01/05 14:09:26, bokan wrote:
> > 
> > Right, I'm just confused as to why that's the case and if we could clarify
the
> > reason in code.
> 
> Indeed ... me too. I'll update when I have an answer.

So apparently when this function is called, the node's document() has already
been changed (despite the function's name ...).

In order to keep node.willMoveToNewDocument() parameter-less, i.e. using
node.document(), I've changed the call location in this latest patch.

[commit-bot: I haz the power, 2017-01-12 16:48:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 16:48:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2017-01-12 16:54:48]

lgtm % comments

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:1812: if (document().frameHost())
Could we pass in the newDocument and only do this if the frameHost is different?

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:1828: if (document().frameHost())
And to match the above, add a check that we're moving to a different frameHost

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp (right):

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp:55: // Before
node.document() changes, allow the node to do pre-move changes.
Please add a comment in TreeScopeAdopter::moveNodeToNewDocument that
node.document() may already have been changed and that's why oldDocument is
passed in.

[wjmaclean, 2017-01-13 17:36:26]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 17:36:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2017-01-13 17:40:30]

I've reverted to calling node.willMoveToNewDocument inside of
TreeScopeAdopter::moveNodeToNewDocument() due to the multiple call sites for the
latter function, and in particular the differences in how the tree scope is
updated for the shadow dom nodes. I think it's safe and easier to understand
this way.

I think I've incorporated your other comments.

Let me know if this is still-l-g-t-m.

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:1812: if (document().frameHost())
On 2017/01/12 16:54:48, bokan wrote:
> Could we pass in the newDocument and only do this if the frameHost is
different?

Done.

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:1828: if (document().frameHost())
On 2017/01/12 16:54:48, bokan wrote:
> And to match the above, add a check that we're moving to a different frameHost

Done.

Let me know if a DCHECK is what you had intended here.

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp (right):

https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp:55: // Before
node.document() changes, allow the node to do pre-move changes.
On 2017/01/12 16:54:48, bokan wrote:
> Please add a comment in TreeScopeAdopter::moveNodeToNewDocument that
> node.document() may already have been changed and that's why oldDocument is
> passed in.

Done.

[dtapuska, 2017-01-13 18:24:27]

On 2017/01/13 17:40:30, wjmaclean wrote:
> I've reverted to calling node.willMoveToNewDocument inside of
> TreeScopeAdopter::moveNodeToNewDocument() due to the multiple call sites for
the
> latter function, and in particular the differences in how the tree scope is
> updated for the shadow dom nodes. I think it's safe and easier to understand
> this way.
> 
> I think I've incorporated your other comments.
> 
> Let me know if this is still-l-g-t-m.
> 
>
https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Node.cpp:1812: if (document().frameHost())
> On 2017/01/12 16:54:48, bokan wrote:
> > Could we pass in the newDocument and only do this if the frameHost is
> different?
> 
> Done.
> 
>
https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Node.cpp:1828: if (document().frameHost())
> On 2017/01/12 16:54:48, bokan wrote:
> > And to match the above, add a check that we're moving to a different
frameHost
> 
> Done.
> 
> Let me know if a DCHECK is what you had intended here.
> 
>
https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp (right):
> 
>
https://codereview.chromium.org/2585353002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/TreeScopeAdopter.cpp:55: // Before
> node.document() changes, allow the node to do pre-move changes.
> On 2017/01/12 16:54:48, bokan wrote:
> > Please add a comment in TreeScopeAdopter::moveNodeToNewDocument that
> > node.document() may already have been changed and that's why oldDocument is
> > passed in.
> 
> Done.

lgtm

[bokan, 2017-01-13 18:35:10]

https://codereview.chromium.org/2585353002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2585353002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:1834:
DCHECK_NE(document().frameHost(), oldDocument.frameHost());
Can we assume this in general? What if you move between documents in the same
FrameHost? My comment was about avoiding the whole removing and adding to
eventHandlerRegistry if it's not going to change.

[commit-bot: I haz the power, 2017-01-13 18:54:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 18:54:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wjmaclean, 2017-01-13 20:04:24]

https://codereview.chromium.org/2585353002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2585353002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:1834:
DCHECK_NE(document().frameHost(), oldDocument.frameHost());
On 2017/01/13 18:35:10, bokan wrote:
> Can we assume this in general? What if you move between documents in the same
> FrameHost? My comment was about avoiding the whole removing and adding to
> eventHandlerRegistry if it's not going to change.

Ha ha ... apparently not according to the bots!

But now I better understand what you intended, so I'm running a revised patch on
the bots.

[wjmaclean, 2017-01-13 20:04:32]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 20:05:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 21:57:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 21:57:13]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-01-16 16:19:16]

still lgtm

[wjmaclean, 2017-01-16 16:20:04]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2017-01-16 16:20:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2585353002/#ps80001
(title: "Use more sensible checks.")

[commit-bot: I haz the power, 2017-01-16 16:20:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 18:06:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484583604536740,
"parent_rev": "6ab84d52733cf4fd32e2827600b52c7cd84b101c", "commit_rev":
"95a1418e25fe0ef90aa7eced41f73e3e7db1f22c"}

[commit-bot: I haz the power, 2017-01-16 18:07:05]

Description was changed from

==========
Detach Event Listener Properties before moving Node.

In preparation for storing EventListenerProperties on a per-frame basis,
this CL makes sure old properties are cleared before moving the node
iterators between the frameHosts.

BUG=
==========

to

==========
Detach Event Listener Properties before moving Node.

In preparation for storing EventListenerProperties on a per-frame basis,
this CL makes sure old properties are cleared before moving the node
iterators between the frameHosts.

BUG=

Review-Url: https://codereview.chromium.org/2585353002
Cr-Commit-Position: refs/heads/master@{#443920}
Committed:
https://chromium.googlesource.com/chromium/src/+/95a1418e25fe0ef90aa7eced41f7...
==========

[commit-bot: I haz the power, 2017-01-16 18:07:07]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/95a1418e25fe0ef90aa7eced41f7...