cc: Make LayerTreeImpl dump layer list to FrameViewer and Devtools

cc: Make LayerTreeImpl dump layer list to FrameViewer and Devtools

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

Chrome://tracing will show a layer_tree_impl without depth, this is
expected.

This CL is blocked by Issue 1947553002, and potentially influenced by
the LayerTreeImpl's JSON issue.

Devtools need to deal with the new format information. On the devtools
side, we make the first drawsContent layer "contentRoot" and treat all
other layers as the contentRoot's child.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/3971e0b42893122987ca9647055a079eb1e01e3d
Cr-Commit-Position: refs/heads/master@{#397744}

[sunxd, 2016-05-03 17:43:52]

Description was changed from

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

BUG=605940
==========

to

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-05-03 17:48:09]

Description was changed from

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

Chrome://tracing will show a layer_tree_impl without depth, this is
expected.

This CL is blocked by Issue 1947553002, and potentially influenced by
the LayerTreeImpl's JSON issue.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-05-03 17:48:10]

sunxd@chromium.org changed reviewers:
+ ajuma@chromium.org, jaydasika@chromium.org, nduca@chromium.org,
vollick@chromium.org

[sunxd, 2016-05-03 20:30:05]

sunxd@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2016-05-05 18:20:59]

https://codereview.chromium.org/1945813002/diff/1/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1945813002/diff/1/cc/trees/layer_tree_impl.h#...
cc/trees/layer_tree_impl.h:161: const LayerListIterator<LayerImpl> begin()
const;
Making the return type const here is not very useful. If anything this should
return LayerListIterator<const LayerImpl>?

[sunxd, 2016-05-05 19:25:25]

https://codereview.chromium.org/1945813002/diff/1/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1945813002/diff/1/cc/trees/layer_tree_impl.h#...
cc/trees/layer_tree_impl.h:161: const LayerListIterator<LayerImpl> begin()
const;
On 2016/05/05 18:20:59, vmpstr wrote:
> Making the return type const here is not very useful. If anything this should
> return LayerListIterator<const LayerImpl>?

Done.

[sunxd, 2016-05-31 14:37:43]

Description was changed from

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

Chrome://tracing will show a layer_tree_impl without depth, this is
expected.

This CL is blocked by Issue 1947553002, and potentially influenced by
the LayerTreeImpl's JSON issue.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer and Devtools

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

Chrome://tracing will show a layer_tree_impl without depth, this is
expected.

This CL is blocked by Issue 1947553002, and potentially influenced by
the LayerTreeImpl's JSON issue.

Devtools need to deal with the new format information. On the devtools
side, we make the first drawsContent layer "contentRoot" and treat all
other layers as the contentRoot's child.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-05-31 14:37:44]

sunxd@chromium.org changed reviewers:
+ caseq@chromium.org

[sunxd, 2016-05-31 14:52:17]

Devtools also uses the information we dump in cc and needs to deal with the new
format.

On the devtools
side, we make the first drawsContent layer "contentRoot" and treat all
other layers as the contentRoot's child.

https://codereview.chromium.org/1945813002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/tracing/layer-tree-expected.txt
(right):

https://codereview.chromium.org/1945813002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/layer-tree-expected.txt:11:
#document
These extra layers are hidden previously because they are not descendants of
"#document" in the left side. But now we make the contentRoot #document to be
the parent of every layer, they are visible by the dumpLayerTree call.

https://codereview.chromium.org/1945813002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js
(right):

https://codereview.chromium.org/1945813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js:353:
this._root = this._contentRoot;
Before this line, the layer tree has no root, but ever after this, it can be
treated as if we still have tree hierarchy in cc.

[vmpstr, 2016-05-31 22:16:16]

Mostly I have questions about why the layout tests are changing with this
change.

https://codereview.chromium.org/1945813002/diff/60001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1945813002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:163: LayerListIterator<LayerImpl> begin() const;
You should either have LayerListIterator<const LayerImpl> here or just add const
qualifier to the existing begin/end functions. I don't think you're buying much
with non-const if you have const ones that return the same type. Wdyt?

