Generate Mojom Types in Dart (Take 2)

Generate Mojom Types in Dart (Take 2)

This CL modifies the Dart generator to produce mojom type information.

This includes a test to ensure that the output is expected.

The previous CL (located at https://codereview.chromium.org/1433183002/)
has been replaced with this one.


In order for this to compile, hacks were added to the Dart embedder
package builder. This is acceptable because those files are supposed
to be removed in the future.

All .mojom.dart files were regenerated in this CL.


BUG=https://github.com/domokit/mojo/issues/596
R=azani@chromium.org, zra@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/a49d2b24078041ffa78e7e8d9c72ffed213f7881

[alexfandrianto, 2015-12-18 01:42:54]

Description was changed from

==========
Generate Mojom Types in Dart (Take 2)

This CL modifies the Dart generator to produce mojom type information.

This includes a test to ensure that the output is expected.

The previous CL (located at https://codereview.chromium.org/1433183002/)
has been replaced with this one.


In order for this to compile, hacks were added to the Dart embedder
package builder. This is acceptable because those files are supposed
to be removed in the future.

All .mojom.dart files were regenerated in this CL.


BUG=https://github.com/domokit/mojo/issues/596
==========

to

==========
Generate Mojom Types in Dart (Take 2)

This CL modifies the Dart generator to produce mojom type information.

This includes a test to ensure that the output is expected.

The previous CL (located at https://codereview.chromium.org/1433183002/)
has been replaced with this one.


In order for this to compile, hacks were added to the Dart embedder
package builder. This is acceptable because those files are supposed
to be removed in the future.

All .mojom.dart files were regenerated in this CL.


BUG=https://github.com/domokit/mojo/issues/596
==========

[alexfandrianto, 2015-12-18 01:42:54]

alexfandrianto@google.com changed reviewers:
+ azani@chromium.org, johnmccutchan@google.com, rudominer@chromium.org,
zra@google.com

[alexfandrianto, 2015-12-18 02:10:38]

On 2015/12/18 01:42:54, alexfandrianto wrote:
> mailto:alexfandrianto@google.com changed reviewers:
> + mailto:azani@chromium.org, mailto:johnmccutchan@google.com,
mailto:rudominer@chromium.org,
> mailto:zra@google.com

The bots are failing to patch for some reason... :(

[alexfandrianto, 2015-12-18 23:14:54]

On 2015/12/18 02:10:38, alexfandrianto wrote:
> On 2015/12/18 01:42:54, alexfandrianto wrote:
> > mailto:alexfandrianto@google.com changed reviewers:
> > + mailto:azani@chromium.org, mailto:johnmccutchan@google.com,
> mailto:rudominer@chromium.org,
> > mailto:zra@google.com
> 
> The bots are failing to patch for some reason... :(

Since there were changes in the Go CL, I'll wait until that one is in before
updating this.

[azani, 2016-01-12 19:07:22]

Hey Alex,

Are you waiting for us on this or are you still working on it?

[alexfandrianto, 2016-01-12 19:24:58]

On 2016/01/12 19:07:22, azani wrote:
> Hey Alex,
> 
> Are you waiting for us on this or are you still working on it?

Still working on it. I want to test something locally first. If that works, I'll
upload a new patchset and ask you to review. Thanks for checking in!

[alexfandrianto, 2016-01-15 18:16:29]

On 2016/01/12 19:24:58, alexfandrianto wrote:
> On 2016/01/12 19:07:22, azani wrote:
> > Hey Alex,
> > 
> > Are you waiting for us on this or are you still working on it?
> 
> Still working on it. I want to test something locally first. If that works,
I'll
> upload a new patchset and ask you to review. Thanks for checking in!

Phew. I think that counts as passing the tests, so PTAL.

I will post numbers regarding the deltas in file size (with mojom type
generation and without) later today.

[zra, 2016-01-15 21:01:34]

Sorry if you've already answered some of these questions for me before. I
haven't looked at this since last year =)

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/embe...
File mojo/dart/embedder/embedder.gni (right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/embe...
mojo/dart/embedder/embedder.gni:17: is_bindings_types = ""
Make this a boolean flag passed by the invoker, e.g.:

dart_embedder_package(...) {
  ...
  is_bindings_types = true
}

Then, test on that to pass or not pass a real command line flag to
dart_list_generated_bindings.py, e.g '--bindings-types'

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/tool...
File mojo/dart/embedder/tools/dart_embedder_url_mappings.py (right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/tool...
mojo/dart/embedder/tools/dart_embedder_url_mappings.py:30: if
f.endswith('service_describer.mojom') or \
Shouldn't 'bindings/types' already be in dirname somewhere? I'd think we can use
that somehow to calculate the right path without needing this hack.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/tool...
File mojo/dart/embedder/tools/dart_list_generated_bindings.py (right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/tool...
mojo/dart/embedder/tools/dart_list_generated_bindings.py:42: # A hacky solution
that splits the interface_control_messages.mojom file
What would be a non-hacky solution?

It sounds like the module annotation in interface_control_messages.mojom might
be wrong. Maybe that should be fixed first in a separate CL.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:234:
}
Missing whitespace.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:235:
mojom_types.MojomStruct _dart_to_cpp_EchoArgs__() {
Even though they're library-private, it would be nice to have these symbols be
Dart-styled, e.g.: '_dartToCppEchoArgsStructType' or something like that.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:414:
..identifier = '_dart_to_cpp_EchoArgsList__'
Is this string part of some publicly visible, language agnostic API? If so, then
I guess this is okay, but if this string is Dart-specific, then the name should
probably be Dart-styled, e.g. 'dartToCppEchoArgsList', or at least
'dart_to_cpp_EchoArgsList'

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:491:
..shortName = '_CppSideStartTestParams'
In the generated Dart bindings, this symbol is private. Should it still be
exposed to introspection tools?

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:705:
)),
Indentation problem.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:966:
..ordinal = _CppSide_startTestName..parameters =
_dart_to_cpp_CppSide_StartTest_Params__(),_CppSide_testFinishedName: new
mojom_types.MojomMethod()
This doesn't look right. 

Missing newline here: _CppSide_startTestName..parameters = ... ?

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1197:

Extra newline.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1225:
dynamic getTopLevelInterface([Function responseFactory = null]) {
Indentation.

These functions can also be written with '=>' instead of '{ return ..; }'

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1227:
}
Missing newline.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1228:
dynamic getTypeDefinition(String typeKey,[Function responseFactory = null]) {
Missing space after ,

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1440:

Lots of extra newlines.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1447:
map["_dart_to_cpp_EchoArgs__"] =
Indentation.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1450:
map["_dart_to_cpp_EchoArgsList__"] =
Can these keys be Dart-styled?

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1484:
var _MojomDesc;
Map<String, mojom_types.UserDefinedType> _mojomDesc;

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:34:
{% set mapping = '_MojomDesc' %}
_mojomDesc

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:29:
..declData = (new {{typepkg}}DeclarationData()..shortName =
'{{method|name|upper_camel}}')
Usually we don't have much control over line length in the generated code, but
if we can avoid long lines, like here, I think we should try to do that.

Why does this need to be UpperCamelCase? If you want the Dart-styled name, it
should be just method|name. If you want the name from the .mojom, it should be
method.name.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:40:
{{ raise("Bad type name given") }}
Maybe append the name that was bad here.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:68:
{{ raise("Unsupported type") }}
Maybe append the type here.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:101:
..declData = (new {{typepkg}}DeclarationData()..shortName =
'{{field|name|upper_camel}}')
ditto re upper_camel.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:144:
..declData = (new {{typepkg}}DeclarationData()..shortName =
'{{field|name|upper_camel}}')
ditto

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:161:
..declData = (new {{typepkg}}DeclarationData()..shortName =
'{{field|name|upper_camel}}')
ditto

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:264: def
GetMojomTypeValue(kind, typepkg=''):
I think it would be more consistent to move this logic to a template macro.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:508: def
GetIdentifier(kind):
This name is too generic for what (I think) this function does. The name
generation functions that you're adding should be made more distinct from e.g.
GetNameForElement. There should be no confusion about which one should be used
when the next person comes along to add a feature.

Also, unless the names generated here have some significance outside of Dart,
they should probably use GetNameForElement to make names with Dart styling.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:534: return
"_%s__" % GetIdentifier(kind)
Why are there trailing underscores?

[alexfandrianto, 2016-01-15 23:05:57]

Thanks, zra. I'll take a look. (Writing your comments in one of the *.mojom.dart
files was very helpful.)

I'm using UpperCamelCase in the DeclarationData because it is metadata needed
for the v23proxy (Vanadium proxy between Mojo and Vanadium servers). Dart had to
do this for compatiblity with Go (which uses UpperCamelCase). I did attempt to
keep variable names lowerCamelCase.


Very briefly, for Dart (where the files are checked in), I did `find mojo -name
"*.mojom.dart" | xargs wc -l` in my Mojo checkout.

I found that the *.mojom.dart files have grown from 109065 lines (master) to
139043 lines (this branch). This is a 27.5% increase in number of lines.

When I did some of this manually, I ended up with about the same number (+26.8%
in the _mojo_for_test_only package).

[azani, 2016-01-15 23:21:57]

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:705:
)),
On 2016/01/15 21:01:33, zra wrote:
> Indentation problem.

To clarify, this mojom struct is automatically generated to reflect the response
parameters of a method. (In other words, the response parameters are implemented
as a struct) The MojomMethod should provide enough information for clients.

[zra, 2016-01-15 23:33:17]

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:29:
..declData = (new {{typepkg}}DeclarationData()..shortName =
'{{method|name|upper_camel}}')
So, method|name can do arbitrary things to make method's name be Dart-styled,
and it might also need to do name-mangling to fix future name-collision bugs. If
you need to generate a string here to be compatible with Go, then that probably
shouldn't depend on method|name.

[alexfandrianto, 2016-01-15 23:49:00]

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:29:
..declData = (new {{typepkg}}DeclarationData()..shortName =
'{{method|name|upper_camel}}')
On 2016/01/15 23:33:17, zra wrote:
> So, method|name can do arbitrary things to make method's name be Dart-styled,
> and it might also need to do name-mangling to fix future name-collision bugs.
If
> you need to generate a string here to be compatible with Go, then that
probably
> shouldn't depend on method|name.

While Mojo does do method matching based on index, Vanadium uses string names to
identify methods, so we understand that there can be problems if the way method
names are generated changes. If name-mangling is needed, I would expect both the
Go and Dart mojom generators to do it similarly, so I'm not too concerned yet.
It should be okay to leave this aside since the only thing that can break from
is the Vanadium-Mojo proxy.

[zra, 2016-01-19 18:09:47]

On 2016/01/15 23:49:00, alexfandrianto wrote:
> If name-mangling is needed, I would expect both the
> Go and Dart mojom generators to do it similarly.

Unfortunately, we can't rely on this. For example, Dart and Go have different
keywords, and would therefore have a need to mangle around different sets of
identifiers.

Anyway, all I'm saying is that you can't use |name for something that has
visibility beyond Dart code. Someone working on the Dart bindings should be able
to change |name without worrying about breaking Vanadium.

[alexfandrianto, 2016-01-20 00:08:29]

Responded to comments (excluding how to handle method|name). Will need to think
about that one more.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/embe...
File mojo/dart/embedder/embedder.gni (right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/embe...
mojo/dart/embedder/embedder.gni:17: is_bindings_types = ""
On 2016/01/15 21:01:33, zra wrote:
> Make this a boolean flag passed by the invoker, e.g.:
> 
> dart_embedder_package(...) {
>   ...
>   is_bindings_types = true
> }
> 
> Then, test on that to pass or not pass a real command line flag to
> dart_list_generated_bindings.py, e.g '--bindings-types'

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/tool...
File mojo/dart/embedder/tools/dart_embedder_url_mappings.py (right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/tool...
mojo/dart/embedder/tools/dart_embedder_url_mappings.py:30: if
f.endswith('service_describer.mojom') or \
On 2016/01/15 21:01:33, zra wrote:
> Shouldn't 'bindings/types' already be in dirname somewhere? I'd think we can
use
> that somehow to calculate the right path without needing this hack.

Actually, it isn't. These 3 files are placed in
//mojo/public/interfaces/bindings along with interface_control_messages.mojom
(which doesn't belong in the same module as them).

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/tool...
File mojo/dart/embedder/tools/dart_list_generated_bindings.py (right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/embedder/tool...
mojo/dart/embedder/tools/dart_list_generated_bindings.py:42: # A hacky solution
that splits the interface_control_messages.mojom file
On 2016/01/15 21:01:34, zra wrote:
> What would be a non-hacky solution?
> 
> It sounds like the module annotation in interface_control_messages.mojom might
> be wrong. Maybe that should be fixed first in a separate CL.

The weird thing about the files in //mojo/public/interfaces/bindings is that:
* interface_control_messages.mojom has the module mojo
* mojom_files, mojom_types, and service_describer have the module
mojo.bindings.types;

I may be the first to run into a "weird" folder of mojoms like this, so this
hasn't been exercised until now.

I think a better solution would read the file and parse the module line to
determine where the generated files ought to go. As-is, all generated files go
to the same folder for the same build rule, but the .mojom.dart don't always
agree with that because of the module value.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:234:
}
On 2016/01/15 21:01:33, zra wrote:
> Missing whitespace.

It's not easy to make this good, so while I will try to improve this, I would
suggest using dartfmt instead.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:235:
mojom_types.MojomStruct _dart_to_cpp_EchoArgs__() {
On 2016/01/15 21:01:33, zra wrote:
> Even though they're library-private, it would be nice to have these symbols be
> Dart-styled, e.g.: '_dartToCppEchoArgsStructType' or something like that.

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:414:
..identifier = '_dart_to_cpp_EchoArgsList__'
On 2016/01/15 21:01:33, zra wrote:
> Is this string part of some publicly visible, language agnostic API? If so,
then
> I guess this is okay, but if this string is Dart-specific, then the name
should
> probably be Dart-styled, e.g. 'dartToCppEchoArgsList', or at least
> 'dart_to_cpp_EchoArgsList'

Yes, it is public to all.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:491:
..shortName = '_CppSideStartTestParams'
On 2016/01/15 21:01:33, zra wrote:
> In the generated Dart bindings, this symbol is private. Should it still be
> exposed to introspection tools?

Yes, in discussions, we decided that we would only hide this if mojom type
generation is turned off and could later add more flags to modify generation as
needed.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:705:
)),
On 2016/01/15 21:01:33, zra wrote:
> Indentation problem.

It's hard to get nested macros right, but it looks better with indent(6) and
indent(8). I still recommend dartfmt as a long-term solution though.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:966:
..ordinal = _CppSide_startTestName..parameters =
_dart_to_cpp_CppSide_StartTest_Params__(),_CppSide_testFinishedName: new
mojom_types.MojomMethod()
On 2016/01/15 21:01:33, zra wrote:
> This doesn't look right. 
> 
> Missing newline here: _CppSide_startTestName..parameters = ... ?

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1197:

On 2016/01/15 21:01:33, zra wrote:
> Extra newline.

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1225:
dynamic getTopLevelInterface([Function responseFactory = null]) {
On 2016/01/15 21:01:33, zra wrote:
> Indentation.
> 
> These functions can also be written with '=>' instead of '{ return ..; }'

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1227:
}
On 2016/01/15 21:01:33, zra wrote:
> Missing newline.

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1228:
dynamic getTypeDefinition(String typeKey,[Function responseFactory = null]) {
On 2016/01/15 21:01:33, zra wrote:
> Missing space after ,

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:34:
{% set mapping = '_MojomDesc' %}
On 2016/01/15 21:01:33, zra wrote:
> _mojomDesc

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:29:
..declData = (new {{typepkg}}DeclarationData()..shortName =
'{{method|name|upper_camel}}')
On 2016/01/15 21:01:34, zra wrote:
> Usually we don't have much control over line length in the generated code, but
> if we can avoid long lines, like here, I think we should try to do that.
> 
> Why does this need to be UpperCamelCase? If you want the Dart-styled name, it
> should be just method|name. If you want the name from the .mojom, it should be
> method.name.

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:40:
{{ raise("Bad type name given") }}
On 2016/01/15 21:01:34, zra wrote:
> Maybe append the name that was bad here.

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:68:
{{ raise("Unsupported type") }}
On 2016/01/15 21:01:34, zra wrote:
> Maybe append the type here.

Done.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:264: def
GetMojomTypeValue(kind, typepkg=''):
On 2016/01/15 21:01:33, zra wrote:
> I think it would be more consistent to move this logic to a template macro.

Pretty much all the languages use a map for the simple/handle/string kinds, so
the inconsistency must be the string concatenations.

It really takes a lot longer code-wise if I remove the map and need to add more
filters, so I've moved as much of it over as I can without making it overly
painful.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:508: def
GetIdentifier(kind):
On 2016/01/15 21:01:33, zra wrote:
> This name is too generic for what (I think) this function does. The name
> generation functions that you're adding should be made more distinct from e.g.
> GetNameForElement. There should be no confusion about which one should be used
> when the next person comes along to add a feature.
> 
> Also, unless the names generated here have some significance outside of Dart,
> they should probably use GetNameForElement to make names with Dart styling.

This was combined with GetMojomTypeIdentifier below. This is used to uniquely
identify mojom types across languages.

https://codereview.chromium.org/1539673003/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:534: return
"_%s__" % GetIdentifier(kind)
On 2016/01/15 21:01:33, zra wrote:
> Why are there trailing underscores?

This has to match between languages, and the Go one already has trailing
underscores.

[alexfandrianto, 2016-01-25 20:17:59]

PTAL.

In addition to my comments from before, I've added name_unsafe as a filter [used
by ShortNames], which uses GetNameForElementUnsafe (the version that doesn't
check for protected keywords).

Further, since a recent change (https://codereview.chromium.org/1586123010) has
limited Mojom type information generated for test files, I have done the same
for the _mojo_for_test_only package.

[azani, 2016-01-26 00:29:23]

https://codereview.chromium.org/1539673003/diff/60001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1539673003/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:490: # Returns a
string of the form package.path.TypeName - the full identifier
Can you make that a docstring?

https://codereview.chromium.org/1539673003/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:540:
'name_unsafe': GetNameForElementUnsafe,
Could you name this something that expresses the fact that this is related to
dynamic types generation? Also, maybe fact that into a separate module so it can
be used by other languages too.

[alexfandrianto, 2016-01-26 02:17:52]

On 2016/01/26 00:29:23, azani wrote:
>
https://codereview.chromium.org/1539673003/diff/60001/mojo/public/tools/bindi...
> File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):
> 
>
https://codereview.chromium.org/1539673003/diff/60001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/generators/mojom_dart_generator.py:490: # Returns a
> string of the form package.path.TypeName - the full identifier
> Can you make that a docstring?
> 
>
https://codereview.chromium.org/1539673003/diff/60001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/generators/mojom_dart_generator.py:540:
> 'name_unsafe': GetNameForElementUnsafe,
> Could you name this something that expresses the fact that this is related to
> dynamic types generation? Also, maybe fact that into a separate module so it
can
> be used by other languages too.

I've done both in the new patchset. The Mojom Type functions were moved into
module.py

[alexfandrianto, 2016-01-26 18:46:30]

On 2016/01/26 02:17:52, alexfandrianto wrote:
> On 2016/01/26 00:29:23, azani wrote:
> >
>
https://codereview.chromium.org/1539673003/diff/60001/mojo/public/tools/bindi...
> > File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):
> > 
> >
>
https://codereview.chromium.org/1539673003/diff/60001/mojo/public/tools/bindi...
> > mojo/public/tools/bindings/generators/mojom_dart_generator.py:490: # Returns
a
> > string of the form package.path.TypeName - the full identifier
> > Can you make that a docstring?
> > 
> >
>
https://codereview.chromium.org/1539673003/diff/60001/mojo/public/tools/bindi...
> > mojo/public/tools/bindings/generators/mojom_dart_generator.py:540:
> > 'name_unsafe': GetNameForElementUnsafe,
> > Could you name this something that expresses the fact that this is related
to
> > dynamic types generation? Also, maybe fact that into a separate module so it
> can
> > be used by other languages too.
> 
> I've done both in the new patchset. The Mojom Type functions were moved into
> module.py

(The android test failure seems unrelated, unless I'm missing something.) PTAL

[zra, 2016-01-27 18:15:12]

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/40001/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:234:
}
On 2016/01/20 00:08:28, alexfandrianto wrote:
> On 2016/01/15 21:01:33, zra wrote:
> > Missing whitespace.
> 
> It's not easy to make this good, so while I will try to improve this, I would
> suggest using dartfmt instead.

We may need to compare source files directly to each other to decide that they
are the same. Using dartfmt---considering that Mojo SDK consumers may be using
different Dart SDK versions---would confound our attempts to do that. So, we
can't rely on dartfmt to fix up these files.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/embedder/tool...
File mojo/dart/embedder/tools/dart_list_generated_bindings.py (right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/embedder/tool...
mojo/dart/embedder/tools/dart_list_generated_bindings.py:43: # A hacky solution
that splits the interface_control_messages.mojom file
@johnmccutchan Please review the changes to this script and
//mojo/dart/embedder/tools/dart_embedder_url_mappings.py. Please work with
@alexfandrianto to find a non-hacky solution, or otherwise add a TODO here with
a plan for removing the hacks.

@alexfandrianto Please get approval from @johnmccutchan before submitting
changes to this and the other script.

Thanks!

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:322:
..type = (new mojom_types.Type()..arrayType = (new mojom_types.ArrayType()
Other fields of type seem to be initialized on their own lines. I couldn't find
where the newline here is missing, but it would be great to do the same thing
here.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:433:

Extra newlines.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1016:
dynamic getTopLevelInterface([Function responseFactory = null]) =>
indentation

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1483:

Extra newlines.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import.mojom.dart:106:
}
Missing newline.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import2.mojom.dart
(left):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import2.mojom.dart:12:

Please leave this newline.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import2.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import2.mojom.dart:337:
map["sample_import2_Color__"] =
indentation

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/proxy.dart (right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/proxy.dart:42: /// provide type information.
Methods called may return null or an error.
It's unclear if you mean the `description` getter could return an error, or if a
method it describes could return an error.

I'd recommend cutting this comment down a bit. I think all that's needed is the
first sentence, and a warning that it might be null.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/stub.dart (right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/stub.dart:136: /// Returns a service
description, which exposes the mojom type information
Ditto.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
File mojo/dart/packages/mojom/lib/src/generate.dart (right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
mojo/dart/packages/mojom/lib/src/generate.dart:166: final generateTypeInfoFlag =
packageName == mojoTestPackage ?
Please add a TODO and file a bug to remove this hack. Whether to generate type
info should be a flag to the command.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/enum_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/enum_definition.tmpl:2: 
I wonder if newlines like this one are contributing to the extra newlines in the
result.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/enum_definition.tmpl:58: 
Ditto.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/enum_definition.tmpl:60: 
Ditto.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:15:
dynamic getTopLevelInterface([Function responseFactory = null]) =>
Here and below, `= null` isn't necessary. If no default is specified, the
default is null.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:16:
_{{interface|mojom_type_identifier|lower_camel}}();
4 space indent on a continued line.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:18:
dynamic getTypeDefinition(String typeKey, [Function responseFactory = null]) =>
Long line.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:26:
dynamic getTypeDefinition(String typeKey, [Function responseFactory = null]) =>
Long line.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:29: 
Newline problems might be coming from here and below.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:53:
{{import.unique_name}}.getAllMojomTypeDefinitions().forEach((String s,
{{typepkg}}UserDefinedType udt) {
We could probably do something about this long line.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/struct_definition.tmpl:107:

Extra newline?

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl:115: 
I wonder if extra newlines like this one might be contributing to all the extra
newlines in the generated code.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:264: raise
Exception('Missing case for kind: %s' % kind)
Won't _kind_to_mojom_type[kind] already throw an exception if kind is missing a
case?

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:481: def
GetPackageName(module):
This is only called in one place. I'd just inline it there.

[alexfandrianto, 2016-01-28 03:45:13]

The templates are now producing a lot better-formatted code.

Still need to talk to Cutch about the embedder changes.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:322:
..type = (new mojom_types.Type()..arrayType = (new mojom_types.ArrayType()
On 2016/01/27 18:15:11, zra wrote:
> Other fields of type seem to be initialized on their own lines. I couldn't
find
> where the newline here is missing, but it would be great to do the same thing
> here.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import.mojom.dart:106:
}
On 2016/01/27 18:15:11, zra wrote:
> Missing newline.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import2.mojom.dart
(left):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import2.mojom.dart:12:

On 2016/01/27 18:15:11, zra wrote:
> Please leave this newline.

I think I figured that one out. It was enums who dropped this line.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import2.mojom.dart
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/imported/sample_import2.mojom.dart:337:
map["sample_import2_Color__"] =
On 2016/01/27 18:15:11, zra wrote:
> indentation

I think it looks better now.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/proxy.dart (right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/proxy.dart:42: /// provide type information.
Methods called may return null or an error.
On 2016/01/27 18:15:12, zra wrote:
> It's unclear if you mean the `description` getter could return an error, or if
a
> method it describes could return an error.
> 
> I'd recommend cutting this comment down a bit. I think all that's needed is
the
> first sentence, and a warning that it might be null.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/stub.dart (right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/stub.dart:136: /// Returns a service
description, which exposes the mojom type information
On 2016/01/27 18:15:12, zra wrote:
> Ditto.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
File mojo/dart/packages/mojom/lib/src/generate.dart (right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/dart/packages/mojo...
mojo/dart/packages/mojom/lib/src/generate.dart:166: final generateTypeInfoFlag =
packageName == mojoTestPackage ?
On 2016/01/27 18:15:12, zra wrote:
> Please add a TODO and file a bug to remove this hack. Whether to generate type
> info should be a flag to the command.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/enum_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/enum_definition.tmpl:2: 
On 2016/01/27 18:15:12, zra wrote:
> I wonder if newlines like this one are contributing to the extra newlines in
the
> result.

This one's probably okay since it's not part of the macro definition.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/enum_definition.tmpl:58: 
On 2016/01/27 18:15:12, zra wrote:
> Ditto.

This was supposed to be intentional, but it is affecting the *.mojom.dart files
that don't include type info.
Modified.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/enum_definition.tmpl:60: 
On 2016/01/27 18:15:12, zra wrote:
> Ditto.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:15:
dynamic getTopLevelInterface([Function responseFactory = null]) =>
On 2016/01/27 18:15:12, zra wrote:
> Here and below, `= null` isn't necessary. If no default is specified, the
> default is null.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:16:
_{{interface|mojom_type_identifier|lower_camel}}();
On 2016/01/27 18:15:12, zra wrote:
> 4 space indent on a continued line.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:18:
dynamic getTypeDefinition(String typeKey, [Function responseFactory = null]) =>
On 2016/01/27 18:15:12, zra wrote:
> Long line.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:26:
dynamic getTypeDefinition(String typeKey, [Function responseFactory = null]) =>
On 2016/01/27 18:15:12, zra wrote:
> Long line.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:29: 
On 2016/01/27 18:15:12, zra wrote:
> Newline problems might be coming from here and below.

Newline removed

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:53:
{{import.unique_name}}.getAllMojomTypeDefinitions().forEach((String s,
{{typepkg}}UserDefinedType udt) {
On 2016/01/27 18:15:12, zra wrote:
> We could probably do something about this long line.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/struct_definition.tmpl:107:

On 2016/01/27 18:15:12, zra wrote:
> Extra newline?

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl
(right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl:115: 
On 2016/01/27 18:15:12, zra wrote:
> I wonder if extra newlines like this one might be contributing to all the
extra
> newlines in the generated code.

Done.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:264: raise
Exception('Missing case for kind: %s' % kind)
On 2016/01/27 18:15:12, zra wrote:
> Won't _kind_to_mojom_type[kind] already throw an exception if kind is missing
a
> case?

Yes, it returns a KeyError. That should be enough to produce a stack trace, so
I've removed these lines.

https://codereview.chromium.org/1539673003/diff/70144/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:481: def
GetPackageName(module):
On 2016/01/27 18:15:12, zra wrote:
> This is only called in one place. I'd just inline it there.

Inlined.

[zra, 2016-01-28 19:19:21]

This looks much better, thanks!

lgtm but please wait for comments from other reviewers and the trybots to turn
green.

[alexfandrianto, 2016-01-28 23:43:45]

On 2016/01/28 19:19:21, zra wrote:
> This looks much better, thanks!
> 
> lgtm but please wait for comments from other reviewers and the trybots to turn
> green.

Spoke with Cutch offline. azani, are you okay with this? I'm ready to submit. :)

[azani, 2016-01-29 18:35:42]

lgtm

[alexfandrianto, 2016-01-29 20:59:09]

Description was changed from

==========
Generate Mojom Types in Dart (Take 2)

This CL modifies the Dart generator to produce mojom type information.

This includes a test to ensure that the output is expected.

The previous CL (located at https://codereview.chromium.org/1433183002/)
has been replaced with this one.


In order for this to compile, hacks were added to the Dart embedder
package builder. This is acceptable because those files are supposed
to be removed in the future.

All .mojom.dart files were regenerated in this CL.


BUG=https://github.com/domokit/mojo/issues/596
==========

to

==========
Generate Mojom Types in Dart (Take 2)

This CL modifies the Dart generator to produce mojom type information.

This includes a test to ensure that the output is expected.

The previous CL (located at https://codereview.chromium.org/1433183002/)
has been replaced with this one.


In order for this to compile, hacks were added to the Dart embedder
package builder. This is acceptable because those files are supposed
to be removed in the future.

All .mojom.dart files were regenerated in this CL.


BUG=https://github.com/domokit/mojo/issues/596
R=azani@chromium.org, zra@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/a49d2b24078041ffa78e7e8d9c7...
==========

[alexfandrianto, 2016-01-29 20:59:11]

Committed patchset #13 (id:170001) manually as
a49d2b24078041ffa78e7e8d9c72ffed213f7881 (presubmit successful).