Add support for native bindings generation to dartgenerator.py.

Add support for native bindings generation to dartgenerator.py.

R=antonm@google.com,sra@google.com,vsm@google.com

Committed: https://code.google.com/p/dart/source/detail?r=3855

[sra1, 2012-01-26 05:35:30]

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:442: else:
Is it possible to generate everything at once?

[podivilov, 2012-01-26 08:28:51]

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:442: else:
On 2012/01/26 05:35:30, sra1 wrote:
> Is it possible to generate everything at once?

Yes, but it would be a little bit slower, will produce lots of unused output in
Dartium, and put unversioned files under client/dom. Why do you think we should
generate everything at once? Should native files live under svn in that case?

[antonm, 2012-01-26 11:06:14]

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartdomgenerator.py (right):

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartdomgenerator.py:87: if not native:
cannot you move this logic into main of this file instead?

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:1363: def _BaseClassName(self, interface):
it looks like you can simplify this function w/ early returns: you never reuse
base var:

if not interface.parents:
  return 'DOMWrapperBase'

etc.

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:1486: if
(self._HasNativeIndexGetter(self._interface)):
nit: no need in extra ()

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:1497: if 'HasCustomIndexSetter' in
self._interface.ext_attrs:
for symmetry, shouldn't you add _HasNativeIndexSetter method?  or inline
_HasNativeIndexGetter?

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:1603: '\n'
looks like if you keep a blank line before $MEMBERS in template, this, imho,
ugly leading '\n' may go away.  But if it's a common pattern here, so it be.

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:1604: '  $TYPE operator[](int index) {
return $METHOD(this, index); }\n'
$TYPE operator[](int index) => $METHOD(this, index) ?

Maybe later, up to you.

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2120:
self._FilePathForDartImpl(interface_name))
do you want to invoke this _FilePathForDartImpl twice?  I am not trying to speed
things up, just to have one invocation.  Minor though.

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2160: """Generates Dart and c++
implementation for one DOM IDL interface."""
nit: C++

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2214: def _HasNativeIndexGetter(self,
interface):
do you need this override?

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2285: if info.type_name != 'void':
do you need this if?  Maybe introduce prefix which is 'return ' if type_name !=
'void' and empty string otherwise?

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/src/native_F...
File client/dom/src/native_FactoryProviders.dart (right):

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/src/native_F...
client/dom/src/native_FactoryProviders.dart:15: factory
Float32Array.fromList(List<num> list) =>
FactoryProviderImplementation.F32(list);
should it be num or double?  esp. in the case of integer factories?

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/templates/do...
File client/dom/templates/dom/native/dom_impl.darttemplate (right):

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/templates/do...
client/dom/templates/dom/native/dom_impl.darttemplate:13:
#source('$AUXILIARY_DIR/native_DOMWrapperBase.dart');
cannot we have a single file in AUXILARY_DIR which sources all the necessary
files?  So we don't need to tweak a template to add a new one?

[podivilov, 2012-01-26 14:18:56]

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartdomgener...
File client/dom/scripts/dartdomgenerator.py (right):

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartdomgener...
client/dom/scripts/dartdomgenerator.py:87: if not native:
On 2012/01/26 11:06:14, antonm wrote:
> cannot you move this logic into main of this file instead?

Done.

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
File client/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:1363: def _BaseClassName(self, interface):
On 2012/01/26 11:06:14, antonm wrote:
> it looks like you can simplify this function w/ early returns: you never reuse
> base var:
> 
> if not interface.parents:
>   return 'DOMWrapperBase'
> 
> etc.

Done.

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:1486: if
(self._HasNativeIndexGetter(self._interface)):
On 2012/01/26 11:06:14, antonm wrote:
> nit: no need in extra ()

Done.

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:1497: if 'HasCustomIndexSetter' in
self._interface.ext_attrs:
On 2012/01/26 11:06:14, antonm wrote:
> for symmetry, shouldn't you add _HasNativeIndexSetter method?  or inline
> _HasNativeIndexGetter?

Done.

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:1603: '\n'
On 2012/01/26 11:06:14, antonm wrote:
> looks like if you keep a blank line before $MEMBERS in template, this, imho,
> ugly leading '\n' may go away.  But if it's a common pattern here, so it be.

You could not rely on ordering here, native index getter may easily appear after
something else.

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:1604: '  $TYPE operator[](int index) {
return $METHOD(this, index); }\n'
On 2012/01/26 11:06:14, antonm wrote:
> $TYPE operator[](int index) => $METHOD(this, index) ?
> 
> Maybe later, up to you.

