Generate Mojom Types for Dart

Generate Mojom Types for Dart

This commit adds mojom type information to Dart, similarly to how it
was done for Go in https://codereview.chromium.org/1345263002/

The Dart Mojom Types have been generated with minor API tweaks:
Proxy exposes a service description as well as the generated interface.

Since Dart compilation for the Embedder is 'hacky', this CL also
changes the mojom_types.mojom to use a simpler module:
  mojo.bindings.types => mojo
This workaround needed to make it work with the Embedder hack is worse.
However, once it is removed, the module name can likely be restored.

Since mojo/dart/packages checks in their *.mojom.dart files,
I have regenerated them all to correspond with the template
changes in this CL.

Further, some modifications of the generator script make
it more possible to introduce a flag to toggle type generation.
Related issue: https://github.com/domokit/mojo/issues/558

BUG=

[alexfandrianto, 2015-11-11 03:13:21]

Description was changed from

==========
Generate Mojom Types for Dart

This commit adds mojom type information to Dart, similarly to how it
was done for Go in https://codereview.chromium.org/1345263002/

The Dart Mojom Types have been generated with minor API tweaks:
Proxy exposes a service description as well as the generated interface.

Since Dart compilation for the Embedder is 'hacky', this CL also
changes the mojom_types.mojom to use a simpler module:
  mojo.bindings.types => mojo
This workaround needed to make it work with the Embedder hack is worse.
However, once it is removed, the module name can likely be restored.

BUG=
==========

to

==========
Generate Mojom Types for Dart

This commit adds mojom type information to Dart, similarly to how it
was done for Go in https://codereview.chromium.org/1345263002/

The Dart Mojom Types have been generated with minor API tweaks:
Proxy exposes a service description as well as the generated interface.

Since Dart compilation for the Embedder is 'hacky', this CL also
changes the mojom_types.mojom to use a simpler module:
  mojo.bindings.types => mojo
This workaround needed to make it work with the Embedder hack is worse.
However, once it is removed, the module name can likely be restored.

BUG=
==========

[alexfandrianto, 2015-11-11 03:13:21]

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

[rudominer, 2015-11-13 01:43:00]

Alex can you take a look at the test failures and see if they are related to
your change.

[rudominer, 2015-11-13 01:54:27]

Alex would you please move your files from mojo/public/dart to
mojo/dart/packages because of https://codereview.chromium.org/1441033002/.

I would like to patch in your CL and try it out but I can't do that now.

[rudominer, 2015-11-13 02:18:19]

Just did one small pass so far.

https://codereview.chromium.org/1433183002/diff/1/mojo/dart/test/bindings_gen...
File mojo/dart/test/bindings_generation_test.dart (right):

https://codereview.chromium.org/1433183002/diff/1/mojo/dart/test/bindings_gen...
mojo/dart/test/bindings_generation_test.dart:279: // Now compare the labels to
verify that the enum labels match the expected ones.
line too long

https://codereview.chromium.org/1433183002/diff/1/mojo/dart/test/bindings_gen...
mojo/dart/test/bindings_generation_test.dart:313: // Now compare the fields to
verify that the struct fields match the expected ones.
line too long

https://codereview.chromium.org/1433183002/diff/1/mojo/public/dart/mojo/lib/s...
File mojo/public/dart/mojo/lib/src/proxy.dart (right):

