cc : Move screen space scale factor to root transform node

cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) was having the scale baked in.
(and this caused the bug)

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6
Cr-Commit-Position: refs/heads/master@{#427366}

[jaydasika, 2016-10-11 19:35:10]

Description was changed from

==========
cc : Move screen space scale factor to root transform node

BUG=651584
==========

to

==========
cc : Move screen space scale factor to root transform node

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-10-11 21:48:20]

Description was changed from

==========
cc : Move screen space scale factor to root transform node

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) would have the scale baked in.
(and this caused the bug)

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-10-11 21:49:16]

Description was changed from

==========
cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) would have the scale baked in.
(and this caused the bug)

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) was having the scale baked in.
(and this caused the bug)

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-10-11 21:54:19]

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

[jaydasika, 2016-10-11 21:54:20]

Sending it out for initial comments. I still need to add a test for the bug its
fixing, but I have verified that it fixes the bug.

https://codereview.chromium.org/2408243002/diff/20001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2408243002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:224: void SetDeviceTransform(const gfx::Transform&
transform,
This needs to be renamed. SetContentsRootPostLocalTransform could be one. Any
better suggestions ?

[jaydasika, 2016-10-12 00:30:22]

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

[commit-bot: I haz the power, 2016-10-12 00:31:02]

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, 2016-10-12 01:45:50]

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

[commit-bot: I haz the power, 2016-10-12 01:45:51]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[jaydasika, 2016-10-12 22:22:52]

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

[commit-bot: I haz the power, 2016-10-12 22:23:21]

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

[jaydasika, 2016-10-12 23:05:08]

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

[commit-bot: I haz the power, 2016-10-12 23:05:38]

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

[jaydasika, 2016-10-12 23:07:48]

Made changes to PAC to put device scale factor into the root transform node
there too and added a cc unit test for the bug.

[commit-bot: I haz the power, 2016-10-13 00:48:30]

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

[commit-bot: I haz the power, 2016-10-13 00:48:31]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2016-10-13 21:16:18]

Thanks for pushing this for merge to 55. Overall looks good.

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

https://codereview.chromium.org/2408243002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:9503: float device_scale_factor =
2.f;
Could you add a case with device scale factor = 1.f before this to show that the
device scale factor would change the clip_rect?

Thanks.

https://codereview.chromium.org/2408243002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/2408243002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:461:
// TODO(jaydasika) : We shouldn't set ToScreen and FromScreen of root
Why does this not work in cc right now?

[jaydasika, 2016-10-13 21:46:48]

https://codereview.chromium.org/2408243002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/2408243002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:461:
// TODO(jaydasika) : We shouldn't set ToScreen and FromScreen of root
On 2016/10/13 21:16:18, weiliangc wrote:
> Why does this not work in cc right now?

It doesn't work. We only update transforms from node 1. In cc, you can see a
similar update for node 0 in SetScreenSpaceScale. I plan to remove them in a
follow-up.

[jaydasika, 2016-10-13 22:02:22]

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

https://codereview.chromium.org/2408243002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:9503: float device_scale_factor =
2.f;
On 2016/10/13 21:16:18, weiliangc wrote:
> Could you add a case with device scale factor = 1.f before this to show that
the
> device scale factor would change the clip_rect?
> 
> Thanks.

Done.

[jaydasika, 2016-10-13 22:02:32]

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

[commit-bot: I haz the power, 2016-10-13 22:02:50]

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, 2016-10-13 23:14:29]

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

[commit-bot: I haz the power, 2016-10-13 23:14:30]

Dry run: 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-10-14 05:31:55]

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

[commit-bot: I haz the power, 2016-10-14 05:32:08]

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, 2016-10-14 06:48:54]

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

[commit-bot: I haz the power, 2016-10-14 06:48:55]

Dry run: This issue passed the CQ dry run.

[ajuma, 2016-10-14 23:16:03]

lgtm on behalf of Wei (who asked me offline to lgtm this)

[jaydasika, 2016-10-14 23:19:00]

jaydasika@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[jaydasika, 2016-10-14 23:19:01]

+ pdr/chrishtr for changes to PaintArtifactCompositor

[pdr., 2016-10-14 23:54:22]

On 2016/10/14 at 23:19:01, jaydasika wrote:
> + pdr/chrishtr for changes to PaintArtifactCompositor

PaintArtifactCompositor.cpp LGTM

[jaydasika, 2016-10-14 23:56:04]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-10-14 23:56:24]

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

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

[commit-bot: I haz the power, 2016-10-15 00:32:39]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jaydasika, 2016-10-15 03:08:33]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-10-15 03:08:37]

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-10-15 03:52:03]

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

[commit-bot: I haz the power, 2016-10-15 03:52:04]

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

[jaydasika, 2016-10-20 19:14:58]

Patchset #7 (id:120001) has been deleted

[jaydasika, 2016-10-20 23:23:01]

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

[commit-bot: I haz the power, 2016-10-20 23:23:40]

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, 2016-10-21 01:22:38]

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

