Build mapping from element id to transform/effect nodes.

Build mapping from element id to transform/effect nodes.

Update cc PropertyTreeBuilder to build additional maps from element id
(used for animation purposes) to the involved transform and effect
nodes.

This map is currently unused but will be used in subsequent patches so
as to eventually allow us to remove the existing maps from layer id to
these same nodes.

This is also a required step for SPv2 composited animation support
wherein, again in a subsequent patch, we will populate these maps in
PaintArtifactCompositor.

BUG=674258
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[wkorman, 2017-01-04 21:03:47]

Description was changed from

==========
Build mapping from element id to transform/effect nodes.

Update cc PropertyTreeBuilder to build additional maps from element id
(used for animation purposes) to the involved transform and effect
nodes.

This map is currently unused but will be used in subsequent patches so
as to eventually allow us to remove the existing maps from layer id to
these same nodes.

This is also a required step for SPv2 composited animation support
wherein, again in a subsequent patch, we will populate these maps in
PaintArtifactCompositor.

BUG=674258
==========

to

==========
Build mapping from element id to transform/effect nodes.

Update cc PropertyTreeBuilder to build additional maps from element id
(used for animation purposes) to the involved transform and effect
nodes.

This map is currently unused but will be used in subsequent patches so
as to eventually allow us to remove the existing maps from layer id to
these same nodes.

This is also a required step for SPv2 composited animation support
wherein, again in a subsequent patch, we will populate these maps in
PaintArtifactCompositor.

BUG=674258
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[wkorman, 2017-01-04 21:06:16]

wkorman@chromium.org changed reviewers:
+ ajuma@chromium.org, chrishtr@chromium.org

[wkorman, 2017-01-04 21:06:16]

Still aiming for small incremental patches. Two questions on this one:

1. Where is the right place to put a unit test? It looks like either
layer_tree_host_impl_unittest, layer_tree_impl_unittest or
layer_tree_host_common_unittest but I figured best to just ask. Will write once
I know where to put.

2. Should we put this behind a flag in some form or is it ok as-is?

[ajuma, 2017-01-04 22:33:57]

Description was changed from

==========
Build mapping from element id to transform/effect nodes.

Update cc PropertyTreeBuilder to build additional maps from element id
(used for animation purposes) to the involved transform and effect
nodes.

This map is currently unused but will be used in subsequent patches so
as to eventually allow us to remove the existing maps from layer id to
these same nodes.

This is also a required step for SPv2 composited animation support
wherein, again in a subsequent patch, we will populate these maps in
PaintArtifactCompositor.

BUG=674258
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Build mapping from element id to transform/effect nodes.

Update cc PropertyTreeBuilder to build additional maps from element id
(used for animation purposes) to the involved transform and effect
nodes.

This map is currently unused but will be used in subsequent patches so
as to eventually allow us to remove the existing maps from layer id to
these same nodes.

This is also a required step for SPv2 composited animation support
wherein, again in a subsequent patch, we will populate these maps in
PaintArtifactCompositor.

BUG=674258
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ajuma, 2017-01-04 22:44:21]

On 2017/01/04 21:06:16, wkorman wrote:
> Still aiming for small incremental patches. Two questions on this one:
> 
> 1. Where is the right place to put a unit test? It looks like either
> layer_tree_host_impl_unittest, layer_tree_impl_unittest or
> layer_tree_host_common_unittest but I figured best to just ask. Will write
once
> I know where to put.

A LayerTreeHostTest (in layer_tree_host_unittest.cc) would let you test both
that the trees are constructed correctly and that the maps are being correctly
propagated through commit and activation.

> 2. Should we put this behind a flag in some form or is it ok as-is?

I think it's ok as-is.

Just as a complete aside: jaydasika and I were talking yesterday about whether
we should change the unordered_maps we have in PropertyTrees to sorted vectors
(since for the typical number of items we have, sorted vectors + binary search
are probably a better choice than unordered_map). Continuing with the map
approach for now seems fine, but I'll file a bug for considering changing these
to vectors.

[wkorman, 2017-01-09 21:41:55]

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:964: EXPECT_EQ(3U,
child_impl_->layer_tree_impl()
We don't currently set the element id on the transform/effect nodes (though
maybe we'll find we want to do that later; scroll node does have it, for
example) so there wasn't much I saw to check in the test other than that the new
nodes exist, and go away when expected. You had mentioned explicitly to check
the tree structure, so if there's more I should do here let me know.

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

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/property_tree....
cc/trees/property_tree.h:545: element_id_to_effect_node_index;
Checkpoint: have omitted the _map suffix that the four maps at line 530 - 534
use as it seems redundant, but wanted to call it out as it's deviant from
existing code. If this seems reasonable then I'd propose renaming the others as
well to remove it as subsequent patch. Else am happy to add _map.

[wkorman, 2017-01-09 22:01:56]

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

[commit-bot: I haz the power, 2017-01-09 22:03:46]

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

[ajuma, 2017-01-10 00:02:36]

