Apply container offset and scroll between transform and container perspective.

Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of container.
5. Apply clip of container.

This is incorrect. It should do steps 1 and 4 after step 2 and before step 3.

GeometryMapper already did this correctly. (And VisualRectMappingTest verifies
this).

BUG=699140
Review-Url: https://codereview.chromium.org/2808613002
Cr-Commit-Position: refs/heads/master@{#463436}
Committed: https://chromium.googlesource.com/chromium/src/+/2a2f87ba99ef135283c6cf81a173189e78357965

[chrishtr, 2017-04-08 18:31:15]

Description was changed from

==========
none


none


none


none


none


none


none


none


none

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

to

==========
Apply container offset and clip after transform, and clip after perspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. 
3. Apply scroll offset of child.
4. Apply clip of container.

BUG=699140
==========

[chrishtr, 2017-04-08 18:32:42]

Description was changed from

==========
Apply container offset and clip after transform, and clip after perspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. 
3. Apply scroll offset of child.
4. Apply clip of container.

BUG=699140
==========

to

==========
Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of child.
5. Apply clip of container.

This is incorrect. It should do step 4 before step 3.

BUG=699140
==========

[chrishtr, 2017-04-08 18:33:00]

Description was changed from

==========
Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of child.
5. Apply clip of container.

This is incorrect. It should do step 4 before step 3.

BUG=699140
==========

to

==========
Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of child.
5. Apply clip of container.

This is incorrect. It should do steps 1 and 4 after step 2 and before step 3.

BUG=699140
==========

[chrishtr, 2017-04-08 18:37:02]

Description was changed from

==========
Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of child.
5. Apply clip of container.

This is incorrect. It should do steps 1 and 4 after step 2 and before step 3.

BUG=699140
==========

to

==========
Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of child.
5. Apply clip of container.

This is incorrect. It should do steps 1 and 4 after step 2 and before step 3.

GeometryMapper already did this correctly. (And VisualRectMappingTest verifies
this).

BUG=699140
==========

[chrishtr, 2017-04-08 18:37:12]

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

[commit-bot: I haz the power, 2017-04-08 18:37:26]

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-04-08 19:50:03]

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

[commit-bot: I haz the power, 2017-04-08 19:50:03]

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

[chrishtr, 2017-04-08 20:40:51]

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

[commit-bot: I haz the power, 2017-04-08 20:41:01]

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-04-08 21:22:47]

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

[commit-bot: I haz the power, 2017-04-08 21:22:47]

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

[chrishtr, 2017-04-10 17:48:03]

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

[commit-bot: I haz the power, 2017-04-10 17:48: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, 2017-04-10 19:34:33]

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

[commit-bot: I haz the power, 2017-04-10 19:34:34]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-04-10 19:37:33]

chrishtr@chromium.org changed reviewers:
+ trchen@chromium.org