https://codereview.chromium.org/1433183002/diff/1/mojo/public/dart/mojo/lib/s...
mojo/public/dart/mojo/lib/src/proxy.dart:40: /// ServiceRequest.
"this ServiceRequest" doesn't make sense. This object is called "Proxy". If you
think that this is the right place to put this getter can you please try to give
an explanation for the getter than makes sense in this context (to somebody who
doesn't already know what a ServiceDescription is.) I

[alexfandrianto, 2015-11-13 22:11:01]

https://codereview.chromium.org/1433183002/diff/1/mojo/dart/test/bindings_gen...
File mojo/dart/test/bindings_generation_test.dart (right):

https://codereview.chromium.org/1433183002/diff/1/mojo/dart/test/bindings_gen...
mojo/dart/test/bindings_generation_test.dart:279: // Now compare the labels to
verify that the enum labels match the expected ones.
On 2015/11/13 02:18:18, rudominer wrote:
> line too long

Done.

https://codereview.chromium.org/1433183002/diff/1/mojo/dart/test/bindings_gen...
mojo/dart/test/bindings_generation_test.dart:313: // Now compare the fields to
verify that the struct fields match the expected ones.
On 2015/11/13 02:18:18, rudominer wrote:
> line too long

Done.

https://codereview.chromium.org/1433183002/diff/1/mojo/public/dart/mojo/lib/s...
File mojo/public/dart/mojo/lib/src/proxy.dart (right):

https://codereview.chromium.org/1433183002/diff/1/mojo/public/dart/mojo/lib/s...
mojo/public/dart/mojo/lib/src/proxy.dart:40: /// ServiceRequest.
On 2015/11/13 02:18:18, rudominer wrote:
> "this ServiceRequest" doesn't make sense. This object is called "Proxy". If
you
> think that this is the right place to put this getter can you please try to
give
> an explanation for the getter than makes sense in this context (to somebody
who
> doesn't already know what a ServiceDescription is.) I

Good catch. I copy-pasted this from Go, so I will take another pass over my
description.

[rudominer, 2015-11-18 16:43:49]

Hi Alex, where are we with this patch? In my mind I'm still waiting on you. Is
that correct?

[rudominer, 2015-11-18 16:43:49]

Hi Alex, where are we with this patch? In my mind I'm still waiting on you. Is
that correct?

[alexfandrianto, 2015-11-18 18:39:58]

Yes, this has been ready. Is the convention to say PTAL?

The reason I uploaded a patchset yesterday is so that my team members can start
using this, and it wasn't working with the latest HEAD.

PTAL.

[zra, 2015-11-18 19:24:31]

zra@google.com changed reviewers:
+ zra@google.com

[zra, 2015-11-18 19:24:36]

DBC

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

https://codereview.chromium.org/1433183002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:43:
final {{descpkg}}ServiceDescription serviceDescription =
This is probably not the right place for this. See comment below.

https://codereview.chromium.org/1433183002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:91:
{{descpkg}}ServiceDescription get serviceDescription =>
This seems like the right place. See comment below.

https://codereview.chromium.org/1433183002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:135:
class _{{interface|name}}ProxyCalls extends {{interface|name}} {
Sorry for the lack of comments here. The ProxyCalls class has been carefully
constructed to try to avoid naming conflicts. Currently the only potential
conflict comes from the '_proxyImpl' field. This change introduces another
potential conflict by extending {{interface|name}}, which now has a new field.
It looks like that field has been added to the ProxyImpl class. Why does it need
to go in both places? If you're just trying to get more convenient access to the
serviceDescription, you can add a forwarding getter to the Proxy class below.

[alexfandrianto, 2015-11-19 23:27:23]

We chatted offline and decided to move the serviceDescription off the abstract
class and onto the Stub. It will eventually be up to Dart service writers to
choose to expose this serviceDescription to the (not yet existent)
ServiceDescriber implementation.

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

https://codereview.chromium.org/1433183002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:135:
class _{{interface|name}}ProxyCalls extends {{interface|name}} {
On 2015/11/18 19:24:36, zra wrote:
> Sorry for the lack of comments here. The ProxyCalls class has been carefully
> constructed to try to avoid naming conflicts. Currently the only potential
> conflict comes from the '_proxyImpl' field. This change introduces another
> potential conflict by extending {{interface|name}}, which now has a new field.
> It looks like that field has been added to the ProxyImpl class. Why does it
need
> to go in both places? If you're just trying to get more convenient access to
the
> serviceDescription, you can add a forwarding getter to the Proxy class below.

As discussed offline, the reason for these changes were to expose
serviceDescription to the server-side. However, we have thought of a workaround
that only needs us to add serviceDescription to the stub.

[alexfandrianto, 2015-11-20 02:07:04]

The tests are quite green now, so PTAL. Now that I'm aware, I'll look for that
too.

Is there a way to be notified when the tests fail or complete successfully?

[azani, 2015-11-20 20:17:59]

Thanks for taking this on Alex.

first pass.

Can you update the CL description to talk about why all the mojom.dart files are
being updated.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/interfaces/...
File mojo/public/interfaces/bindings/mojom_files.mojom (right):

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/mojom_files.mojom:6: module mojo;
Can you give me a pointer to where I should look in order to see what makes this
necessary?

Also maybe add a TODO?

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

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:45:
{%- if type|identifier|identifier_check('WriteMojomType') -%} {#- Already
printed out -#}
Sorry, I can't figure out why we would expect to call writeMojomType more than
once on a particular type.

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

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:442: return
hasattr(kind, 'imported_from') and kind.imported_from is not None
unless you need to differentiate between an empty dict and None, the convention
is to omit "is not None".

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:463:
identifier_cache = {}
I don't think you're actually using this as a cache. Can you maybe use a more
appropriate name.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:471: return ''
This looks like an error condition. Can you raise an exception here?

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:473: # Most kinds
have a name, but those that don't should rely on their spec.
In which case does a kind not have a name? You should not rely on spec as it may
go away in the future.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:482:
identifier_cache[cache_name] = {}
You could use a defaultdict instead.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:491: # Get the
mojom type's identifier suffix.
Put this comment in the function's =docstring.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:506:
'identifier_check': CheckIdentifier,
Can you explain what the purpose of identifier_store and identifier_check is?
I'm really uncomfortable with running functions with side-effects as part of the
template generation.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:533:
should_gen_mojom_types = True
Given that most people likely won't want to generate the type information, maybe
we should implement that in that CL.

Also, I think TODOs should generally have a username associated with them.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:550: "typepkg":
'%s.' % _mojom_types_pkg_short \
In order to enhance readability, you could just set all those values in the
dictionary for the common case, and then remove them later in the function if
need be.

[alexfandrianto, 2015-11-21 03:58:34]

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/interfaces/...
File mojo/public/interfaces/bindings/mojom_files.mojom (right):

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/mojom_files.mojom:6: module mojo;
On 2015/11/20 20:17:59, azani wrote:
> Can you give me a pointer to where I should look in order to see what makes
this
> necessary?
> 
> Also maybe add a TODO?

It's not very simple to explain, but I will try. zra or Cutch can clarify.

The problem is that Dart also builds something called the 'embedder'.
https://github.com/domokit/mojo/blob/master/mojo/dart/embedder/BUILD.gn

In this BUILD rule it copies over the *.mojom.dart files from application,
bindings, and core to something called the dart_embedder_packages in
"out/Debug/gen/dart_embedder_packages/mojo/mojo". Unfortunately, the copying
(and path amending) is done in a simple fashion. Even if some .mojom files are
supposed to be generated in a subfolder, they won't get copied over correctly.

We determined that since the embedder is supposed to disappear soon, it is more
straightforward to temporarily change this module name, and change it back once
the embedder issues go away.

I will add a TODO for that. Is it okay to create an issue even though this CL is
still in-progress?

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

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:45:
{%- if type|identifier|identifier_check('WriteMojomType') -%} {#- Already
printed out -#}
On 2015/11/20 20:17:59, azani wrote:
> Sorry, I can't figure out why we would expect to call writeMojomType more than
> once on a particular type.

One reason writeMojomType might be called more than once for a particular type
is for recursive types or for composite types that contain other
structs/unions/interfaces/enums.

For example, a struct that contains itself as a field would need to instead
write a type reference instead of defining itself again.

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

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:442: return
hasattr(kind, 'imported_from') and kind.imported_from is not None
On 2015/11/20 20:17:58, azani wrote:
> unless you need to differentiate between an empty dict and None, the
convention
> is to omit "is not None".

I didn't know that, thanks. (I like being very specific in what I'm comparing.)

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:463:
identifier_cache = {}
On 2015/11/20 20:17:59, azani wrote:
> I don't think you're actually using this as a cache. Can you maybe use a more
> appropriate name.

How about identifier_store?

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:471: return ''
On 2015/11/20 20:17:58, azani wrote:
> This looks like an error condition. Can you raise an exception here?

I call GetIdentifier with every kind, including simple kinds, so I prefer
returning '' instead of throwing.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:473: # Most kinds
have a name, but those that don't should rely on their spec.
On 2015/11/20 20:17:58, azani wrote:
> In which case does a kind not have a name? You should not rely on spec as it
may
> go away in the future.

I moved the name computation earlier, so I have no more reliance on spec.
Thanks for pointing that out. (I also changed Go to mirror these changes.)

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:482:
identifier_cache[cache_name] = {}
On 2015/11/20 20:17:58, azani wrote:
> You could use a defaultdict instead.

I'll use defaultdict(lambda : defaultdict(bool))

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:491: # Get the
mojom type's identifier suffix.
On 2015/11/20 20:17:59, azani wrote:
> Put this comment in the function's =docstring.

Done.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:506:
'identifier_check': CheckIdentifier,
On 2015/11/20 20:17:59, azani wrote:
> Can you explain what the purpose of identifier_store and identifier_check is?
> I'm really uncomfortable with running functions with side-effects as part of
the
> template generation.

The purpose is to detect if we have already generated information for a type
before. It breaks cycles and repeats.

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:533:
should_gen_mojom_types = True
On 2015/11/20 20:17:59, azani wrote:
> Given that most people likely won't want to generate the type information,
maybe
> we should implement that in that CL.
> 
> Also, I think TODOs should generally have a username associated with them.

I tried to plumb this most of the way through. This needs to be on for the tests
to pass.
I've modified public/tools/bindings/mojom_bindings_generator_v1.py, but I don't
know who calls this one, so I left a TODO there.
https://github.com/domokit/mojo/issues/558

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:550: "typepkg":
'%s.' % _mojom_types_pkg_short \
On 2015/11/20 20:17:59, azani wrote:
> In order to enhance readability, you could just set all those values in the
> dictionary for the common case, and then remove them later in the function if
> need be.

Done.

[alexfandrianto, 2015-11-21 04:00:23]

Description was changed from

==========
Generate Mojom Types for Dart

This commit adds mojom type information to Dart, similarly to how it
was done for Go in https://codereview.chromium.org/1345263002/

The Dart Mojom Types have been generated with minor API tweaks:
Proxy exposes a service description as well as the generated interface.

Since Dart compilation for the Embedder is 'hacky', this CL also
changes the mojom_types.mojom to use a simpler module:
  mojo.bindings.types => mojo
This workaround needed to make it work with the Embedder hack is worse.
However, once it is removed, the module name can likely be restored.

BUG=
==========

to

==========
Generate Mojom Types for Dart

This commit adds mojom type information to Dart, similarly to how it
was done for Go in https://codereview.chromium.org/1345263002/

The Dart Mojom Types have been generated with minor API tweaks:
Proxy exposes a service description as well as the generated interface.

Since Dart compilation for the Embedder is 'hacky', this CL also
changes the mojom_types.mojom to use a simpler module:
  mojo.bindings.types => mojo
This workaround needed to make it work with the Embedder hack is worse.
However, once it is removed, the module name can likely be restored.

Since mojo/dart/packages checks in their *.mojom.dart files, I have regenerated
them all to correspond with the template changes in this CL.

Further, some modifications of the generator script make it more possible to
introduce a flag to toggle type generation. (Related issue:
https://github.com/domokit/mojo/issues/558)

BUG=
==========

[alexfandrianto, 2015-11-21 04:00:55]

Description was changed from

==========
Generate Mojom Types for Dart

This commit adds mojom type information to Dart, similarly to how it
was done for Go in https://codereview.chromium.org/1345263002/

The Dart Mojom Types have been generated with minor API tweaks:
Proxy exposes a service description as well as the generated interface.

Since Dart compilation for the Embedder is 'hacky', this CL also
changes the mojom_types.mojom to use a simpler module:
  mojo.bindings.types => mojo
This workaround needed to make it work with the Embedder hack is worse.
However, once it is removed, the module name can likely be restored.

Since mojo/dart/packages checks in their *.mojom.dart files, I have regenerated
them all to correspond with the template changes in this CL.

Further, some modifications of the generator script make it more possible to
introduce a flag to toggle type generation. (Related issue:
https://github.com/domokit/mojo/issues/558)

BUG=
==========

to

==========
Generate Mojom Types for Dart

This commit adds mojom type information to Dart, similarly to how it
was done for Go in https://codereview.chromium.org/1345263002/

The Dart Mojom Types have been generated with minor API tweaks:
Proxy exposes a service description as well as the generated interface.

Since Dart compilation for the Embedder is 'hacky', this CL also
changes the mojom_types.mojom to use a simpler module:
  mojo.bindings.types => mojo
This workaround needed to make it work with the Embedder hack is worse.
However, once it is removed, the module name can likely be restored.

Since mojo/dart/packages checks in their *.mojom.dart files,
I have regenerated them all to correspond with the template
changes in this CL.

Further, some modifications of the generator script make
it more possible to introduce a flag to toggle type generation.
Related issue: https://github.com/domokit/mojo/issues/558

BUG=
==========

[azani, 2015-11-21 17:06:47]

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

https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:45:
{%- if type|identifier|identifier_check('WriteMojomType') -%} {#- Already
printed out -#}
On 2015/11/21 03:58:34, alexfandrianto wrote:
> On 2015/11/20 20:17:59, azani wrote:
> > Sorry, I can't figure out why we would expect to call writeMojomType more
than
> > once on a particular type.
> 
> One reason writeMojomType might be called more than once for a particular type
> is for recursive types or for composite types that contain other
> structs/unions/interfaces/enums.
> 
> For example, a struct that contains itself as a field would need to instead
> write a type reference instead of defining itself again.

I don't think you want to call writeMojomType on the types of the fields of a
struct. The type of an individual field should always be defined somewhere else.
So if you're writing the type of a field, you should always write it as a type
reference.

[alexfandrianto, 2015-11-23 20:07:07]

On 2015/11/21 17:06:47, azani wrote:
>
https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
> File
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl
> (right):
> 
>
https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
>
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:45:
> {%- if type|identifier|identifier_check('WriteMojomType') -%} {#- Already
> printed out -#}
> On 2015/11/21 03:58:34, alexfandrianto wrote:
> > On 2015/11/20 20:17:59, azani wrote:
> > > Sorry, I can't figure out why we would expect to call writeMojomType more
> than
> > > once on a particular type.
> > 
> > One reason writeMojomType might be called more than once for a particular
type
> > is for recursive types or for composite types that contain other
> > structs/unions/interfaces/enums.
> > 
> > For example, a struct that contains itself as a field would need to instead
> > write a type reference instead of defining itself again.
> 
> I don't think you want to call writeMojomType on the types of the fields of a
> struct. The type of an individual field should always be defined somewhere
else.
> So if you're writing the type of a field, you should always write it as a type
> reference.

Yes, you are right. I should have looked more closely instead of relying on what
used to be the case. I believe this case is now covered by the topLevel flag,
which means I can simplify my identifier_store for both Go and Dart.

[alexfandrianto, 2015-11-24 02:43:20]

On 2015/11/23 20:07:07, alexfandrianto wrote:
> On 2015/11/21 17:06:47, azani wrote:
> >
>
https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
> > File
> mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl
> > (right):
> > 
> >
>
https://codereview.chromium.org/1433183002/diff/90001/mojo/public/tools/bindi...
> >
>
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:45:
> > {%- if type|identifier|identifier_check('WriteMojomType') -%} {#- Already
> > printed out -#}
> > On 2015/11/21 03:58:34, alexfandrianto wrote:
> > > On 2015/11/20 20:17:59, azani wrote:
> > > > Sorry, I can't figure out why we would expect to call writeMojomType
more
> > than
> > > > once on a particular type.
> > > 
> > > One reason writeMojomType might be called more than once for a particular
> type
> > > is for recursive types or for composite types that contain other
> > > structs/unions/interfaces/enums.
> > > 
> > > For example, a struct that contains itself as a field would need to
instead
> > > write a type reference instead of defining itself again.
> > 
> > I don't think you want to call writeMojomType on the types of the fields of
a
> > struct. The type of an individual field should always be defined somewhere
> else.
> > So if you're writing the type of a field, you should always write it as a
type
> > reference.
> 
> Yes, you are right. I should have looked more closely instead of relying on
what
> used to be the case. I believe this case is now covered by the topLevel flag,
> which means I can simplify my identifier_store for both Go and Dart.

I've made that change, and it looks like tests pass again. PTAL

[zra, 2015-11-24 17:34:44]

Mostly questions, nits, and cleanup requests.

https://codereview.chromium.org/1433183002/diff/110001/mojo/dart/tools/presub...
File mojo/dart/tools/presubmit/check_mojom_dart.py (right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/dart/tools/presub...
mojo/dart/tools/presubmit/check_mojom_dart.py:316: if
check_for_bindings_machinery_changes(affected_files):
Is the new code below still needed after this check?

https://codereview.chromium.org/1433183002/diff/110001/mojo/dart/tools/presub...
mojo/dart/tools/presubmit/check_mojom_dart.py:360: # If the Python generator or
dart template files changed, skip this check.
Is this case not already covered by the check_for_bindings_machinery_changes
check above?

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:16:
class _{{interface|name}}ServiceDescription extends
{{descpkg}}ServiceDescription {
Should this be extends or implements?

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:17:
// Avoid infinite loop by overriding serviceDescription field.
I don't understand this comment.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:35:
{% set mapping = '_MojomDesc__' %}
Trailing underscores are a bit unusual in Dart code. A leading underscore is
sufficient to make something library-private.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:38:
var {{mapping}} = _initDescriptions();
I find it a bit easier to read when globals like this are initialized on first
use. So down in getAllMojomTypeDefinitions you could do something like:

if ({{mapping}} == null) {
  {{mapping}} = _initDescriptions();
}
return {{mapping}};

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:40:
Map<String, {{typepkg}}UserDefinedType> _initDescriptions() {
In the generated code for this function there's a lot of extra whitespace. Is it
possible to do anythinga bout that?

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:41:
var {{temp_mapping}} = new Map<String, {{typepkg}}UserDefinedType>();
If it is not necessary to preserve iteration order, consider using HashMap here
instead.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl:17:
{% elif type|is_enum_kind %}
Maybe here and below is where the extra whitespace is coming from.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl:44:
ERROR: UNSUPPORTED TYPE
Should this case cause bindings generation to fail? It seems like making it fail
somewhere in the python script would be preferable, rather than generating
something that isn't Dart code. Then here you could do something like 'throw
"Unreachable"'.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:38:
BAD TYPE NAME GIVEN
In the Dart parser/compiler is not the right place for this to fail.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:66:
ERROR: UNSUPPORTED TYPE
ditto

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl:68:
default:
If this is an error or unreachable, that should be indicated somehow, probably
by throwing an exception.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl:89:
default:
ditto.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:7: from
collections import defaultdict
I'm not familiar with python conventions for import order, but this looks a bit
out of place here.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:152: # The
mojom_types.mojom and service_describer.mojom files are special because
It looks like you might be trying to avoid importing these libraries where they
aren't needed. If so, that might be good to mention here. If that isn't the
case, then I'm not sure I see why these imports can't be hardcoded into the
template.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:464: # The
identifier_store tracks identifiers that have been used so that code
Is this needed for the new code generation you're adding, or does it fix some
bug in the existing generated code? If it's the latter we should add a
regression test and do this fix in a separate CL.

[azani, 2015-11-24 20:02:02]

Can you please split the changes to the go code generator into another CL
please? This CL is already quite big and adding the go changes in makes it
harder to review.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl:6:
{%- if type|identifier|identifier_check -%}{# Already printed out #}
In what situation would you call registerType on the same type twice? It looks
like you call it by iterating over the objects defined in the module. They
should all be defined once only.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:7: from
collections import defaultdict
The prescribed style is that you import the module and then access the module's
content.

So you would switch this to "import collections" and then call
collections.defaultdict

[azani, 2015-12-03 19:40:58]

On 2015/11/24 20:02:02, azani wrote:
> Can you please split the changes to the go code generator into another CL
> please? This CL is already quite big and adding the go changes in makes it
> harder to review.
> 
>
https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
> File
>
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl
> (right):
> 
>
https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
>
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl:6:
> {%- if type|identifier|identifier_check -%}{# Already printed out #}
> In what situation would you call registerType on the same type twice? It looks
> like you call it by iterating over the objects defined in the module. They
> should all be defined once only.
> 
>
https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
> File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):
> 
>
https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
> mojo/public/tools/bindings/generators/mojom_dart_generator.py:7: from
> collections import defaultdict
> The prescribed style is that you import the module and then access the
module's
> content.
> 
> So you would switch this to "import collections" and then call
> collections.defaultdict

Hey Alex,

This is not to rush you, but my impression is that we're waiting on you to make
changes based on the feedback we gave you. I just want to make sure you're not
waiting on us.

[alexfandrianto, 2015-12-03 20:00:34]

On 2015/12/03 19:40:58, azani wrote:
> On 2015/11/24 20:02:02, azani wrote:
> > Can you please split the changes to the go code generator into another CL
> > please? This CL is already quite big and adding the go changes in makes it
> > harder to review.
> > 
> >
>
https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
> > File
> >
>
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl
> > (right):
> > 
> >
>
https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
> >
>
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl:6:
> > {%- if type|identifier|identifier_check -%}{# Already printed out #}
> > In what situation would you call registerType on the same type twice? It
looks
> > like you call it by iterating over the objects defined in the module. They
> > should all be defined once only.
> > 
> >
>
https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
> > File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):
> > 
> >
>
https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
> > mojo/public/tools/bindings/generators/mojom_dart_generator.py:7: from
> > collections import defaultdict
> > The prescribed style is that you import the module and then access the
> module's
> > content.
> > 
> > So you would switch this to "import collections" and then call
> > collections.defaultdict
> 
> Hey Alex,
> 
> This is not to rush you, but my impression is that we're waiting on you to
make
> changes based on the feedback we gave you. I just want to make sure you're not
> waiting on us.

Thanks for the bump. I've been busy recently, so thanks for waiting.

I think this CL can be split into 3 parts.
mojom_types mojom_files service_describer module change (minor)
Go generator changes (minor)
Dart generator changes (major)

[alexfandrianto, 2015-12-18 01:53:19]

See the new CL: https://codereview.chromium.org/1539673003/

https://codereview.chromium.org/1433183002/diff/110001/mojo/dart/tools/presub...
File mojo/dart/tools/presubmit/check_mojom_dart.py (right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/dart/tools/presub...
mojo/dart/tools/presubmit/check_mojom_dart.py:316: if
check_for_bindings_machinery_changes(affected_files):
On 2015/11/24 17:34:44, zra wrote:
> Is the new code below still needed after this check?

Not using this anymore.

https://codereview.chromium.org/1433183002/diff/110001/mojo/dart/tools/presub...
mojo/dart/tools/presubmit/check_mojom_dart.py:360: # If the Python generator or
dart template files changed, skip this check.
On 2015/11/24 17:34:44, zra wrote:
> Is this case not already covered by the check_for_bindings_machinery_changes
> check above?

Not using this anymore.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:16:
class _{{interface|name}}ServiceDescription extends
{{descpkg}}ServiceDescription {
On 2015/11/24 17:34:44, zra wrote:
> Should this be extends or implements?

It doesn't matter, but I think the preference was to implement, so I switched
it.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:17:
// Avoid infinite loop by overriding serviceDescription field.
On 2015/11/24 17:34:44, zra wrote:
> I don't understand this comment.

It's not needed anymore.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:35:
{% set mapping = '_MojomDesc__' %}
On 2015/11/24 17:34:44, zra wrote:
> Trailing underscores are a bit unusual in Dart code. A leading underscore is
> sufficient to make something library-private.

Removed trailing underscores

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:38:
var {{mapping}} = _initDescriptions();
On 2015/11/24 17:34:44, zra wrote:
> I find it a bit easier to read when globals like this are initialized on first
> use. So down in getAllMojomTypeDefinitions you could do something like:
> 
> if ({{mapping}} == null) {
>   {{mapping}} = _initDescriptions();
> }
> return {{mapping}};

Done.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:40:
Map<String, {{typepkg}}UserDefinedType> _initDescriptions() {
On 2015/11/24 17:34:44, zra wrote:
> In the generated code for this function there's a lot of extra whitespace. Is
it
> possible to do anythinga bout that?

I've somehow reduced it a lot. More -'s help.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:41:
var {{temp_mapping}} = new Map<String, {{typepkg}}UserDefinedType>();
On 2015/11/24 17:34:44, zra wrote:
> If it is not necessary to preserve iteration order, consider using HashMap
here
> instead.

I switched this to HashMap. To use it, I am now conditionally importing
dart:collections depending on whether or not we are generating mojom types.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl:17:
{% elif type|is_enum_kind %}
On 2015/11/24 17:34:44, zra wrote:
> Maybe here and below is where the extra whitespace is coming from.

Done.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/mojom_reference_macros.tmpl:44:
ERROR: UNSUPPORTED TYPE
On 2015/11/24 17:34:44, zra wrote:
> Should this case cause bindings generation to fail? It seems like making it
fail
> somewhere in the python script would be preferable, rather than generating
> something that isn't Dart code. Then here you could do something like 'throw
> "Unreachable"'.

Changed enough such that it doesn't need to error.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:38:
BAD TYPE NAME GIVEN
On 2015/11/24 17:34:44, zra wrote:
> In the Dart parser/compiler is not the right place for this to fail.

Added a failure mode.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/mojom_type_macros.tmpl:66:
ERROR: UNSUPPORTED TYPE
On 2015/11/24 17:34:44, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl
(right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl:68:
default:
On 2015/11/24 17:34:44, zra wrote:
> If this is an error or unreachable, that should be indicated somehow, probably
> by throwing an exception.

Someone else fixed this.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/dart_templates/union_definition.tmpl:89:
default:
On 2015/11/24 17:34:44, zra wrote:
> ditto.

Done.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:7: from
collections import defaultdict
On 2015/11/24 20:02:02, azani wrote:
> The prescribed style is that you import the module and then access the
module's
> content.
> 
> So you would switch this to "import collections" and then call
> collections.defaultdict

Not needed anymore. No more identifiercache

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:152: # The
mojom_types.mojom and service_describer.mojom files are special because
On 2015/11/24 17:34:44, zra wrote:
> It looks like you might be trying to avoid importing these libraries where
they
> aren't needed. If so, that might be good to mention here. If that isn't the
> case, then I'm not sure I see why these imports can't be hardcoded into the
> template.

Done.

https://codereview.chromium.org/1433183002/diff/110001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:464: # The
identifier_store tracks identifiers that have been used so that code
On 2015/11/24 17:34:44, zra wrote:
> Is this needed for the new code generation you're adding, or does it fix some
> bug in the existing generated code? If it's the latter we should add a
> regression test and do this fix in a separate CL.

Removed

[azani, 2015-12-22 14:35:14]

Are you withdrawing that CL in favor of the "take 2" CL? If so, can you close
this one? (It's the little X in a circle on the left of "Issue" in the top left
corner.)

[alexfandrianto, 2015-12-22 18:20:05]

On 2015/12/22 14:35:14, azani wrote:
> Are you withdrawing that CL in favor of the "take 2" CL? If so, can you close
> this one? (It's the little X in a circle on the left of "Issue" in the top
left
> corner.)

Yes, withdrawn. I'll rebase "take 2" once I submit
https://codereview.chromium.org/1508293004/