Migrate more functions from MathUtil to gfx::Transform

Migrate more functions from MathUtil to gfx::Transform

This patch (1) removes rotateEulerAngles entirely (2) migrates some util
functions from MathUtil to transform_util, and (3) moves all the MathUtil unit
tests that actually belonged in ui/gfx/.

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

[shawnsingh, 2013-01-07 22:32:24]

PTAL, thanks.

I was a little unsure what the preferred way to add a "test common" file in
ui/gfx would be.  I added test/transform_test_common.*  and added it to the gyp
file, too.

[danakj, 2013-01-07 23:22:35]

does cc_tests.gyp need to depend on ui_unittests.gyp for these helper methods?
should they be a separate gyp module then?

https://codereview.chromium.org/11774005/diff/1/cc/math_util_unittest.cc
File cc/math_util_unittest.cc (right):

https://codereview.chromium.org/11774005/diff/1/cc/math_util_unittest.cc#newc...
cc/math_util_unittest.cc:15: #include "ui/gfx/transform_util.h"
?

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
File ui/gfx/test/transform_test_common.cc (right):

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
ui/gfx/test/transform_test_common.cc:10: {
{ on previous line

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
ui/gfx/test/transform_test_common.cc:37: {
same

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
File ui/gfx/test/transform_test_common.h (right):

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
ui/gfx/test/transform_test_common.h:8: #include "ui/gfx/transform.h"
sort

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
ui/gfx/test/transform_test_common.h:14: // web_transformation_matrix_unittests,
but for now they are
does this comment make sense anymore?

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
ui/gfx/test/transform_test_common.h:65: }
}  // namespace gfx

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

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_unittest.cc#...
ui/gfx/transform_unittest.cc:1392: {
chromium style here. { on previous line for all these tests.

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_unittest.cc#...
ui/gfx/transform_unittest.cc:1400: EXPECT_ROW1_EQ(2036, 2292, 2548, 2804, C);
float literals should have .f or .0f suffix all through here.

double literals should have .0 suffix all through here.

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_unittest.cc#...
ui/gfx/transform_unittest.cc:1609: EXPECT_ROW1_NEAR(0, 0, 6, 0, A,
ERROR_THRESHOLD);
constants are named kErrorThreshold

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

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_util.h#newco...
ui/gfx/transform_util.h:63: UI_EXPORT void FlattenTransformTo2d(gfx::Transform&
transform);
Why not a member of Transform?

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_util.h#newco...
ui/gfx/transform_util.h:71: // given in column-major order. Recommended to use
this only for unit tests.
Let's stick test-only things into test/

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_util.h#newco...
ui/gfx/transform_util.h:72: UI_EXPORT Transform CreateGfxTransform(
The Gfx part is a bit redundant with the namespace this lives in, can we drop
it?

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_util.h#newco...
ui/gfx/transform_util.h:86: UI_EXPORT Transform CreateFlattenedTransform(const
Transform& transform);
Why are we breaking the pattern of always modifying existing matrices and not
creating new ones here? Should we just provide one way to flatten - ie
FlattenTransformTo2d?

[shawnsingh, 2013-01-07 23:59:53]

Thanks for review, will address all your comments even though I'm replying only
to a few of them below =)

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
File ui/gfx/test/transform_test_common.h (right):

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
ui/gfx/test/transform_test_common.h:8: #include "ui/gfx/transform.h"
On 2013/01/07 23:22:35, danakj wrote:
> sort

I wonder why this did not get caught by presubmit checks...

https://codereview.chromium.org/11774005/diff/1/ui/gfx/test/transform_test_co...
ui/gfx/test/transform_test_common.h:14: // web_transformation_matrix_unittests,
but for now they are
On 2013/01/07 23:22:35, danakj wrote:
> does this comment make sense anymore?

It's not out-dated or wrong, but maybe it's value is not very high either.  I
can remove it.

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

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_unittest.cc#...
ui/gfx/transform_unittest.cc:1609: EXPECT_ROW1_NEAR(0, 0, 6, 0, A,
ERROR_THRESHOLD);
On 2013/01/07 23:22:35, danakj wrote:
> constants are named kErrorThreshold

This is a macro, not a constant.  Do you want to change it to a constant?

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

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_util.h#newco...
ui/gfx/transform_util.h:63: UI_EXPORT void FlattenTransformTo2d(gfx::Transform&
transform);
On 2013/01/07 23:22:35, danakj wrote:
> Why not a member of Transform?

No strong reason;  I'll move it into class.

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_util.h#newco...
ui/gfx/transform_util.h:86: UI_EXPORT Transform CreateFlattenedTransform(const
Transform& transform);
On 2013/01/07 23:22:35, danakj wrote:
> Why are we breaking the pattern of always modifying existing matrices and not
> creating new ones here? Should we just provide one way to flatten - ie
> FlattenTransformTo2d?

not "breaking" so much as keeping status quo.... but, no problem, I'll see if I
can change it in this patch, too.

[Ian Vollick, 2013-01-08 00:41:21]

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

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_util.h#newco...
ui/gfx/transform_util.h:72: UI_EXPORT Transform CreateGfxTransform(
On 2013/01/07 23:22:35, danakj wrote:
> The Gfx part is a bit redundant with the namespace this lives in, can we drop
> it?
How would you feel about adding some gfx::Transform ctors that takes 16 and 6
doubles?

[danakj, 2013-01-08 00:45:08]

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

https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_unittest.cc#...
ui/gfx/transform_unittest.cc:1609: EXPECT_ROW1_NEAR(0, 0, 6, 0, A,
ERROR_THRESHOLD);
On 2013/01/07 23:59:53, shawnsingh wrote:
> On 2013/01/07 23:22:35, danakj wrote:
> > constants are named kErrorThreshold
> 
> This is a macro, not a constant.  Do you want to change it to a constant?

Oh, I see. I guess it's fine.

[shawnsingh, 2013-01-08 18:55:30]

On 2013/01/08 00:41:21, vollick wrote:
> https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_util.h
> File ui/gfx/transform_util.h (right):
> 
>
https://codereview.chromium.org/11774005/diff/1/ui/gfx/transform_util.h#newco...
> ui/gfx/transform_util.h:72: UI_EXPORT Transform CreateGfxTransform(
> On 2013/01/07 23:22:35, danakj wrote:
> > The Gfx part is a bit redundant with the namespace this lives in, can we
drop
> > it?
> How would you feel about adding some gfx::Transform ctors that takes 16 and 6
> doubles?

Doing this probably means we don't need the extra transform_test_common, as
well.   So yeah I'd like to do this =)

[shawnsingh, 2013-01-08 23:38:17]

PTAL

(1) Fixed the .0 issue in a large portion of transform_unittests.cc
(2) This time there was no need for the test/transform_test_common.* stuff
(3) Reduced flattening ops to Transform::FlattenTo2d()
(4) Also moved the createGfxTransform stuff into Transform
(5) I think I did address all other feedback, too.

thanks!

[Ian Vollick, 2013-01-08 23:52:54]

https://codereview.chromium.org/11774005/diff/10001/ui/gfx/transform.h
File ui/gfx/transform.h (right):

https://codereview.chromium.org/11774005/diff/10001/ui/gfx/transform.h#newcode47
ui/gfx/transform.h:47: double col4row1, double col4row2, double col4row3, double
col4row4);
I'm sorry to start on this again, but can we please do row major instead? This
new constructor has horizontal columns and vertical rows, which is a little
weird, and it doesn't match how matrices are displayed in the spec.

[danakj, 2013-01-09 14:33:46]

https://codereview.chromium.org/11774005/diff/10001/ui/gfx/transform.h
File ui/gfx/transform.h (right):

https://codereview.chromium.org/11774005/diff/10001/ui/gfx/transform.h#newcode47
ui/gfx/transform.h:47: double col4row1, double col4row2, double col4row3, double
col4row4);
On 2013/01/08 23:52:54, vollick wrote:
> I'm sorry to start on this again, but can we please do row major instead? This
> new constructor has horizontal columns and vertical rows, which is a little
> weird, and it doesn't match how matrices are displayed in the spec.

+1

OOO for a bit today, will review this later.

[shawnsingh, 2013-01-09 19:58:55]

PTAL - I converted the constructors to row-major form.

It's worth noting that there are a number of caveats to doing this:
- now our 16-element constructor is transposed compared to WebKit's
TransformationMatrix.
- column-major is pretty standard, as I understand it
- there might be (I haven't experimented, no concrete evidence) a slight
performance edge to column-major, since that is also the layout of the
underlying data structure.

Just FYI.  Actually I agree that row-major format is more intuitive and readable
for these constructors, and they are not too high-profile critical path places.

[danakj, 2013-01-11 04:14:12]

LGTM+nits

https://codereview.chromium.org/11774005/diff/17001/cc/draw_quad_unittest.cc
File cc/draw_quad_unittest.cc (right):

https://codereview.chromium.org/11774005/diff/17001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:28: gfx::Transform quadTransform = gfx::Transform(1, 0,
0.5, 1, 0.5, 0);
nit: these are going to have to become 1.0, 0.0, 0.5, 1.0, 0.5, 0.0 at some
point (ie. use .0 for double literals). might as well do that now?

https://codereview.chromium.org/11774005/diff/17001/cc/layer_unittest.cc
File cc/layer_unittest.cc (right):

https://codereview.chromium.org/11774005/diff/17001/cc/layer_unittest.cc#newc...
cc/layer_unittest.cc:536: EXPECT_SET_NEEDS_COMMIT(1,
testLayer->setSublayerTransform(gfx::Transform(0, 0, 0, 0, 0, 0)));
nit: 0.0s here

https://codereview.chromium.org/11774005/diff/17001/cc/render_pass_unittest.cc
File cc/render_pass_unittest.cc (right):

https://codereview.chromium.org/11774005/diff/17001/cc/render_pass_unittest.c...
cc/render_pass_unittest.cc:37: gfx::Transform transformToRoot =
gfx::Transform(1, 0.5, 0.5, -0.5, -1, 0);
nit: 1.0, 0.5...

https://codereview.chromium.org/11774005/diff/17001/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

https://codereview.chromium.org/11774005/diff/17001/cc/video_layer_impl.cc#ne...
cc/video_layer_impl.cc:33: 1, 0, 0, 0,
nit: here too, .0 suffixes

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

https://codereview.chromium.org/11774005/diff/17001/ui/gfx/transform.cc#newco...
ui/gfx/transform.cc:42: // Initialize column 1
nit: i dont think these comments are explaining much

https://codereview.chromium.org/11774005/diff/17001/ui/gfx/transform.cc#newco...
ui/gfx/transform.cc:70: double x_translation, double y_translation)
can you make this call matrix_(SkMatrix44::kIdentity_Constructor) explicitly?

https://codereview.chromium.org/11774005/diff/17001/ui/gfx/transform.h
File ui/gfx/transform.h (right):

https://codereview.chromium.org/11774005/diff/17001/ui/gfx/transform.h#newcode40
ui/gfx/transform.h:40: // Constructs a transform from explicit 16 matrix
elements. NOTE: Elements
nit: drop the NOTE? the variable names kinda say this already.

Or if you feel strongly, just drop the "NOTE: " part..

[commit-bot: I haz the power, 2013-01-11 21:45:03]

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

[commit-bot: I haz the power, 2013-01-11 22:21:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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.

[shawnsingh, 2013-01-14 18:24:52]

looks like I need to use the .0f syntax to make all my literals as floats, based
on the bot failures.

[jamesr, 2013-01-14 19:22:38]

On 2013/01/14 18:24:52, shawnsingh wrote:
> looks like I need to use the .0f syntax to make all my literals as floats,
based
> on the bot failures.

Yes, specifically for constants that aren't convertible from double->float
losslessly (for instance 0.1, which can't be exactly represented by either
type).

[commit-bot: I haz the power, 2013-01-15 03:00:18]

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

[commit-bot: I haz the power, 2013-01-15 07:49:42]

Change committed as 176832