cc: Replace LayerIterator with iterator that walks layer list and effect tree

cc: Replace LayerIterator with iterator that walks layer list and effect tree

LayerIterator was used to traverse the render surface layer list in
front-to-back order. This assumed that every render surface had a
corresponding layer that could be used to represent it. In the future
we want to be able to create render surfaces for effect nodes that don't
have any owning layer.

This CL replaces LayerIterator with EffectTreeLayerListIterator, an
iterator that visits layer and render surfaces in the same order as
LayerIterator, but using only the layer list and effect tree, not the
render surface layer list.

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

Review-Url: https://codereview.chromium.org/2751783002
Cr-Commit-Position: refs/heads/master@{#461804}
Committed: https://chromium.googlesource.com/chromium/src/+/95621958b771f10bac0a7690cdbc6f610573e8a1

[ajuma, 2017-03-14 18:40:15]

Description was changed from

==========
WIP: Iterator for walking the layer list and effect tree

Not ready for review

BUG=
==========

to

==========
WIP: Iterator for walking the layer list and effect tree

Not ready for review

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

[ajuma, 2017-03-14 20:00:04]

Description was changed from

==========
WIP: Iterator for walking the layer list and effect tree

Not ready for review

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

to

==========
WIP: Iterator for walking the layer list and effect tree

Not ready for review

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

[ajuma, 2017-03-14 20:00:25]

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

[commit-bot: I haz the power, 2017-03-14 20:01:05]

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

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

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

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ajuma, 2017-03-14 20:50:53]

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

[commit-bot: I haz the power, 2017-03-14 20:51:38]

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-15 00:19:32]

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

[commit-bot: I haz the power, 2017-03-15 00:19:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ajuma, 2017-03-21 23:36:49]

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

[commit-bot: I haz the power, 2017-03-21 23:37:17]

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-21 23:40:18]

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

[commit-bot: I haz the power, 2017-03-21 23:40:19]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ajuma, 2017-03-22 15:50:08]

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

[commit-bot: I haz the power, 2017-03-22 15:50:38]

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-22 15:54:25]

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

[commit-bot: I haz the power, 2017-03-22 15:54:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_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_...)
  linux_chromium_tsan_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_...)

[ajuma, 2017-03-22 15:56:51]

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

[commit-bot: I haz the power, 2017-03-22 15:57:06]

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-22 17:02:41]

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

[commit-bot: I haz the power, 2017-03-22 17:02:42]

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

[ajuma, 2017-03-22 18:05:23]

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

[commit-bot: I haz the power, 2017-03-22 18:05:58]

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

[ajuma, 2017-03-22 18:14:01]

Description was changed from

==========
WIP: Iterator for walking the layer list and effect tree

Not ready for review

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

to

==========
cc: Replace LayerIterator with iterator that walks layer list and effect tree

LayerIterator was used to traverse the render surface layer list in
front-to-back order. This assumed that every render surface had a
corresponding layer that can be used to represent it. In the future
we want to be able to create render surfaces for effect nodes that don't
have any owning layer.

This CL replaces LayerIterator with EffectTreeLayerListIterator, an
iterator that visits layer and render surfaces in the same order as
LayerIterator, but using only the layer list and effect tree, not the
render surface layer list.

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

[ajuma, 2017-03-22 18:21:41]

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

[ajuma, 2017-03-22 18:22:08]

Description was changed from

==========
cc: Replace LayerIterator with iterator that walks layer list and effect tree

LayerIterator was used to traverse the render surface layer list in
front-to-back order. This assumed that every render surface had a
corresponding layer that can be used to represent it. In the future
we want to be able to create render surfaces for effect nodes that don't
have any owning layer.

This CL replaces LayerIterator with EffectTreeLayerListIterator, an
iterator that visits layer and render surfaces in the same order as
LayerIterator, but using only the layer list and effect tree, not the
render surface layer list.

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

to

==========
cc: Replace LayerIterator with iterator that walks layer list and effect tree

LayerIterator was used to traverse the render surface layer list in
front-to-back order. This assumed that every render surface had a
corresponding layer that can be used to represent it. In the future
we want to be able to create render surfaces for effect nodes that don't
have any owning layer.

This CL replaces LayerIterator with EffectTreeLayerListIterator, an
iterator that visits layer and render surfaces in the same order as
LayerIterator, but using only the layer list and effect tree, not the
render surface layer list.

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

[ajuma, 2017-03-22 18:22:09]

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

[commit-bot: I haz the power, 2017-03-22 18:22:20]

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

[ajuma, 2017-03-22 18:22:24]

Description was changed from

==========
cc: Replace LayerIterator with iterator that walks layer list and effect tree

LayerIterator was used to traverse the render surface layer list in
front-to-back order. This assumed that every render surface had a
corresponding layer that can be used to represent it. In the future
we want to be able to create render surfaces for effect nodes that don't
have any owning layer.

