Mojo: Simplify typemap usage.

Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this CL,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
Committed: https://crrev.com/b54ee6567f200280a284fc466706962fa810e398
Cr-Commit-Position: refs/heads/master@{#388164}

[Sam McNally, 2016-03-29 04:28:11]

Description was changed from

==========
WIP: Global Mojo typemaps.
==========

to

==========
WIP: Global Mojo typemaps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Sam McNally, 2016-04-04 00:37:44]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-04-04 00:37:59]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-04-04 00:38:37]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-04-04 00:38:55]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-04-04 00:39:08]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2016-04-04 00:39:20]

Patchset #1 (id:100001) has been deleted

[Sam McNally, 2016-04-04 00:39:38]

Patchset #1 (id:120001) has been deleted

[Sam McNally, 2016-04-04 00:39:44]

Patchset #1 (id:140001) has been deleted

[Sam McNally, 2016-04-04 00:39:50]

Patchset #1 (id:160001) has been deleted

[Sam McNally, 2016-04-04 00:39:52]

Patchset #1 (id:180001) has been deleted

[Sam McNally, 2016-04-04 00:39:55]

Patchset #1 (id:200001) has been deleted

[Sam McNally, 2016-04-04 00:39:57]

Patchset #2 (id:220001) has been deleted

[Sam McNally, 2016-04-04 00:39:59]

Patchset #1 (id:240001) has been deleted

[Sam McNally, 2016-04-04 00:40:02]

Patchset #1 (id:260001) has been deleted

[Sam McNally, 2016-04-04 00:40:06]

Patchset #1 (id:280001) has been deleted

[Sam McNally, 2016-04-04 00:40:11]

Patchset #1 (id:300001) has been deleted

[Sam McNally, 2016-04-04 01:42:19]

Description was changed from

==========
WIP: Global Mojo typemaps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This only adds partial GYP support. Typemaps can be specified as GYP
variables instead of directly specifying extra generator command line
arguments and typemaps of dependencies can be automatically applied if
dependencies are correctly specified.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

[Sam McNally, 2016-04-04 02:45:58]

Description was changed from

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This only adds partial GYP support. Typemaps can be specified as GYP
variables instead of directly specifying extra generator command line
arguments and typemaps of dependencies can be automatically applied if
dependencies are correctly specified.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

to

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this cl,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

[Sam McNally, 2016-04-04 03:08:15]

Patchset #1 (id:320001) has been deleted

[Sam McNally, 2016-04-04 03:08:27]

Patchset #1 (id:340001) has been deleted

[Sam McNally, 2016-04-04 03:31:14]

Description was changed from

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this cl,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

to

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this cl,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

[Sam McNally, 2016-04-04 07:26:42]

Patchset #1 (id:360001) has been deleted

[Sam McNally, 2016-04-04 07:27:00]

Patchset #2 (id:400001) has been deleted

[Sam McNally, 2016-04-04 10:06:10]

Description was changed from

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this cl,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

to

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this CL,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

[Sam McNally, 2016-04-04 10:06:35]

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

[yzshen1, 2016-04-05 17:36:14]

