Switch to using dartdomgenerator.py for Dart interface and implementation generation.

Switch to using dartdomgenerator.py for Dart interface and implementation generation.


R=antonm@google.com

Committed: https://src.chromium.org/viewvc/multivm?view=rev&revision=136

[antonm, 2012-01-25 18:56:39]

https://chromiumcodereview.appspot.com/9288026/diff/1/Source/WebCore/bindings...
File Source/WebCore/bindings/dart/custom/DartArrayBufferViewCustom.h (right):

https://chromiumcodereview.appspot.com/9288026/diff/1/Source/WebCore/bindings...
Source/WebCore/bindings/dart/custom/DartArrayBufferViewCustom.h:48: Dart_Handle
arg1 = Dart_GetNativeArgument(args, 0);
shouldn't you rename construct into something else and arg1 into reciever?

https://chromiumcodereview.appspot.com/9288026/diff/1/Source/WebCore/bindings...
File Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm (left):

https://chromiumcodereview.appspot.com/9288026/diff/1/Source/WebCore/bindings...
Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm:1734: if
(defined($DART_INTERFACE)) {
is it the only piece of this file which can R.I.P?

https://chromiumcodereview.appspot.com/9288026/diff/1/Source/WebCore/bindings...
File Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm (right):

https://chromiumcodereview.appspot.com/9288026/diff/1/Source/WebCore/bindings...
Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm:112:
"Float32Array" => 1,
hmm?

https://chromiumcodereview.appspot.com/9288026/diff/1/Source/WebCore/bindings...
File Source/WebCore/bindings/dart/resources/dom_public.dart (left):

https://chromiumcodereview.appspot.com/9288026/diff/1/Source/WebCore/bindings...
Source/WebCore/bindings/dart/resources/dom_public.dart:85:
spawnDomIsolate(Window targetWindow, String entryPoint) {
have you ran LTs?  I wonder where this function is now implemented.  Ditto for
layoutTestController.

[podivilov, 2012-01-25 19:04:05]

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/cu...
File Source/WebCore/bindings/dart/custom/DartArrayBufferViewCustom.h (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/cu...
Source/WebCore/bindings/dart/custom/DartArrayBufferViewCustom.h:48: Dart_Handle
arg1 = Dart_GetNativeArgument(args, 0);
On 2012/01/25 18:56:40, antonm wrote:
> shouldn't you rename construct into something else and arg1 into reciever?

I think construct is the right name, as we construct an Array in this method.
arg1 is first constructor argument, not reciever.

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm (left):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm:1734: if
(defined($DART_INTERFACE)) {
On 2012/01/25 18:56:40, antonm wrote:
> is it the only piece of this file which can R.I.P?

Don't want to add extra noise. Let's delete this script later when it is not
used anymore.

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm:112:
"Float32Array" => 1,
On 2012/01/25 18:56:40, antonm wrote:
> hmm?

See client/dom/src/_FactoryProviders.dart

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/re...
File Source/WebCore/bindings/dart/resources/dom_public.dart (left):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/re...
Source/WebCore/bindings/dart/resources/dom_public.dart:85:
spawnDomIsolate(Window targetWindow, String entryPoint) {
On 2012/01/25 18:56:40, antonm wrote:
> have you ran LTs?  I wonder where this function is now implemented.  Ditto for
> layoutTestController.

See http://codereview.chromium.org/9290020/.

[Anton Muhin, 2012-01-26 12:13:35]

Getting close

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/cu...
File Source/WebCore/bindings/dart/custom/DartArrayBufferViewCustom.h (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/cu...
Source/WebCore/bindings/dart/custom/DartArrayBufferViewCustom.h:48: Dart_Handle
arg1 = Dart_GetNativeArgument(args, 0);
On 2012/01/25 19:04:05, podivilov wrote:
> On 2012/01/25 18:56:40, antonm wrote:
> > shouldn't you rename construct into something else and arg1 into reciever?
> 
> I think construct is the right name, as we construct an Array in this method.
> arg1 is first constructor argument, not reciever.

I think factory is ways better.

My understanding: constructor is method invoked on already allocated instance,
while factory both creates and initializes the object.  WDYT?

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/cu...
Source/WebCore/bindings/dart/custom/DartArrayBufferViewCustom.h:67: if (result)
care to add comment why you don't use ::returnValue here?

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/dartium.gyp (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/dartium.gyp:353:
'<(dart_auxiliary_dir)/DOMType.dart',
maybe depend on content of the whole dir?

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm:112:
"Float32Array" => 1,
On 2012/01/25 19:04:05, podivilov wrote:
> On 2012/01/25 18:56:40, antonm wrote:
> > hmm?
> 
> See client/dom/src/_FactoryProviders.dart

Sorry, may you elaborate?  If this knowledge is hidden in other place, do we
need this map at all?

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py:84:
#fremontcutbuilder.main()
if you're going to commit it, please, remove commented out code.

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py:86: import
dartdomgenerator
nit: may we move import into standard place?

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py:87:
dartdomgenerator.GenerateDOM(True, outputFilePath)
it might be a good idea to use parameter name here to clarify meaning of True

[podivilov, 2012-01-30 18:27:54]

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/cu...
File Source/WebCore/bindings/dart/custom/DartArrayBufferViewCustom.h (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/cu...
Source/WebCore/bindings/dart/custom/DartArrayBufferViewCustom.h:48: Dart_Handle
arg1 = Dart_GetNativeArgument(args, 0);
On 2012/01/26 12:13:35, antonmuhin wrote:
> On 2012/01/25 19:04:05, podivilov wrote:
> > On 2012/01/25 18:56:40, antonm wrote:
> > > shouldn't you rename construct into something else and arg1 into reciever?
> > 
> > I think construct is the right name, as we construct an Array in this
method.
> > arg1 is first constructor argument, not reciever.
> 
> I think factory is ways better.
> 
> My understanding: constructor is method invoked on already allocated instance,
> while factory both creates and initializes the object.  WDYT?

Done.

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/dartium.gyp (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/dartium.gyp:353:
'<(dart_auxiliary_dir)/DOMType.dart',
On 2012/01/26 12:13:35, antonmuhin wrote:
> maybe depend on content of the whole dir?
afaik gyp doesn't support globs.

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm:112:
"Float32Array" => 1,
On 2012/01/26 12:13:35, antonmuhin wrote:
> On 2012/01/25 19:04:05, podivilov wrote:
> > On 2012/01/25 18:56:40, antonm wrote:
> > > hmm?
> > 
> > See client/dom/src/_FactoryProviders.dart
> 
> Sorry, may you elaborate?  If this knowledge is hidden in other place, do we
> need this map at all?

TypedArray constructors are declared with a single argument in
_FactoryProviders.dart. But i don't see how can we use that knowledge here.

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py:84:
#fremontcutbuilder.main()
On 2012/01/26 12:13:35, antonmuhin wrote:
> if you're going to commit it, please, remove commented out code.

Done.

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py:86: import
dartdomgenerator
On 2012/01/26 12:13:35, antonmuhin wrote:
> nit: may we move import into standard place?

Please note sys.path.insert above.

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py:87:
dartdomgenerator.GenerateDOM(True, outputFilePath)
On 2012/01/26 12:13:35, antonmuhin wrote:
> it might be a good idea to use parameter name here to clarify meaning of True

Done.

[antonm, 2012-01-31 10:48:02]

LGTM modulo comments

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/CodeGeneratorDart.pm:112:
"Float32Array" => 1,
Sorry, I wasn't clear enough.

I believe those exceptions (*Array family) had been added recently by Kolya. 
And native counterparts to those constructors are custom anyway.  So I wonder if
we need to keep this overrides for number of arguments in this perl script at
all.

On 2012/01/30 18:27:54, podivilov wrote:
> On 2012/01/26 12:13:35, antonmuhin wrote:
> > On 2012/01/25 19:04:05, podivilov wrote:
> > > On 2012/01/25 18:56:40, antonm wrote:
> > > > hmm?
> > > 
> > > See client/dom/src/_FactoryProviders.dart
> > 
> > Sorry, may you elaborate?  If this knowledge is hidden in other place, do we
> > need this map at all?
> 
> TypedArray constructors are declared with a single argument in
> _FactoryProviders.dart. But i don't see how can we use that knowledge here.

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
File Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py (right):

http://codereview.chromium.org/9288026/diff/1/Source/WebCore/bindings/dart/gy...
Source/WebCore/bindings/dart/gyp/scripts/build_dart_snapshot.py:86: import
dartdomgenerator
On 2012/01/30 18:27:54, podivilov wrote:
> On 2012/01/26 12:13:35, antonmuhin wrote:
> > nit: may we move import into standard place?
> 
> Please note sys.path.insert above.

I see.  May you group sys.path manipulations and import statements into a single
block to make it more obvious for future readers?

http://codereview.chromium.org/9288026/diff/7001/Source/WebCore/bindings/dart...
File Source/WebCore/bindings/dart/gyp/dartium.gyp (right):

http://codereview.chromium.org/9288026/diff/7001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/gyp/dartium.gyp:331: 'dart_auxiliary_dir':
'<(dart_dir)/client/dom/src',
nit: we technically have dart_client_dir var above it might be a good idea to
introduce dart_client_dom dir, up to you.