Just a few nits about the test:

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:946: auto
root_element_id_to_transform_node_index =
int child_transform_node_index

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:948: auto
root_element_id_to_effect_node_index =
int child_effect_node_index

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:961: EXPECT_EQ(0,
root_element_id_to_effect_node_index);
These 0s are because kTestElementId isn't present in the map, right? It'd be
good to check for that explicitly (to distinguish from 0 being the actual
index).

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:981: EXPECT_EQ(0,
root_element_id_to_effect_node_index);
Same comment about checking that kTestElementId isn't present in the maps.

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

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/property_tree....
cc/trees/property_tree.h:545: element_id_to_effect_node_index;
On 2017/01/09 21:41:55, wkorman wrote:
> Checkpoint: have omitted the _map suffix that the four maps at line 530 - 534
> use as it seems redundant, but wanted to call it out as it's deviant from
> existing code. If this seems reasonable then I'd propose renaming the others
as
> well to remove it as subsequent patch. Else am happy to add _map.

Seems reasonable, and renaming the rest to match in a subsequent patch sound
good.

[wkorman, 2017-01-10 00:13:09]

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:946: auto
root_element_id_to_transform_node_index =
On 2017/01/10 at 00:02:36, ajuma wrote:
> int child_transform_node_index

Moved inline to the one case statement where it's now used.

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:948: auto
root_element_id_to_effect_node_index =
On 2017/01/10 at 00:02:36, ajuma wrote:
> int child_effect_node_index

Moved inline to the one case statement where it's now used.

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:961: EXPECT_EQ(0,
root_element_id_to_effect_node_index);
On 2017/01/10 at 00:02:36, ajuma wrote:
> These 0s are because kTestElementId isn't present in the map, right? It'd be
good to check for that explicitly (to distinguish from 0 being the actual
index).

Good point, revised here and below to find() == end() instead.

https://codereview.chromium.org/2612093002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:981: EXPECT_EQ(0,
root_element_id_to_effect_node_index);
On 2017/01/10 at 00:02:36, ajuma wrote:
> Same comment about checking that kTestElementId isn't present in the maps.

Done.

[ajuma, 2017-01-10 00:20:51]

Thanks, lgtm

[wkorman, 2017-01-10 00:24:08]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-01-10 00:25:09]

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-10 03:33:54]

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

[commit-bot: I haz the power, 2017-01-10 03:33:56]

Failed to apply patch for cc/trees/property_tree_builder.cc:
While running git apply --index -p1;
  error: patch failed: cc/trees/property_tree_builder.cc:1095
  error: cc/trees/property_tree_builder.cc: patch does not apply

Patch:       cc/trees/property_tree_builder.cc
Index: cc/trees/property_tree_builder.cc
diff --git a/cc/trees/property_tree_builder.cc
b/cc/trees/property_tree_builder.cc
index
91d812c2558c2f646b638151ffcf4736e7e3ef01..f4e882152e6cccf237450aa80739ff9ca1a00533
100644
--- a/cc/trees/property_tree_builder.cc
+++ b/cc/trees/property_tree_builder.cc
@@ -622,6 +622,13 @@ bool AddTransformNodeIfNeeded(
   data_for_children->property_trees->transform_id_to_index_map[layer->id()] =
       node->id;
 
+  // For animation subsystem purposes, if this layer has a compositor element
+  // id, we build a map from that id to this transform node.
+  if (layer->element_id()) {
+    data_for_children->property_trees
+        ->element_id_to_transform_node_index[layer->element_id()] = node->id;
+  }
+
   node->scrolls = is_scrollable;
   node->should_be_snapped = is_snapped;
   node->flattens_inherited_transform = data_for_children->should_flatten;
@@ -1095,6 +1102,13 @@ bool AddEffectNodeIfNeeded(
   data_for_children->property_trees->effect_id_to_index_map[layer->id()] =
       data_for_children->effect_tree_parent;
 
+  // For animation subsystem purposes, if this layer has a compositor element
+  // id, we build a map from that id to this effect node.
+  if (layer->element_id()) {
+    data_for_children->property_trees
+        ->element_id_to_effect_node_index[layer->element_id()] = node_id;
+  }
+
   std::vector<std::unique_ptr<CopyOutputRequest>> layer_copy_requests;
   TakeCopyRequests(layer, &layer_copy_requests);
   for (auto& it : layer_copy_requests) {

[wkorman, 2017-01-10 21:10:46]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-01-10 21:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2612093002/#ps140001
(title: "Sync to head.")

[commit-bot: I haz the power, 2017-01-10 21:11: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-01-10 22:29:15]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484082646376400,
"parent_rev": "0d4d0fd0973238c92bbd629960eeed5b3481b48c", "commit_rev":
"e14c8ed4a52032ed6569a95febe1d2c73af782dc"}

[commit-bot: I haz the power, 2017-01-10 22:29:44]

Description was changed from

==========
Build mapping from element id to transform/effect nodes.

Update cc PropertyTreeBuilder to build additional maps from element id
(used for animation purposes) to the involved transform and effect
nodes.

This map is currently unused but will be used in subsequent patches so
as to eventually allow us to remove the existing maps from layer id to
these same nodes.

This is also a required step for SPv2 composited animation support
wherein, again in a subsequent patch, we will populate these maps in
PaintArtifactCompositor.

BUG=674258
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Build mapping from element id to transform/effect nodes.

Update cc PropertyTreeBuilder to build additional maps from element id
(used for animation purposes) to the involved transform and effect
nodes.

This map is currently unused but will be used in subsequent patches so
as to eventually allow us to remove the existing maps from layer id to
these same nodes.

This is also a required step for SPv2 composited animation support
wherein, again in a subsequent patch, we will populate these maps in
PaintArtifactCompositor.

BUG=674258
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-01-10 22:29:45]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/e14c8ed4a52032ed6569a95febe1...