Migrate most of cc/ from WebKit::WebTransformationMatrix to gfx::Transform

Animation code is not yet migrated by this patch, due to WebTransformOperations.

BUG=159972
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=169609

[shawnsingh, 2012-11-21 21:27:03]

vollick@ kindly agreed to take a look at this patch.
danakj@ and enne@ as owners
enne@ - at least needs to sign off on the webkit/compositor_bindings part.

Essentially I used a sed script to replace most code, and manually fixed up the
rest of the code.  Most of it was blind drudgery, but there are some changes to
note:

Please let me know if there is anything I can explain/do to make this patch
easier to review.

[shawnsingh, 2012-11-21 21:32:02]

Just to give some context to this patch:

1. vollick has some gfx::Transform API cleanups underway right now, too.  After
that patch the "Preconcat" naming will go away.

2. a follow up patch will hopefully migrate WebTransformOperations and animation
code.

--- at that point, the dependency should be successfully gone.

3. further follow-up patches will phase out the MathUtil wrappers in favor of
putting them in gfx::Transform..

[Ian Vollick, 2012-11-21 22:07:09]

I had to run before I could finish my review, but here are the things I've
noticed so far.

https://codereview.chromium.org/11308153/diff/1/cc/float_quad_unittest.cc
File cc/float_quad_unittest.cc (right):

https://codereview.chromium.org/11308153/diff/1/cc/float_quad_unittest.cc#new...
cc/float_quad_unittest.cc:16: // TODO(danakj) Move this test to ui/gfx/ when we
don't use Transform.
Could this test move now, then?

