Implement unit tests and temporary MathUtil wrappers for transform functionality

Implement unit tests and temporary MathUtil wrappers for transform functionality

These unit tests and wrappers have a short-term home in MathUtil so that we can
safely migrate from WebTransformationMatrix to gfx::Transform in a follow-up
patch. After the migration, we will then proceed to phase out this temporary
code and beef up ui/gfx/transform.h nicely.

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

[shawnsingh, 2012-11-16 08:07:16]

requesting danakj@ to look at entire patch

requesting sky@ to look at the trivial fixes to transform.h --> unit test that
exposed the divide-by-zero error is already part of this patch in
math_util_unittests, and those tests will be migrated to ui/gfx at a later time.

enne@ just in case you have some opinions, but if you're busy no problem

[Ian Vollick, 2012-11-16 13:44:46]

Nice, I really like these changes. I'm excited for their addition to
gfx::Transform.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc
File cc/math_util.cc (right):

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode401
cc/math_util.cc:401: // full matrix inversion when the matrix was actually
invertible.
Now that SkMatrix44::determinant is public, would could easily write an
efficient gfx::Transform::IsInvertible().

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode421
cc/math_util.cc:421: 
SkMatrix44 now has getDouble for accessing matrix elements as doubles rather
than get(..) which returns SkMScalars. Perhaps those would be more appropriate
here? Same comment goes for the other get(..)'s in this file.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode451
cc/math_util.cc:451: // hard-coding each matrix instead of calling
rotateAxisAngle().
Actually, it would be nice if SkMatrix44 noticed when you passed it one of the
canonical basis vectors and did something clever so you didn't have to do
something smart here. That said, it would be nice if we could write out each of
these matrices and multiply them symbolically. We could probably construct their
product directly rather than multiplying these three components.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode467
cc/math_util.cc:467: // point for the axis.
This is a good point, but this would ideally be a vector backed by doubles. It
would also be nice if we could construct vectors and points that are backed by
doubles in general. This would simplify much of the code in transform_util which
exists simply because we don't have these classes.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode475
cc/math_util.cc:475: transform.GetInverse(&result);
GetInverse should be decorated with WARN_UNUSED_RESULT -- this code really
shouldn't compile.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode483
cc/math_util.cc:483: matrix.set(0, 2, 0);
setDouble

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode510
cc/math_util.cc:510: matrix.set(0, 0, m11);
setDouble.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode521
cc/math_util.cc:521: // Initialize column 2
Copy pasta -- column 3. Here and below.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode541
cc/math_util.cc:541: matrix.set(0, 0, a);
setDouble

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h
File cc/math_util.h (right):

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h#newcode130
cc/math_util.h:130: static void makeIdentity(gfx::Transform*);
Would transform = gfx::Transform() suffice?

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h#newcode131
cc/math_util.h:131: static void rotateEulerAngles(gfx::Transform*, float eulerX,
float eulerY, float eulerZ); // TODO: verify old webcore API was euler angles?
These should be doubles now that gfx::Transform is backed by doubles. This goes
for the functions below as well.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h#newcode133
cc/math_util.h:133: static gfx::Transform inverse(const gfx::Transform&);
I think this should be pulled from the API. gfx::Transform already has an
inverse and unlike this function, it's able to report when we can't invert due
to a singular matrix (which, now that I'm looking at it, should use
WARN_UNUSED_RESULT). Although we have an isInvertible function above, there's no
way to guarantee that people have used it before calling inverse. In the same
way that we want to force people to consider clipping when mapping points, we
should also force people to consider singular matrices.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h#newcode140
cc/math_util.h:140: float m41, float m42, float m43, float m44);
This is a bit of a matter of preference, but I prefer to interpret WebCore's mXY
as column X, row Y -- that is, I like to think of us multipling our
points/vectors on the right.  So m41 (i.e., translateX) would be in the upper
right -- as in, http://www.songho.ca/opengl/gl_transform.html. It's the way I'm
used to thinking about transformation matrices. That said, other people think
about things transposed and multiply their vectors on the left -- D3D does:
http://msdn.microsoft.com/en-us/library/windows/desktop/bb172573(v=vs.85).aspx.
Although this layout of arguments may be consistent with our storage,  I'd
prefer to transpose them. Also, the comments in layer_tree_host_common also
suggest that we accumulate matrices on the right, so transposing these would
make us consistent with that comment.

http://codereview.chromium.org/11316043/diff/1/cc/math_util_unittest.cc
File cc/math_util_unittest.cc (right):

