Move temporary MathUtil wrappers to permanent home in gfx::Transform

Move temporary MathUtil wrappers to permanent home in gfx::Transform

This patch moves some of the temporary wrappers to their permanent home. Unit
tests remain in math_util_unittests for now, but no test coverage is lost.

Added 3 static accessors to Vector3dF help readability when calling RotateAbout().

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

[shawnsingh, 2012-11-28 00:18:11]

PTAL, thanks!

At this point, I feel OK migrating all in one patch - if reviewers think that
will be easier for them.  I thought it'll be easier to review in bite-size
chunks, so I did not yet phase out the entire MathUtil stuff.

[danakj, 2012-11-28 00:42:11]

Seems that only uses of RotateAbout() pass in one of XAxis, YAxis, or ZAxis
(excluding tests). So why not make the API only allow rotation around one of
those vectors, and then you don't need to pass extra floats around on the stack
to do the rotate.

Like, gfx::Transform::RotateAboutXAxis(double deg)?

https://codereview.chromium.org/11418197/diff/1/cc/layer_sorter_unittest.cc
File cc/layer_sorter_unittest.cc (right):

https://codereview.chromium.org/11418197/diff/1/cc/layer_sorter_unittest.cc#n...
cc/layer_sorter_unittest.cc:97: throughMatrix.RotateAbout(gfx::Vector3dF(0, 1,
0), 45);
yaxis?

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

https://codereview.chromium.org/11418197/diff/1/ui/gfx/transform.cc#newcode243
ui/gfx/transform.cc:243: // TODO (shawnsingh) make this perform more efficiently
- we do not
is there a bug for this, for M25?

[shawnsingh, 2012-11-28 06:00:50]

Addressed your feedback.

Filed bug http://code.google.com/p/chromium/issues/detail?id=163062.   I'm
planning to work on it within the next few days, anyway.

[shawnsingh, 2012-11-28 06:17:58]

On 2012/11/28 06:00:50, shawnsingh wrote:
> Addressed your feedback.
> 
> Filed bug http://code.google.com/p/chromium/issues/detail?id=163062.   I'm
> planning to work on it within the next few days, anyway.

Just remembered that the faster IsBackFaceVisible() implementation is blocked on
some decisions we need to make about how to better access individual elements of
the matrix.  Either that, or we decide to implement this in Skia and make
gfx::transform just a wrapper about it.  In any case, it needs to be solved by
m25.

[danakj, 2012-11-28 20:38:03]

On 2012/11/28 06:17:58, shawnsingh wrote:
> On 2012/11/28 06:00:50, shawnsingh wrote:
> > Addressed your feedback.
> > 
> > Filed bug http://code.google.com/p/chromium/issues/detail?id=163062.   I'm
> > planning to work on it within the next few days, anyway.
> 
> Just remembered that the faster IsBackFaceVisible() implementation is blocked
on
> some decisions we need to make about how to better access individual elements
of
> the matrix.  Either that, or we decide to implement this in Skia and make
> gfx::transform just a wrapper about it.  In any case, it needs to be solved by
> m25.

What decisions are blocking it?

[Ian Vollick, 2012-11-28 20:41:55]

On 2012/11/28 20:38:03, danakj wrote:
> On 2012/11/28 06:17:58, shawnsingh wrote:
> > On 2012/11/28 06:00:50, shawnsingh wrote:
> > > Addressed your feedback.
> > > 
> > > Filed bug http://code.google.com/p/chromium/issues/detail?id=163062.   I'm
> > > planning to work on it within the next few days, anyway.
> > 
> > Just remembered that the faster IsBackFaceVisible() implementation is
blocked
> on
> > some decisions we need to make about how to better access individual
elements
> of
> > the matrix.  Either that, or we decide to implement this in Skia and make
> > gfx::transform just a wrapper about it.  In any case, it needs to be solved
by
> > m25.
> 
> What decisions are blocking it?

I think the concern was that getDouble(a, b) might be more expensive that
accessing the internal array directly. Maybe we could assume that it's fast, and
if we find out that it's not, we can fix getDouble?

[danakj, 2012-11-28 20:42:58]

https://codereview.chromium.org/11418197/diff/2004/cc/math_util_unittest.cc
File cc/math_util_unittest.cc (right):

