Add JsInitializerGenerator

Add JsInitializerGenerator

BUG=

[justinfagnani, 2014-09-19 03:20:26]

justinfagnani@google.com changed reviewers:
+ sigmund@google.com

[justinfagnani, 2014-09-19 03:20:27]

This CL adds generation of the JS initializer file for exported libraries and
the entry point. The generated test now works completely from generated code.

Next steps:
 * Try to get JS initialization working in the dynamic version (calling eval()?)
 * Much more test coverage for getters, static methods, constructor variations,
etc.
 * Remove mirrors and noSuchMethod() in the transformed output

[alexandre.ardhuin, 2014-09-19 12:38:59]

alexandre.ardhuin@gmail.com changed reviewers:
+ alexandre.ardhuin@gmail.com

[alexandre.ardhuin, 2014-09-19 12:39:00]

Sorry if my comment are not directly related to this CL.
I tried the build of test_sources/non_transformed and looked at
package:test/library.dart__init_js__.dart 

Let me know if it is not the good place and where you prefer to receive those
kind of comments.

https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
File lib/src/transformer/dart_initializer_generator.dart (right):

https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
lib/src/transformer/dart_initializer_generator.dart:70: JsObject lib =
parent['${library.name}'];
JsObject should be prefixed by "js."

https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
lib/src/transformer/dart_initializer_generator.dart:141: void
_generateField(ExportedField f) {
static fields enter this function.

https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
lib/src/transformer/dart_initializer_generator.dart:150: 'set': new
js.JsFunction.withThis((o, v) => (o[$JS_PREFIX.DART_OBJECT_PROPERTY] as
$className).$name = v),
dart getters enter this function and shouldn't generate a setter.

https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
lib/src/transformer/dart_initializer_generator.dart:189: "${name}:
_getOptionalArg(__js_named_parameters_map__, '${name}')");
_getOptionalArg doesn't seem to be defined ?

[justinfagnani, 2014-09-19 18:47:24]

On 2014/09/19 12:39:00, alexandre.ardhuin wrote:
> Sorry if my comment are not directly related to this CL.
> I tried the build of test_sources/non_transformed and looked at
> package:test/library.dart__init_js__.dart 
> 
> Let me know if it is not the good place and where you prefer to receive those
> kind of comments.
> 
>
https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
> File lib/src/transformer/dart_initializer_generator.dart (right):
> 
>
https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
> lib/src/transformer/dart_initializer_generator.dart:70: JsObject lib =
> parent['${library.name}'];
> JsObject should be prefixed by "js."
> 
>
https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
> lib/src/transformer/dart_initializer_generator.dart:141: void
> _generateField(ExportedField f) {
> static fields enter this function.
> 
>
https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
> lib/src/transformer/dart_initializer_generator.dart:150: 'set': new
> js.JsFunction.withThis((o, v) => (o[$JS_PREFIX.DART_OBJECT_PROPERTY] as
> $className).$name = v),
> dart getters enter this function and shouldn't generate a setter.
> 
>
https://chromiumcodereview.appspot.com/580283004/diff/1/lib/src/transformer/d...
> lib/src/transformer/dart_initializer_generator.dart:189: "${name}:
> _getOptionalArg(__js_named_parameters_map__, '${name}')");
> _getOptionalArg doesn't seem to be defined ?

I think this is as good a place as any for the moment, but soon issues in the
github repo or PR's against the 0.4.0 branch should work. Thanks so much for the
comments!

[Siggi Cherem (dart-lang), 2014-09-19 22:47:10]

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
File lib/src/transformer/entry_point_transformer.dart (right):

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
lib/src/transformer/entry_point_transformer.dart:72: for (Asset asset in
initializerAssets.where((a) => a != null)) {
I didn't realize this last time, it might be simpler/cheaper to just do:

for (Asset asset in initializerAssets) {
  if (asset == null) continue;
  ...

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
lib/src/transformer/entry_point_transformer.dart:78:
dartInitializerCalls.writeln("  $prefix.initializeJavaScriptLibrary();");
nit: 80 col (below too)

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
lib/src/transformer/entry_point_transformer.dart:111: .where((Asset a) => a !=
null)
nit: remove the type here?

https://codereview.chromium.org/580283004/diff/1/test/transformer/entry_point...
File test/transformer/entry_point_transformer_test.dart (right):

https://codereview.chromium.org/580283004/diff/1/test/transformer/entry_point...
test/transformer/entry_point_transformer_test.dart:43: //       
expect(dartSource, contains(
replace prints by test in comment?

https://codereview.chromium.org/580283004/diff/1/test/transformer/js_initiali...
File test/transformer/js_initializer_generator_test.dart (right):

https://codereview.chromium.org/580283004/diff/1/test/transformer/js_initiali...
test/transformer/js_initializer_generator_test.dart:61: //      expect(source,
contains('initializeJavaScriptLibrary()'));
same here?

[justinfagnani, 2014-09-22 20:51:35]

On 2014/09/19 22:47:10, Siggi Cherem (dart-lang) wrote:
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
> File lib/src/transformer/entry_point_transformer.dart (right):
> 
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
> lib/src/transformer/entry_point_transformer.dart:72: for (Asset asset in
> initializerAssets.where((a) => a != null)) {
> I didn't realize this last time, it might be simpler/cheaper to just do:
> 
> for (Asset asset in initializerAssets) {
>   if (asset == null) continue;
>   ...
> 
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
> lib/src/transformer/entry_point_transformer.dart:78:
> dartInitializerCalls.writeln("  $prefix.initializeJavaScriptLibrary();");
> nit: 80 col (below too)
> 
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
> lib/src/transformer/entry_point_transformer.dart:111: .where((Asset a) => a !=
> null)
> nit: remove the type here?
> 
>
https://codereview.chromium.org/580283004/diff/1/test/transformer/entry_point...
> File test/transformer/entry_point_transformer_test.dart (right):
> 
>
https://codereview.chromium.org/580283004/diff/1/test/transformer/entry_point...
> test/transformer/entry_point_transformer_test.dart:43: //       
> expect(dartSource, contains(
> replace prints by test in comment?
> 
>
https://codereview.chromium.org/580283004/diff/1/test/transformer/js_initiali...
> File test/transformer/js_initializer_generator_test.dart (right):
> 
>
https://codereview.chromium.org/580283004/diff/1/test/transformer/js_initiali...
> test/transformer/js_initializer_generator_test.dart:61: //      expect(source,
> contains('initializeJavaScriptLibrary()'));
> same here?

Submitted in:
https://github.com/dart-lang/js-interop/commit/0738f67f1b762e275f7342b12025c9...

[justinfagnani, 2014-09-22 20:54:02]

On 2014/09/22 20:51:35, justinfagnani wrote:
> On 2014/09/19 22:47:10, Siggi Cherem (dart-lang) wrote:
> >
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
> > File lib/src/transformer/entry_point_transformer.dart (right):
> > 
> >
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
> > lib/src/transformer/entry_point_transformer.dart:72: for (Asset asset in
> > initializerAssets.where((a) => a != null)) {
> > I didn't realize this last time, it might be simpler/cheaper to just do:
> > 
> > for (Asset asset in initializerAssets) {
> >   if (asset == null) continue;
> >   ...
> > 
> >
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
> > lib/src/transformer/entry_point_transformer.dart:78:
> > dartInitializerCalls.writeln("  $prefix.initializeJavaScriptLibrary();");
> > nit: 80 col (below too)
> > 
> >
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
> > lib/src/transformer/entry_point_transformer.dart:111: .where((Asset a) => a
!=
> > null)
> > nit: remove the type here?
> > 
> >
>
https://codereview.chromium.org/580283004/diff/1/test/transformer/entry_point...
> > File test/transformer/entry_point_transformer_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/580283004/diff/1/test/transformer/entry_point...
> > test/transformer/entry_point_transformer_test.dart:43: //       
> > expect(dartSource, contains(
> > replace prints by test in comment?
> > 
> >
>
https://codereview.chromium.org/580283004/diff/1/test/transformer/js_initiali...
> > File test/transformer/js_initializer_generator_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/580283004/diff/1/test/transformer/js_initiali...
> > test/transformer/js_initializer_generator_test.dart:61: //     
expect(source,
> > contains('initializeJavaScriptLibrary()'));
> > same here?
> 
> Submitted in:
>
https://github.com/dart-lang/js-interop/commit/0738f67f1b762e275f7342b12025c9...

Oops, crossed issue tabs, this hasn't been submitted.

[justinfagnani, 2014-09-22 21:50:21]

PTAL

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/dart_ini...
File lib/src/transformer/dart_initializer_generator.dart (right):

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/dart_ini...
lib/src/transformer/dart_initializer_generator.dart:141: void
_generateField(ExportedField f) {
On 2014/09/19 12:39:00, alexandre.ardhuin wrote:
> static fields enter this function.

I fixed this in the next CL

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/dart_ini...
lib/src/transformer/dart_initializer_generator.dart:150: 'set': new
js.JsFunction.withThis((o, v) => (o[$JS_PREFIX.DART_OBJECT_PROPERTY] as
$className).$name = v),
On 2014/09/19 12:39:00, alexandre.ardhuin wrote:
> dart getters enter this function and shouldn't generate a setter.

Fixed in the next CL

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/dart_ini...
lib/src/transformer/dart_initializer_generator.dart:189: "${name}:
_getOptionalArg(__js_named_parameters_map__, '${name}')");
On 2014/09/19 12:38:59, alexandre.ardhuin wrote:
> _getOptionalArg doesn't seem to be defined ?

Acknowledged. I'll add tests for method invocation and fix this.

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
File lib/src/transformer/entry_point_transformer.dart (right):

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
lib/src/transformer/entry_point_transformer.dart:72: for (Asset asset in
initializerAssets.where((a) => a != null)) {
On 2014/09/19 22:47:10, Siggi Cherem (dart-lang) wrote:
> I didn't realize this last time, it might be simpler/cheaper to just do:
> 
> for (Asset asset in initializerAssets) {
>   if (asset == null) continue;
>   ...

I like this style, it reads well to me.

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
lib/src/transformer/entry_point_transformer.dart:78:
dartInitializerCalls.writeln("  $prefix.initializeJavaScriptLibrary();");
On 2014/09/19 22:47:09, Siggi Cherem (dart-lang) wrote:
> nit: 80 col (below too)

Done.

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
lib/src/transformer/entry_point_transformer.dart:111: .where((Asset a) => a !=
null)
On 2014/09/19 22:47:10, Siggi Cherem (dart-lang) wrote:
> nit: remove the type here?

Done.

https://codereview.chromium.org/580283004/diff/1/test/transformer/entry_point...
File test/transformer/entry_point_transformer_test.dart (right):

https://codereview.chromium.org/580283004/diff/1/test/transformer/entry_point...
test/transformer/entry_point_transformer_test.dart:43: //       
expect(dartSource, contains(
On 2014/09/19 22:47:10, Siggi Cherem (dart-lang) wrote:
> replace prints by test in comment?

Done.

https://codereview.chromium.org/580283004/diff/1/test/transformer/js_initiali...
File test/transformer/js_initializer_generator_test.dart (right):

https://codereview.chromium.org/580283004/diff/1/test/transformer/js_initiali...
test/transformer/js_initializer_generator_test.dart:61: //      expect(source,
contains('initializeJavaScriptLibrary()'));
On 2014/09/19 22:47:10, Siggi Cherem (dart-lang) wrote:
> same here?

Done.

[Siggi Cherem (dart-lang), 2014-09-22 22:42:58]

lgtm

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
File lib/src/transformer/entry_point_transformer.dart (right):

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/entry_po...
lib/src/transformer/entry_point_transformer.dart:72: for (Asset asset in
initializerAssets.where((a) => a != null)) {
On 2014/09/22 21:50:21, justinfagnani wrote:
> On 2014/09/19 22:47:10, Siggi Cherem (dart-lang) wrote:
> > I didn't realize this last time, it might be simpler/cheaper to just do:
> > 
> > for (Asset asset in initializerAssets) {
> >   if (asset == null) continue;
> >   ...
> 
> I like this style, it reads well to me.

we'll agree to disagree :) 

It feels noisy to me - I'd prefer the initializersAssets result to be filtered
already though.

https://codereview.chromium.org/580283004/diff/20001/lib/src/transformer/entr...
File lib/src/transformer/entry_point_transformer.dart (right):

https://codereview.chromium.org/580283004/diff/20001/lib/src/transformer/entr...
lib/src/transformer/entry_point_transformer.dart:70: var dartFuture =
Future.wait(dartInitializerFutures).
style nit: move . to the next line

[justinfagnani, 2014-09-23 01:29:53]

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/dart_ini...
File lib/src/transformer/dart_initializer_generator.dart (right):

https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/dart_ini...
lib/src/transformer/dart_initializer_generator.dart:70: JsObject lib =
parent['${library.name}'];
On 2014/09/19 12:38:59, alexandre.ardhuin wrote:
> JsObject should be prefixed by "js."

Done.

https://codereview.chromium.org/580283004/diff/20001/lib/src/transformer/entr...
File lib/src/transformer/entry_point_transformer.dart (right):

https://codereview.chromium.org/580283004/diff/20001/lib/src/transformer/entr...
lib/src/transformer/entry_point_transformer.dart:70: var dartFuture =
Future.wait(dartInitializerFutures).
On 2014/09/22 22:42:58, Siggi Cherem (dart-lang) wrote:
> style nit: move . to the next line

Done.

[justinfagnani, 2014-09-23 01:33:10]

On 2014/09/23 01:29:53, justinfagnani wrote:
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/dart_ini...
> File lib/src/transformer/dart_initializer_generator.dart (right):
> 
>
https://codereview.chromium.org/580283004/diff/1/lib/src/transformer/dart_ini...
> lib/src/transformer/dart_initializer_generator.dart:70: JsObject lib =
> parent['${library.name}'];
> On 2014/09/19 12:38:59, alexandre.ardhuin wrote:
> > JsObject should be prefixed by "js."
> 
> Done.
> 
>
https://codereview.chromium.org/580283004/diff/20001/lib/src/transformer/entr...
> File lib/src/transformer/entry_point_transformer.dart (right):
> 
>
https://codereview.chromium.org/580283004/diff/20001/lib/src/transformer/entr...
> lib/src/transformer/entry_point_transformer.dart:70: var dartFuture =
> Future.wait(dartInitializerFutures).
> On 2014/09/22 22:42:58, Siggi Cherem (dart-lang) wrote:
> > style nit: move . to the next line
> 
> Done.

committed in
https://github.com/dart-lang/js-interop/commit/456118f9835daf8cc0bc1f80456370...