gfx::Transform API clean-up

gfx::Transform API clean-up

We have too many ways to do the same thing in gfx::Transform, and their names
can lead to confusion. We have the notion of Concat-ing and Preconcat-ing.
We've borrowed this verbage from skia. a.preConcat(b) means a = a * b. This may
seem counter-intuitive, but is the correct definition if we are multiplying our
points/vectors on the right.

That said, we almost always want to pre-concat. This what is done throughout
WebKit. To simplify matters, rather than having ConcatFoo and PreconcatFoo, we
will now only have Foo which does what PreconcatFoo used to.

Furthermore, we also have SetFoo which is almost always used immediately after
a transform is created, so Foo would do fine (with the optimization mentioned
below).

Another bit of redundant code eliminated by this CL is
InterpolatedTransform::FactorTRS. This function was brittle and naive, and now
that gfx::Transform::Blend exists, it needs to go away.

Other minor changes rolled into this cleanup:
 - RotateAbout now takes the newly minted Vector3dF
 - The Foo functions mentioned above also check for identity to avoid
   needless matrix multiplications.

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

[Ian Vollick, 2012-11-16 19:26:31]

shawnsingh@ how do you feel about the API changes?
sky@ for ash and views
danakj@ for gfx

[danakj, 2012-11-16 19:46:12]

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

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform.cc:156: result.Rotate(interpolated_degrees);
I didn't like this change with Set at first, but seeing the implementation of
the Rotate() etc methods now, I think this is pretty cool. I am hoping that when
we drop this into calcDrawEtc, we will get a drop in our multiply times, and
increase our frame rate just like that.

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform.cc:167: gfx::Vector3dF axis,
const&

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform.cc:177: gfx::Vector3dF axis,
const&

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform.cc:355:
InterpolatedMatrixTransform::InterpolateButDoNotCompose(float t) const {
Shawn can you double check the mathy bits here? I have trouble verifying this
stuff for correctness.

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform.h
File ui/gfx/interpolated_transform.h (right):

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform.h:114:
InterpolatedAxisAngleRotation(gfx::Vector3dF axis,
const& to match ui/gfx style.

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform.h:117:
InterpolatedAxisAngleRotation(gfx::Vector3dF axis,
const&

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
File ui/gfx/interpolated_transform_unittest.cc (left):

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform_unittest.cc:117: TEST(InterpolatedTransformTest,
FactorTRS) {
Can you point me at the test that replaces this?

[shawnsingh, 2012-11-16 20:51:25]

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

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform.cc:355:
InterpolatedMatrixTransform::InterpolateButDoNotCompose(float t) const {
On 2012/11/16 19:46:12, danakj wrote:
> Shawn can you double check the mathy bits here? I have trouble verifying this
> stuff for correctness.

It seems like the implementation of this function should be simply called
GetInterpolatedTransform(t).

In particular the "but do not compose" part seems like a misnomer?  The
implementation seems to be taking two decomposed transforms, blending them, and
then "re-composing" the blended decomposition into a transform, right?  so "but
do not compose" seems wrong as I understand it...

[shawnsingh, 2012-11-16 20:53:58]

Can we please wait on this patch so that the unit tests in
http://codereview.chromium.org/11316043/ land first?

Thanks...

[Ian Vollick, 2012-11-16 21:51:23]

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

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform.cc:167: gfx::Vector3dF axis,
On 2012/11/16 19:46:12, danakj wrote:
> const&

Done, here and in the other spots.

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform.cc:355:
InterpolatedMatrixTransform::InterpolateButDoNotCompose(float t) const {
On 2012/11/16 20:51:25, shawnsingh wrote:
> On 2012/11/16 19:46:12, danakj wrote:
> > Shawn can you double check the mathy bits here? I have trouble verifying
this
> > stuff for correctness.
> 
> It seems like the implementation of this function should be simply called
> GetInterpolatedTransform(t).
> 
> In particular the "but do not compose" part seems like a misnomer?  The
> implementation seems to be taking two decomposed transforms, blending them,
and
> then "re-composing" the blended decomposition into a transform, right?  so
"but
> do not compose" seems wrong as I understand it...

This class is analogous to WebTransformOperations. It's a string of things that
can each be interpolated individually, and ultimately produce a matrix when
these interpolated bits are composed/multiplied.

Very soon, this class going to disappear. It will be completely obsolete when
WebTransformOperations has GTFO'd. So I don't think it deserves much attention.
That said, the meaning of the name is
Interpolate-This-Guy-But-Do-Not-Combine-The-Result-With-The-Next-Guy. This
allows ::Interpolate(t) to be implemented as this->InterpolateButDoNotCompose(t)
* child_->Interpolate(t).

I think the confusion comes from the fact that the word compose has been used
here, which now has loaded meaning given the Decompose work in transform_util.
This is change in the verbage I'd been using. Previously, I'd used the word
'factor' rather than 'decompose', so I think the situation was less confusing
then. But no sense renaming things now since this class is going to get deleted
soon anyhow.

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
File ui/gfx/interpolated_transform_unittest.cc (left):

https://codereview.chromium.org/11418040/diff/1/ui/gfx/interpolated_transform...
ui/gfx/interpolated_transform_unittest.cc:117: TEST(InterpolatedTransformTest,
FactorTRS) {
On 2012/11/16 19:46:12, danakj wrote:
> Can you point me at the test that replaces this?

This test did still seem useful, so I've ported it and put it in
transform_unittest.

[shawnsingh, 2012-11-20 18:53:05]

Thanks for letting the other patch land... I think this is a decent time to move
forward now.

I'm about to submit the massive sed-script migration for review today.  I'm sure
there will be conflicts between this patch and that one, but I don't think we
have to be blocked on each other anymore.

[shawnsingh, 2012-11-20 18:57:01]

FYI, I think we need to port the math_util unit tests to transform as part of
this patch  (i.e. whatever new APIs implement the same functionality).

That way we can ensure we had no gap in coverage where difficult-to-deal-with
semantics differences would creep in.

[danakj, 2012-11-20 19:00:11]

On 2012/11/20 18:57:01, shawnsingh wrote:
> FYI, I think we need to port the math_util unit tests to transform as part of
> this patch  (i.e. whatever new APIs implement the same functionality).

Why not do that separately?

> That way we can ensure we had no gap in coverage where difficult-to-deal-with
> semantics differences would creep in.

[shawnsingh, 2012-11-20 19:11:41]

Upon closer inspection, the only minor impact of what I'm suggesting:

We should port the operator* unit tests from math_util, since that seems to be
new functionality being added to gfx::Transform in this patch.

[danakj, 2012-11-20 19:15:55]

I see, I agree porting those unit tests would make sense, as the functions have
no test coverage here atm. Vollick can you port those tests as part of this?

ui/gfx LGTM.

Sky can you please take a look at the rest?

[Ian Vollick, 2012-11-20 21:53:22]

On 2012/11/20 19:15:55, danakj wrote:
> I see, I agree porting those unit tests would make sense, as the functions
have
> no test coverage here atm. Vollick can you port those tests as part of this?

Sounds good. Done.

[Ian Vollick, 2012-11-23 17:16:17]

On 2012/11/20 21:53:22, vollick wrote:
> On 2012/11/20 19:15:55, danakj wrote:
> > I see, I agree porting those unit tests would make sense, as the functions
> have
> > no test coverage here atm. Vollick can you port those tests as part of this?
> 
> Sounds good. Done.

sky@ I think this ready for review now. Can you please take a look when you have
a moment?

[danakj, 2012-11-23 18:06:24]

cc/ LGTM too

[sky, 2012-11-26 14:42:11]

LGTM

[commit-bot: I haz the power, 2012-11-26 18:15:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11418040/10005

[commit-bot: I haz the power, 2012-11-26 20:13:11]

Change committed as 169476