https://codereview.chromium.org/11418197/diff/2004/cc/math_util_unittest.cc#n...
cc/math_util_unittest.cc:795: A.RotateAbout(gfx::Vector3dF(0, 0, 0), 45);
I guess in my previous comment, I was also wondering why we need these tests,
and this API for arbitrary axis values at all? Since there's no users of it
except these tests.

[Ian Vollick, 2012-11-28 20:45:31]

On 2012/11/28 20:42:58, danakj wrote:
> https://codereview.chromium.org/11418197/diff/2004/cc/math_util_unittest.cc
> File cc/math_util_unittest.cc (right):
> 
>
https://codereview.chromium.org/11418197/diff/2004/cc/math_util_unittest.cc#n...
> cc/math_util_unittest.cc:795: A.RotateAbout(gfx::Vector3dF(0, 0, 0), 45);
> I guess in my previous comment, I was also wondering why we need these tests,
> and this API for arbitrary axis values at all? Since there's no users of it
> except these tests.

WebTransformOperations will need this to interpolate rotations about an
arbitrary axis.

[danakj, 2012-11-28 20:48:16]

On 2012/11/28 20:41:55, vollick wrote:
> On 2012/11/28 20:38:03, danakj wrote:
> > On 2012/11/28 06:17:58, shawnsingh wrote:
> > > On 2012/11/28 06:00:50, shawnsingh wrote:
> > > > Addressed your feedback.
> > > > 
> > > > Filed bug http://code.google.com/p/chromium/issues/detail?id=163062.  
I'm
> > > > planning to work on it within the next few days, anyway.
> > > 
> > > Just remembered that the faster IsBackFaceVisible() implementation is
> blocked
> > on
> > > some decisions we need to make about how to better access individual
> elements
> > of
> > > the matrix.  Either that, or we decide to implement this in Skia and make
> > > gfx::transform just a wrapper about it.  In any case, it needs to be
solved
> by
> > > m25.
> > 
> > What decisions are blocking it?
> 
> I think the concern was that getDouble(a, b) might be more expensive that
> accessing the internal array directly. Maybe we could assume that it's fast,
and
> if we find out that it's not, we can fix getDouble?

I see. When the methods are inline, it should be about the same? Which Skia
folks are doing now (thanks james!).

I think multiplication and inverse are going to continue to dominate anything
else in this class for a while.

[shawnsingh, 2012-11-28 20:49:37]

More discussion should definitely happen elsewhere... but the current state was
that we felt that the approach of gfx::Transform wrapping SkMatrix44 might be
causing unnecessary overhead.  In particular, accessing individual elements is
not inlined, and implementations of some functions could be optimized a little
if we can easily access individual elements at low cost.  Also, the idea that
any improvements have to be upstreamed to skia and then let to roll is painful,
but it doesn't seem necessary given that SkMatrix44 might not even be used by
Skia...

[danakj, 2012-11-28 20:52:17]

LGTM with some tests for RotateAbout() added.

https://codereview.chromium.org/11418197/diff/2004/cc/math_util_unittest.cc
File cc/math_util_unittest.cc (right):

https://codereview.chromium.org/11418197/diff/2004/cc/math_util_unittest.cc#n...
cc/math_util_unittest.cc:736: A.RotateAboutZAxis(90);
I think you should duplicate this test with RotateAbout(), as now that function
is only tested with 1,1,1 and 0,0,0

https://codereview.chromium.org/11418197/diff/2004/cc/math_util_unittest.cc#n...
cc/math_util_unittest.cc:795: A.RotateAbout(gfx::Vector3dF(0, 0, 0), 45);
On 2012/11/28 20:42:59, danakj wrote:
> I guess in my previous comment, I was also wondering why we need these tests,
> and this API for arbitrary axis values at all? Since there's no users of it
> except these tests.

To answer my own question, I see this being used in transform interpolation in
ui/gfx.

[shawnsingh, 2012-11-28 22:20:42]

Thanks for review.

new patch has draconian coverage for all 4 functions now =)   I'll upload the
patch and put it through CQ shortly.

[commit-bot: I haz the power, 2012-11-28 22:44:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/11418197/1012

[commit-bot: I haz the power, 2012-11-28 23:31:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.