Split transform_converters.h from geometry_type_converters.h.

Split transform_converters.h from geometry_type_converters.h.

This makes geometry_type_converters.h independent of skia. A target
can use geometry_type_converters.h without picking up a dependency on
skia inadvertently.

Create transform_converters.h for the converters to and from
gfx::Transform, which is implemented using skia's SkMatrix44.

R=jamesr@chromium.org,msw@chromium.org
BUG=457280
Committed: https://crrev.com/ee4710dab91c8e01882a5b6902fbd171a8e48a50
Cr-Commit-Position: refs/heads/master@{#316252}

[wtc, 2015-02-11 00:33:18]

James, Mike: please review.

An alternative solution is to move the Transform converters to
mojo/converters/surfaces/surfaces_type_converters.{h,cc} because the Transform
converters are only used in the mojo/converters/surfaces/ directory.  Would you
prefer that?

https://codereview.chromium.org/911793002/diff/100001/mojo/converters/geometr...
File mojo/converters/geometry/BUILD.gn (left):

https://codereview.chromium.org/911793002/diff/100001/mojo/converters/geometr...
mojo/converters/geometry/BUILD.gn:5: # GYP version:
mojo/mojo_converters.gypi:mojo_geometry_lib

I removed this comment because there is no mojo_converters.gypi in the Chromium
source tree.

[wtc, 2015-02-12 15:57:47]

wtc@chromium.org changed reviewers:
+ jamesr@chromium.org, msw@chromium.org

[wtc, 2015-02-12 15:58:25]

James, Mike: please review. (I neglected to add you to the reviewers list.)

[jamesr, 2015-02-12 18:26:20]

https://codereview.chromium.org/911793002/diff/100001/mojo/converters/geometr...
File mojo/converters/geometry/BUILD.gn (right):

https://codereview.chromium.org/911793002/diff/100001/mojo/converters/geometr...
mojo/converters/geometry/BUILD.gn:50: defines = [ "MOJO_GEOMETRY_IMPLEMENTATION"
]
You can't have two components with the same export macro - on windows the
declspec will be wrong since it's different for things inside the component and
things outside.  If you want two components then you need different export
macros and headers.

[wtc, 2015-02-12 22:36:38]

https://codereview.chromium.org/911793002/diff/100001/mojo/converters/geometr...
File mojo/converters/geometry/BUILD.gn (right):

https://codereview.chromium.org/911793002/diff/100001/mojo/converters/geometr...
mojo/converters/geometry/BUILD.gn:50: defines = [ "MOJO_GEOMETRY_IMPLEMENTATION"
]

Hmm... you're right. I overlooked this issue.

I just analyzed the code. Since the two components are independent of each
other, it should be fine for the two components to share the same export macro
and header. These grep commands show the two *_converters.cc files do not
include the .h file of the other component:

wtc@fips:~/chrome-ensemble.2/src/mojo/converters/geometry$ ls
BUILD.gn                     geometry_type_converters.h  transform_converters.h
DEPS                         mojo_geometry_export.h
geometry_type_converters.cc  transform_converters.cc
wtc@fips:~/chrome-ensemble.2/src/mojo/converters/geometry$ grep
transform_converters.h *
BUILD.gn:    "transform_converters.h",
transform_converters.cc:#include
"mojo/converters/geometry/transform_converters.h"
wtc@fips:~/chrome-ensemble.2/src/mojo/converters/geometry$ grep
geometry_type_converters.h *
BUILD.gn:    "geometry_type_converters.h",
geometry_type_converters.cc:#include
"mojo/converters/geometry/geometry_type_converters.h"

If you think this is error-prone, would you like me to move the Transform
converters to the mojo/converters/surfaces component, which is the only user of
the Transform converters?

[jamesr, 2015-02-12 22:40:29]

I think it's overly confusing to have them in the same component since if there
is ever an #include (not even a dependency) between the two it'll produce an
inscrutable declspec linker error on windows.  I would simply create another
export macro for the transform component.  We'll definitely want to use it
independently of the surfaces converters down the line since things like media
will want to send transforms around without depending (directly) on surfaces.

[wtc, 2015-02-12 23:04:13]

I added the MOJO_TRANSFORM_EXPORT macro and the mojo_transform_export.h header.
Please review patch set 7. Thanks.

[jamesr, 2015-02-13 01:06:46]

lgtm

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
File mojo/converters/geometry/BUILD.gn (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
mojo/converters/geometry/BUILD.gn:37: "//ui/gfx",
public_deps are also deps, you don't need to say this twice

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
File mojo/converters/geometry/mojo_transform_export.h (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
mojo/converters/geometry/mojo_transform_export.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
File mojo/converters/geometry/transform_converters.cc (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
mojo/converters/geometry/transform_converters.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
File mojo/converters/geometry/transform_converters.h (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
mojo/converters/geometry/transform_converters.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/surface...
File mojo/converters/surfaces/tests/BUILD.gn (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/surface...
mojo/converters/surfaces/tests/BUILD.gn:15:
"//mojo/converters/geometry:geometry",
geometry:geometry is redundant, you can drop it

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/surface...
mojo/converters/surfaces/tests/BUILD.gn:16:
"//mojo/converters/geometry:transform",
the transform converters should probably live in //mojo/converters/transform

[wtc, 2015-02-13 02:55:13]

New patchsets have been uploaded after l-g-t-m from jamesr@chromium.org

[wtc, 2015-02-13 03:04:20]

James: thanks for the review. I implemented all of your suggestions in patch set
8.

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
File mojo/converters/geometry/BUILD.gn (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
mojo/converters/geometry/BUILD.gn:37: "//ui/gfx",
On 2015/02/13 01:06:46, jamesr wrote:
> public_deps are also deps, you don't need to say this twice

Done.

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
File mojo/converters/geometry/mojo_transform_export.h (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
mojo/converters/geometry/mojo_transform_export.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/13 01:06:46, jamesr wrote:
> 2015

Done.

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
File mojo/converters/geometry/transform_converters.cc (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
mojo/converters/geometry/transform_converters.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/13 01:06:46, jamesr wrote:
> 2015

Done.

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
File mojo/converters/geometry/transform_converters.h (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/geometr...
mojo/converters/geometry/transform_converters.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/13 01:06:46, jamesr wrote:
> 2015

Done.

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/surface...
File mojo/converters/surfaces/tests/BUILD.gn (right):

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/surface...
mojo/converters/surfaces/tests/BUILD.gn:15:
"//mojo/converters/geometry:geometry",
On 2015/02/13 01:06:46, jamesr wrote:
> geometry:geometry is redundant, you can drop it

Done.

https://codereview.chromium.org/911793002/diff/120001/mojo/converters/surface...
mojo/converters/surfaces/tests/BUILD.gn:16:
"//mojo/converters/geometry:transform",
On 2015/02/13 01:06:46, jamesr wrote:
> the transform converters should probably live in //mojo/converters/transform

Done.

[wtc, 2015-02-13 03:24:23]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2015-02-13 03:25:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/911793002/140001

[commit-bot: I haz the power, 2015-02-13 03:32:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-13 03:32:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[wtc, 2015-02-13 17:48:31]

wtc@chromium.org changed reviewers:
+ vollick@chromium.org

[wtc, 2015-02-13 17:52:59]

Ian, Mike:

I need you to review and approve the dependency of the new
mojo/converters/transform directory on "+ui/gfx/transform.h".

I split the mojo::Transform/gfx::Transform converters from
mojo/converters/geometry so that one can use mojo/converters/geometry without
picking up a dependency on skia.

The new mojo/converters/transform directory contains the
mojo::Transform/gfx::Transform converters. Its DEPS file is based on
mojo/converters/geometry/DEPS, with "+ui/gfx/geometry" removed.

Thank you.

[msw, 2015-02-13 18:25:35]

lgtm with a nit q

https://codereview.chromium.org/911793002/diff/140001/mojo/converters/transfo...
File mojo/converters/transform/transform_converters.h (right):

https://codereview.chromium.org/911793002/diff/140001/mojo/converters/transfo...
mojo/converters/transform/transform_converters.h:9: #include
"third_party/mojo_services/src/geometry/public/interfaces/geometry.mojom.h"
nit q: Would it be useful at all to split the Transform struct into a separate
transform.mojom ?

[wtc, 2015-02-13 18:59:09]

https://codereview.chromium.org/911793002/diff/140001/mojo/converters/transfo...
File mojo/converters/transform/transform_converters.h (right):

https://codereview.chromium.org/911793002/diff/140001/mojo/converters/transfo...
mojo/converters/transform/transform_converters.h:9: #include
"third_party/mojo_services/src/geometry/public/interfaces/geometry.mojom.h"

On 2015/02/13 18:25:35, msw wrote:
> nit q: Would it be useful at all to split the Transform struct into a separate
> transform.mojom ?

Yes, I had the same idea and implemented it in patch set 4.

When I discussed this with James, he thought that we didn't need to expose this
distinction at the mojom level. So I reverted the .mojom split in patch set 5.

[wtc, 2015-02-13 18:59:45]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2015-02-13 19:00:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/911793002/140001

[commit-bot: I haz the power, 2015-02-13 19:05:00]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-13 19:05:33]

Patchset 8 (id:??) landed as
https://crrev.com/ee4710dab91c8e01882a5b6902fbd171a8e48a50
Cr-Commit-Position: refs/heads/master@{#316252}

[picksi, 2015-02-19 12:19:17]

While perf sheriffing, this CL (along with several others) was reported by the
bisect bots as a possible cause of the regression shown here:

https://chromeperf.appspot.com/group_report?bug_id=459175

It seems unlikely to be the root cause, but please can you take a moment to
confirm that this CL is innocent! Thanks.