Build property trees for the HUD layer when using layer lists

Build property trees for the HUD layer when using layer lists

When using layer lists in the compositor (aka, SPV2 mode), the hud
layer created in the layer tree host will not have property tree
nodes set. This patch ensures these nodes are set by the layer tree
host.

With this patch, running with the following flags does not crash:
--enable-slimming-paint-v2 --show-paint-rects

BUG=698066
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2776473002
Cr-Commit-Position: refs/heads/master@{#459287}
Committed: https://chromium.googlesource.com/chromium/src/+/cccbe41d5ebad5b6e7789c9a18cbdb1bf3b06794

[pdr., 2017-03-23 15:59:39]

pdr@chromium.org changed reviewers:
+ ajuma@chromium.org

[pdr., 2017-03-23 15:59:52]

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

[commit-bot: I haz the power, 2017-03-23 16:00:13]

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-03-23 17:25:45]

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

[commit-bot: I haz the power, 2017-03-23 17:25:46]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-03-23 17:41:27]

https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h#newcode441
cc/layers/layer.h:441: void copyPropertyTreeStateTo(HeadsUpDisplayLayer*
hud_layer) const;
not-blink-nit: Upper-case c :)

At a higher level, since we only need this for the specific case of copying root
layer state to the hud layer, I wonder if this would be better off as a helper
that did just that in LayerTreeHost rather than here.

Alternatively, if this seems like something we might want more generally, then
this should take a Layer* instead of a HeadsUpDisplayLayer*.

[pdr., 2017-03-23 18:18:10]

On 2017/03/23 at 17:41:27, ajuma wrote:
> https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h
> File cc/layers/layer.h (right):
> 
> https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h#newcode441
> cc/layers/layer.h:441: void copyPropertyTreeStateTo(HeadsUpDisplayLayer*
hud_layer) const;
> not-blink-nit: Upper-case c :)
> 
> At a higher level, since we only need this for the specific case of copying
root layer state to the hud layer, I wonder if this would be better off as a
helper that did just that in LayerTreeHost rather than here.
> 
> Alternatively, if this seems like something we might want more generally, then
this should take a Layer* instead of a HeadsUpDisplayLayer*.

The issue is that we don't have access to Layer's private members (e.g.,
transform_tree_index_) and the accessors for it don't always return it exactly
(e.g., transform_tree_index()). To implement this in LayerTreeHost we'd have to
add raw accessors to the tree indices which seems messy. I don't think this will
be needed elsewhere so my hope is using HeadsUpDisplayLayer* instead of Layer*
will ensure that.

[ajuma, 2017-03-23 18:38:12]

On 2017/03/23 18:18:10, pdr. wrote:
> On 2017/03/23 at 17:41:27, ajuma wrote:
> > https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h
> > File cc/layers/layer.h (right):
> > 
> >
https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h#newcode441
> > cc/layers/layer.h:441: void copyPropertyTreeStateTo(HeadsUpDisplayLayer*
> hud_layer) const;
> > not-blink-nit: Upper-case c :)
> > 
> > At a higher level, since we only need this for the specific case of copying
> root layer state to the hud layer, I wonder if this would be better off as a
> helper that did just that in LayerTreeHost rather than here.
> > 
> > Alternatively, if this seems like something we might want more generally,
then
> this should take a Layer* instead of a HeadsUpDisplayLayer*.
> 
> The issue is that we don't have access to Layer's private members (e.g.,
> transform_tree_index_) and the accessors for it don't always return it exactly
> (e.g., transform_tree_index()). To implement this in LayerTreeHost we'd have
to
> add raw accessors to the tree indices which seems messy. I don't think this
will
> be needed elsewhere so my hope is using HeadsUpDisplayLayer* instead of Layer*
> will ensure that.