https://codereview.chromium.org/11308153/diff/1/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11308153/diff/1/cc/gl_renderer.cc#newcode1045
cc/gl_renderer.cc:1045: {
use SkMatrix44::asColMajorf

https://codereview.chromium.org/11308153/diff/1/cc/layer_tree_host_common_uni...
File cc/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/11308153/diff/1/cc/layer_tree_host_common_uni...
cc/layer_tree_host_common_unittest.cc:634:
MathUtil::rotateEulerAngles(&rotationAboutYAxis, 0, 30, 0);
Why not rotationAboutYAxis.PreconcatRotateAbout(Point3F(0, 1, 0), 30)? Then can
we get rid of the euler angle stuff?

https://codereview.chromium.org/11308153/diff/1/cc/layer_tree_host_common_uni...
cc/layer_tree_host_common_unittest.cc:981:
MathUtil::rotateEulerAngles(&rotationAboutZ, 0, 0, 90);
same comment here. We can pass the canonical vector to PreconcatRotateAbout,
right? (Here and elsewhere).

[danakj, 2012-11-22 01:06:00]

cc/layer_tree_host_impl_unittest.cc:4516:5: note: in instantiation of function
template specialization
'testing::internal::EqHelper<false>::Compare<WebKit::WebTransformationMatrix,
gfx::Transform>' requested here
    EXPECT_EQ(expectedImplTransform, m_hostImpl->rootLayer()->implTransform());

One of those is a gfx::T and one is a Wk::WTM

[shawnsingh, 2012-11-23 08:10:45]

Next patch is rebased to tip of tree, addressed reviewer feedback.

Main change between this and previous patch:   some new code appeared in
content/common/cc_messages*  which also needs migrationalizing.

piman@ could you please review these new changes in content/common?  Thank you
in advance.

https://codereview.chromium.org/11308153/diff/1/cc/float_quad_unittest.cc
File cc/float_quad_unittest.cc (right):

https://codereview.chromium.org/11308153/diff/1/cc/float_quad_unittest.cc#new...
cc/float_quad_unittest.cc:16: // TODO(danakj) Move this test to ui/gfx/ when we
don't use Transform.
On 2012/11/21 22:07:09, vollick wrote:
> Could this test move now, then?

It still requires gfx::Transform to support something analogous to mapQuad
first, for now we have to use MathUtil.  I'll fix the comment.

https://codereview.chromium.org/11308153/diff/1/cc/layer_tree_host_common_uni...
File cc/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/11308153/diff/1/cc/layer_tree_host_common_uni...
cc/layer_tree_host_common_unittest.cc:634:
MathUtil::rotateEulerAngles(&rotationAboutYAxis, 0, 30, 0);
On 2012/11/21 22:07:09, vollick wrote:
> Why not rotationAboutYAxis.PreconcatRotateAbout(Point3F(0, 1, 0), 30)? Then
can
> we get rid of the euler angle stuff?

I started doing this and realized that I feel uncomfortable having it in this
patch.  This would be the first "meaningful" change in the patch, everything
else is dumb sed/replace.

It's a very simple change we can do in a separate follow-up, which could make
life much easier if something really does go wrong.  Is that OK with you?

[piman, 2012-11-23 17:11:15]

LGTM for content/ and ui/compositor/

[danakj, 2012-11-24 02:34:54]

Some nitty things. And please s/Transform/gfx::Transform/ everywhere, and make
sure there's no case of "using gfx::Transform" in cc/.

Then it LGTM

https://codereview.chromium.org/11308153/diff/3003/cc/animation_curve.h
File cc/animation_curve.h (right):

https://codereview.chromium.org/11308153/diff/3003/cc/animation_curve.h#newco...
cc/animation_curve.h:12: #include "ui/gfx/transform.h"
seems like this header isn't needed here

https://codereview.chromium.org/11308153/diff/3003/cc/damage_tracker.cc
File cc/damage_tracker.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/damage_tracker.cc#newco...
cc/damage_tracker.cc:13: using gfx::Transform;
don't using it, let's say gfx:: explicitly at each use, as we do for rect etc.

https://codereview.chromium.org/11308153/diff/3003/cc/keyframed_animation_cur...
File cc/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/keyframed_animation_cur...
cc/keyframed_animation_curve.cc:7: using gfx::Transform;
erase this line, or undo both changes in this file.

https://codereview.chromium.org/11308153/diff/3003/cc/keyframed_animation_cur...
File cc/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/keyframed_animation_cur...
cc/keyframed_animation_curve_unittest.cc:7: #include
<public/WebTransformOperations.h>
can undo all the changes in this file

https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc#newcode311
cc/layer_impl.cc:311: m_drawTransform.matrix().getDouble(0, 0),
m_drawTransform.matrix().getDouble(1, 0), m_drawTransform.matrix().getDouble(2,
0), m_drawTransform.matrix().getDouble(3, 0),
I think these should be (0,0) (0,1) etc.. invert this. It's debugging info,
shouldn't break tests, and it'd be nice to have the output match the internals
of the matrix still.

https://codereview.chromium.org/11308153/diff/3003/cc/layer_tree_host_common_...
File cc/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/layer_tree_host_common_...
cc/layer_tree_host_common_unittest.cc:815:
expectedChildTransform.matrix().setIdentity();
note: we should have API on gfx::Transform to never need to access matrix().

https://codereview.chromium.org/11308153/diff/3003/cc/math_util.cc
File cc/math_util.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/math_util.cc#newcode414
cc/math_util.cc:414: return mInv.getDouble(2, 2) < 0;
This is a lot simpler than WebCore::TransformationMatrix's equivalent method..
how come?

https://codereview.chromium.org/11308153/diff/3003/cc/math_util.h
File cc/math_util.h (right):

https://codereview.chromium.org/11308153/diff/3003/cc/math_util.h#newcode78
cc/math_util.h:78: // Background: Transform code in WebCore does not do the
right thing in
Guess this comment is now a bit out of date.

https://codereview.chromium.org/11308153/diff/3003/cc/math_util.h#newcode118
cc/math_util.h:118: // Temporary API to ease migration from gfx::Transform
Heh, undo the change on this line.

https://codereview.chromium.org/11308153/diff/3003/cc/quad_culler_unittest.cc
File cc/quad_culler_unittest.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/quad_culler_unittest.cc...
cc/quad_culler_unittest.cc:92: Transform childTransform;             \
move the \

https://codereview.chromium.org/11308153/diff/3003/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/render_surface_impl.cc#...
cc/render_surface_impl.cc:83: m_drawTransform.matrix().getDouble(0, 0),
m_drawTransform.matrix().getDouble(1, 0), m_drawTransform.matrix().getDouble(2,
0), m_drawTransform.matrix().getDouble(3, 0),
Here also, make the output match the actual insides of the matrix, don't
transpose it.

https://codereview.chromium.org/11308153/diff/3003/cc/solid_color_layer_impl.h
File cc/solid_color_layer_impl.h (right):

https://codereview.chromium.org/11308153/diff/3003/cc/solid_color_layer_impl....
cc/solid_color_layer_impl.h:10: #include "ui/gfx/transform.h"
erase this line

https://codereview.chromium.org/11308153/diff/3003/cc/test/geometry_test_util...
File cc/test/geometry_test_utils.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/test/geometry_test_util...
cc/test/geometry_test_utils.cc:38:
EXPECT_FLOAT_EQ((expected).matrix().getDouble(0, 0),
(actual).matrix().getDouble(0, 0));
Use EXPECT_DOUBLE_EQ

https://codereview.chromium.org/11308153/diff/3003/cc/tiled_layer_impl.h
File cc/tiled_layer_impl.h (right):

https://codereview.chromium.org/11308153/diff/3003/cc/tiled_layer_impl.h#newc...
cc/tiled_layer_impl.h:10: #include "ui/gfx/transform.h"
erase this line

[shawnsingh, 2012-11-24 02:56:14]

danakj@ - one quick question and two responses.  Will wait for your reply before
landing.  thanks for review!

https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc#newcode311
cc/layer_impl.cc:311: m_drawTransform.matrix().getDouble(0, 0),
m_drawTransform.matrix().getDouble(1, 0), m_drawTransform.matrix().getDouble(2,
0), m_drawTransform.matrix().getDouble(3, 0),
On 2012/11/24 02:34:54, danakj wrote:
> I think these should be (0,0) (0,1) etc.. invert this. It's debugging info,
> shouldn't break tests, and it'd be nice to have the output match the internals
> of the matrix still.

Just to clarify, This patch doesn't change the way the elements would be output
as debug info, it's the same as always (outputting one column at a time).  Are
you sure you want me to transpose it?

https://codereview.chromium.org/11308153/diff/3003/cc/math_util.cc
File cc/math_util.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/math_util.cc#newcode414
cc/math_util.cc:414: return mInv.getDouble(2, 2) < 0;
On 2012/11/24 02:34:54, danakj wrote:
> This is a lot simpler than WebCore::TransformationMatrix's equivalent method..
> how come?

The difference is in the TODO.  We should be able to grab only a few
determinant/cofactor parts of the matrix instead of computing a full inverse. 
The WebCore version does that optimization, and I still have to do that here. 
There's a bit of API / design work that needs to be plumbed before we can do
that (the cofactor / determinant stuff in skia should probably be modified to
support this).

https://codereview.chromium.org/11308153/diff/3003/cc/test/geometry_test_util...
File cc/test/geometry_test_utils.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/test/geometry_test_util...
cc/test/geometry_test_utils.cc:38:
EXPECT_FLOAT_EQ((expected).matrix().getDouble(0, 0),
(actual).matrix().getDouble(0, 0));
On 2012/11/24 02:34:54, danakj wrote:
> Use EXPECT_DOUBLE_EQ

I'll give it a try, but we might not be able to do that.  At some point a while
ago, I did have a comment on this code that mentioned we need to check only for
float accuracy even though the matrix uses doubles - because some operations
that we compute transforms for started with floats.

I'm not sure what happened to that comment, I think it disappeared while people
were moving this code around different places.

[danakj, 2012-11-24 03:20:08]

https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc#newcode311
cc/layer_impl.cc:311: m_drawTransform.matrix().getDouble(0, 0),
m_drawTransform.matrix().getDouble(1, 0), m_drawTransform.matrix().getDouble(2,
0), m_drawTransform.matrix().getDouble(3, 0),
On 2012/11/24 02:56:14, shawnsingh wrote:
> On 2012/11/24 02:34:54, danakj wrote:
> > I think these should be (0,0) (0,1) etc.. invert this. It's debugging info,
> > shouldn't break tests, and it'd be nice to have the output match the
internals
> > of the matrix still.
> 
> Just to clarify, This patch doesn't change the way the elements would be
output
> as debug info, it's the same as always (outputting one column at a time).  Are
> you sure you want me to transpose it?

Ya, It used to output in or order, and we should do the same, even tho the
values will be transposed now. Let's get used to the new SkMatrix internal
order.

https://codereview.chromium.org/11308153/diff/3003/cc/math_util.cc
File cc/math_util.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/math_util.cc#newcode414
cc/math_util.cc:414: return mInv.getDouble(2, 2) < 0;
On 2012/11/24 02:56:14, shawnsingh wrote:
> On 2012/11/24 02:34:54, danakj wrote:
> > This is a lot simpler than WebCore::TransformationMatrix's equivalent
method..
> > how come?
> 
> The difference is in the TODO.  We should be able to grab only a few
> determinant/cofactor parts of the matrix instead of computing a full inverse. 
> The WebCore version does that optimization, and I still have to do that here. 
> There's a bit of API / design work that needs to be plumbed before we can do
> that (the cofactor / determinant stuff in skia should probably be modified to
> support this).

I see, okay.

https://codereview.chromium.org/11308153/diff/3003/cc/test/geometry_test_util...
File cc/test/geometry_test_utils.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/test/geometry_test_util...
cc/test/geometry_test_utils.cc:38:
EXPECT_FLOAT_EQ((expected).matrix().getDouble(0, 0),
(actual).matrix().getDouble(0, 0));
On 2012/11/24 02:56:14, shawnsingh wrote:
> On 2012/11/24 02:34:54, danakj wrote:
> > Use EXPECT_DOUBLE_EQ
> 
> I'll give it a try, but we might not be able to do that.  At some point a
while
> ago, I did have a comment on this code that mentioned we need to check only
for
> float accuracy even though the matrix uses doubles - because some operations
> that we compute transforms for started with floats.
> 
> I'm not sure what happened to that comment, I think it disappeared while
people
> were moving this code around different places.

I see. If it's a problem, keeping it as it has been is okay.

[danakj, 2012-11-24 03:24:11]

https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc#newcode311
cc/layer_impl.cc:311: m_drawTransform.matrix().getDouble(0, 0),
m_drawTransform.matrix().getDouble(1, 0), m_drawTransform.matrix().getDouble(2,
0), m_drawTransform.matrix().getDouble(3, 0),
On 2012/11/24 03:20:09, danakj wrote:
> On 2012/11/24 02:56:14, shawnsingh wrote:
> > On 2012/11/24 02:34:54, danakj wrote:
> > > I think these should be (0,0) (0,1) etc.. invert this. It's debugging
info,
> > > shouldn't break tests, and it'd be nice to have the output match the
> internals
> > > of the matrix still.
> > 
> > Just to clarify, This patch doesn't change the way the elements would be
> output
> > as debug info, it's the same as always (outputting one column at a time). 
Are
> > you sure you want me to transpose it?
> 
> Ya, It used to output in or order, and we should do the same, even tho the
> values will be transposed now. Let's get used to the new SkMatrix internal
> order.

s/or order/row order/.

[Ian Vollick, 2012-11-24 04:47:27]

On 2012/11/24 03:24:11, danakj wrote:
> https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc
> File cc/layer_impl.cc (right):
> 
> https://codereview.chromium.org/11308153/diff/3003/cc/layer_impl.cc#newcode311
> cc/layer_impl.cc:311: m_drawTransform.matrix().getDouble(0, 0),
> m_drawTransform.matrix().getDouble(1, 0),
m_drawTransform.matrix().getDouble(2,
> 0), m_drawTransform.matrix().getDouble(3, 0),
> On 2012/11/24 03:20:09, danakj wrote:
> > On 2012/11/24 02:56:14, shawnsingh wrote:
> > > On 2012/11/24 02:34:54, danakj wrote:
> > > > I think these should be (0,0) (0,1) etc.. invert this. It's debugging
> info,
> > > > shouldn't break tests, and it'd be nice to have the output match the
> > internals
> > > > of the matrix still.
> > > 
> > > Just to clarify, This patch doesn't change the way the elements would be
> > output
> > > as debug info, it's the same as always (outputting one column at a time). 
> Are
> > > you sure you want me to transpose it?
> > 
> > Ya, It used to output in or order, and we should do the same, even tho the
> > values will be transposed now. Let's get used to the new SkMatrix internal
> > order.
> 
> s/or order/row order/.

I also agree that we should transpose this, though for different reasons. I
think we should always present matrices to the user, either in output, in API
functions for setting all matrix entries, or in our comments, in the way we want
users to reason about them. In a way that's consistent mathematically. The
comments in layer_tree_host_common indicate that we multiply our points/vectors
on the right, so our translation transforms ought to look like,

[ 1 0 0 tx ]
[ 0 1 0 ty ]
[ 0 0 1 tz ]
[ 0 0 0 1  ]

rather than the transpose. I think it would be clearer if that's how they looked
when we printed them, too. It's also consistent with the way matrices are
presented here http://www.w3.org/TR/css3-transforms/.

It turns out that the SkMatrix44 storage, like TransformationMatrix, is column
major, but I think that's irrelevant. It's an implementation detail that should
not affect how we reason about matrices semantically. If we decide to change the
internal storage of SkMatrix44 one day, that should be hidden behind the
SkMatrix44 API and shouldn't affect any of our code.

[Ian Vollick, 2012-11-24 04:54:41]

On 2012/11/23 08:10:45, shawnsingh wrote:
> Next patch is rebased to tip of tree, addressed reviewer feedback.
> 
> Main change between this and previous patch:   some new code appeared in
> content/common/cc_messages*  which also needs migrationalizing.
> 
> piman@ could you please review these new changes in content/common?  Thank you
> in advance.
> 
> https://codereview.chromium.org/11308153/diff/1/cc/float_quad_unittest.cc
> File cc/float_quad_unittest.cc (right):
> 
>
https://codereview.chromium.org/11308153/diff/1/cc/float_quad_unittest.cc#new...
> cc/float_quad_unittest.cc:16: // TODO(danakj) Move this test to ui/gfx/ when
we
> don't use Transform.
> On 2012/11/21 22:07:09, vollick wrote:
> > Could this test move now, then?
> 
> It still requires gfx::Transform to support something analogous to mapQuad
> first, for now we have to use MathUtil.  I'll fix the comment.
> 
>
https://codereview.chromium.org/11308153/diff/1/cc/layer_tree_host_common_uni...
> File cc/layer_tree_host_common_unittest.cc (right):
> 
>
https://codereview.chromium.org/11308153/diff/1/cc/layer_tree_host_common_uni...
> cc/layer_tree_host_common_unittest.cc:634:
> MathUtil::rotateEulerAngles(&rotationAboutYAxis, 0, 30, 0);
> On 2012/11/21 22:07:09, vollick wrote:
> > Why not rotationAboutYAxis.PreconcatRotateAbout(Point3F(0, 1, 0), 30)? Then
> can
> > we get rid of the euler angle stuff?
> 
> I started doing this and realized that I feel uncomfortable having it in this
> patch.  This would be the first "meaningful" change in the patch, everything
> else is dumb sed/replace.
> 
> It's a very simple change we can do in a separate follow-up, which could make
> life much easier if something really does go wrong.  Is that OK with you?

Yup, sgtm.

[shawnsingh, 2012-11-24 06:27:04]

On 2012/11/24 03:24:11, danakj wrote:

> > Ya, It used to output in or order, and we should do the same, even tho the
> > values will be transposed now. Let's get used to the new SkMatrix internal
> > order.

OK, I will change it, no problem =)
But... I don't think what you said is correct. That debug output had always
output in column major format, not row-major.

