cc: Move Layer Id to Node Map to Individual Property Tree Private

cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
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/2753933005
Cr-Commit-Position: refs/heads/master@{#457850}
Committed: https://chromium.googlesource.com/chromium/src/+/42c074f058c39ca559d47e168dade668b5ec419e

[weiliangc, 2017-03-16 01:41:14]

Description was changed from

==========
cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
==========

to

==========
cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[weiliangc, 2017-03-16 01:41:15]

Description was changed from

==========
cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[weiliangc, 2017-03-16 01:41:25]

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

[commit-bot: I haz the power, 2017-03-16 01:41:54]

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

[weiliangc, 2017-03-16 01:45:01]

Description was changed from

==========
cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-03-16 01:51:31]

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

[commit-bot: I haz the power, 2017-03-16 01:51:31]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  linux_chromium_asan_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_...)

[weiliangc, 2017-03-16 03:08:20]

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

[commit-bot: I haz the power, 2017-03-16 03:08:36]

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-16 05:53:44]

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

[commit-bot: I haz the power, 2017-03-16 05:53:45]

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_...)

[weiliangc, 2017-03-16 14:24:02]

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

[commit-bot: I haz the power, 2017-03-16 14:24:19]

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-16 16:03:15]

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

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

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-03-16 17:15:32]

lgtm, thanks for simplifying this logic!

