Use surface contents scale from effect tree (3)

Use surface contents scale from effect tree (3)

With this CL, all call sites in draw_property_utils use
surface contents scale from effect tree.

BUG=622372
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/baede2d92aa847c591c93e5256e7a957461f0c6b
Cr-Commit-Position: refs/heads/master@{#406429}

[jaydasika, 2016-07-19 21:22:42]

Description was changed from

==========
Use surface contents scale from effect tree (3)

BUG=622372
==========

to

==========
Use surface contents scale from effect tree (3)

BUG=622372
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-07-19 21:26:16]

Description was changed from

==========
Use surface contents scale from effect tree (3)

BUG=622372
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Use surface contents scale from effect tree (3)

With this CL, all call sites in draw_property_utils use 
surface contents scale from effect tree.

BUG=622372
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-07-19 21:26:35]

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

[jaydasika, 2016-07-19 21:26:51]

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

[jaydasika, 2016-07-19 21:26:52]

PTAL

[commit-bot: I haz the power, 2016-07-19 21:27:31]

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

[jaydasika, 2016-07-19 21:45:59]

removed some dead code

[ajuma, 2016-07-19 21:59:58]

Generally looks good, I just have some nits.

https://codereview.chromium.org/2164833002/diff/40001/cc/layers/render_surfac...
File cc/layers/render_surface_unittest.cc (right):

https://codereview.chromium.org/2164833002/diff/40001/cc/layers/render_surfac...
cc/layers/render_surface_unittest.cc:111: draw_property_utils::ComputeEffects(
Would it make sense to call active_tree()->UpdateDrawProperties() here instead?

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1207: // Surfaces need to apply their surface
contents scale. we don't have to
Nit: capitalize 'w'

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1209: if (layer->transform_tree_index() != 0) {
Should this be checking for the root surface rather than the root transform node
(since we could have a layer with a non-root transform node that still draws
into the root render surface)?

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1395: if (render_surface->TransformTreeIndex()
!= 0) {
Checking the effect tree index might be cleaner here (since it means we're not
depending on each surface creating a transform node).

[jaydasika, 2016-07-19 22:56:12]

https://codereview.chromium.org/2164833002/diff/40001/cc/layers/render_surfac...
File cc/layers/render_surface_unittest.cc (right):

https://codereview.chromium.org/2164833002/diff/40001/cc/layers/render_surfac...
cc/layers/render_surface_unittest.cc:111: draw_property_utils::ComputeEffects(
On 2016/07/19 21:59:58, ajuma wrote:
> Would it make sense to call active_tree()->UpdateDrawProperties() here
instead?

Done.

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1207: // Surfaces need to apply their surface
contents scale. we don't have to
On 2016/07/19 21:59:58, ajuma wrote:
> Nit: capitalize 'w'

Done.

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1209: if (layer->transform_tree_index() != 0) {
On 2016/07/19 21:59:58, ajuma wrote:
> Should this be checking for the root surface rather than the root transform
node
> (since we could have a layer with a non-root transform node that still draws
> into the root render surface)?

this is inside the else (= owns_non_root_surface). So I think we can remove the
check completely.

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1395: if (render_surface->TransformTreeIndex()
!= 0) {
On 2016/07/19 21:59:58, ajuma wrote:
> Checking the effect tree index might be cleaner here (since it means we're not
> depending on each surface creating a transform node).

All callsites that apply surface content scale right now check transform tree
index. I remember trying to use the effect tree index in one of the earlier CLs.
But many tests hit the DCHECK in VerifySublayerScalesMatch as
transform_tree_index = 0 doesn't imply effect_tree_index = 0.
This needs to be sorted out, but I would prefer to do it in a seperate CL.

[jaydasika, 2016-07-19 22:56:21]

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

[commit-bot: I haz the power, 2016-07-19 22:57:09]

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

[ajuma, 2016-07-19 23:08:38]

Thanks, lgtm

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2164833002/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1395: if (render_surface->TransformTreeIndex()
!= 0) {
On 2016/07/19 22:56:11, jaydasika wrote:
> On 2016/07/19 21:59:58, ajuma wrote:
> > Checking the effect tree index might be cleaner here (since it means we're
not
> > depending on each surface creating a transform node).
> 
> All callsites that apply surface content scale right now check transform tree
> index. I remember trying to use the effect tree index in one of the earlier
CLs.
> But many tests hit the DCHECK in VerifySublayerScalesMatch as
> transform_tree_index = 0 doesn't imply effect_tree_index = 0.
> This needs to be sorted out, but I would prefer to do it in a seperate CL.

Sounds good.

[commit-bot: I haz the power, 2016-07-20 00:22:31]

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

[commit-bot: I haz the power, 2016-07-20 00:22:32]

Dry run: This issue passed the CQ dry run.

[jaydasika, 2016-07-20 00:28:36]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-07-20 00:29:17]

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, 2016-07-20 00:35:06]

Description was changed from

==========
Use surface contents scale from effect tree (3)

With this CL, all call sites in draw_property_utils use 
surface contents scale from effect tree.

BUG=622372
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Use surface contents scale from effect tree (3)

With this CL, all call sites in draw_property_utils use 
surface contents scale from effect tree.

BUG=622372
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-20 00:35:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-20 00:35:17]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 00:37:01]

Description was changed from

==========
Use surface contents scale from effect tree (3)

With this CL, all call sites in draw_property_utils use 
surface contents scale from effect tree.

BUG=622372
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Use surface contents scale from effect tree (3)

With this CL, all call sites in draw_property_utils use
surface contents scale from effect tree.

BUG=622372
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/baede2d92aa847c591c93e5256e7a957461f0c6b
Cr-Commit-Position: refs/heads/master@{#406429}
==========

[commit-bot: I haz the power, 2016-07-20 00:37:02]

Patchset 4 (id:??) landed as
https://crrev.com/baede2d92aa847c591c93e5256e7a957461f0c6b
Cr-Commit-Position: refs/heads/master@{#406429}