http://codereview.chromium.org/11316043/diff/1/cc/math_util_unittest.cc#newco...
cc/math_util_unittest.cc:238: matrix.set(0, 0, 10);
setDouble -- here and below. Also, I think that chromium style dictates that
double constants below be appended with .0 (they're floats at present, but I
think they should be doubles).

http://codereview.chromium.org/11316043/diff/1/cc/math_util_unittest.cc#newco...
cc/math_util_unittest.cc:423: EXPECT_TRUE(A == B);
EXPECT_EQ?

http://codereview.chromium.org/11316043/diff/1/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

http://codereview.chromium.org/11316043/diff/1/ui/gfx/transform.cc#newcode120
ui/gfx/transform.cc:120: return;
Good catch. Thank you!

[danakj, 2012-11-16 16:25:15]

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h
File cc/math_util.h (right):

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h#newcode130
cc/math_util.h:130: static void makeIdentity(gfx::Transform*);
On 2012/11/16 13:44:46, vollick wrote:
> Would transform = gfx::Transform() suffice?

I've seen a pattern in the existing classes to provide both operator= and method
setters, such as SetRect(x,y,w,h). I think a MakeIdentity() follows the existing
pattern allowing one to avoid allocating and assigning.

[shawnsingh, 2012-11-16 17:22:35]

Awesome - thanks for the detailed look.   Sorry I accidentally left you out on
the original reviewers list, didn't mean to do that.

One reply I had, about the transpose stuff, and things like inverse() - actually
I agree with what you're saying, but keeping some remnants of webcore style in
our wrappers will help ease the major sed script migration.  So if it's OK with
you, I prefer to address some of your API points after the migration (many of
then I think we can do now, though).  Is that OK with you?

[shawnsingh, 2012-11-16 17:25:26]

oh, and btw, after we had taken a closer look, we concluded that skia and
webcore are actually *not transposed from each other.  It all boiled down to the
naming that skia used - "B.preconcat(A)" meant to compute B*A.

So if I had some comment like that, it should have only been referring to the
indexing notation... I'll check and fix it if needed =)

[sky, 2012-11-16 21:44:11]

Dana is an owner for ui/gfx/transform now, you no longer need me. I'm removing
myself as a reviewer.

[shawnsingh, 2012-11-17 00:38:31]

So here is a more complete response to the reviews...   and everything I did not
directly respond to has been addressed in the latest patch coming in a moment.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc
File cc/math_util.cc (right):

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode451
cc/math_util.cc:451: // hard-coding each matrix instead of calling
rotateAxisAngle().
On 2012/11/16 13:44:46, vollick wrote:
> Actually, it would be nice if SkMatrix44 noticed when you passed it one of the
> canonical basis vectors and did something clever so you didn't have to do
> something smart here. That said, it would be nice if we could write out each
of
> these matrices and multiply them symbolically. We could probably construct
their
> product directly rather than multiplying these three components.

I think I understand you're saying that we should make skia recognize the
special degenerate axes.  But I don't understand the second half of what you're
saying?   are you suggesting we should construct the product of 3 or 4 matrices
by computing each element manually rather than using the multiplifaction
operator?

http://codereview.chromium.org/11316043/diff/1/cc/math_util.cc#newcode467
cc/math_util.cc:467: // point for the axis.
On 2012/11/16 13:44:46, vollick wrote:
> This is a good point, but this would ideally be a vector backed by doubles. It
> would also be nice if we could construct vectors and points that are backed by
> doubles in general. This would simplify much of the code in transform_util
which
> exists simply because we don't have these classes.

Sounds like musings for follow-up patch, right?  aside from making these values
double for now, seems like this is the way we can do it for now.  Please let me
know if you intended me to change this code somehow, other than float-->double
which is done now.

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h
File cc/math_util.h (right):

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h#newcode133
cc/math_util.h:133: static gfx::Transform inverse(const gfx::Transform&);
On 2012/11/16 13:44:46, vollick wrote:
> I think this should be pulled from the API. gfx::Transform already has an
> inverse and unlike this function, it's able to report when we can't invert due
> to a singular matrix (which, now that I'm looking at it, should use
> WARN_UNUSED_RESULT). Although we have an isInvertible function above, there's
no
> way to guarantee that people have used it before calling inverse. In the same
> way that we want to force people to consider clipping when mapping points, we
> should also force people to consider singular matrices.

As mentioned above, I agree with this, but I would like to keep this for
migration purposes for now, to keep migration less complicated. hope that's OK
=)