First part of comments... (haven't reviewed py files.)

Thanks!

https://codereview.chromium.org/1832703002/diff/380001/gpu/command_buffer/com...
File gpu/command_buffer/common/capabilities.typemap (right):

https://codereview.chromium.org/1832703002/diff/380001/gpu/command_buffer/com...
gpu/command_buffer/common/capabilities.typemap:12: type_mappings = [
"gpu.mojom.Capabilities=gpu::Capabilities" ]
Does it make sense to use a map instead? Something like:

type_mappings = { "gpu.mojom.Capabilities" : "gpu::Capabilities" }

I feel that it is easier to read and parse. WDYT?

https://codereview.chromium.org/1832703002/diff/380001/mojo/mojo_public.gyp
File mojo/mojo_public.gyp (left):

https://codereview.chromium.org/1832703002/diff/380001/mojo/mojo_public.gyp#o...
mojo/mojo_public.gyp:518: 'mojom_variant': 'test_variant',
Instead of moving them into "mojo_public_test_interfaces_mojom_files", maybe it
makes sense to either:
- let them remain in this target which is an isolated test focusing on the
variant feature; or
- simply delete them, because we should have enough test coverage with other
files in "mojo_public_test_interfaces_mojom_files" being compiled into
"mojo_public_test_interfaces_blink".

WDYT?

https://codereview.chromium.org/1832703002/diff/380001/mojo/mojom_bindings_ge...
File mojo/mojom_bindings_generator_explicit.gypi (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/mojom_bindings_ge...
mojo/mojom_bindings_generator_explicit.gypi:72: 'action_name':
'<(_target_name)__type_mappings',
nit: it seems a little inconsistent to use "__" (two chars) here because we only
use one for things like '<(_target_name)_mojom_bindings_generator'.

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/struct_with_traits.typemap (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/struct_with_traits.typemap:11:
"//mojo/public/cpp/bindings/tests/struct_with_traits_impl_traits.cc",
Is "struct_with_triats_impl.cc" missing?
Do we need to also specify deps that sources depend on?
I noticed that gpu_command_buffer.typemap specifies deps but not sources. I
wonder why is the difference.

A document explaining the design would help reviewers and future users to
understand.

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
File mojo/public/tools/bindings/blink_bindings_configuration.gni (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
mojo/public/tools/bindings/blink_bindings_configuration.gni:9: _typemaps = [
I thought previously you said users didn't have to add typemaps to a global list
explicitly?

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
mojo/public/tools/bindings/mojom.gni:89: foreach(bindings_configuration,
_bindings_configurations) {
Some interfaces are not intended to be used by blink, it seems a waste to
generate code for them. WDYT?

[Sam McNally, 2016-04-06 00:24:19]

Patchset #2 (id:420001) has been deleted

[Sam McNally, 2016-04-06 07:02:53]

https://codereview.chromium.org/1832703002/diff/380001/gpu/command_buffer/com...
File gpu/command_buffer/common/capabilities.typemap (right):

https://codereview.chromium.org/1832703002/diff/380001/gpu/command_buffer/com...
gpu/command_buffer/common/capabilities.typemap:12: type_mappings = [
"gpu.mojom.Capabilities=gpu::Capabilities" ]
On 2016/04/05 17:36:14, yzshen1 wrote:
> Does it make sense to use a map instead? Something like:
> 
> type_mappings = { "gpu.mojom.Capabilities" : "gpu::Capabilities" }
> 
> I feel that it is easier to read and parse. WDYT?

I would prefer something more like that, but GN doesn't support maps.

https://codereview.chromium.org/1832703002/diff/380001/mojo/mojo_public.gyp
File mojo/mojo_public.gyp (left):

https://codereview.chromium.org/1832703002/diff/380001/mojo/mojo_public.gyp#o...
mojo/mojo_public.gyp:518: 'mojom_variant': 'test_variant',
On 2016/04/05 17:36:14, yzshen1 wrote:
> Instead of moving them into "mojo_public_test_interfaces_mojom_files", maybe
it
> makes sense to either:
> - let them remain in this target which is an isolated test focusing on the
> variant feature; or
> - simply delete them, because we should have enough test coverage with other
> files in "mojo_public_test_interfaces_mojom_files" being compiled into
> "mojo_public_test_interfaces_blink".
> 
> WDYT?

Deleted them.

https://codereview.chromium.org/1832703002/diff/380001/mojo/mojom_bindings_ge...
File mojo/mojom_bindings_generator_explicit.gypi (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/mojom_bindings_ge...
mojo/mojom_bindings_generator_explicit.gypi:72: 'action_name':
'<(_target_name)__type_mappings',
On 2016/04/05 17:36:14, yzshen1 wrote:
> nit: it seems a little inconsistent to use "__" (two chars) here because we
only
> use one for things like '<(_target_name)_mojom_bindings_generator'.

Done.

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/struct_with_traits.typemap (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/struct_with_traits.typemap:11:
"//mojo/public/cpp/bindings/tests/struct_with_traits_impl_traits.cc",
On 2016/04/05 17:36:14, yzshen1 wrote:
> Is "struct_with_triats_impl.cc" missing?
> Do we need to also specify deps that sources depend on?
> I noticed that gpu_command_buffer.typemap specifies deps but not sources. I
> wonder why is the difference.

The typemap should have:
- deps for the native type
- public_headers containing the header of the native type
- traits_headers containing the header with the StructTraits for the native type
- sources containing the implementation of the StructTraits for the native type
- type_mappings

For native-only types, the typemap should have:
- deps for the native type and its ParamTraits
- public_headers containing the header of the native type
- traits_headers containing the header with the ParamTraits for the native type
- type_mappings

This was missing a dependency on a target containing StructWithTraitsImpl.

> 
> A document explaining the design would help reviewers and future users to
> understand.

Yes.

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
File mojo/public/tools/bindings/blink_bindings_configuration.gni (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
mojo/public/tools/bindings/blink_bindings_configuration.gni:9: _typemaps = [
On 2016/04/05 17:36:14, yzshen1 wrote:
> I thought previously you said users didn't have to add typemaps to a global
list
> explicitly?

Typemaps don't have to be added to this file explicitly; they could be listed in
some other gni file that this one reads. The alternative would be a Python
script to scan for all the typemaps, but that would slow down gn gen; on Linux a
simple Python script to find all the .typemap files takes 3 seconds.

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
mojo/public/tools/bindings/mojom.gni:89: foreach(bindings_configuration,
_bindings_configurations) {
On 2016/04/05 17:36:14, yzshen1 wrote:
> Some interfaces are not intended to be used by blink, it seems a waste to
> generate code for them. WDYT?

If nothing depends on the blink variant mojoms then they won't be generated.

[yzshen1, 2016-04-06 16:00:47]

https://codereview.chromium.org/1832703002/diff/380001/gpu/command_buffer/com...
File gpu/command_buffer/common/capabilities.typemap (right):

https://codereview.chromium.org/1832703002/diff/380001/gpu/command_buffer/com...
gpu/command_buffer/common/capabilities.typemap:12: type_mappings = [
"gpu.mojom.Capabilities=gpu::Capabilities" ]
On 2016/04/06 07:02:53, Sam McNally wrote:
> On 2016/04/05 17:36:14, yzshen1 wrote:
> > Does it make sense to use a map instead? Something like:
> > 
> > type_mappings = { "gpu.mojom.Capabilities" : "gpu::Capabilities" }
> > 
> > I feel that it is easier to read and parse. WDYT?
> 
> I would prefer something more like that, but GN doesn't support maps.

Ah. That is a little unfortunate.

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/struct_with_traits.typemap (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/struct_with_traits.typemap:11:
"//mojo/public/cpp/bindings/tests/struct_with_traits_impl_traits.cc",
On 2016/04/06 07:02:53, Sam McNally wrote:
> On 2016/04/05 17:36:14, yzshen1 wrote:
> > Is "struct_with_triats_impl.cc" missing?
> > Do we need to also specify deps that sources depend on?
> > I noticed that gpu_command_buffer.typemap specifies deps but not sources. I
> > wonder why is the difference.
> 
> The typemap should have:
> - deps for the native type
> - public_headers containing the header of the native type
> - traits_headers containing the header with the StructTraits for the native
type
> - sources containing the implementation of the StructTraits for the native
type
> - type_mappings
> 
> For native-only types, the typemap should have:
> - deps for the native type and its ParamTraits
> - public_headers containing the header of the native type
> - traits_headers containing the header with the ParamTraits for the native
type
> - type_mappings
> 
> This was missing a dependency on a target containing StructWithTraitsImpl.
> 
> > 
> > A document explaining the design would help reviewers and future users to
> > understand.
> 
> Yes.

Can |sources| in case #1 be compiled into one of the targets listed in |deps|
instead? That way case #1 and #2 seem more consistent.

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
File mojo/public/tools/bindings/blink_bindings_configuration.gni (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
mojo/public/tools/bindings/blink_bindings_configuration.gni:9: _typemaps = [
On 2016/04/06 07:02:53, Sam McNally wrote:
> On 2016/04/05 17:36:14, yzshen1 wrote:
> > I thought previously you said users didn't have to add typemaps to a global
> list
> > explicitly?
> 
> Typemaps don't have to be added to this file explicitly; they could be listed
in
> some other gni file that this one reads. The alternative would be a Python
> script to scan for all the typemaps, but that would slow down gn gen; on Linux
a
> simple Python script to find all the .typemap files takes 3 seconds.

Yeah, scanning using py script seems sad.

By "they could be listed in some other gni file that this one reads", how would
the gni file look like? Could you point me to an example?

[Sam McNally, 2016-04-07 00:10:56]

Patchset #5 (id:500001) has been deleted

[Sam McNally, 2016-04-07 00:19:43]

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/struct_with_traits.typemap (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/struct_with_traits.typemap:11:
"//mojo/public/cpp/bindings/tests/struct_with_traits_impl_traits.cc",
On 2016/04/06 16:00:47, yzshen1 wrote:
> On 2016/04/06 07:02:53, Sam McNally wrote:
> > On 2016/04/05 17:36:14, yzshen1 wrote:
> > > Is "struct_with_triats_impl.cc" missing?
> > > Do we need to also specify deps that sources depend on?
> > > I noticed that gpu_command_buffer.typemap specifies deps but not sources.
I
> > > wonder why is the difference.
> > 
> > The typemap should have:
> > - deps for the native type
> > - public_headers containing the header of the native type
> > - traits_headers containing the header with the StructTraits for the native
> type
> > - sources containing the implementation of the StructTraits for the native
> type
> > - type_mappings
> > 
> > For native-only types, the typemap should have:
> > - deps for the native type and its ParamTraits
> > - public_headers containing the header of the native type
> > - traits_headers containing the header with the ParamTraits for the native
> type
> > - type_mappings
> > 
> > This was missing a dependency on a target containing StructWithTraitsImpl.
> > 
> > > 
> > > A document explaining the design would help reviewers and future users to
> > > understand.
> > 
> > Yes.
> 
> Can |sources| in case #1 be compiled into one of the targets listed in |deps|
> instead? That way case #1 and #2 seem more consistent.

No, the sources in #1 depend on the generated mojom headers.

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
File mojo/public/tools/bindings/blink_bindings_configuration.gni (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/tools/bind...
mojo/public/tools/bindings/blink_bindings_configuration.gni:9: _typemaps = [
On 2016/04/06 16:00:47, yzshen1 wrote:
> On 2016/04/06 07:02:53, Sam McNally wrote:
> > On 2016/04/05 17:36:14, yzshen1 wrote:
> > > I thought previously you said users didn't have to add typemaps to a
global
> > list
> > > explicitly?
> > 
> > Typemaps don't have to be added to this file explicitly; they could be
listed
> in
> > some other gni file that this one reads. The alternative would be a Python
> > script to scan for all the typemaps, but that would slow down gn gen; on
Linux
> a
> > simple Python script to find all the .typemap files takes 3 seconds.
> 
> Yeah, scanning using py script seems sad.
> 
> By "they could be listed in some other gni file that this one reads", how
would
> the gni file look like? Could you point me to an example?

I've extracted the typemap list into imported gni files.

[yzshen1, 2016-04-08 19:58:25]

The change mostly looks good. Thanks!

Have you verified the following behavior, for both GYP and GN?
- changing anything in a .typemap file triggers a rebuild of relevant targets
(but not all mojom targets of the entire codebase).
- adding and removing registered .typemap files triggers a rebuild of relevant
targets.
- changing anything in files pointed to by a .typemap file triggers a rebuild of
relevant targets.

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/struct_with_traits.typemap (right):

https://codereview.chromium.org/1832703002/diff/380001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/struct_with_traits.typemap:11:
"//mojo/public/cpp/bindings/tests/struct_with_traits_impl_traits.cc",
On 2016/04/07 00:19:43, Sam McNally wrote:
> On 2016/04/06 16:00:47, yzshen1 wrote:
> > On 2016/04/06 07:02:53, Sam McNally wrote:
> > > On 2016/04/05 17:36:14, yzshen1 wrote:
> > > > Is "struct_with_triats_impl.cc" missing?
> > > > Do we need to also specify deps that sources depend on?
> > > > I noticed that gpu_command_buffer.typemap specifies deps but not
sources.
> I
> > > > wonder why is the difference.
> > > 
> > > The typemap should have:
> > > - deps for the native type
> > > - public_headers containing the header of the native type
> > > - traits_headers containing the header with the StructTraits for the
native
> > type
> > > - sources containing the implementation of the StructTraits for the native
> > type
> > > - type_mappings
> > > 
> > > For native-only types, the typemap should have:
> > > - deps for the native type and its ParamTraits
> > > - public_headers containing the header of the native type
> > > - traits_headers containing the header with the ParamTraits for the native
> > type
> > > - type_mappings
> > > 
> > > This was missing a dependency on a target containing StructWithTraitsImpl.
> > > 
> > > > 
> > > > A document explaining the design would help reviewers and future users
to
> > > > understand.
> > > 
> > > Yes.
> > 
> > Can |sources| in case #1 be compiled into one of the targets listed in
|deps|
> > instead? That way case #1 and #2 seem more consistent.
> 
> No, the sources in #1 depend on the generated mojom headers.

Of course. :) Thanks for explaining.

https://codereview.chromium.org/1832703002/diff/520001/gpu/command_buffer/com...
File gpu/command_buffer/common/typemaps.gni (right):

https://codereview.chromium.org/1832703002/diff/520001/gpu/command_buffer/com...
gpu/command_buffer/common/typemaps.gni:6:
"//gpu/command_buffer/common/capabilities.typemap",
Question: is it allowed to use relative path?

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
File mojo/mojom_bindings_generator.gypi (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
mojo/mojom_bindings_generator.gypi:62: 'output': '<(mojom_typemap_file)',
nit: I feel that the name is pretty similar to mojom_typemaps. Does it make
sense to name it something like generated_typemap_file?

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
mojo/mojom_bindings_generator.gypi:72:
'<(DEPTH)/mojo/public/tools/bindings/generate_type_mappings.py',
Do we need to add those files listed in mojom_typemaps as input too?

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/blink_typemaps.gni (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/blink_typemaps.gni:6:
"//mojo/public/cpp/bindings/tests/test_native_types_blink.typemap",
nit: order them alphabetically, please.

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/struct_with_traits.typemap (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/struct_with_traits.typemap:9: [
"//mojo/public/cpp/bindings/tests/struct_with_traits_impl_traits.h" ]
(not the problem of this CL) the naming of this file and relative files is
poor... :)

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

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:59: sources = [
nit: according to GN style guide, sources should be above public_deps.
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
File mojo/public/tools/bindings/blink_bindings_configuration.gni (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
mojo/public/tools/bindings/blink_bindings_configuration.gni:5: variant = "wtf"
I noticed that existing real-world usage in blink/ use "wtf" variant. I am not
sure whether blink folks expect that all mojo interfaces in blink/ will be
enforced to have this variant.

(I personally feel that "blink" may make more sense. But the current interfaces
are defined in a namespace "blink.mojom" so they want to avoid
"blink::mojom::blink".)

It would be nice to inform them about this to avoid any future surprises. The CL
needs webkit OWNERs' LGs anyway. Or we could bring this up on next week's onion
soup sync up meeting.

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generate_type_mappings.py (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
mojo/public/tools/bindings/generate_type_mappings.py:7: import json
alphabetically, please.

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
mojo/public/tools/bindings/generate_type_mappings.py:9: 
please add a comment about what this file does, explaining the input format that
it expects and the output format.

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
mojo/public/tools/bindings/generate_type_mappings.py:43:
parser.add_argument('--dependency', type=str, action='append', default=[])
Please add help for args.

[Sam McNally, 2016-04-11 03:33:13]

Patchset #7 (id:560001) has been deleted

[Sam McNally, 2016-04-11 04:40:27]

On 2016/04/08 19:58:25, yzshen1 wrote:
> The change mostly looks good. Thanks!
> 
> Have you verified the following behavior, for both GYP and GN?
> - changing anything in a .typemap file triggers a rebuild of relevant targets
> (but not all mojom targets of the entire codebase).

Yes.

> - adding and removing registered .typemap files triggers a rebuild of relevant
> targets.

Yes.

> - changing anything in files pointed to by a .typemap file triggers a rebuild
of
> relevant targets.

Yes, but with GYP those files need to be duplicated in the mojom target sources.

https://codereview.chromium.org/1832703002/diff/520001/gpu/command_buffer/com...
File gpu/command_buffer/common/typemaps.gni (right):

https://codereview.chromium.org/1832703002/diff/520001/gpu/command_buffer/com...
gpu/command_buffer/common/typemaps.gni:6:
"//gpu/command_buffer/common/capabilities.typemap",
On 2016/04/08 19:58:25, yzshen1 wrote:
> Question: is it allowed to use relative path?

No. I can't find a way to distinguish between relative and absolute paths in GN
so they have to either all be relative or all be absolute.

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
File mojo/mojom_bindings_generator.gypi (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
mojo/mojom_bindings_generator.gypi:62: 'output': '<(mojom_typemap_file)',
On 2016/04/08 19:58:25, yzshen1 wrote:
> nit: I feel that the name is pretty similar to mojom_typemaps. Does it make
> sense to name it something like generated_typemap_file?

Done.

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
mojo/mojom_bindings_generator.gypi:72:
'<(DEPTH)/mojo/public/tools/bindings/generate_type_mappings.py',
On 2016/04/08 19:58:25, yzshen1 wrote:
> Do we need to add those files listed in mojom_typemaps as input too?

No, they're only used by GYP to generate the command line.

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/blink_typemaps.gni (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/blink_typemaps.gni:6:
"//mojo/public/cpp/bindings/tests/test_native_types_blink.typemap",
On 2016/04/08 19:58:25, yzshen1 wrote:
> nit: order them alphabetically, please.

Done.

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

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:59: sources = [
On 2016/04/08 19:58:25, yzshen1 wrote:
> nit: according to GN style guide, sources should be above public_deps.
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

Done.

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generate_type_mappings.py (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
mojo/public/tools/bindings/generate_type_mappings.py:7: import json
On 2016/04/08 19:58:25, yzshen1 wrote:
> alphabetically, please.

Done.

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
mojo/public/tools/bindings/generate_type_mappings.py:9: 
On 2016/04/08 19:58:25, yzshen1 wrote:
> please add a comment about what this file does, explaining the input format
that
> it expects and the output format.

Done.

https://codereview.chromium.org/1832703002/diff/520001/mojo/public/tools/bind...
mojo/public/tools/bindings/generate_type_mappings.py:43:
parser.add_argument('--dependency', type=str, action='append', default=[])
On 2016/04/08 19:58:25, yzshen1 wrote:
> Please add help for args.

Done.

[yzshen1, 2016-04-11 16:02:56]

LGTM with a couple of nits.

Thanks!

https://codereview.chromium.org/1832703002/diff/520001/gpu/command_buffer/com...
File gpu/command_buffer/common/typemaps.gni (right):

https://codereview.chromium.org/1832703002/diff/520001/gpu/command_buffer/com...
gpu/command_buffer/common/typemaps.gni:6:
"//gpu/command_buffer/common/capabilities.typemap",
On 2016/04/11 04:40:27, Sam McNally wrote:
> On 2016/04/08 19:58:25, yzshen1 wrote:
> > Question: is it allowed to use relative path?
> 
> No. I can't find a way to distinguish between relative and absolute paths in
GN
> so they have to either all be relative or all be absolute.

Okay. Please add notes for this kind of things in the user tutorial (when it
exists).

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
File mojo/mojom_bindings_generator.gypi (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
mojo/mojom_bindings_generator.gypi:72:
'<(DEPTH)/mojo/public/tools/bindings/generate_type_mappings.py',
On 2016/04/11 04:40:27, Sam McNally wrote:
> On 2016/04/08 19:58:25, yzshen1 wrote:
> > Do we need to add those files listed in mojom_typemaps as input too?
> 
> No, they're only used by GYP to generate the command line.

(Sorry if I misunderstand how/when an action is triggered.) If "mojom_typemaps"
are not listed as "inputs", when those files' contents are changed, will GYP
know that this target needs to be re-compiled?

[Sam McNally, 2016-04-12 07:54:31]

sammc@chromium.org changed reviewers:
+ brettw@chromium.org, dcheng@chromium.org, esprehn@chromium.org,
haraken@chromium.org, jam@chromium.org

[Sam McNally, 2016-04-12 07:54:32]

+esprehn and haraken for blink changes

+dcheng for gpu/ipc/common/BUILD.gn

+brettw for GN use in mojo/public/tools/bindings/mojom.gni

+jam for everything else (mus, content, gpu and url)

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
File mojo/mojom_bindings_generator.gypi (right):

https://codereview.chromium.org/1832703002/diff/520001/mojo/mojom_bindings_ge...
mojo/mojom_bindings_generator.gypi:72:
'<(DEPTH)/mojo/public/tools/bindings/generate_type_mappings.py',
On 2016/04/11 16:02:55, yzshen1 wrote:
> On 2016/04/11 04:40:27, Sam McNally wrote:
> > On 2016/04/08 19:58:25, yzshen1 wrote:
> > > Do we need to add those files listed in mojom_typemaps as input too?
> > 
> > No, they're only used by GYP to generate the command line.
> 
> (Sorry if I misunderstand how/when an action is triggered.) If
"mojom_typemaps"
> are not listed as "inputs", when those files' contents are changed, will GYP
> know that this target needs to be re-compiled?

Those files are only used when GYP runs as arguments to
format_typemap_generator_args.py. If they change, build/gyp_chromium needs to be
rerun to pick up the changes.

[Ken Rockot(use gerrit already), 2016-04-12 15:16:25]

lgtm

[jam, 2016-04-12 15:28:23]

lgtm

[dcheng, 2016-04-13 00:51:40]

lgtm

[ortuno, 2016-04-13 15:35:44]

https://codereview.chromium.org/1832703002/diff/580001/third_party/WebKit/pub...
File third_party/WebKit/public/BUILD.gn (left):

https://codereview.chromium.org/1832703002/diff/580001/third_party/WebKit/pub...
third_party/WebKit/public/BUILD.gn:177: # GYP version:
WebKit/public/blink.gyp:mojo_bindings_blink_mojom
Do we still the target for the variant in blink.gyp?

[Sam McNally, 2016-04-14 00:08:20]

Patchset #9 (id:620001) has been deleted

[Sam McNally, 2016-04-14 03:27:43]

https://codereview.chromium.org/1832703002/diff/580001/third_party/WebKit/pub...
File third_party/WebKit/public/BUILD.gn (left):

https://codereview.chromium.org/1832703002/diff/580001/third_party/WebKit/pub...
third_party/WebKit/public/BUILD.gn:177: # GYP version:
WebKit/public/blink.gyp:mojo_bindings_blink_mojom
On 2016/04/13 15:35:44, ortuno wrote:
> Do we still the target for the variant in blink.gyp?

This CL doesn't affect how the gyp mojom targets work with variants. I've
updated the GYP <-> GN comments to reflect the GN changes.

[Sam McNally, 2016-04-19 06:54:20]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-04-19 06:54:21]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, dcheng@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1832703002/#ps700001
(title: "add missing mash dependency")

[commit-bot: I haz the power, 2016-04-19 06:54:38]

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

[commit-bot: I haz the power, 2016-04-19 06:56:20]

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

[commit-bot: I haz the power, 2016-04-19 06:56:21]

Try jobs failed on following builders:
  win_optional_gpu_tests_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[Sam McNally, 2016-04-19 07:01:27]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2016-04-19 07:01:42]

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

[commit-bot: I haz the power, 2016-04-19 08:07:47]

Description was changed from

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this CL,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

to

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this CL,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

[commit-bot: I haz the power, 2016-04-19 08:07:49]

Committed patchset #12 (id:700001)

[commit-bot: I haz the power, 2016-04-19 08:09:30]

Description was changed from

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this CL,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202
==========

to

==========
Mojo: Simplify typemap usage.

Previously, to use native types in mojo interfaces, the correct typemaps
needed to be set on each target depending on the mojom target containing
that interface. Additionally, each variant required a duplicate set of
mojom targets for all the mojom files required by that variant.

This CL changes the mojom GN template to generate a mojom target for
each variant, applying the appropriate typemaps when generating the
bindings. This is controlled by mojo bindings configurations that
aggregate all of the typemaps for that configuration and specify the
variant for that configuration. To accommodate this change, this also
changes the typemap format to be one GN-format file per mojom file,
setting variables that specify the typemap. At build time, the typemap
information and any dependencies are used to generate a JSON typemap of
the old JSON format containing all typemaps for the bindings being
generated as well as their dependencies.

This makes minor changes to typemap support in GYP. With this CL,
typemaps can be specified as GYP variables instead of directly
specifying them in extra generator command line arguments.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
BUG=596202

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

[commit-bot: I haz the power, 2016-04-19 08:09:32]

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

[yzshen1, 2016-04-19 15:43:21]

On 2016/04/19 08:09:32, commit-bot: I haz the power wrote:
> Patchset 12 (id:??) landed as
> https://crrev.com/b54ee6567f200280a284fc466706962fa810e398
> Cr-Commit-Position: refs/heads/master@{#388164}

Hi, Sam.

According to the mail thread
https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-...
it seems people agreed on using "blink" as the variant instead of "wtf".

Are you planning to update that? Thanks!

[Sam McNally, 2016-04-20 00:04:58]

On 2016/04/19 15:43:21, yzshen1 wrote:
> On 2016/04/19 08:09:32, commit-bot: I haz the power wrote:
> > Patchset 12 (id:??) landed as
> > https://crrev.com/b54ee6567f200280a284fc466706962fa810e398
> > Cr-Commit-Position: refs/heads/master@{#388164}
> 
> Hi, Sam.
> 
> According to the mail thread
>
https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-...
> it seems people agreed on using "blink" as the variant instead of "wtf".
> 
> Are you planning to update that? Thanks!

Yes. This CL was starting to feel a bit big so it seemed easier to update that
in a follow-up CL: https://codereview.chromium.org/1902263004/.