cc : Reland move screen space scale factor to root transform node

cc : Reland move screen space scale factor to root transform node

This is relanding the CL :
https://codereview.chromium.org/2408243002
If DT is the device transform, DSF is matrix scaled by device scale
factor and page scale factor for root, RT is matrix translated by root's
position, the screen space transform of root layer should be DT*DSF*RT,
https://codereview.chromium.org/2408243002 changed this to DSF*DT*RT and
that caused https://crbug.com/660047. This CL fixes it.

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

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

[jaydasika, 2016-11-01 18:58:38]

Description was changed from

==========
cc : Reland move screen space scale factor to root transform node

This is relanding the CL :
https://codereview.chromium.org/2408243002
If DT is the device transform, DSF is matrix scaled by device scale
factor and page scale factor for root, RT is matrix translated by root's
position, the screen space transform of root layer should be DT*DSF*RT,
https://codereview.chromium.org/2408243002 changed this to DSF*DT*RT and
that caused https://crbug.com/660047. This CL fixes it.

BUG=644582
==========

to

==========
cc : Reland move screen space scale factor to root transform node

This is relanding the CL :
https://codereview.chromium.org/2408243002
If DT is the device transform, DSF is matrix scaled by device scale
factor and page scale factor for root, RT is matrix translated by root's
position, the screen space transform of root layer should be DT*DSF*RT,
https://codereview.chromium.org/2408243002 changed this to DSF*DT*RT and
that caused https://crbug.com/660047. This CL fixes it.

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

[jaydasika, 2016-11-01 20:54:02]

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

[commit-bot: I haz the power, 2016-11-01 20:54:46]

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

[jaydasika, 2016-11-01 20:54:48]

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

[jaydasika, 2016-11-01 20:54:48]

PTAL

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

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

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

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

[jaydasika, 2016-11-01 23:58:45]

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

[commit-bot: I haz the power, 2016-11-01 23:59:07]

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-11-02 00:52:53]

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

[commit-bot: I haz the power, 2016-11-02 00:52:54]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2016-11-02 15:38:21]

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

https://codereview.chromium.org/2473453002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:710: void
TransformTree::SetDeviceTransformScaleFactor(
Could this function be folded into above function? I assume the device transform
passed in here should be same as device transform in above function?

[jaydasika, 2016-11-02 17:36:49]

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

[jaydasika, 2016-11-02 17:36:55]

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

https://codereview.chromium.org/2473453002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:710: void
TransformTree::SetDeviceTransformScaleFactor(
On 2016/11/02 15:38:21, weiliangc wrote:
> Could this function be folded into above function? I assume the device
transform
> passed in here should be same as device transform in above function?

Done.

[commit-bot: I haz the power, 2016-11-02 17:37:13]

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

[weiliangc, 2016-11-02 20:20:11]

LGTM

[jaydasika, 2016-11-02 20:21:51]

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

[jaydasika, 2016-11-02 20:21:51]

+pdr for PaintArtifactCompositor

[pdr., 2016-11-02 20:27:25]

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

https://codereview.chromium.org/2473453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:457:
// transform
Nit: the comment wrapping is a little weird here. Please rewrap to something
like:

// TODO(jaydasika): We shouldn't set ToScreeen and FromScreen of root
// transform node here. They should be set while updating transform
// tree in cc.

https://codereview.chromium.org/2473453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:464:
DCHECK(invertible);
Why is invertible required now but wasn't in your previous patch?

Do we need to update ancestors_are_invertible?

[jaydasika, 2016-11-02 20:45:47]

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

https://codereview.chromium.org/2473453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:457:
// transform
On 2016/11/02 20:27:25, pdr. wrote:
> Nit: the comment wrapping is a little weird here. Please rewrap to something
> like:
> 
> // TODO(jaydasika): We shouldn't set ToScreeen and FromScreen of root
> // transform node here. They should be set while updating transform
> // tree in cc.

Done.

https://codereview.chromium.org/2473453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:464:
DCHECK(invertible);
On 2016/11/02 20:27:25, pdr. wrote:
> Why is invertible required now but wasn't in your previous patch?
> 
> Do we need to update ancestors_are_invertible?

In the previous patch, it was
if (!toScreen.GetInverse(to_screen)
  ancestors_are_invertible = false
I realized that since its just a scale matrix, inverse will always exist and so
I changed it to DCHECK (ancestors_are_invertible is initliazed to true so we
don't have to explicitly set it to true here).

[pdr., 2016-11-02 20:51:54]

On 2016/11/02 at 20:45:47, jaydasika wrote:
>
https://codereview.chromium.org/2473453002/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):
> 
>
https://codereview.chromium.org/2473453002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:457:
// transform
> On 2016/11/02 20:27:25, pdr. wrote:
> > Nit: the comment wrapping is a little weird here. Please rewrap to something
> > like:
> > 
> > // TODO(jaydasika): We shouldn't set ToScreeen and FromScreen of root
> > // transform node here. They should be set while updating transform
> > // tree in cc.
> 
> Done.
> 
>
https://codereview.chromium.org/2473453002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:464:
DCHECK(invertible);
> On 2016/11/02 20:27:25, pdr. wrote:
> > Why is invertible required now but wasn't in your previous patch?
> > 
> > Do we need to update ancestors_are_invertible?
> 
> In the previous patch, it was
> if (!toScreen.GetInverse(to_screen)
>   ancestors_are_invertible = false
> I realized that since its just a scale matrix, inverse will always exist and
so I changed it to DCHECK (ancestors_are_invertible is initliazed to true so we
don't have to explicitly set it to true here).

SGTM LGTM

[jaydasika, 2016-11-02 20:52:42]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-11-02 20:52:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
weiliangc@chromium.org
Link to the patchset: https://codereview.chromium.org/2473453002/#ps60001
(title: "comments")

[commit-bot: I haz the power, 2016-11-02 20:53:06]

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-11-02 22:34:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-02 22:36:59]

Description was changed from

==========
cc : Reland move screen space scale factor to root transform node

This is relanding the CL :
https://codereview.chromium.org/2408243002
If DT is the device transform, DSF is matrix scaled by device scale
factor and page scale factor for root, RT is matrix translated by root's
position, the screen space transform of root layer should be DT*DSF*RT,
https://codereview.chromium.org/2408243002 changed this to DSF*DT*RT and
that caused https://crbug.com/660047. This CL fixes it.

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

to

==========
cc : Reland move screen space scale factor to root transform node

This is relanding the CL :
https://codereview.chromium.org/2408243002
If DT is the device transform, DSF is matrix scaled by device scale
factor and page scale factor for root, RT is matrix translated by root's
position, the screen space transform of root layer should be DT*DSF*RT,
https://codereview.chromium.org/2408243002 changed this to DSF*DT*RT and
that caused https://crbug.com/660047. This CL fixes it.

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

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

[commit-bot: I haz the power, 2016-11-02 22:37:00]

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