https://codereview.chromium.org/2753933005/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2753933005/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1243: if
(property_trees_.effect_tree.FindNodeFromId(layer->id())) {
Since we're not using the node, this could FindNodeIndexFromId instead, or we
could keep the result for FindNodeFromId and skip the call to Node below.

https://codereview.chromium.org/2753933005/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1266: if
(property_trees_.transform_tree.FindNodeFromId(layer->id())) {
Here too.

https://codereview.chromium.org/2753933005/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2753933005/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:1705: if (!effect_tree.FindNodeFromId(id))
If we save the result of this call we can avoid the first FindNodeFromId below.

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

https://codereview.chromium.org/2753933005/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.h:101: int FindNodeIndexFromId(int id) const {
Would 'FromOwningId' instead of 'FromId' be clearer for these names?

[weiliangc, 2017-03-16 20:16:57]

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

[weiliangc, 2017-03-16 20:19:57]

Patchset #4 (id:60001) has been deleted

[weiliangc, 2017-03-16 20:20:54]

+pdf for Blink

addressed review comments.

[weiliangc, 2017-03-16 20:27:10]

On 2017/03/16 20:20:54, weiliangc wrote:
> +pdf for Blink
> 
> addressed review comments.

Oooooops sorry pdr.

[pdr., 2017-03-16 20:30:28]

bikesheeeeddd

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

https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
cc/trees/property_tree.h:103: int FindNodeIndexFromOwningId(int id) const {
I think we should move away from using the "owning id" term because layers don't
really own nodes anymore. WDYT of switching this to be something like
FindNodeIndexFromLayerId?

https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
cc/trees/property_tree.h:111: void InsertOwningIdForNode(const T* node, int id)
{
WDYT of using Set* instead of Insert*?

[pdr., 2017-03-16 20:31:39]

On 2017/03/16 at 20:30:28, pdr. wrote:
> bikesheeeeddd
> 
> https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree.h
> File cc/trees/property_tree.h (right):
> 
>
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
> cc/trees/property_tree.h:103: int FindNodeIndexFromOwningId(int id) const {
> I think we should move away from using the "owning id" term because layers
don't really own nodes anymore. WDYT of switching this to be something like
FindNodeIndexFromLayerId?
> 
>
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
> cc/trees/property_tree.h:111: void InsertOwningIdForNode(const T* node, int
id) {
> WDYT of using Set* instead of Insert*?

Ha, I now see Ali asked for the exact opposite. @Ali, do you think using LayerId
instead of OwningLayerId would be clearer long-term?

[ajuma, 2017-03-16 20:46:27]

On 2017/03/16 20:31:39, pdr. wrote:
> On 2017/03/16 at 20:30:28, pdr. wrote:
> > bikesheeeeddd
> > 
> >
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree.h
> > File cc/trees/property_tree.h (right):
> > 
> >
>
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
> > cc/trees/property_tree.h:103: int FindNodeIndexFromOwningId(int id) const {
> > I think we should move away from using the "owning id" term because layers
> don't really own nodes anymore. WDYT of switching this to be something like
> FindNodeIndexFromLayerId?
> > 
> >
>
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
> > cc/trees/property_tree.h:111: void InsertOwningIdForNode(const T* node, int
> id) {
> > WDYT of using Set* instead of Insert*?
> 
> Ha, I now see Ali asked for the exact opposite. @Ali, do you think using
LayerId
> instead of OwningLayerId would be clearer long-term?

If this is LayerId we'll still have to change it long-term (to something like
ElementId), since that still implies a correspondence between Layers and nodes.
OwningId (not OwningLayerId) might be slightly-more future proof in that sense
(since we could think of elements as now owning the nodes). But I don't really
feel strongly about this, as long as it's more specific that just plain Id :)

[pdr., 2017-03-16 21:36:04]

On 2017/03/16 at 20:46:27, ajuma wrote:
> On 2017/03/16 20:31:39, pdr. wrote:
> > On 2017/03/16 at 20:30:28, pdr. wrote:
> > > bikesheeeeddd
> > > 
> > >
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree.h
> > > File cc/trees/property_tree.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
> > > cc/trees/property_tree.h:103: int FindNodeIndexFromOwningId(int id) const
{
> > > I think we should move away from using the "owning id" term because layers
> > don't really own nodes anymore. WDYT of switching this to be something like
> > FindNodeIndexFromLayerId?
> > > 
> > >
> >
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
> > > cc/trees/property_tree.h:111: void InsertOwningIdForNode(const T* node,
int
> > id) {
> > > WDYT of using Set* instead of Insert*?
> > 
> > Ha, I now see Ali asked for the exact opposite. @Ali, do you think using
LayerId
> > instead of OwningLayerId would be clearer long-term?
> 
> If this is LayerId we'll still have to change it long-term (to something like
ElementId), since that still implies a correspondence between Layers and nodes.
OwningId (not OwningLayerId) might be slightly-more future proof in that sense
(since we could think of elements as now owning the nodes). But I don't really
feel strongly about this, as long as it's more specific that just plain Id :)

Ahh, this map is effectively the opposite mapping of
PropertyNode->owning_layer_id. (I thought it with the opposite of
Layer->XYZ_tree_index()). I agree that "Owning" is important here.

What do you think of the following changes:
layer_id_to_node_index -> owning_layer_id_to_node_index
InsertOwningIdForNode -> InsertOwningLayerIdForNode
FindNodeFromOwningId -> FindNodeFromOwningLayerId
FindNodeIndexFromOwningId -> FindNodeIndexFromOwningLayerId

Lastly, because there is a 1:1 mapping between owning layers and nodes in this
map, what do you think of Set* instead of Insert*?

[ajuma, 2017-03-16 21:41:47]

On 2017/03/16 21:36:04, pdr. wrote:
> On 2017/03/16 at 20:46:27, ajuma wrote:
> > On 2017/03/16 20:31:39, pdr. wrote:
> > > On 2017/03/16 at 20:30:28, pdr. wrote:
> > > > bikesheeeeddd
> > > > 
> > > >
> https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree.h
> > > > File cc/trees/property_tree.h (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
> > > > cc/trees/property_tree.h:103: int FindNodeIndexFromOwningId(int id)
const
> {
> > > > I think we should move away from using the "owning id" term because
layers
> > > don't really own nodes anymore. WDYT of switching this to be something
like
> > > FindNodeIndexFromLayerId?
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2753933005/diff/80001/cc/trees/property_tree....
> > > > cc/trees/property_tree.h:111: void InsertOwningIdForNode(const T* node,
> int
> > > id) {
> > > > WDYT of using Set* instead of Insert*?
> > > 
> > > Ha, I now see Ali asked for the exact opposite. @Ali, do you think using
> LayerId
> > > instead of OwningLayerId would be clearer long-term?
> > 
> > If this is LayerId we'll still have to change it long-term (to something
like
> ElementId), since that still implies a correspondence between Layers and
nodes.
> OwningId (not OwningLayerId) might be slightly-more future proof in that sense
> (since we could think of elements as now owning the nodes). But I don't really
> feel strongly about this, as long as it's more specific that just plain Id :)
> 
> Ahh, this map is effectively the opposite mapping of
> PropertyNode->owning_layer_id. (I thought it with the opposite of
> Layer->XYZ_tree_index()). I agree that "Owning" is important here.
> 
> What do you think of the following changes:
> layer_id_to_node_index -> owning_layer_id_to_node_index
> InsertOwningIdForNode -> InsertOwningLayerIdForNode
> FindNodeFromOwningId -> FindNodeFromOwningLayerId
> FindNodeIndexFromOwningId -> FindNodeIndexFromOwningLayerId
> 
> Lastly, because there is a 1:1 mapping between owning layers and nodes in this
> map, what do you think of Set* instead of Insert*?

Those changes all sgtm

[weiliangc, 2017-03-16 22:12:56]

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

[weiliangc, 2017-03-16 22:13:09]

PTAL

[commit-bot: I haz the power, 2017-03-16 22:13:51]

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

[pdr., 2017-03-16 22:17:41]

LGTM

https://codereview.chromium.org/2753933005/diff/100001/cc/input/scrollbar_ani...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2753933005/diff/100001/cc/input/scrollbar_ani...
cc/input/scrollbar_animation_controller.cc:349: // yet have valid
layer_id_to_node_index entries in effect tree as property
Nit layer_id_to_node_index -> owning_layer_id_to_node_index

[weiliangc, 2017-03-16 22:22:17]

The CQ bit was unchecked by weiliangc@chromium.org

[weiliangc, 2017-03-16 22:29:06]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2017-03-16 22:29:06]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2753933005/#ps120001
(title: "commtent change")

[commit-bot: I haz the power, 2017-03-16 22:29:46]

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-16 22:38:56]

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

[commit-bot: I haz the power, 2017-03-16 22:38:58]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[weiliangc, 2017-03-17 15:34:29]

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

[commit-bot: I haz the power, 2017-03-17 15:35:03]

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-17 16:19:09]

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

[commit-bot: I haz the power, 2017-03-17 16:19:10]

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

[weiliangc, 2017-03-17 17:53:33]

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

[commit-bot: I haz the power, 2017-03-17 17:54:18]

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-17 19:14:33]

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

[commit-bot: I haz the power, 2017-03-17 19:14:34]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2017-03-17 19:17:07]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2017-03-17 19:17:08]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2753933005/#ps140001
(title: "renaming clean up")

[commit-bot: I haz the power, 2017-03-17 19:17:47]

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-17 19:28:34]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489778227745430,
"parent_rev": "0921e8319f228e406019b434701ac9f181ee8ea1", "commit_rev":
"42c074f058c39ca559d47e168dade668b5ec419e"}

[commit-bot: I haz the power, 2017-03-17 19:29:15]

Description was changed from

==========
cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
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/2753933005
Cr-Commit-Position: refs/heads/master@{#457850}
Committed:
https://chromium.googlesource.com/chromium/src/+/42c074f058c39ca559d47e168dad...
==========

[commit-bot: I haz the power, 2017-03-17 19:29:17]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/42c074f058c39ca559d47e168dad...