This change should produce zero diff in wrapping dom generated files.

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2160: """Generates Dart and c++
implementation for one DOM IDL interface."""
On 2012/01/26 11:06:14, antonm wrote:
> nit: C++

Done.

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2214: def _HasNativeIndexGetter(self,
interface):
On 2012/01/26 11:06:14, antonm wrote:
> do you need this override?

Yes, it repeats the logic in CodeGeneratorDart.pm.

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2285: if info.type_name != 'void':
On 2012/01/26 11:06:14, antonm wrote:
> do you need this if?  Maybe introduce prefix which is 'return ' if type_name
!=
> 'void' and empty string otherwise?

Note that else clause has additional return statement. I believe code is more
readable with this if.

http://codereview.chromium.org/9290020/diff/1/client/dom/src/native_FactoryPr...
File client/dom/src/native_FactoryProviders.dart (right):

http://codereview.chromium.org/9290020/diff/1/client/dom/src/native_FactoryPr...
client/dom/src/native_FactoryProviders.dart:15: factory
Float32Array.fromList(List<num> list) =>
FactoryProviderImplementation.F32(list);
On 2012/01/26 11:06:14, antonm wrote:
> should it be num or double?  esp. in the case of integer factories?

See _FactoryProviders.dart. We should change both files at once, I think it is
out of this cl's scope.

http://codereview.chromium.org/9290020/diff/1/client/dom/templates/dom/native...
File client/dom/templates/dom/native/dom_impl.darttemplate (right):

http://codereview.chromium.org/9290020/diff/1/client/dom/templates/dom/native...
client/dom/templates/dom/native/dom_impl.darttemplate:13:
#source('$AUXILIARY_DIR/native_DOMWrapperBase.dart');
On 2012/01/26 11:06:14, antonm wrote:
> cannot we have a single file in AUXILARY_DIR which sources all the necessary
> files?  So we don't need to tweak a template to add a new one?

unfortunately, you can source files only from library.

[antonm, 2012-01-26 14:32:05]

LGTM if my last question is a typical stupid question.

http://codereview.chromium.org/9290020/diff/2002/client/dom/scripts/dartgener...
File client/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/9290020/diff/2002/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2108: # Generate dom_public.dart
nit: do not we need a trailing dot here?

http://codereview.chromium.org/9290020/diff/2002/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2115: # Generate dom_impl.dart
ditto

http://codereview.chromium.org/9290020/diff/2002/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2187: def _HasNativeIndexGetter(self,
interface):
why do not you repeat _HasNativeIndexSetter?

[podivilov, 2012-01-26 14:52:37]

Thanks!

http://codereview.chromium.org/9290020/diff/2002/client/dom/scripts/dartgener...
File client/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/9290020/diff/2002/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2108: # Generate dom_public.dart
On 2012/01/26 14:32:05, antonm wrote:
> nit: do not we need a trailing dot here?

Done.

http://codereview.chromium.org/9290020/diff/2002/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2115: # Generate dom_impl.dart
On 2012/01/26 14:32:05, antonm wrote:
> ditto

Done.

http://codereview.chromium.org/9290020/diff/2002/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2187: def _HasNativeIndexGetter(self,
interface):
On 2012/01/26 14:32:05, antonm wrote:
> why do not you repeat _HasNativeIndexSetter?

_HasNativeIndexGetter has a different implementation in
NativeImplementationGenerator, _HasNativeIndexSetter is exactly the same.

[sra1, 2012-01-26 18:32:16]

I have made several comments about the direction in which I would like the code
to move.

If we all agree, I'm LGTM.

What I want to know is if restructure part of the generator, how do I know if I
made a mistake?  Is there a nice methodology for capturing the old and new
generated native version?

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9290020/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:442: else:
On 2012/01/26 08:28:51, podivilov wrote:
> On 2012/01/26 05:35:30, sra1 wrote:
> > Is it possible to generate everything at once?
> 
> Yes, but it would be a little bit slower, will produce lots of unused output
in
> Dartium, and put unversioned files under client/dom. Why do you think we
should
> generate everything at once? Should native files live under svn in that case?

I want to make sure that all targets *can* be generated at once, and then select
which subset from the command line.
When we get an IDL change we will be able to see the whole effect, then fix the
targets independently.
We will be generating dart:dom and dart:html and the dartium bindings.  It will
be easy to miss one.