Also just to reiterate what Ian said, in slightly different words - Both
SkMatrix44 and WebTransformationMatrix have the same math conventions and the
same (column major) data layout - they are not transposed. The only difference
between them is the notation convention of how to access their elements:

the x component of translation, 1st row, 4th column:
 webTransform.m41() == skia44.getDouble(0, 3);

[shawnsingh, 2012-11-25 21:11:16]

Enne@ can you please review the webkit/compositor_bindings changes?  Thanks in
advance.

After getting that final LGTM, I'll still wait for
https://codereview.chromium.org/11418040/ to land.

[enne (OOO), 2012-11-26 14:49:19]

webkit/compositor_bindings lgtm

(It seems like gfx::Transform should have an SkMatrix44 ctor, but maybe that's
just me.)

[danakj, 2012-11-26 16:57:51]

On 2012/11/26 14:49:19, enne wrote:
> webkit/compositor_bindings lgtm
> 
> (It seems like gfx::Transform should have an SkMatrix44 ctor, but maybe that's
> just me.)

I think I'd rather keep SkMatrix44 entirely out of the API for transform.
Dealing with SkMScalar etc is unfun.

[shawnsingh, 2012-11-27 01:06:32]

Hi Ben, it looks like I might need your approval for the content/common changes.
 can you PTAL?  thanks in advance.

[piman, 2012-11-27 01:10:05]

content/common LGTM. I'm OWNER, however you'll need security folks to review the
cc_messages.h changes.

[shawnsingh, 2012-11-27 01:19:50]

Oh ok, thanks.  I was just blindly following presubmit checks.

+tsepez, I believe he reviewed the other most recent changes to cc_messages.h
from danakj, for security.

[Tom Sepez, 2012-11-27 02:36:22]

LGTM on messages.

[Ben Goodger (Google), 2012-11-27 16:38:45]

lgtm