cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

In LayerTreeTest, TreeSynchronizer pushes the layer structures from main
thread to impl thread. In the SPv2 plan, we will eventually have no tree
hierarchy on impl side. This means that the synchronizer no longer pushes
a tree structure, instead it pushes a list. We need to make the unit tests
use other ways to access layer_impls in the pushed impl tree.

BUG=604467
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/1ae6803fb4dcb9a40bc50d889c349869c3dcd83c
Cr-Commit-Position: refs/heads/master@{#388875}

[sunxd, 2016-04-20 19:05:35]

Description was changed from

==========
cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

BUG=604467
==========

to

==========
cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

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

[sunxd, 2016-04-20 19:21:54]

Description was changed from

==========
cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

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

to

==========
cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

In LayerTreeTest, TreeSynchronizer pushes the layer structures from main
thread to impl thread. In the SPv2 plan, we will eventually have no tree
hierarchy on impl side. This means that the synchronizer no longer pushes
a tree structure, instead it pushes a list. We need to make the unit tests
use other ways to access layer_impls in the pushed impl tree.

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

[sunxd, 2016-04-20 19:21:54]

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

[jaydasika, 2016-04-20 20:26:09]

lgtm

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:2878: //   
expected_push_properties_grandchild2_impl_;
Remove the commented code and also modify the comment to say what this case is
now doing.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:2882:
++expected_push_properties_grandchild2_impl_;
change the comment to grandchild2_impl

[ajuma, 2016-04-20 21:40:52]

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_damage.cc:374: Layer* content_layer_;
This should be a scoped_refptr.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest_occlusion.cc (right):

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:73: Layer* child_;
scoped_refptr

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:129: Layer* child_;
scoped_refptr

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:202: Layer* child_;
scoped_refptr

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:268: Layer* child_;
scoped_refptr

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:349: Layer* child_;
scoped_refptr

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest_picture.cc (right):

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_picture.cc:63: picture_id = picture->id();
Changing picture_id on the main thread will accessing it on the compositor
thread is something to avoid (even if there's an argument that it happens to be
safe in this case). Please use two different variables, and use
source_frame_number on the compositor side to decide which one to use.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_picture.cc:132: int picture_id;
nit: picture_id_

[sunxd, 2016-04-21 17:31:16]

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:2878: //   
expected_push_properties_grandchild2_impl_;
On 2016/04/20 20:26:09, jaydasika wrote:
> Remove the commented code and also modify the comment to say what this case is
> now doing.

Done.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:2882:
++expected_push_properties_grandchild2_impl_;
On 2016/04/20 20:26:09, jaydasika wrote:
> change the comment to grandchild2_impl

Done.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_damage.cc:374: Layer* content_layer_;
On 2016/04/20 21:40:52, ajuma wrote:
> This should be a scoped_refptr.

Done.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest_occlusion.cc (right):

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:73: Layer* child_;
On 2016/04/20 21:40:52, ajuma wrote:
> scoped_refptr

Done.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:129: Layer* child_;
On 2016/04/20 21:40:52, ajuma wrote:
> scoped_refptr

Done.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:202: Layer* child_;
On 2016/04/20 21:40:52, ajuma wrote:
> scoped_refptr

Done.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:268: Layer* child_;
On 2016/04/20 21:40:52, ajuma wrote:
> scoped_refptr

Done.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_occlusion.cc:349: Layer* child_;
On 2016/04/20 21:40:52, ajuma wrote:
> scoped_refptr

Done.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest_picture.cc (right):

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_picture.cc:63: picture_id = picture->id();
On 2016/04/20 21:40:52, ajuma wrote:
> Changing picture_id on the main thread will accessing it on the compositor
> thread is something to avoid (even if there's an argument that it happens to
be
> safe in this case). Please use two different variables, and use
> source_frame_number on the compositor side to decide which one to use.

Done.

https://codereview.chromium.org/1905713002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_picture.cc:132: int picture_id;
On 2016/04/20 21:40:52, ajuma wrote:
> nit: picture_id_

Done.

[ajuma, 2016-04-21 17:52:24]

https://codereview.chromium.org/1905713002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_picture.cc (right):

https://codereview.chromium.org/1905713002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_picture.cc:121: int picture_id =
impl->active_tree()->source_frame_number() < 2
Should this be 3 instead of 2? (Since source_frame_number gets incremented
during activation.)

[sunxd, 2016-04-21 18:05:48]

https://codereview.chromium.org/1905713002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_picture.cc (right):

https://codereview.chromium.org/1905713002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_picture.cc:121: int picture_id =
impl->active_tree()->source_frame_number() < 2
On 2016/04/21 17:52:24, ajuma wrote:
> Should this be 3 instead of 2? (Since source_frame_number gets incremented
> during activation.)

I don't know why but I observed that on impl side the source_frame_number_ is
set to -1 initially, so it's always one less than the host number.

[ajuma, 2016-04-21 19:04:30]

Thanks, lgtm

[sunxd, 2016-04-21 19:44:56]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2016-04-21 19:44:57]

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

[commit-bot: I haz the power, 2016-04-21 19:45:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905713002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905713002/40001

[commit-bot: I haz the power, 2016-04-21 20:15:22]

Description was changed from

==========
cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

In LayerTreeTest, TreeSynchronizer pushes the layer structures from main
thread to impl thread. In the SPv2 plan, we will eventually have no tree
hierarchy on impl side. This means that the synchronizer no longer pushes
a tree structure, instead it pushes a list. We need to make the unit tests
use other ways to access layer_impls in the pushed impl tree.

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

to

==========
cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

In LayerTreeTest, TreeSynchronizer pushes the layer structures from main
thread to impl thread. In the SPv2 plan, we will eventually have no tree
hierarchy on impl side. This means that the synchronizer no longer pushes
a tree structure, instead it pushes a list. We need to make the unit tests
use other ways to access layer_impls in the pushed impl tree.

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

[commit-bot: I haz the power, 2016-04-21 20:15:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:38:35]

Description was changed from

==========
cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

In LayerTreeTest, TreeSynchronizer pushes the layer structures from main
thread to impl thread. In the SPv2 plan, we will eventually have no tree
hierarchy on impl side. This means that the synchronizer no longer pushes
a tree structure, instead it pushes a list. We need to make the unit tests
use other ways to access layer_impls in the pushed impl tree.

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

to

==========
cc: Remove LayerImpl::children() calls from descendants of LayerTreeTest

In LayerTreeTest, TreeSynchronizer pushes the layer structures from main
thread to impl thread. In the SPv2 plan, we will eventually have no tree
hierarchy on impl side. This means that the synchronizer no longer pushes
a tree structure, instead it pushes a list. We need to make the unit tests
use other ways to access layer_impls in the pushed impl tree.

BUG=604467
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/1ae6803fb4dcb9a40bc50d889c349869c3dcd83c
Cr-Commit-Position: refs/heads/master@{#388875}
==========

[commit-bot: I haz the power, 2016-04-22 19:38:36]

Patchset 3 (id:??) landed as
https://crrev.com/1ae6803fb4dcb9a40bc50d889c349869c3dcd83c
Cr-Commit-Position: refs/heads/master@{#388875}