Good point. In this case though, I think the accessors will give us values that
match (since if they don't, it means they're returning invalid property tree
indexes for the root, which would be unexpected since property trees have
already been built by this point), so using them could work (with DCHECKs that
we're not getting kInvalidNodeId).

[pdr., 2017-03-23 20:50:02]

On 2017/03/23 at 18:38:12, ajuma wrote:
> On 2017/03/23 18:18:10, pdr. wrote:
> > On 2017/03/23 at 17:41:27, ajuma wrote:
> > > https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h
> > > File cc/layers/layer.h (right):
> > > 
> > >
https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h#newcode441
> > > cc/layers/layer.h:441: void copyPropertyTreeStateTo(HeadsUpDisplayLayer*
> > hud_layer) const;
> > > not-blink-nit: Upper-case c :)
> > > 
> > > At a higher level, since we only need this for the specific case of
copying
> > root layer state to the hud layer, I wonder if this would be better off as a
> > helper that did just that in LayerTreeHost rather than here.
> > > 
> > > Alternatively, if this seems like something we might want more generally,
then
> > this should take a Layer* instead of a HeadsUpDisplayLayer*.
> > 
> > The issue is that we don't have access to Layer's private members (e.g.,
> > transform_tree_index_) and the accessors for it don't always return it
exactly
> > (e.g., transform_tree_index()). To implement this in LayerTreeHost we'd have
to
> > add raw accessors to the tree indices which seems messy. I don't think this
will
> > be needed elsewhere so my hope is using HeadsUpDisplayLayer* instead of
Layer*
> > will ensure that.
> 
> Good point. In this case though, I think the accessors will give us values
that match (since if they don't, it means they're returning invalid property
tree indexes for the root, which would be unexpected since property trees have
already been built by this point), so using them could work (with DCHECKs that
we're not getting kInvalidNodeId).

I got stuck in a local minima when making this patch. I thought I had tried that
and it didn't work, but that was because I was not setting the property tree
sequence number. Anyway, long story short, PTAL with simpler patch.

[ajuma, 2017-03-23 21:04:24]

On 2017/03/23 20:50:02, pdr. wrote:
> On 2017/03/23 at 18:38:12, ajuma wrote:
> > On 2017/03/23 18:18:10, pdr. wrote:
> > > On 2017/03/23 at 17:41:27, ajuma wrote:
> > > > https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h
> > > > File cc/layers/layer.h (right):
> > > > 
> > > >
> https://codereview.chromium.org/2776473002/diff/1/cc/layers/layer.h#newcode441
> > > > cc/layers/layer.h:441: void copyPropertyTreeStateTo(HeadsUpDisplayLayer*
> > > hud_layer) const;
> > > > not-blink-nit: Upper-case c :)
> > > > 
> > > > At a higher level, since we only need this for the specific case of
> copying
> > > root layer state to the hud layer, I wonder if this would be better off as
a
> > > helper that did just that in LayerTreeHost rather than here.
> > > > 
> > > > Alternatively, if this seems like something we might want more
generally,
> then
> > > this should take a Layer* instead of a HeadsUpDisplayLayer*.
> > > 
> > > The issue is that we don't have access to Layer's private members (e.g.,
> > > transform_tree_index_) and the accessors for it don't always return it
> exactly
> > > (e.g., transform_tree_index()). To implement this in LayerTreeHost we'd
have
> to
> > > add raw accessors to the tree indices which seems messy. I don't think
this
> will
> > > be needed elsewhere so my hope is using HeadsUpDisplayLayer* instead of
> Layer*
> > > will ensure that.
> > 
> > Good point. In this case though, I think the accessors will give us values
> that match (since if they don't, it means they're returning invalid property
> tree indexes for the root, which would be unexpected since property trees have
> already been built by this point), so using them could work (with DCHECKs that
> we're not getting kInvalidNodeId).
> 
> I got stuck in a local minima when making this patch. I thought I had tried
that
> and it didn't work, but that was because I was not setting the property tree
> sequence number. Anyway, long story short, PTAL with simpler patch.

Thanks, lgtm

[pdr., 2017-03-23 21:05:20]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-03-23 21:06:13]

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-03-23 23:58:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490303120993710,
"parent_rev": "cb5adf100c71f97edfdf0fdc427873b32cf7ae9a", "commit_rev":
"cccbe41d5ebad5b6e7789c9a18cbdb1bf3b06794"}

[commit-bot: I haz the power, 2017-03-23 23:59:13]

Description was changed from

==========
Build property trees for the HUD layer when using layer lists

When using layer lists in the compositor (aka, SPV2 mode), the hud
layer created in the layer tree host will not have property tree
nodes set. This patch ensures these nodes are set by the layer tree
host.

With this patch, running with the following flags does not crash:
--enable-slimming-paint-v2 --show-paint-rects

BUG=698066
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Build property trees for the HUD layer when using layer lists

When using layer lists in the compositor (aka, SPV2 mode), the hud
layer created in the layer tree host will not have property tree
nodes set. This patch ensures these nodes are set by the layer tree
host.

With this patch, running with the following flags does not crash:
--enable-slimming-paint-v2 --show-paint-rects

BUG=698066
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2776473002
Cr-Commit-Position: refs/heads/master@{#459287}
Committed:
https://chromium.googlesource.com/chromium/src/+/cccbe41d5ebad5b6e7789c9a18cb...
==========

[commit-bot: I haz the power, 2017-03-23 23:59:14]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cccbe41d5ebad5b6e7789c9a18cb...