[commit-bot: I haz the power, 2016-10-21 01:22:39]

Dry run: 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-10-21 17:38:17]

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

[commit-bot: I haz the power, 2016-10-21 17:39: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, 2016-10-21 19:17:14]

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

[commit-bot: I haz the power, 2016-10-21 19:17:20]

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...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jaydasika, 2016-10-21 21:59:38]

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

[commit-bot: I haz the power, 2016-10-21 22:00:05]

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

[jaydasika, 2016-10-22 00:18:23]

PTAL (see the diff b/w Patch 7 and the last patch for new changes)

https://codereview.chromium.org/2408243002/diff/220001/cc/trees/effect_node.cc
File cc/trees/effect_node.cc (right):

https://codereview.chromium.org/2408243002/diff/220001/cc/trees/effect_node.c...
cc/trees/effect_node.cc:36: target_id(1),
Effect node 0 is a dummy node, the actual root is node 1. So, setting default
target to 1. This is required because screen space surface contents scale for
root is stored on node 1.

[jaydasika, 2016-10-22 00:18:41]

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

[commit-bot: I haz the power, 2016-10-22 00:19:36]

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, 2016-10-22 03:12:39]

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

[commit-bot: I haz the power, 2016-10-22 03:12:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jaydasika, 2016-10-22 04:01:01]

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

[commit-bot: I haz the power, 2016-10-22 04:01:33]

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, 2016-10-22 05:33:45]

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

[commit-bot: I haz the power, 2016-10-22 05:33:47]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2016-10-24 21:29:36]

Overall looks like reasonable approach. Since this patch is meant to be merged
into M55, probably best to run cc_unittest on after merge on your own.

Thanks for pushing this!

https://codereview.chromium.org/2408243002/diff/220001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2408243002/diff/220001/cc/trees/property_tree...
cc/trees/property_tree.cc:709: post_local.matrix().postScale(1.f /
device_transform_scale_components.x(),
This assumes device transform's scale components is part of the scale on the
root surface, right? Can this assumption be tested/guaranteed? For example is
SetDeviceTransform always called with SetScreenSpaceScale? Could these two calls
being bundled together?

[jaydasika, 2016-10-25 01:20:42]

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

[jaydasika, 2016-10-25 01:20:59]

https://codereview.chromium.org/2408243002/diff/220001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2408243002/diff/220001/cc/trees/property_tree...
cc/trees/property_tree.cc:709: post_local.matrix().postScale(1.f /
device_transform_scale_components.x(),
On 2016/10/24 21:29:36, weiliangc wrote:
> This assumes device transform's scale components is part of the scale on the
> root surface, right? Can this assumption be tested/guaranteed? For example is
> SetDeviceTransform always called with SetScreenSpaceScale? Could these two
calls
> being bundled together?

Done.

[commit-bot: I haz the power, 2016-10-25 01:21:13]

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, 2016-10-25 01:55:14]

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

[commit-bot: I haz the power, 2016-10-25 01:55:15]

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

[jaydasika, 2016-10-25 01:56:06]

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

[commit-bot: I haz the power, 2016-10-25 01:56:43]

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, 2016-10-25 02:58:30]

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

[commit-bot: I haz the power, 2016-10-25 02:58:31]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2016-10-25 15:43:58]

LGTM

[weiliangc, 2016-10-25 15:44:10]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2016-10-25 15:44:11]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2408243002/#ps240001
(title: "comments")

[commit-bot: I haz the power, 2016-10-25 15:44:31]

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-10-25 15:50:16]

Description was changed from

==========
cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) was having the scale baked in.
(and this caused the bug)

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) was having the scale baked in.
(and this caused the bug)

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-25 15:50:18]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-10-25 16:14:32]

Description was changed from

==========
cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) was having the scale baked in.
(and this caused the bug)

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Move screen space scale factor to root transform node

This CL computes the screen space scale factor which is
combined form scale factors of device transform, device
scale factor and page scale factor(if required), and stores
it on the root transform node as its surface contents scale.
This also implicitly fixes a clipping bug.
TransformTree::ComputeTransforms(a, b) should return the
transform between a and b without the surface contents
scale. But, since screen space scale was baked into the
local transform of the contents root node (before this CL),
ComputeTransform(a, root) was having the scale baked in.
(and this caused the bug)

BUG=651584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6
Cr-Commit-Position: refs/heads/master@{#427366}
==========

[commit-bot: I haz the power, 2016-10-25 16:14:34]

Patchset 12 (id:??) landed as
https://crrev.com/61fa0fb418a3f77665e0bc1be88ec5a2984c6dc6
Cr-Commit-Position: refs/heads/master@{#427366}

[jaydasika, 2016-10-27 20:55:06]

A revert of this CL (patchset #12 id:240001) has been created in
https://codereview.chromium.org/2452303003/ by jaydasika@chromium.org.

The reason for reverting is:
https://bugs.chromium.org/p/chromium/issues/detail?id=660047.