https://chromiumcodereview.appspot.com/9290020/diff/1017/client/dom/scripts/d...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9290020/diff/1017/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:368: lib_dir=None, native=False):
I would like eventually (i.e. not this CL)
to move towards passing a config object that contains the flags to generate
different versions and the various parameters 

if self._config.generate_native:
  ... self._config.native_xxx_dir ...
if self._config.generated_editor:
  ...

https://chromiumcodereview.appspot.com/9290020/diff/1017/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2073: self._dom_public_imports_emitter =
emitter.Emitter()
The other generators use the pattern of generating a list of pathnames and
passing it to self._GenerateLibFile.
We can make _GenerateLibFile take extra keyword arguments and pass them to the
template.

https://chromiumcodereview.appspot.com/9290020/diff/1017/client/dom/scripts/d...
client/dom/scripts/dartgenerator.py:2266: self._members_emitter.Emit('  $TYPE
$NATIVE_NAME($PARAMS) native "$(INTERFACE)$(NATIVE_NAME)_Callback";\n',
Line too long.
Sometimes this is unavoidable, but you can break this string
'xx yy'
-->
'xx '
'yy'

[podivilov, 2012-01-30 19:23:50]

Thanks for the comments, PTAL.

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
File client/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/9290020/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:442: else:
On 2012/01/26 18:32:16, sra1 wrote:
> On 2012/01/26 08:28:51, podivilov wrote:
> > On 2012/01/26 05:35:30, sra1 wrote:
> > > Is it possible to generate everything at once?
> > 
> > Yes, but it would be a little bit slower, will produce lots of unused output
> in
> > Dartium, and put unversioned files under client/dom. Why do you think we
> should
> > generate everything at once? Should native files live under svn in that
case?
> 
> I want to make sure that all targets *can* be generated at once, and then
select
> which subset from the command line.
> When we get an IDL change we will be able to see the whole effect, then fix
the
> targets independently.
> We will be generating dart:dom and dart:html and the dartium bindings.  It
will
> be easy to miss one.
> 
> 

Added --systems switch. It defaults to 'frog,wrapping' and allows to generate
everything at once.

http://codereview.chromium.org/9290020/diff/1017/client/dom/scripts/dartgener...
File client/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/9290020/diff/1017/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:368: lib_dir=None, native=False):
On 2012/01/26 18:32:16, sra1 wrote:
> I would like eventually (i.e. not this CL)
> to move towards passing a config object that contains the flags to generate
> different versions and the various parameters 
> 
> if self._config.generate_native:
>   ... self._config.native_xxx_dir ...
> if self._config.generated_editor:
>   ...

I suggest that we pass systems list here, and eventually replace it with config
object.

http://codereview.chromium.org/9290020/diff/1017/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2073: self._dom_public_imports_emitter =
emitter.Emitter()
On 2012/01/26 18:32:16, sra1 wrote:
> The other generators use the pattern of generating a list of pathnames and
> passing it to self._GenerateLibFile.
> We can make _GenerateLibFile take extra keyword arguments and pass them to the
> template.
> 

Replaced emitters with files lists like it's done in other systems. Not sure if
we can extract some common code here - native system will generate cpp code and
cpp "libs" (like resolver and DartDerivedSourcesAll.cpp).

http://codereview.chromium.org/9290020/diff/1017/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2266: self._members_emitter.Emit('  $TYPE
$NATIVE_NAME($PARAMS) native "$(INTERFACE)$(NATIVE_NAME)_Callback";\n',
On 2012/01/26 18:32:16, sra1 wrote:
> Line too long.
> Sometimes this is unavoidable, but you can break this string
> 'xx yy'
> -->
> 'xx '
> 'yy'

Done.

[antonm, 2012-02-01 13:03:43]

LGTM

http://codereview.chromium.org/9290020/diff/7001/client/dom/scripts/dartgener...
File client/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/9290020/diff/7001/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:400: if 'native' in systems:
nit: some concerns: systems may have system we don't support and we probably
want to protect against this and overall in check looks slightly unpythonic to
me.

Maybe something like:

def makeNativeSystem():
  ...
  return natie_system

def makeWrappingSystem():
  ...


allSystems = {
  'native': makeNativeSystem,
  'wrapping': makeWrappingSystem,
  ...
}

for system in systems:
  self._systems.append(allSystems[system]())

?

Feel free to ignore.