cc : Update render surfaces using LayerListIterator instead of treewalk

cc : Update render surfaces using LayerListIterator instead of treewalk

This is the update render surfaces treewalk in draw_property_utils.cc
BUG=594024
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2411692cd8bd39dc354fe8fef2a0a948a3df19fd
Cr-Commit-Position: refs/heads/master@{#382767}

[jaydasika, 2016-03-21 23:05:52]

Description was changed from

==========
cc : Update render surfaces using LayerListIterator instead of treewalk

BUG=594024
==========

to

==========
cc : Update render surfaces using LayerListIterator instead of treewalk

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

[jaydasika, 2016-03-22 00:19:06]

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

[jaydasika, 2016-03-22 00:19:07]

PTAL

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

https://codereview.chromium.org/1823833002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:567: root_layer_->transform_tree_index() == -1);
have to remove this (added it to make unit tests pass), but I am unsure about
the alternative. I tried building property trees and setting page scale layer
before this function is called to make the tests take the other path (if
statement), but LayerById(page_scale_layer_id) returns null inspite of that (I
think its because of the ownership of these layers, they are scoped_ptr created
in the test). Any thoughts on what to do ?

[ajuma, 2016-03-22 14:41:55]

https://codereview.chromium.org/1823833002/diff/1/cc/test/layer_tree_host_com...
File cc/test/layer_tree_host_common_test.cc (right):

https://codereview.chromium.org/1823833002/diff/1/cc/test/layer_tree_host_com...
cc/test/layer_tree_host_common_test.cc:170: if
(host_impl()->active_tree()->property_trees()->needs_rebuild)
Why does this depend on whether we need to rebuild?

https://codereview.chromium.org/1823833002/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1823833002/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:470:
UpdateRenderSurfacesWithEffectTreeInternal(effect_tree, layer);
Since there's no templating anymore, can we remove the indirection here and just
rename UpdateRenderSurfacesWithEffectTreeInternal to
UpdateRenderSurfacesWithEffectTree?

https://codereview.chromium.org/1823833002/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:473: void
UpdateRenderSurfacesNonRootSurfacesDisabled(LayerImpl* layer) {
Can this be deleted now?

https://codereview.chromium.org/1823833002/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:493: layer->SetHasRenderSurface(false);
To avoid code duplication, how about turning this logic into a (templated)
UpdateRenderSurfaceForLayer that's called while recursing over the layer tree
for Layer and while iterating over the tree for LayerImpl?

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

https://codereview.chromium.org/1823833002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:567: root_layer_->transform_tree_index() == -1);
On 2016/03/22 00:19:07, jaydasika wrote:
> have to remove this (added it to make unit tests pass), but I am unsure about
> the alternative. I tried building property trees and setting page scale layer
> before this function is called to make the tests take the other path (if
> statement), but LayerById(page_scale_layer_id) returns null inspite of that (I
> think its because of the ownership of these layers, they are scoped_ptr
created
> in the test). Any thoughts on what to do ?

Could it be that the page_scale_layer_id is invalid? LayerImpls get added to the
map in their constructor and removed in their destructor, so if the id is valid,
it should be case that LayerById(id) returns non-null. If the
page_scale_layer_id is indeed valid, try adding some logging to
LayerTreeImpl::RegisterLayer and ::UnregisterLayer to see if these are behaving
as expected.

[jaydasika, 2016-03-22 20:48:36]

https://codereview.chromium.org/1823833002/diff/1/cc/test/layer_tree_host_com...
File cc/test/layer_tree_host_common_test.cc (right):

https://codereview.chromium.org/1823833002/diff/1/cc/test/layer_tree_host_com...
cc/test/layer_tree_host_common_test.cc:170: if
(host_impl()->active_tree()->property_trees()->needs_rebuild)
On 2016/03/22 14:41:54, ajuma wrote:
> Why does this depend on whether we need to rebuild?

Whoops, it should be the other way around, if we are going to build property
trees, we don't need to update page scale factor as property tree builder will
take care of it. Will remove this anyway as there is  no harm in doing it twice.

https://codereview.chromium.org/1823833002/diff/1/cc/trees/draw_property_util...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1823833002/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:470:
UpdateRenderSurfacesWithEffectTreeInternal(effect_tree, layer);
On 2016/03/22 14:41:54, ajuma wrote:
> Since there's no templating anymore, can we remove the indirection here and
just
> rename UpdateRenderSurfacesWithEffectTreeInternal to
> UpdateRenderSurfacesWithEffectTree?

Done.

https://codereview.chromium.org/1823833002/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:473: void
UpdateRenderSurfacesNonRootSurfacesDisabled(LayerImpl* layer) {
On 2016/03/22 14:41:54, ajuma wrote:
> Can this be deleted now?

Done.

https://codereview.chromium.org/1823833002/diff/1/cc/trees/draw_property_util...
cc/trees/draw_property_utils.cc:493: layer->SetHasRenderSurface(false);
On 2016/03/22 14:41:55, ajuma wrote:
> To avoid code duplication, how about turning this logic into a (templated)
> UpdateRenderSurfaceForLayer that's called while recursing over the layer tree
> for Layer and while iterating over the tree for LayerImpl?

Done.

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

https://codereview.chromium.org/1823833002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:4941: //
BuildingPropertyTreesForTesting doesn't take into account the scale
Will correct the weird formatting of the comment.

[ajuma, 2016-03-22 21:04:59]

lgtm

Please expand the description to clarify which render surface update this CL
addresses (that is, it's the one in draw_property_utils).

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

https://codereview.chromium.org/1823833002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:368: // and the page scale layer.
It looks like there are only a few (<5) tests that call
ExecuteCalculateDrawProperties with non-1 page scale. How about (in a follow-up,
to avoid making this CL too big) removing the page scale layer argument from
LayerTreeHostCommonTestBase::ExecuteCalculateDrawProperties, and instead make
the tests that need this responsible for setting it and for setting any non-1
page scale factor they want?

[jaydasika, 2016-03-22 21:47:08]

Description was changed from

==========
cc : Update render surfaces using LayerListIterator instead of treewalk

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

to

==========
cc : Update render surfaces using LayerListIterator instead of treewalk

This is the update render surfaces treewalk in draw_property_utils.cc
BUG=594024
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-03-22 22:20:30]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-03-22 22:20:30]

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/1823833002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-22 22:20:58]

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

[commit-bot: I haz the power, 2016-03-22 23:19:21]

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

[commit-bot: I haz the power, 2016-03-22 23:19:22]

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

[jaydasika, 2016-03-23 01:03:17]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-03-23 01:03:17]

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/1823833002/#ps80001
(title: "Fix ASAN unittest crashes")

[commit-bot: I haz the power, 2016-03-23 01:03:45]

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

[commit-bot: I haz the power, 2016-03-23 01:56:22]

Description was changed from

==========
cc : Update render surfaces using LayerListIterator instead of treewalk

This is the update render surfaces treewalk in draw_property_utils.cc
BUG=594024
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Update render surfaces using LayerListIterator instead of treewalk

This is the update render surfaces treewalk in draw_property_utils.cc
BUG=594024
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-03-23 01:56:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-23 01:57:45]

Description was changed from

==========
cc : Update render surfaces using LayerListIterator instead of treewalk

This is the update render surfaces treewalk in draw_property_utils.cc
BUG=594024
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Update render surfaces using LayerListIterator instead of treewalk

This is the update render surfaces treewalk in draw_property_utils.cc
BUG=594024
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2411692cd8bd39dc354fe8fef2a0a948a3df19fd
Cr-Commit-Position: refs/heads/master@{#382767}
==========

[commit-bot: I haz the power, 2016-03-23 01:57:46]

Patchset 5 (id:??) landed as
https://crrev.com/2411692cd8bd39dc354fe8fef2a0a948a3df19fd
Cr-Commit-Position: refs/heads/master@{#382767}