Mojo bindings: Fix typemap includes.

Mojo bindings: Fix typemap includes.

Previously, a header defining a StructTraits specialization could not
include its corresponding generated mojom header, due to the mojom
header including the StructTraits header. This required a brittle set of
forward declarations to correctly avoid this; in practice, this mostly
led to StructTraits that were nominally for a particular variant, but
included the generated mojom header from the default variant.

This CL fixes the issue by splitting typemap headers into
public_headers, which define the native type and are included by the
generated mojom header, and headers which define the StructTraits
specialization for the native type and are only included by the
generated mojom source file, allowing the StructTraits header to include
the generated mojom header.

BUG=596202
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/a1107b7873a0b930ff197da68c3eaacd751839d5
Cr-Commit-Position: refs/heads/master@{#382893}

[Sam McNally, 2016-03-21 22:44:29]

Description was changed from

==========
Mojo bindings: Fix typemap includes.

BUG=596202
==========

to

==========
Mojo bindings: Fix typemap includes.

BUG=596202
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Sam McNally, 2016-03-22 17:01:22]

Description was changed from

==========
Mojo bindings: Fix typemap includes.

BUG=596202
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Mojo bindings: Fix typemap includes.

Previously, a header defining a StructTraits specialization could not
include its corresponding generated mojom header, due to the mojom
header including the StructTraits header. This required a brittle set of
forward declarations to avoid this; in practice, this mostly led to
StructTraits that were nominally for a particular  variant, but
including the generated mojom header from the default variant.

This CL fixes the issue by splitting typemap headers into
public_headers, which define the native type and are included by the
generated mojom header, and headers which define the StructTraits
specialization for the native type and are only included by the
generated mojom source file, allowing the StructTraits header to include
the generated mojom header.

BUG=596202
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Sam McNally, 2016-03-22 17:14:54]

Description was changed from

==========
Mojo bindings: Fix typemap includes.

Previously, a header defining a StructTraits specialization could not
include its corresponding generated mojom header, due to the mojom
header including the StructTraits header. This required a brittle set of
forward declarations to avoid this; in practice, this mostly led to
StructTraits that were nominally for a particular  variant, but
including the generated mojom header from the default variant.

This CL fixes the issue by splitting typemap headers into
public_headers, which define the native type and are included by the
generated mojom header, and headers which define the StructTraits
specialization for the native type and are only included by the
generated mojom source file, allowing the StructTraits header to include
the generated mojom header.

BUG=596202
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Mojo bindings: Fix typemap includes.

Previously, a header defining a StructTraits specialization could not
include its corresponding generated mojom header, due to the mojom
header including the StructTraits header. This required a brittle set of
forward declarations to correctly avoid this; in practice, this mostly
led to StructTraits that were nominally for a particular variant, but
included the generated mojom header from the default variant.

This CL fixes the issue by splitting typemap headers into
public_headers, which define the native type and are included by the
generated mojom header, and headers which define the StructTraits
specialization for the native type and are only included by the
generated mojom source file, allowing the StructTraits header to include
the generated mojom header.

BUG=596202
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Sam McNally, 2016-03-22 17:15:38]

sammc@chromium.org changed reviewers:
+ rockot@chromium.org, yzshen@chromium.org

[yzshen1, 2016-03-22 17:43:32]

First part of comments... Thanks!

https://codereview.chromium.org/1821073002/diff/80001/gpu/command_buffer/comm...
File gpu/command_buffer/common/mojo.typemap (right):