This CL replaces LayerIterator with EffectTreeLayerListIterator, an
iterator that visits layer and render surfaces in the same order as
LayerIterator, but using only the layer list and effect tree, not the
render surface layer list.

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

to

==========
cc: Replace LayerIterator with iterator that walks layer list and effect tree

LayerIterator was used to traverse the render surface layer list in
front-to-back order. This assumed that every render surface had a
corresponding layer that could be used to represent it. In the future
we want to be able to create render surfaces for effect nodes that don't
have any owning layer.

This CL replaces LayerIterator with EffectTreeLayerListIterator, an
iterator that visits layer and render surfaces in the same order as
LayerIterator, but using only the layer list and effect tree, not the
render surface layer list.

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

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

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

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

Dry run: This issue passed the CQ dry run.

[jaydasika, 2017-03-22 20:49:42]

https://codereview.chromium.org/2751783002/diff/220001/cc/layers/effect_tree_...
File cc/layers/effect_tree_layer_list_iterator.cc (right):

https://codereview.chromium.org/2751783002/diff/220001/cc/layers/effect_tree_...
cc/layers/effect_tree_layer_list_iterator.cc:29: if (effect_tree_->size() >
EffectTree::kContentsRootNodeId &&
Root render surface should always exist, right ? In that case, can this be a
DCHECK ?

https://codereview.chromium.org/2751783002/diff/220001/cc/layers/effect_tree_...
cc/layers/effect_tree_layer_list_iterator.cc:76: break;
Should current_effect_tree_index_ be set to EffectTree::kContentsRootId here ?

https://codereview.chromium.org/2751783002/diff/220001/cc/layers/effect_tree_...
cc/layers/effect_tree_layer_list_iterator.cc:129: state_ =
State::TARGET_SURFACE;
Is there an underlying assumption here that
lowest_common_effect_tree_ancestor_index_ = next_effect_tree_index_ in this case
? If yes, is that always true ?

https://codereview.chromium.org/2751783002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2751783002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:946: }
Lines 923-946 was common code for all three states before. Now, its only for
EffectTreeLayerListIterator::State::LAYER. Why ?

https://codereview.chromium.org/2751783002/diff/220001/cc/trees/occlusion_tra...
File cc/trees/occlusion_tracker.cc (left):

https://codereview.chromium.org/2751783002/diff/220001/cc/trees/occlusion_tra...
cc/trees/occlusion_tracker.cc:189: surface->HasCopyRequest() &&
finished_target->IsHidden();
Can LayerImpl::IsHidden be deleted as its not being used anymore ?

[ajuma, 2017-03-24 15:01:48]

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

[commit-bot: I haz the power, 2017-03-24 15:02:57]

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

[ajuma, 2017-03-24 17:25:07]

https://codereview.chromium.org/2751783002/diff/220001/cc/layers/effect_tree_...
File cc/layers/effect_tree_layer_list_iterator.cc (right):

https://codereview.chromium.org/2751783002/diff/220001/cc/layers/effect_tree_...
cc/layers/effect_tree_layer_list_iterator.cc:29: if (effect_tree_->size() >
EffectTree::kContentsRootNodeId &&
On 2017/03/22 20:49:42, jaydasika wrote:
> Root render surface should always exist, right ? In that case, can this be a
> DCHECK ?

Done. A root render surface exists as long as we have at least one layer. There
was a single EffectTreeLayerListIteratorTest that had an empty tree, so I
changed that to a tree with no drawn layers.

https://codereview.chromium.org/2751783002/diff/220001/cc/layers/effect_tree_...
cc/layers/effect_tree_layer_list_iterator.cc:76: break;
On 2017/03/22 20:49:42, jaydasika wrote:
> Should current_effect_tree_index_ be set to EffectTree::kContentsRootId here ?

Not necessarily, it should (already) be the render target effect tree index of
the last drawn layer. This might be kContentsRoodId, but can also be something
else if the last drawn layer contributes to some other surface (e.g., if we have
the layer tree root->filter->layer, where |layer| is the only layer that draws
content, then current_effect_tree_index_ would be |filter|'s effect node id).

https://codereview.chromium.org/2751783002/diff/220001/cc/layers/effect_tree_...
cc/layers/effect_tree_layer_list_iterator.cc:129: state_ =
State::TARGET_SURFACE;
On 2017/03/22 20:49:42, jaydasika wrote:
> Is there an underlying assumption here that
> lowest_common_effect_tree_ancestor_index_ = next_effect_tree_index_ in this
case
> ? If yes, is that always true ?

No, the assumption is only that the lowest_common_effect_tree_ancestor_index
node is a proper ancestor of the current_effect_tree_index node. That means
there can't be anything else contributing to the current node. Added a comment
about this.

https://codereview.chromium.org/2751783002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2751783002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:946: }
On 2017/03/22 20:49:42, jaydasika wrote:
> Lines 923-946 was common code for all three states before. Now, its only for
> EffectTreeLayerListIterator::State::LAYER. Why ?

append_quads_data only gets populated in the layer case, so the code was a no-op
for the other states. It needed to be cleaned up here since it refers to a
layer; previously we always had a layer to refer to, but now in the other states
we don't have a layer.

https://codereview.chromium.org/2751783002/diff/220001/cc/trees/occlusion_tra...
File cc/trees/occlusion_tracker.cc (left):

https://codereview.chromium.org/2751783002/diff/220001/cc/trees/occlusion_tra...
cc/trees/occlusion_tracker.cc:189: surface->HasCopyRequest() &&
finished_target->IsHidden();
On 2017/03/22 20:49:42, jaydasika wrote:
> Can LayerImpl::IsHidden be deleted as its not being used anymore ?

Good catch, there was still one unit test using it, so I changed that to get
screen space opacity from the layer's effect node.

[commit-bot: I haz the power, 2017-03-24 17:49:10]

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

[commit-bot: I haz the power, 2017-03-24 17:49:11]

Dry run: This issue passed the CQ dry run.

[jaydasika, 2017-03-24 18:53:53]

https://codereview.chromium.org/2751783002/diff/240001/cc/layers/effect_tree_...
File cc/layers/effect_tree_layer_list_iterator.cc (right):

https://codereview.chromium.org/2751783002/diff/240001/cc/layers/effect_tree_...
cc/layers/effect_tree_layer_list_iterator.cc:30: if
(effect_tree_->GetRenderSurface(EffectTree::kContentsRootNodeId)) {
Isn't this always true after DCHECKing that effect tree's size > 1 ?

[ajuma, 2017-03-24 22:26:14]

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

[ajuma, 2017-03-24 22:26:44]

https://codereview.chromium.org/2751783002/diff/240001/cc/layers/effect_tree_...
File cc/layers/effect_tree_layer_list_iterator.cc (right):

https://codereview.chromium.org/2751783002/diff/240001/cc/layers/effect_tree_...
cc/layers/effect_tree_layer_list_iterator.cc:30: if
(effect_tree_->GetRenderSurface(EffectTree::kContentsRootNodeId)) {
On 2017/03/24 18:53:53, jaydasika wrote:
> Isn't this always true after DCHECKing that effect tree's size > 1 ?

Good point, removed.

[commit-bot: I haz the power, 2017-03-24 22:26:45]

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

[jaydasika, 2017-03-24 22:32:31]

lgtm

Thanks for all the comments, made it easy to follow.

[commit-bot: I haz the power, 2017-03-24 23:40:48]

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

[commit-bot: I haz the power, 2017-03-24 23:40:49]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-04-04 17:09:16]

The CQ bit was checked by ajuma@chromium.org

[ajuma, 2017-04-04 17:09:17]

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

[commit-bot: I haz the power, 2017-04-04 17:09:51]

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-04-04 19:58:21]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1491325756906960,
"parent_rev": "dfdfa5a0d0415d80b0f284e14e90638c4b54b025", "commit_rev":
"95621958b771f10bac0a7690cdbc6f610573e8a1"}

[commit-bot: I haz the power, 2017-04-04 19:59:15]

Description was changed from

==========
cc: Replace LayerIterator with iterator that walks layer list and effect tree

LayerIterator was used to traverse the render surface layer list in
front-to-back order. This assumed that every render surface had a
corresponding layer that could be used to represent it. In the future
we want to be able to create render surfaces for effect nodes that don't
have any owning layer.

This CL replaces LayerIterator with EffectTreeLayerListIterator, an
iterator that visits layer and render surfaces in the same order as
LayerIterator, but using only the layer list and effect tree, not the
render surface layer list.

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

to

==========
cc: Replace LayerIterator with iterator that walks layer list and effect tree

LayerIterator was used to traverse the render surface layer list in
front-to-back order. This assumed that every render surface had a
corresponding layer that could be used to represent it. In the future
we want to be able to create render surfaces for effect nodes that don't
have any owning layer.

This CL replaces LayerIterator with EffectTreeLayerListIterator, an
iterator that visits layer and render surfaces in the same order as
LayerIterator, but using only the layer list and effect tree, not the
render surface layer list.

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

Review-Url: https://codereview.chromium.org/2751783002
Cr-Commit-Position: refs/heads/master@{#461804}
Committed:
https://chromium.googlesource.com/chromium/src/+/95621958b771f10bac0a7690cdbc...
==========

[commit-bot: I haz the power, 2017-04-04 19:59:17]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/95621958b771f10bac0a7690cdbc...