cc: Compute sublayer scale from effect tree

cc: Compute sublayer scale from effect tree

BUG=622372
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9d8d00a82de0233739c6f5ae4666a5ed3ad324d5
Cr-Commit-Position: refs/heads/master@{#404033}

[jaydasika, 2016-07-01 20:14:16]

Description was changed from

==========
cc: Compute sublayer scale from effect tree

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

to

==========
cc: Compute sublayer scale from effect tree

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

[jaydasika, 2016-07-01 20:28:35]

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

[jaydasika, 2016-07-01 20:28:35]

PTAL

[ajuma, 2016-07-04 15:11:51]

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

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:73: if (target_node_id != 0) {
Does node 0 need to be special-cased only because sublayer_layer is always (1,1)
there, or does something else go wrong in that case? Please add a comment.

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:81: transform_tree.Node(target_node_id);
Since this is used only for the DCHECK, it might be better to move this into the
DCHECK rather than creating a local variable for it.

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:82:
DCHECK(target_transform_node->data.sublayer_scale ==
Can this be a DCHECK_EQ instead?

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:933: if (destination_id != 0) {
Same comment as above about special-casing node 0.

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:939:
DCHECK(target_transform_node->data.sublayer_scale ==
DCHECK_EQ (and then we get the error message for free)

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1135:
DCHECK(target_transform_node->data.sublayer_scale ==
Here too, same comments about node 0 and DCHECK_EQ :)

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1141: }
Would it make sense to write a helper function with this logic and re-use it in
the other places above where we're computing and using sublayer scale?

[jaydasika, 2016-07-06 18:52:31]

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

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:73: if (target_node_id != 0) {
On 2016/07/04 15:11:50, ajuma wrote:
> Does node 0 need to be special-cased only because sublayer_layer is always
(1,1)
> there, or does something else go wrong in that case? Please add a comment.

Done.

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:81: transform_tree.Node(target_node_id);
On 2016/07/04 15:11:50, ajuma wrote:
> Since this is used only for the DCHECK, it might be better to move this into
the
> DCHECK rather than creating a local variable for it.

Done.

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:82:
DCHECK(target_transform_node->data.sublayer_scale ==
On 2016/07/04 15:11:50, ajuma wrote:
> Can this be a DCHECK_EQ instead?

DCHECK_EQ doesn't seem to support comparing vectors.

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:933: if (destination_id != 0) {
On 2016/07/04 15:11:50, ajuma wrote:
> Same comment as above about special-casing node 0.

Done.

https://codereview.chromium.org/2117853004/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1141: }
On 2016/07/04 15:11:50, ajuma wrote:
> Would it make sense to write a helper function with this logic and re-use it
in
> the other places above where we're computing and using sublayer scale?

Done.

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

https://codereview.chromium.org/2117853004/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:146: gfx::RectF rect,
Ah, forgot to upload immediately after rebasing. This is all rebase.

[ajuma, 2016-07-06 21:35:29]

Thanks, lgtm

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

https://codereview.chromium.org/2117853004/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1166:
VerifySublayerScalesMatch(effect_node->data.target_id,
This should be called only if DCHECK_IS_ON.

[jaydasika, 2016-07-06 22:18:40]

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

https://codereview.chromium.org/2117853004/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1166:
VerifySublayerScalesMatch(effect_node->data.target_id,
On 2016/07/06 21:35:29, ajuma wrote:
> This should be called only if DCHECK_IS_ON.

Done.

[jaydasika, 2016-07-06 22:18:45]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-07-06 22:18:45]

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/2117853004/#ps80001
(title: ""comments"")

[commit-bot: I haz the power, 2016-07-06 22:19:49]

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-06 23:11:29]

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

[commit-bot: I haz the power, 2016-07-06 23:11:30]

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

[jaydasika, 2016-07-06 23:16:47]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-07-06 23:17:27]

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-07 02:37:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-07 02:39:33]

Description was changed from

==========
cc: Compute sublayer scale from effect tree

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

to

==========
cc: Compute sublayer scale from effect tree

BUG=622372
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9d8d00a82de0233739c6f5ae4666a5ed3ad324d5
Cr-Commit-Position: refs/heads/master@{#404033}
==========

[commit-bot: I haz the power, 2016-07-07 02:39:34]

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