Apply skew on both axes together rather than sequentially on Compositor thread

Apply skew on both axes together rather than sequentially on Compositor
thread.

Currently skew is not implemented correctly on the Compositor thread.
Instead of calling skew(angle_x, angle_y), the implementation calls
skewX(angle_x) followed by skewY(angle_y). This leads to a different
result. This patch applied skew on both axes together rather than
calling skewX(angle_x) and skewX(angle_Y) sequentially. FF and IE
correctly call skew(angle_x, angle_y).

BUG=268468
Committed: https://crrev.com/8ca8ee6d5165acb571a5f9d298ca3d80310e5f15
Cr-Commit-Position: refs/heads/master@{#347089}

[nainar1, 2015-09-01 05:36:37]

nainar@google.com changed reviewers:
+ ajuma@chromium.org, alancutter@chromium.org, timloh@chromium.org

[nainar1, 2015-09-01 08:00:10]

Hi all,

Could you please take a look at the patch?

Thanks!

[ajuma, 2015-09-01 13:25:10]

Mostly looks good. Please look into the remaining compilation failures (try
building cc_unittests locally; that should find most of them I think). Please
also update the patch summary to say what the patch does -- for example,
something like "Apply skew on both axes together rather than sequentially"

https://codereview.chromium.org/1325803002/diff/60001/ui/gfx/transform_unitte...
File ui/gfx/transform_unittest.cc (left):

https://codereview.chromium.org/1325803002/diff/60001/ui/gfx/transform_unitte...
ui/gfx/transform_unittest.cc:960: // error.
This test and comment still seem to have value. Do the issues described in the
comment still apply? If so, I'd suggest adapting this to use the new Skew
method.

https://codereview.chromium.org/1325803002/diff/60001/ui/gfx/transform_unitte...
File ui/gfx/transform_unittest.cc (right):

https://codereview.chromium.org/1325803002/diff/60001/ui/gfx/transform_unitte...
ui/gfx/transform_unittest.cc:1875: EXPECT_ROW4_EQ(0.0f, 0.0f, 0.0f, 1.0f, A);
Please also test the case where we're skewing along both axes.

[nainar1, 2015-09-02 01:43:33]

nainar@google.com changed reviewers:
+ nainar@google.com

[nainar1, 2015-09-02 01:43:33]

Hi ajuma,

I have incorporated your comments, please take a look at the latest patch set?
Have also updates the description and title to better reflect the patch as you
asked. 
Thanks!

https://codereview.chromium.org/1325803002/diff/60001/ui/gfx/transform_unitte...
File ui/gfx/transform_unittest.cc (left):

https://codereview.chromium.org/1325803002/diff/60001/ui/gfx/transform_unitte...
ui/gfx/transform_unittest.cc:960: // error.
Done.

https://codereview.chromium.org/1325803002/diff/60001/ui/gfx/transform_unitte...
File ui/gfx/transform_unittest.cc (right):

https://codereview.chromium.org/1325803002/diff/60001/ui/gfx/transform_unitte...
ui/gfx/transform_unittest.cc:1875: EXPECT_ROW4_EQ(0.0f, 0.0f, 0.0f, 1.0f, A);
Done.

[nainar1, 2015-09-02 01:43:41]

nainar@google.com changed reviewers:
- nainar@google.com

[ajuma, 2015-09-02 13:15:52]

Thanks, lgtm with nit:

https://codereview.chromium.org/1325803002/diff/120001/ui/gfx/transform_unitt...
File ui/gfx/transform_unittest.cc (right):

https://codereview.chromium.org/1325803002/diff/120001/ui/gfx/transform_unitt...
ui/gfx/transform_unittest.cc:1944: A.Skew(45.0, 0.0);
This should skew along the Y axis rather than the X axis (to match the comment
above).

[nainar, 2015-09-02 13:37:06]

Hi ajuma,

Please see my reply to your comment. Will commit after receiving alancutter and
timloh's lgtm.

Thanks for the review!

https://codereview.chromium.org/1325803002/diff/120001/ui/gfx/transform_unitt...
File ui/gfx/transform_unittest.cc (right):

https://codereview.chromium.org/1325803002/diff/120001/ui/gfx/transform_unitt...
ui/gfx/transform_unittest.cc:1944: A.Skew(45.0, 0.0);
// Test a skew along Y axis only <- This comment was supposed to be followed by
a test case for skewing along Y axis only. I forgot to add that in. This patch
has that. Sorry for the confusion

[ajuma, 2015-09-02 13:43:08]

Note that you'll also need OWNERS reviews for the changes outside cc/.

https://codereview.chromium.org/1325803002/diff/140001/ui/gfx/transform_unitt...
File ui/gfx/transform_unittest.cc (right):

https://codereview.chromium.org/1325803002/diff/140001/ui/gfx/transform_unitt...
ui/gfx/transform_unittest.cc:1940: A.Skew(45.0, 0.0);
This is still skewing along X :)  Please change this to skew along Y.

[nainar, 2015-09-02 14:02:25]

Would the owners in this case be vollick and sky?

https://codereview.chromium.org/1325803002/diff/140001/ui/gfx/transform_unitt...
File ui/gfx/transform_unittest.cc (right):

https://codereview.chromium.org/1325803002/diff/140001/ui/gfx/transform_unitt...
ui/gfx/transform_unittest.cc:1940: A.Skew(45.0, 0.0);
Done.

[ajuma, 2015-09-02 14:06:33]

On 2015/09/02 14:02:25, nainar1 wrote:
> Would the owners in this case be vollick and sky?

Yes, that would work.

[nainar, 2015-09-02 14:08:05]

Thank you! Adding them to reviewers.

[nainar, 2015-09-02 14:09:18]

nainar@chromium.org changed reviewers:
+ sky@chromium.org, vollick@chromium.org

[nainar, 2015-09-02 14:18:03]

Adding owners as reviewers:

+sky for mojo/
+vollick for ui/gfx/

Thanks!

[Ian Vollick, 2015-09-02 14:26:58]

On 2015/09/02 14:18:03, nainar1 wrote:
> Adding owners as reviewers:
> 
> +sky for mojo/
> +vollick for ui/gfx/
> 
> Thanks!

ui/gfx lgtm.

[sky, 2015-09-02 15:42:15]

LGTM

[Timothy Loh, 2015-09-02 21:09:21]

lgtm

[alancutter (OOO until 2018), 2015-09-03 00:55:40]

lgtm

[nainar, 2015-09-03 00:56:42]

The CQ bit was checked by nainar@chromium.org

[nainar, 2015-09-03 00:56:42]

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/1325803002/#ps150011
(title: "Test for skewY")

[commit-bot: I haz the power, 2015-09-03 00:57:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1325803002/150011
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1325803002/150011

[commit-bot: I haz the power, 2015-09-03 01:04:16]

Committed patchset #9 (id:150011)

[commit-bot: I haz the power, 2015-09-03 01:05:08]

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