[chrishtr, 2017-04-10 19:37:35]

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1234: // TODO(chrishtr):
This applies perspective before the transform. Why?
??? Weird.

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2309:
transform.TranslateRight(offset_in_container.Width().ToFloat(),
I can't come up with a case where the change here made a difference.
Don't know why - it should matter which order the transform and translation
happen in.

[trchen, 2017-04-10 21:15:19]

> 1. Apply layout offset from child to container.
> 2. Apply transform of child.
> 3. Apply perspective of container.
> 4. Apply scroll offset of child.

I think you meant scroll offset of the container.

> 5. Apply clip of container.
----
Your doubts is the exact reason why I feel we should prioritize cleaning up our
matrix classes... There's a CL that took me 3 times to get orders right. :(

Let's unify some wordings first. This convention is used by everywhere in the
world except WebKit.

1. We always use column vectors for points. When we say "applying a transform T
to point P", we mean T * P. This also implies operations apply right-to-left.
When we say "applying Op1 to a point first, then Op2, then Op3", we mean Op3 *
Op2 * Op1 * P.

2. When we say pre-something, such as pre-translate, we mean the operation
applies before everything else already in the matrix. i.e. m.preTranslate(x, y)
means m = m * Translate(x, y).

3. Likewise, post-something means applying something after everything already in
the matrix.

4. We use row-major for element indices. i.e.
[ m11 m12 m13 m14 ]
[ m21 m22 m23 m24 ]
[ m31 m32 m33 m34 ]
[ m41 m42 m43 m44 ]

Now the following are the WTFs:

5. CSS transform specs uses column vectors too. i.e. Op3 * Op2 * Op1 * P

6. However CSS transform spec indices matrix elements in col-major order. i.e.
[ m11 m21 m31 m41 ]
[ m12 m22 m32 m42 ]
[ m13 m23 m33 m43 ]
[ m14 m24 m34 m44 ]

7. TransformationMatrix.h store and speak in col-major order too. i.e.
this->m_matrix[3][0] == this->m41() == translate_x

8. TransformationMatrix.h names its operation using row vector order, i.e.
translate means pre-translate. m.translate(x,y) means m = m * Translate(x, y)
translateRight means post-translate. m.postTranslate(x, y) means m =
Translate(x, y) * m
multiply means pre-multiply. Comments in TransformationMatrix.cpp lied about it.

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1234: // TODO(chrishtr):
This applies perspective before the transform. Why?
On 2017/04/10 19:37:35, chrishtr wrote:
> ??? Weird.

This looks correct. See explanation above.

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2309:
transform.TranslateRight(offset_in_container.Width().ToFloat(),
On 2017/04/10 19:37:35, chrishtr wrote:
> I can't come up with a case where the change here made a difference.
> Don't know why - it should matter which order the transform and translation
> happen in.

The code here is equivalent. See explanation above.

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2325: // TODO(chrishtr):
This applies perspective before the transform. Why?
This lgtm. It's TransformationMatrix.h lbtm. :/

[chrishtr, 2017-04-10 21:34:48]

Description was changed from

==========
Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of child.
5. Apply clip of container.

This is incorrect. It should do steps 1 and 4 after step 2 and before step 3.

GeometryMapper already did this correctly. (And VisualRectMappingTest verifies
this).

BUG=699140
==========

to

==========
Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of container.
5. Apply clip of container.

This is incorrect. It should do steps 1 and 4 after step 2 and before step 3.

GeometryMapper already did this correctly. (And VisualRectMappingTest verifies
this).

BUG=699140
==========

[chrishtr, 2017-04-10 21:38:28]

Fixed the CL description also.

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2309:
transform.TranslateRight(offset_in_container.Width().ToFloat(),
On 2017/04/10 at 21:15:18, trchen wrote:
> On 2017/04/10 19:37:35, chrishtr wrote:
> > I can't come up with a case where the change here made a difference.
> > Don't know why - it should matter which order the transform and translation
> > happen in.
> 
> The code here is equivalent. See explanation above.

Ok.

https://codereview.chromium.org/2808613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2325: // TODO(chrishtr):
This applies perspective before the transform. Why?
On 2017/04/10 at 21:15:19, trchen wrote:
> This lgtm. It's TransformationMatrix.h lbtm. :/

Removed the comment.

[chrishtr, 2017-04-10 21:38:39]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-10 21:38:39]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2808613002/#ps100001
(title: "none")

[commit-bot: I haz the power, 2017-04-10 21:39:42]

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-10 23:10:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491860319164280,
"parent_rev": "4389ecfd97b746892b3ec0d85137e9f62b69ba3a", "commit_rev":
"2a2f87ba99ef135283c6cf81a173189e78357965"}

[commit-bot: I haz the power, 2017-04-10 23:11:33]

Description was changed from

==========
Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of container.
5. Apply clip of container.

This is incorrect. It should do steps 1 and 4 after step 2 and before step 3.

GeometryMapper already did this correctly. (And VisualRectMappingTest verifies
this).

BUG=699140
==========

to

==========
Apply container offset and scroll between transform and containerperspective.

Previously, we mapped a visual rect from a child box to its container like this:

1. Apply layout offset from child to container.
2. Apply transform of child.
3. Apply perspective of container.
4. Apply scroll offset of container.
5. Apply clip of container.

This is incorrect. It should do steps 1 and 4 after step 2 and before step 3.

GeometryMapper already did this correctly. (And VisualRectMappingTest verifies
this).

BUG=699140

Review-Url: https://codereview.chromium.org/2808613002
Cr-Commit-Position: refs/heads/master@{#463436}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a2f87ba99ef135283c6cf81a173...
==========

[commit-bot: I haz the power, 2017-04-10 23:11:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2a2f87ba99ef135283c6cf81a173...