https://codereview.chromium.org/1821073002/diff/80001/gpu/command_buffer/comm...
gpu/command_buffer/common/mojo.typemap:12: "headers": [
Could we consider more specific names? I feel difficult to determine what
headers should go into either section by looking at the names.

Does it make sense to use "type_headers" and "struct_traits_headers" (or
"serialization_headers" which gives us more freedom to rename/refactor
StructTraits later)?

https://codereview.chromium.org/1821073002/diff/80001/mojo/mojo_public.gyp
File mojo/mojo_public.gyp (right):

https://codereview.chromium.org/1821073002/diff/80001/mojo/mojo_public.gyp#ne...
mojo/mojo_public.gyp:405:
'public/interfaces/bindings/tests/math_calculator.mojom',
It makes more sense to me to rename this target to
"mojo_public_test_native_types_{blink,chromium}" and only include those
necessary mojom files. WDYT?

https://codereview.chromium.org/1821073002/diff/80001/mojo/mojo_public.gyp#ne...
mojo/mojo_public.gyp:439:
'public/interfaces/bindings/tests/math_calculator.mojom',
ditto

https://codereview.chromium.org/1821073002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/1821073002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:37: # TODO: crbug.com/596202
Uncomment and update these files?

One question: it seems we don't express things like
rect_blink.h/rect_blink_traits.h as input explicitly for any build target. If
those files are changed, we will fail to re-compile affected targets, right?

[Ken Rockot(use gerrit already), 2016-03-22 18:00:10]

https://codereview.chromium.org/1821073002/diff/80001/url/mojo/origin.typemap
File url/mojo/origin.typemap (right):

https://codereview.chromium.org/1821073002/diff/80001/url/mojo/origin.typemap...
url/mojo/origin.typemap:12: "headers": [
nit: I wonder if we should rename this to traits_headers since in practice it
should only be used to provide the definitions needed for serialization
implementation.

https://codereview.chromium.org/1821073002/diff/80001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/1821073002/diff/80001/url/url.gyp#newcode72
url/url.gyp:72: 'target_name': 'url_interfaces_mojom',
nit: Maybe we should rename this target to url_mojom_chromium? Or I guess we
could adopt the convention that no suffix/variant implies chromium variant.

https://codereview.chromium.org/1821073002/diff/80001/url/url.gyp#newcode74
url/url.gyp:74: 'variables': {
Why is this no longer chromium variant? Or is it because we are adopting the
aforementioned convention?

[Sam McNally, 2016-03-22 19:22:03]

https://codereview.chromium.org/1821073002/diff/80001/gpu/command_buffer/comm...
File gpu/command_buffer/common/mojo.typemap (right):

https://codereview.chromium.org/1821073002/diff/80001/gpu/command_buffer/comm...
gpu/command_buffer/common/mojo.typemap:12: "headers": [
On 2016/03/22 17:43:32, yzshen1 wrote:
> Could we consider more specific names? I feel difficult to determine what
> headers should go into either section by looking at the names.
> 
> Does it make sense to use "type_headers" and "struct_traits_headers" (or
> "serialization_headers" which gives us more freedom to rename/refactor
> StructTraits later)?

Ken and I discussed this and ended up with public_headers and traits_headers.

https://codereview.chromium.org/1821073002/diff/80001/mojo/mojo_public.gyp
File mojo/mojo_public.gyp (right):

https://codereview.chromium.org/1821073002/diff/80001/mojo/mojo_public.gyp#ne...
mojo/mojo_public.gyp:405:
'public/interfaces/bindings/tests/math_calculator.mojom',
On 2016/03/22 17:43:32, yzshen1 wrote:
> It makes more sense to me to rename this target to
> "mojo_public_test_native_types_{blink,chromium}" and only include those
> necessary mojom files. WDYT?

The reason for this is there's a dependency from test_native_types.mojom on
rect.mojom. I'd prefer for the per-variant targets to not diverge from the
non-variant target.

https://codereview.chromium.org/1821073002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/1821073002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:37: # TODO: crbug.com/596202
On 2016/03/22 17:43:32, yzshen1 wrote:
> Uncomment and update these files?

Done.
> 
> One question: it seems we don't express things like
> rect_blink.h/rect_blink_traits.h as input explicitly for any build target. If
> those files are changed, we will fail to re-compile affected targets, right?
> 

Yes. I expect we will need to add "typemap_sources" to mojom to correctly
specify dependencies. Strict dependency enforcement of includes would be so
nice.

https://codereview.chromium.org/1821073002/diff/80001/url/mojo/origin.typemap
File url/mojo/origin.typemap (right):

https://codereview.chromium.org/1821073002/diff/80001/url/mojo/origin.typemap...
url/mojo/origin.typemap:12: "headers": [
On 2016/03/22 18:00:10, Ken Rockot wrote:
> nit: I wonder if we should rename this to traits_headers since in practice it
> should only be used to provide the definitions needed for serialization
> implementation.

Done.

https://codereview.chromium.org/1821073002/diff/80001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/1821073002/diff/80001/url/url.gyp#newcode72
url/url.gyp:72: 'target_name': 'url_interfaces_mojom',
On 2016/03/22 18:00:10, Ken Rockot wrote:
> nit: Maybe we should rename this target to url_mojom_chromium? Or I guess we
> could adopt the convention that no suffix/variant implies chromium variant.

I think we're going to default to no suffix/variant implies chromium because of
all the existing mojom targets.

https://codereview.chromium.org/1821073002/diff/80001/url/url.gyp#newcode74
url/url.gyp:74: 'variables': {
On 2016/03/22 18:00:10, Ken Rockot wrote:
> Why is this no longer chromium variant? Or is it because we are adopting the
> aforementioned convention?

It was either this or change all the other mojom targets to the chromium
variant. //content depends on this and this CL adds enforcement (only in GN) of
same-variant mojom dependencies.

[Ken Rockot(use gerrit already), 2016-03-22 19:28:40]

lgtm but please wait for yuzhu's full review

[yzshen1, 2016-03-22 20:29:14]

https://codereview.chromium.org/1821073002/diff/80001/mojo/mojo_public.gyp
File mojo/mojo_public.gyp (right):

https://codereview.chromium.org/1821073002/diff/80001/mojo/mojo_public.gyp#ne...
mojo/mojo_public.gyp:405:
'public/interfaces/bindings/tests/math_calculator.mojom',
On 2016/03/22 19:22:03, Sam McNally wrote:
> On 2016/03/22 17:43:32, yzshen1 wrote:
> > It makes more sense to me to rename this target to
> > "mojo_public_test_native_types_{blink,chromium}" and only include those
> > necessary mojom files. WDYT?
> 
> The reason for this is there's a dependency from test_native_types.mojom on
> rect.mojom. I'd prefer for the per-variant targets to not diverge from the
> non-variant target.

Could we make this list a variable that can be shared? Otherwise they will go
out of sync without being noticed fairly soon.

https://codereview.chromium.org/1821073002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/1821073002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/BUILD.gn:37: # TODO: crbug.com/596202
On 2016/03/22 19:22:03, Sam McNally wrote:
> On 2016/03/22 17:43:32, yzshen1 wrote:
> > Uncomment and update these files?
> 
> Done.
> > 
> > One question: it seems we don't express things like
> > rect_blink.h/rect_blink_traits.h as input explicitly for any build target.
If
> > those files are changed, we will fail to re-compile affected targets, right?
> > 
> 
> Yes. I expect we will need to add "typemap_sources" to mojom to correctly
> specify dependencies. Strict dependency enforcement of includes would be so
> nice.

Yeah.
I am concerned that without such enforcement we will easily run into annoying
compilation issues.

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/BUILD.gn:38: "rect_chromium.h",
Please also make corresponding changes to mojo_edk_tests.gyp

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/BUILD.gn:53: "struct_with_traits_impl_traits.h",
Why we include "struct_with_traits_impl_traits.*" here but not
"rect_{blink,chromium}_traits.*"?

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:126:
":test_interfaces_variant_test",
"test_variant" depends on none of those files, I think there is no need to add
this new target.

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:60: 
nit: one empty line (instead of two) between class members?

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:80: def
FormatForTypemap(self):
How about FormatForMojom? It is actually the format how we express a qualified
name in mojom. We use it not only for type map.

[Sam McNally, 2016-03-22 21:54:31]

https://codereview.chromium.org/1821073002/diff/80001/mojo/mojo_public.gyp
File mojo/mojo_public.gyp (right):

https://codereview.chromium.org/1821073002/diff/80001/mojo/mojo_public.gyp#ne...
mojo/mojo_public.gyp:405:
'public/interfaces/bindings/tests/math_calculator.mojom',
On 2016/03/22 20:29:13, yzshen1 wrote:
> On 2016/03/22 19:22:03, Sam McNally wrote:
> > On 2016/03/22 17:43:32, yzshen1 wrote:
> > > It makes more sense to me to rename this target to
> > > "mojo_public_test_native_types_{blink,chromium}" and only include those
> > > necessary mojom files. WDYT?
> > 
> > The reason for this is there's a dependency from test_native_types.mojom on
> > rect.mojom. I'd prefer for the per-variant targets to not diverge from the
> > non-variant target.
> 
> Could we make this list a variable that can be shared? Otherwise they will go
> out of sync without being noticed fairly soon.

Done.

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/BUILD.gn:38: "rect_chromium.h",
On 2016/03/22 20:29:13, yzshen1 wrote:
> Please also make corresponding changes to mojo_edk_tests.gyp

Done.

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/BUILD.gn:53: "struct_with_traits_impl_traits.h",
On 2016/03/22 20:29:13, yzshen1 wrote:
> Why we include "struct_with_traits_impl_traits.*" here but not
> "rect_{blink,chromium}_traits.*"?

Done.

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:126:
":test_interfaces_variant_test",
On 2016/03/22 20:29:13, yzshen1 wrote:
> "test_variant" depends on none of those files, I think there is no need to add
> this new target.

Done.

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:60: 
On 2016/03/22 20:29:14, yzshen1 wrote:
> nit: one empty line (instead of two) between class members?

Done.

https://codereview.chromium.org/1821073002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:80: def
FormatForTypemap(self):
On 2016/03/22 20:29:14, yzshen1 wrote:
> How about FormatForMojom? It is actually the format how we express a qualified
> name in mojom. We use it not only for type map.

Done.

[yzshen1, 2016-03-22 23:19:19]

LGTM

https://codereview.chromium.org/1821073002/diff/140001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/test_native_types.mojom (right):

https://codereview.chromium.org/1821073002/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/test_native_types.mojom:26: // TODO:
crbug.com/596202
This line can be removed now.

[Sam McNally, 2016-03-22 23:34:30]

sammc@chromium.org changed reviewers:
+ jam@chromium.org

[Sam McNally, 2016-03-22 23:34:31]

+jam for everything outside //mojo

https://codereview.chromium.org/1821073002/diff/140001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/test_native_types.mojom (right):

https://codereview.chromium.org/1821073002/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/test_native_types.mojom:26: // TODO:
crbug.com/596202
On 2016/03/22 23:19:19, yzshen1 wrote:
> This line can be removed now.

Done.

[jam, 2016-03-23 15:18:45]

just one question

https://codereview.chromium.org/1821073002/diff/160001/url/mojo/BUILD.gn
File url/mojo/BUILD.gn (left):

https://codereview.chromium.org/1821073002/diff/160001/url/mojo/BUILD.gn#oldc...
url/mojo/BUILD.gn:7: mojom("url_mojom") {
don't we want to generate the non-typemapped url bindings for when they're used
in other languages?

[Sam McNally, 2016-03-23 17:13:14]

https://codereview.chromium.org/1821073002/diff/160001/url/mojo/BUILD.gn
File url/mojo/BUILD.gn (left):

https://codereview.chromium.org/1821073002/diff/160001/url/mojo/BUILD.gn#oldc...
url/mojo/BUILD.gn:7: mojom("url_mojom") {
On 2016/03/23 15:18:45, jam wrote:
> don't we want to generate the non-typemapped url bindings for when they're
used
> in other languages?

With a typemap, bindings for other languages will be generated. They'll just
ignore the typemap. Variants only generate C++ bindings.

[jam, 2016-03-23 17:31:43]

On 2016/03/23 17:13:14, Sam McNally wrote:
> https://codereview.chromium.org/1821073002/diff/160001/url/mojo/BUILD.gn
> File url/mojo/BUILD.gn (left):
> 
>
https://codereview.chromium.org/1821073002/diff/160001/url/mojo/BUILD.gn#oldc...
> url/mojo/BUILD.gn:7: mojom("url_mojom") {
> On 2016/03/23 15:18:45, jam wrote:
> > don't we want to generate the non-typemapped url bindings for when they're
> used
> > in other languages?
> 
> With a typemap, bindings for other languages will be generated. They'll just
> ignore the typemap. Variants only generate C++ bindings.

lgtm, thanks for the explanation (i'll add this to things we should document :)
)

[Sam McNally, 2016-03-23 17:33:07]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-03-23 17:33:08]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1821073002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2016-03-23 17:33:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1821073002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1821073002/160001

[commit-bot: I haz the power, 2016-03-23 18:52:08]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-23 18:55:02]

Description was changed from

==========
Mojo bindings: Fix typemap includes.

Previously, a header defining a StructTraits specialization could not
include its corresponding generated mojom header, due to the mojom
header including the StructTraits header. This required a brittle set of
forward declarations to correctly avoid this; in practice, this mostly
led to StructTraits that were nominally for a particular variant, but
included the generated mojom header from the default variant.

This CL fixes the issue by splitting typemap headers into
public_headers, which define the native type and are included by the
generated mojom header, and headers which define the StructTraits
specialization for the native type and are only included by the
generated mojom source file, allowing the StructTraits header to include
the generated mojom header.

BUG=596202
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Mojo bindings: Fix typemap includes.

Previously, a header defining a StructTraits specialization could not
include its corresponding generated mojom header, due to the mojom
header including the StructTraits header. This required a brittle set of
forward declarations to correctly avoid this; in practice, this mostly
led to StructTraits that were nominally for a particular variant, but
included the generated mojom header from the default variant.

This CL fixes the issue by splitting typemap headers into
public_headers, which define the native type and are included by the
generated mojom header, and headers which define the StructTraits
specialization for the native type and are only included by the
generated mojom source file, allowing the StructTraits header to include
the generated mojom header.

BUG=596202
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/a1107b7873a0b930ff197da68c3eaacd751839d5
Cr-Commit-Position: refs/heads/master@{#382893}
==========

[commit-bot: I haz the power, 2016-03-23 18:55:04]

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