https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/layers-test.js (right):

https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/layers-test.js:35: if
(root.drawsContent())
Why this change?

https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/tracing/layer-tree.html (right):

https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/layer-tree.html:47: <div id="a"
style="width: 200px; height: 200px; background-color: yellow" class="layer">
Why these changes?

[sunxd, 2016-05-31 22:41:52]

https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/inspector/layers-test.js (right):

https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/layers-test.js:35: if
(root.drawsContent())
On 2016/05/31 22:16:16, vmpstr wrote:
> Why this change?

See the comment in layer-tree.html, the background-color there forces the layer
to be draw.

https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/tracing/layer-tree.html (right):

https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/tracing/layer-tree.html:47: <div id="a"
style="width: 200px; height: 200px; background-color: yellow" class="layer">
On 2016/05/31 22:16:16, vmpstr wrote:
> Why these changes?

When cc composites layers, it composites several layers besides these divs.

The cc::LayerTreeImpl is organized in this way:

+root
+--composited layers (inner/outer viewport layer, page scale layer etc.)
+----document (contentRoot)
+------div a
+--------div b1
+--------div b2
+----------div c
+--------div b3

The TracingLayerTree in LayerTreeModel.js makes the first layer that draws
content to be "ContentRoot".

The function dumpLayerTree (called in line 38) dumps the sub layer tree from
"ContentRoot" instead of the actual root. So previously, the expected result is:
+----document (contentRoot)
+------div a
+--------div b1
+--------div b2
+----------div c
+--------div b3

