Rename Document::attach/detachLayoutTree to Document::initialize/shutdown

Rename Document::attach/detachLayoutTree to Document::initialize/shutdown

Document::attach/detachLayoutTree is confusing. It's indeed attaching/detaching
layout trees but doing more things than that. In particular, Document::detachLayoutTree
does a bunch of disruptive operations unlike other Node::detachLayoutTree.

It's better to rename Document::attach/detachLayoutTree to Document::initialize/shutdown
and make it clear that they can be called only once when the Document gets
initialized/shut down.

BUG=610176
Committed: https://crrev.com/47c70c581412441848e8c2373b8b30aab2d694eb
Cr-Commit-Position: refs/heads/master@{#417175}

[haraken, 2016-09-07 05:09:15]

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

[commit-bot: I haz the power, 2016-09-07 05:09:28]

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

[haraken, 2016-09-07 05:09:36]

haraken@chromium.org changed reviewers:
+ dcheng@chromium.org, esprehn@chromium.org

[haraken, 2016-09-07 05:09:36]

PTAL

[commit-bot: I haz the power, 2016-09-07 06:53:57]

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

[commit-bot: I haz the power, 2016-09-07 06:53:57]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-07 07:23:06]

Hmm, doesn't this mean that if someone calls detachLayoutTree() directly on
Document by mistake, they'll skip all the work in Document::shutdown()? That
seems dangerous.

[haraken, 2016-09-07 07:31:54]

On 2016/09/07 07:23:06, dcheng wrote:
> Hmm, doesn't this mean that if someone calls detachLayoutTree() directly on
> Document by mistake, they'll skip all the work in Document::shutdown()? That
> seems dangerous.

As far as I understand that's the expected behavior, and that's why I want to
land this CL :)

If someone calls Document::detachLayoutTree() directly, nothing should happen
because Document::detachLayoutTree must not be called except when the frame gets
detached. Note that Document::detachLayoutTree does disruptive operations. On
the other hand, other Node::detachLayoutTree can be called multiple times when
you want to detach layout trees (actually they can be re-attached when they are
detached). In that sense, Document::detachLayoutTree and Node::detachLayoutTree
are doing different things.

[dcheng, 2016-09-07 07:41:48]

On 2016/09/07 07:31:54, haraken wrote:
> On 2016/09/07 07:23:06, dcheng wrote:
> > Hmm, doesn't this mean that if someone calls detachLayoutTree() directly on
> > Document by mistake, they'll skip all the work in Document::shutdown()? That
> > seems dangerous.
> 
> As far as I understand that's the expected behavior, and that's why I want to
> land this CL :)
> 
> If someone calls Document::detachLayoutTree() directly, nothing should happen
> because Document::detachLayoutTree must not be called except when the frame
gets
> detached. Note that Document::detachLayoutTree does disruptive operations. On
> the other hand, other Node::detachLayoutTree can be called multiple times when
> you want to detach layout trees (actually they can be re-attached when they
are
> detached). In that sense, Document::detachLayoutTree and
Node::detachLayoutTree
> are doing different things.

I agree Document::detachLayoutTree is weird, since it can only be called once.
Today, this happens to always be safe (even if counterintuitive): even we call
it through Node::detachLayoutTree(), the entire Document is completely shutdown
and detached.

However, now, it's possible for there to be code which was accidentally using
Document::detachLayoutTree() before (maybe it was detaching a collection of
nodes), and now the Document is only partially detached. Is it safe for a
Document to be detached from the layout tree but not detached?

[haraken, 2016-09-07 08:01:22]

On 2016/09/07 07:41:48, dcheng wrote:
> On 2016/09/07 07:31:54, haraken wrote:
> > On 2016/09/07 07:23:06, dcheng wrote:
> > > Hmm, doesn't this mean that if someone calls detachLayoutTree() directly
on
> > > Document by mistake, they'll skip all the work in Document::shutdown()?
That
> > > seems dangerous.
> > 
> > As far as I understand that's the expected behavior, and that's why I want
to
> > land this CL :)
> > 
> > If someone calls Document::detachLayoutTree() directly, nothing should
happen
> > because Document::detachLayoutTree must not be called except when the frame
> gets
> > detached. Note that Document::detachLayoutTree does disruptive operations.
On
> > the other hand, other Node::detachLayoutTree can be called multiple times
when
> > you want to detach layout trees (actually they can be re-attached when they
> are
> > detached). In that sense, Document::detachLayoutTree and
> Node::detachLayoutTree
> > are doing different things.
> 
> I agree Document::detachLayoutTree is weird, since it can only be called once.
> Today, this happens to always be safe (even if counterintuitive): even we call
> it through Node::detachLayoutTree(), the entire Document is completely
shutdown
> and detached.