http://codereview.chromium.org/11316043/diff/1/cc/math_util.h#newcode140
cc/math_util.h:140: float m41, float m42, float m43, float m44);
On 2012/11/16 13:44:46, vollick wrote:
> This is a bit of a matter of preference, but I prefer to interpret WebCore's
mXY
> as column X, row Y -- that is, I like to think of us multipling our
> points/vectors on the right.  So m41 (i.e., translateX) would be in the upper
> right -- as in, http://www.songho.ca/opengl/gl_transform.html. It's the way
I'm
> used to thinking about transformation matrices. That said, other people think
> about things transposed and multiply their vectors on the left -- D3D does:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/bb172573%28v=vs.85%29....
> Although this layout of arguments may be consistent with our storage,  I'd
> prefer to transpose them. Also, the comments in layer_tree_host_common also
> suggest that we accumulate matrices on the right, so transposing these would
> make us consistent with that comment.

Same goes for this comment - this wrapper simply exists so that migration is
simpler.  We can discuss transposing it after migration.  I like the idea that
it would be formatted visually so that rows are actually rows in the code.

http://codereview.chromium.org/11316043/diff/1/cc/math_util_unittest.cc
File cc/math_util_unittest.cc (right):

http://codereview.chromium.org/11316043/diff/1/cc/math_util_unittest.cc#newco...
cc/math_util_unittest.cc:423: EXPECT_TRUE(A == B);
On 2012/11/16 13:44:46, vollick wrote:
> EXPECT_EQ?

I wanted to explicitly make sure the == operator was being used so that it gets
tested.  I'm sure we could verify the behavior of gtest and feel confident we're
testing it, but explicit == operator invocation seemed more appropriate here.

[shawnsingh, 2012-11-17 00:41:30]

danakj@ PTAL -- thanks!

[danakj, 2012-11-17 02:14:17]

LGTM

http://codereview.chromium.org/11316043/diff/4013/cc/math_util.cc
File cc/math_util.cc (right):

http://codereview.chromium.org/11316043/diff/4013/cc/math_util.cc#newcode403
cc/math_util.cc:403: // TODO (shawnsingh): we may need to check fuzzy against an
epsilon here instead?
Is something random going to break and cause a revert because of this? Why not
just use an epsilon now?

http://codereview.chromium.org/11316043/diff/4013/cc/math_util.cc#newcode409
cc/math_util.cc:409: DCHECK(false); // NOT YET IMPLEMENTED!!
NOTREACHED()

http://codereview.chromium.org/11316043/diff/4013/cc/math_util.cc#newcode425
cc/math_util.cc:425: bool hasNoScale = matrix.getDouble(0, 0) == 1
nit: the line wrapping is inconsistent here.

http://codereview.chromium.org/11316043/diff/4013/cc/math_util.cc#newcode502
cc/math_util.cc:502: DCHECK(false); // NOT YET IMPLEMENTED!!
NOTREACHED() but..

We're going to need this to even start the transition, so NOT IMPLEMENTED here
seems a bit funny. The right place to do this is in WebTransformationMatrix, so
I did this: https://bugs.webkit.org/show_bug.cgi?id=102575

Then you can remove this method and you'll get implicit conversions between
webkit/compositor_bindings and cc/

[shawnsingh, 2012-11-17 05:42:40]

http://codereview.chromium.org/11316043/diff/4013/cc/math_util.cc
File cc/math_util.cc (right):

http://codereview.chromium.org/11316043/diff/4013/cc/math_util.cc#newcode502
cc/math_util.cc:502: DCHECK(false); // NOT YET IMPLEMENTED!!
OK I guess you've landed by now, but why is it implicit?  Shouldn't we be
forcing ourselves to be aware where the conversion is taking place?

The "not implemented" stuff was accidentally an oversight.  All the wrappers
here (including "not implemented") were going to be necessary for the migration
patch that I have.

[danakj, 2012-11-17 08:27:21]

http://codereview.chromium.org/11316043/diff/4013/cc/math_util.cc
File cc/math_util.cc (right):

http://codereview.chromium.org/11316043/diff/4013/cc/math_util.cc#newcode502
cc/math_util.cc:502: DCHECK(false); // NOT YET IMPLEMENTED!!
On 2012/11/17 05:42:41, shawnsingh wrote:
> OK I guess you've landed by now, but why is it implicit?  Shouldn't we be
> forcing ourselves to be aware where the conversion is taking place?

It's in and waiting for roll.

Also, it's explicit for now. Once cc/ is completely converted we can switch it
to implicit, as the only place conversion will happen is in the
compositor_bindings, similar to Rect/Point/Size/Vector2d.

[enne (OOO), 2012-11-19 19:29:53]

compositor_bindings lgtm