But now we get a bunch of internal layers (because we make every other layer a
child of "ContentRoot", this approach does not affect the actual use of devtools
except we don't get indents indicating tree hierarchy). I was considering making
dumpLayerTree only dump layers that draw content, so I need to set the
background color here.

However, in another test, "layer-tree-model.html", dumpLayerTree is supposed to
also dump internal layers, so I'm still thinking about solutions: adding an
option to dump drawsContent layers or simply dumping every layer we composite.

[vmpstr, 2016-05-31 22:47:46]

On 2016/05/31 22:41:52, sunxd wrote:
>
https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/http/tests/inspector/layers-test.js
(right):
> 
>
https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/http/tests/inspector/layers-test.js:35: if
> (root.drawsContent())
> On 2016/05/31 22:16:16, vmpstr wrote:
> > Why this change?
> 
> See the comment in layer-tree.html, the background-color there forces the
layer
> to be draw.
> 
>
https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/inspector/tracing/layer-tree.html (right):
> 
>
https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/inspector/tracing/layer-tree.html:47: <div
id="a"
> style="width: 200px; height: 200px; background-color: yellow" class="layer">
> On 2016/05/31 22:16:16, vmpstr wrote:
> > Why these changes?
> 
> When cc composites layers, it composites several layers besides these divs.
> 
> The cc::LayerTreeImpl is organized in this way:
> 
> +root
> +--composited layers (inner/outer viewport layer, page scale layer etc.)
> +----document (contentRoot)
> +------div a
> +--------div b1
> +--------div b2
> +----------div c
> +--------div b3
> 
> The TracingLayerTree in LayerTreeModel.js makes the first layer that draws
> content to be "ContentRoot".
> 
> The function dumpLayerTree (called in line 38) dumps the sub layer tree from
> "ContentRoot" instead of the actual root. So previously, the expected result
is:
> +----document (contentRoot)
> +------div a
> +--------div b1
> +--------div b2
> +----------div c
> +--------div b3
> 
> But now we get a bunch of internal layers (because we make every other layer a
> child of "ContentRoot", this approach does not affect the actual use of
devtools
> except we don't get indents indicating tree hierarchy). I was considering
making
> dumpLayerTree only dump layers that draw content, so I need to set the
> background color here.
> 
> However, in another test, "layer-tree-model.html", dumpLayerTree is supposed
to
> also dump internal layers, so I'm still thinking about solutions: adding an
> option to dump drawsContent layers or simply dumping every layer we composite.

Can we always dump everything and rebaseline the tests that expect less?

[sunxd, 2016-06-01 14:59:50]

On 2016/05/31 22:47:46, vmpstr wrote:
> On 2016/05/31 22:41:52, sunxd wrote:
> >
>
https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/http/tests/inspector/layers-test.js
> (right):
> > 
> >
>
https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/http/tests/inspector/layers-test.js:35: if
> > (root.drawsContent())
> > On 2016/05/31 22:16:16, vmpstr wrote:
> > > Why this change?
> > 
> > See the comment in layer-tree.html, the background-color there forces the
> layer
> > to be draw.
> > 
> >
>
https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/inspector/tracing/layer-tree.html
(right):
> > 
> >
>
https://codereview.chromium.org/1945813002/diff/60001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/inspector/tracing/layer-tree.html:47: <div
> id="a"
> > style="width: 200px; height: 200px; background-color: yellow" class="layer">
> > On 2016/05/31 22:16:16, vmpstr wrote:
> > > Why these changes?
> > 
> > When cc composites layers, it composites several layers besides these divs.
> > 
> > The cc::LayerTreeImpl is organized in this way:
> > 
> > +root
> > +--composited layers (inner/outer viewport layer, page scale layer etc.)
> > +----document (contentRoot)
> > +------div a
> > +--------div b1
> > +--------div b2
> > +----------div c
> > +--------div b3
> > 
> > The TracingLayerTree in LayerTreeModel.js makes the first layer that draws
> > content to be "ContentRoot".
> > 
> > The function dumpLayerTree (called in line 38) dumps the sub layer tree from
> > "ContentRoot" instead of the actual root. So previously, the expected result
> is:
> > +----document (contentRoot)
> > +------div a
> > +--------div b1
> > +--------div b2
> > +----------div c
> > +--------div b3
> > 
> > But now we get a bunch of internal layers (because we make every other layer
a
> > child of "ContentRoot", this approach does not affect the actual use of
> devtools
> > except we don't get indents indicating tree hierarchy). I was considering
> making
> > dumpLayerTree only dump layers that draw content, so I need to set the
> > background color here.
> > 
> > However, in another test, "layer-tree-model.html", dumpLayerTree is supposed
> to
> > also dump internal layers, so I'm still thinking about solutions: adding an
> > option to dump drawsContent layers or simply dumping every layer we
composite.
> 
> Can we always dump everything and rebaseline the tests that expect less?

I think that's reasonable. I'll upload the patch of dumping every layer.

[vmpstr, 2016-06-01 18:47:59]

https://codereview.chromium.org/1945813002/diff/80001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1945813002/diff/80001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:163: LayerListIterator<LayerImpl> begin() const;
Please change this in one of the following ways (either way works for me):

1. Make this return LayerListIterator<const LayerImpl>
2. Remove these functions and add const qualifier to the other begin/end.

[caseq, 2016-06-01 18:55:06]

lgtm % nits

Is the layer hierarchy going to become meaningless altogether, or are you just
removing it because you don't need it? In the latter case, I wonder if we could
just keep the parent id around to reconstruct the tree on the client?

https://codereview.chromium.org/1945813002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js
(right):

https://codereview.chromium.org/1945813002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js:330:
if (root) {
please add the comment that this is a legacy code path that is there for
compatibility only and should eventually be removed.

https://codereview.chromium.org/1945813002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js:351:
this._layers.push(this._innerSetLayers(oldLayersById, layers[i]));
wrong indent.
perhaps, just this._layers = layers.map(this._innerSetLayers.bind(this,
oldLayersById))

https://codereview.chromium.org/1945813002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js:352:
if (layers.length > 0) {
just do the below unconditionally?

[sunxd, 2016-06-01 23:50:40]

On 2016/06/01 18:55:06, caseq wrote:
> lgtm % nits
> 
> Is the layer hierarchy going to become meaningless altogether, or are you just
> removing it because you don't need it? In the latter case, I wonder if we
could
> just keep the parent id around to reconstruct the tree on the client?
 
So the layer tree will become a layer list and a few property trees, so
LayerImpl::AsValueInto
will not dump children anymore, thus there will be no way to find the parent_id.

The frameviewer displays the layers as a list, I think it might be ok as long as
the devtools
display all the layers in cc.

[sunxd, 2016-06-01 23:51:13]

https://codereview.chromium.org/1945813002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js
(right):

https://codereview.chromium.org/1945813002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js:330:
if (root) {
On 2016/06/01 18:55:06, caseq wrote:
> please add the comment that this is a legacy code path that is there for
> compatibility only and should eventually be removed.

Done.

https://codereview.chromium.org/1945813002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js:351:
this._layers.push(this._innerSetLayers(oldLayersById, layers[i]));
On 2016/06/01 18:55:06, caseq wrote:
> wrong indent.
> perhaps, just this._layers = layers.map(this._innerSetLayers.bind(this,
> oldLayersById))

Done.

https://codereview.chromium.org/1945813002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline_model/LayerTreeModel.js:352:
if (layers.length > 0) {
On 2016/06/01 18:55:06, caseq wrote:
> just do the below unconditionally?

Done.

[sunxd, 2016-06-03 14:47:38]

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

[commit-bot: I haz the power, 2016-06-03 14:47:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1945813002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1945813002/100001

[commit-bot: I haz the power, 2016-06-03 16:39:26]

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

[commit-bot: I haz the power, 2016-06-03 16:39:27]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2016-06-03 17:01:13]

cc lgtm

[sunxd, 2016-06-03 17:25:18]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2016-06-03 17:25:19]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/1945813002/#ps100001
(title: "Resolve comments")

[commit-bot: I haz the power, 2016-06-03 17:25:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1945813002/100001

[commit-bot: I haz the power, 2016-06-03 17:31:22]

Description was changed from

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer and Devtools

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

Chrome://tracing will show a layer_tree_impl without depth, this is
expected.

This CL is blocked by Issue 1947553002, and potentially influenced by
the LayerTreeImpl's JSON issue.

Devtools need to deal with the new format information. On the devtools
side, we make the first drawsContent layer "contentRoot" and treat all
other layers as the contentRoot's child.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer and Devtools

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

Chrome://tracing will show a layer_tree_impl without depth, this is
expected.

This CL is blocked by Issue 1947553002, and potentially influenced by
the LayerTreeImpl's JSON issue.

Devtools need to deal with the new format information. On the devtools
side, we make the first drawsContent layer "contentRoot" and treat all
other layers as the contentRoot's child.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-03 17:31:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-03 17:33:12]

Description was changed from

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer and Devtools

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

Chrome://tracing will show a layer_tree_impl without depth, this is
expected.

This CL is blocked by Issue 1947553002, and potentially influenced by
the LayerTreeImpl's JSON issue.

Devtools need to deal with the new format information. On the devtools
side, we make the first drawsContent layer "contentRoot" and treat all
other layers as the contentRoot's child.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Make LayerTreeImpl dump layer list to FrameViewer and Devtools

As compositor will no longer know tree hierarchy information, it is
expected to dump a layer list instead of a layer tree to chrome tracing.

The value root_layer is also deleted because it is used to judge whether
the dumped layer_tree_impl has a list or not on the FrameViewer side.

Chrome://tracing will show a layer_tree_impl without depth, this is
expected.

This CL is blocked by Issue 1947553002, and potentially influenced by
the LayerTreeImpl's JSON issue.

Devtools need to deal with the new format information. On the devtools
side, we make the first drawsContent layer "contentRoot" and treat all
other layers as the contentRoot's child.

BUG=605940
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/3971e0b42893122987ca9647055a079eb1e01e3d
Cr-Commit-Position: refs/heads/master@{#397744}
==========

[commit-bot: I haz the power, 2016-06-03 17:33:13]

Patchset 6 (id:??) landed as
https://crrev.com/3971e0b42893122987ca9647055a079eb1e01e3d
Cr-Commit-Position: refs/heads/master@{#397744}