(First of all, currently there is no place that directly calls
Document::detachLayoutTree(). That's how things are working.)

If someone called Document::detachLayoutTree(), what should happen?

- Currently it wrongly shuts down things in disruptive manner and breaks many
things. Note that by design layout trees can be attached/detached mostly at any
arbitrary time.

- IIUC a right thing to do is do nothing. At the very least, this preserves the
existing behavior because currently no one calls Document::detachLayoutTree()
when the layout tree gets detached.


> 
> However, now, it's possible for there to be code which was accidentally using
> Document::detachLayoutTree() before (maybe it was detaching a collection of
> nodes), and now the Document is only partially detached. Is it safe for a
> Document to be detached from the layout tree but not detached?

So, my answer is yes. At the very least, this preserves the existing behavior.

[dcheng, 2016-09-07 19:40:21]

I think I would feel better about this if we could just make attachLayoutTree()
and detachLayoutTree() on Document assert, since we should probably never call
them directly anyway?

[haraken, 2016-09-08 01:04:09]

On 2016/09/07 19:40:21, dcheng wrote:
> I think I would feel better about this if we could just make
attachLayoutTree()
> and detachLayoutTree() on Document assert, since we should probably never call
> them directly anyway?

What kind of assert do you want? Assert that attachLayoutTree() is called only
at the frame initialization and detachLayoutTree() is called only at the frame
detachment? I can do that but then I guess the naming
attachLayoutTree/detachLayoutTree won't make much sense because it's doing a
totally different thing from other attachLayoutTree/detachLayoutTree...

[esprehn, 2016-09-08 01:06:33]

On 2016/09/08 at 01:04:09, haraken wrote:
> On 2016/09/07 19:40:21, dcheng wrote:
> > I think I would feel better about this if we could just make
attachLayoutTree()
> > and detachLayoutTree() on Document assert, since we should probably never
call
> > them directly anyway?
> 
> What kind of assert do you want? Assert that attachLayoutTree() is called only
at the frame initialization and detachLayoutTree() is called only at the frame
detachment? I can do that but then I guess the naming
attachLayoutTree/detachLayoutTree won't make much sense because it's doing a
totally different thing from other attachLayoutTree/detachLayoutTree...

I'd like us to remove Node::attachLayoutTree() and ::detachLayoutTree() and then
have one on Element and one on Text, and no method on Document. Document has no
concept of attach/detach, it has init() and shutdown() and the process is not
reversible.

[haraken, 2016-09-08 01:13:49]

On 2016/09/08 01:06:33, esprehn wrote:
> On 2016/09/08 at 01:04:09, haraken wrote:
> > On 2016/09/07 19:40:21, dcheng wrote:
> > > I think I would feel better about this if we could just make
> attachLayoutTree()
> > > and detachLayoutTree() on Document assert, since we should probably never
> call
> > > them directly anyway?
> > 
> > What kind of assert do you want? Assert that attachLayoutTree() is called
only
> at the frame initialization and detachLayoutTree() is called only at the frame
> detachment? I can do that but then I guess the naming
> attachLayoutTree/detachLayoutTree won't make much sense because it's doing a
> totally different thing from other attachLayoutTree/detachLayoutTree...
> 
> I'd like us to remove Node::attachLayoutTree() and ::detachLayoutTree() and
then
> have one on Element and one on Text, and no method on Document. Document has
no
> concept of attach/detach, it has init() and shutdown() and the process is not
> reversible.

Chatted with dcheng@ offline and added NOTREACHED to
Document::attach/detachLayoutTree().

How does this look?

[esprehn, 2016-09-08 01:16:27]

lgtm

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

https://codereview.chromium.org/2318913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2130:
ContainerNode::attachLayoutTree(AttachContext());
There's a default argument, you can just do ContainerNode::attachLayoutTree()
without the argument

https://codereview.chromium.org/2318913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2215:
ContainerNode::detachLayoutTree(AttachContext());
ditto

[haraken, 2016-09-08 01:26:00]

>
https://codereview.chromium.org/2318913002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2318913002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:2130:
> ContainerNode::attachLayoutTree(AttachContext());
> There's a default argument, you can just do ContainerNode::attachLayoutTree()
> without the argument
> 
>
https://codereview.chromium.org/2318913002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:2215:
> ContainerNode::detachLayoutTree(AttachContext());
> ditto

Done.

[dcheng, 2016-09-08 01:26:45]

lgtm

[haraken, 2016-09-08 01:28:09]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-09-08 01:28:10]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2318913002/#ps60001
(title: "temp")

[commit-bot: I haz the power, 2016-09-08 01:28:38]

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-09-08 02:56:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 02:58:36]

Description was changed from

==========
Rename Document::attach/detachLayoutTree to Document::initialize/shutdown

Document::attach/detachLayoutTree is confusing. It's indeed attaching/detaching
layout trees but doing more things than that. In particular,
Document::detachLayoutTree
does a bunch of disruptive operations unlike other Node::detachLayoutTree.

It's better to rename Document::attach/detachLayoutTree to
Document::initialize/shutdown
and make it clear that they can be called only once when the Document gets
initialized/shut down.

BUG=610176
==========

to

==========
Rename Document::attach/detachLayoutTree to Document::initialize/shutdown

Document::attach/detachLayoutTree is confusing. It's indeed attaching/detaching
layout trees but doing more things than that. In particular,
Document::detachLayoutTree
does a bunch of disruptive operations unlike other Node::detachLayoutTree.

It's better to rename Document::attach/detachLayoutTree to
Document::initialize/shutdown
and make it clear that they can be called only once when the Document gets
initialized/shut down.

BUG=610176

Committed: https://crrev.com/47c70c581412441848e8c2373b8b30aab2d694eb
Cr-Commit-Position: refs/heads/master@{#417175}
==========

[commit-bot: I haz the power, 2016-09-08 02:58:37]

Patchset 4 (id:??) landed as
https://crrev.com/47c70c581412441848e8c2373b8b30aab2d694eb
Cr-Commit-Position: refs/heads/master@{#417175}