JS templates

JS templates

This CL mostly removes the 'fluent' API and replaces it with templates.

The JS code is parsed and converted (compiled) into an instantiation function.
The instantation function can be reused to stamp out similar copies.

This has made the large helper functions more readable since they are now mostly real JavaScript code.


Notes:

'if (#) stmt' adds a conditional statement if the argument is a Dart
bool.  It constructs the tree if the argument is a JS Expression.

Fixed the statement template arguments so that '#;' is required.

There is a template manager that reuses templates with the same source.
This is persistent between compilation since the strings should be all 'constant'.

I am currently hunting down the strings parsed by 'js(string)'.
I have reduced it from 18k to O(500) for vampire.
The blocker is the runtime type parameters system - it generates an AST, prints it, and reparses it, giving an unbounded number of small templates, mostly with no parameters.  So these templates are not cached and pay the price of repeated compilation.

R=floitsch@google.com

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

[sra1, 2014-04-22 04:09:21]

I suggest starting with the documentation in js/builder.dart
and the test file.

[floitsch, 2014-04-22 13:07:49]

On 2014/04/22 04:09:21, sra1 wrote:
> I suggest starting with the documentation in js/builder.dart
> and the test file.

test file is missing.

[sra1, 2014-04-22 14:45:28]

On 2014/04/22 13:07:49, floitsch wrote:
> On 2014/04/22 04:09:21, sra1 wrote:
> > I suggest starting with the documentation in js/builder.dart
> > and the test file.
> 
> test file is missing.

Sorry about that.  I renamed it and forgot to add it.

[floitsch, 2014-04-22 16:11:17]

LGTM.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js/builder.dart (right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:16: * TODO(sra): Find
the remaining places where js('xxx') used to parse an
I'm currently pushing to remove all statics from the system.
If possible we should make this an instance variable.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:31: var vFoo = new
VariableUse('foo')  --> foo
what does `new VariableUse` have to do with `js`?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:36: The [call] method
constructs an Expression AST.  Since the builder is bound to
Not sure I understand the second sentence. It's just saying that js ==
JsBuilder, right? But that's the first sentence of this whole dartdoc.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:43: The input text can
contain placeholders `#` that are replace with provided
replaced

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:172: template should
probably be writted with `[]` if non-strings are accepted.)
I would say yes. Let's not allow non-string arguments.

I'm not even sure I like "a.#", but I haven't yet seen the use-cases.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:175: Object
initialiers allow placeholders in the key posiition:
initializers position

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:180: TODO: Array
initializers and object initializers could support splicing.  In
TODO(sra) or TODO(issue-number)

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:184: TODO: There are
no placeholders in definition contexts:
ditto.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:190: const JsBuilder
js = const JsBuilder();
In order to get rid of the static state we could remove the global "js" and make
it a member of all the classes that need it.
That is: js-backend would create a new JsBuilder (that contains state for the
template canonicalization). Other classes could then have a js getter that
redirects to backend.js.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:190: const JsBuilder
js = const JsBuilder();
On the other hand I can see how this makes repeated compilations more
efficient...

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:228: // functions, as
this leads to unanticipated capture of temporaries that can
-can-

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:251: Statement
expression = parser.statement();
s/expression/statement

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:310: //Expression
toExpression(expression) {
dead code?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:380: /// * some
statements are still missing (do-while, while, switch.
missing closing parenthesis.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:467: //  // DEPRECATED.
dead?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:481: //Call
call([expression]) {
dead?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js/printer.dart (right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/printer.dart:1115: // TODO(sra):
Introduce a way for indicating in the JS text which varaibles
variables

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js/template.dart (right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/template.dart:22: /*
debug code.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/template.dart:38: /*
ditto.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/template.dart:48: */
missing doc.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/template.dart:108: *
InstantiatorGeneratorVisitor compiles a tree containing [InterpolatedNode]s
This class compiles ...

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/template.dart:166:
assert(identiferRE.hasMatch(value));
remove assert.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/template.dart:325: if (value is
Expression) return value;
So we only allow "#" to be a bool or an Expression, but not for example an
identifier?

js('if (#) return foo', 'bar') ?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/template.dart:444:
List<Instantiator> declarationMakers = node.declarations.map(visit).toList();
long line.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/template.dart:610: throw 'Should
not get here'; // Handled in visitArrayInitializer.
make this an internal error.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart
(right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:420:
//statements.add(new jsAst.Return(receiver[target](arguments)));
dead code.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:484:
$defPropName(Object.prototype, key, table[key]);
We could make defPropAccess a template, and thus avoid a template that depends
on string-interpolation.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:639:
//return js(namer.isolateAccess(backend.getImplementationClass(element)));
dead code.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:875:
parameters.toList().map(declaration).toList();
toList is necessary?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart
(right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:76:
js('function(#) { #; }',
not correctly indented.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:77:
[fields, fields.map((name) => js('this.# = #', [name, name]))])));
split arguments and body in separate lines.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:78: //
TODO(sra): Implement placeholders in VariableDeclaration position:
Move TODO before the 'new'.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:88:
js.statement('''{
make raw string?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:158:
if (fieldInformation.length > 1) reflectable = true;
var reflectable = fieldInformation.length > 1;

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:311:
if (typeof dart_precompiled == "function") {
Not this CL, but I think this is a compile-time constant too ("are we in csp
mode?").

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:604:
return [js('# = #', [js(lazyInitializerName), lazyInitializerFunction])];
lazyInitializerName is a string. Do you have to run it through "js" first?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:736:
return js.statement('''
This should not be cached.
Write TODO, that this should only be cached when declaration names are allowed
with #.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:862:
[js(lazyInitializerName),
lazyInitializerName is a String. Why do we need to run it through "js" ?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:272:
//return js('this')[getterName](<jsAst.Expression>[receiver]);
dead code.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:275:
//return receiver[getterName](<jsAst.Expression>[]);
ditto.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart
(right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart:95: return
js(r'function($receiver, #) { return # }', [parameterNames, expression]);
long line.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart:284: // If
we are loading a deferred library the object class will not be in
long line.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3332: canThrow:
true));
Unless I'm missing something, this is an optimization.
If yes, I would prefer to do this change in a separate CL.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1652:
push(js.js('#.prototype.#.call(#)', [
can we import with show 'js', so that we don't have these "js.js" ?

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1728: //if
(!inputs.isEmpty) {
dead code.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/lib/i...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/lib/i...
sdk/lib/_internal/lib/isolate_helper.dart:229: "(function (f, a) { return
function (e) { f(a, e); }})(#, #)",
I would prefer this in a separate CL.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/lib/i...
sdk/lib/_internal/lib/isolate_helper.dart:956: JS('', "(function (f, a) { return
function (e) { f(a, e); }})(#, #)",
ditto.

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/lib/j...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/237583014/diff/230001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:1137: var body = "(function
$className\$$name(o){return i.$name(o)})";
dead code.

https://codereview.chromium.org/237583014/diff/250001/tests/compiler/dart2js/...
File tests/compiler/dart2js/js_parser_statements_test.dart (right):

https://codereview.chromium.org/237583014/diff/250001/tests/compiler/dart2js/...
tests/compiler/dart2js/js_parser_statements_test.dart:18:
Expect.stringEquals(expect.trim(), jsText.trim());
for simplicity I would just remove all whitespace for both sides.
"for (var x in foo)" -> "for(varxinfoo)"

But I have to admit that the error-messages are not as nice then.

https://codereview.chromium.org/237583014/diff/250001/tests/compiler/dart2js/...
tests/compiler/dart2js/js_parser_statements_test.dart:116: // Maybe we should
make this work?
I don't think it will be necessary. but we will see.

[sra1, 2014-04-23 02:33:49]

Thanks for the review.
Now I try to land it :-)

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js/builder.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:16: * TODO(sra): Find
the remaining places where js('xxx') used to parse an
On 2014/04/22 16:11:18, floitsch wrote:
> I'm currently pushing to remove all statics from the system.
> If possible we should make this an instance variable.

I'm not sure how to find it without making the templates tedious to use.
They are reasonably staged.  In batch mode (and pub) on the second compile most
JS parsing will have been done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:31: var vFoo = new
VariableUse('foo')  --> foo
On 2014/04/22 16:11:18, floitsch wrote:
> what does `new VariableUse` have to do with `js`?

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:36: The [call] method
constructs an Expression AST.  Since the builder is bound to
On 2014/04/22 16:11:18, floitsch wrote:
> Not sure I understand the second sentence. It's just saying that js ==
> JsBuilder, right? But that's the first sentence of this whole dartdoc.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:43: The input text can
contain placeholders `#` that are replace with provided
On 2014/04/22 16:11:18, floitsch wrote:
> replaced

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:172: template should
probably be writted with `[]` if non-strings are accepted.)
On 2014/04/22 16:11:18, floitsch wrote:
> I would say yes. Let's not allow non-string arguments.
> 
> I'm not even sure I like "a.#", but I haven't yet seen the use-cases.

Presumably you saw that a.# was used extensively.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:175: Object
initialiers allow placeholders in the key posiition:
On 2014/04/22 16:11:18, floitsch wrote:
> initializers position

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:180: TODO: Array
initializers and object initializers could support splicing.  In
On 2014/04/22 16:11:18, floitsch wrote:
> TODO(sra) or TODO(issue-number)

I rephased this since I'm not sure it is a TODO.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:184: TODO: There are
no placeholders in definition contexts:
On 2014/04/22 16:11:18, floitsch wrote:
> ditto.

I rephased this since I'm not sure it is a TODO.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:190: const JsBuilder
js = const JsBuilder();
On 2014/04/22 16:11:18, floitsch wrote:
> On the other hand I can see how this makes repeated compilations more
> efficient...

That is part of the plan :-)

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:190: const JsBuilder
js = const JsBuilder();
On 2014/04/22 16:11:18, floitsch wrote:
> In order to get rid of the static state we could remove the global "js" and
make
> it a member of all the classes that need it.
> That is: js-backend would create a new JsBuilder (that contains state for the
> template canonicalization). Other classes could then have a js getter that
> redirects to backend.js.

Or we could make the state independent of any compilation.
If we do have slow leaks we could put in an LRU-like cache.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:228: // functions, as
this leads to unanticipated capture of temporaries that can
On 2014/04/22 16:11:18, floitsch wrote:
> -can-

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:251: Statement
expression = parser.statement();
On 2014/04/22 16:11:18, floitsch wrote:
> s/expression/statement

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:310: //Expression
toExpression(expression) {
On 2014/04/22 16:11:18, floitsch wrote:
> dead code?

Yes.  It was the only one that was slightly painful to remove, so I would not
mind too much if something like it came back to convert Dart mixed element-type
lists into JS expression.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/builder.dart:380: /// * some
statements are still missing (do-while, while, switch.
On 2014/04/22 16:11:18, floitsch wrote:
> missing closing parenthesis.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:467: //  // DEPRECATED.
On 2014/04/22 16:11:18, floitsch wrote:
> dead?

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:481: //Call
call([expression]) {
On 2014/04/22 16:11:18, floitsch wrote:
> dead?

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js/printer.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/printer.dart:1115: // TODO(sra):
Introduce a way for indicating in the JS text which varaibles
On 2014/04/22 16:11:18, floitsch wrote:
> variables

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js/template.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:22: /*
On 2014/04/22 16:11:18, floitsch wrote:
> debug code.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:38: /*
On 2014/04/22 16:11:18, floitsch wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:38: /*
On 2014/04/22 16:11:18, floitsch wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:48: */
On 2014/04/22 16:11:18, floitsch wrote:
> missing doc.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:108: *
InstantiatorGeneratorVisitor compiles a tree containing [InterpolatedNode]s
On 2014/04/22 16:11:18, floitsch wrote:
> This class compiles ...

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:166:
assert(identiferRE.hasMatch(value));
On 2014/04/22 16:11:18, floitsch wrote:
> remove assert.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:325: if (value is
Expression) return value;
On 2014/04/22 16:11:18, floitsch wrote:
> So we only allow "#" to be a bool or an Expression, but not for example an
> identifier?
> 
> js('if (#) return foo', 'bar') ?

That has been fixed while you were reviewing (and added to tests)

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:610: throw 'Should
not get here'; // Handled in visitArrayInitializer.
On 2014/04/22 16:11:18, floitsch wrote:
> make this an internal error.

Is there a way to signal an internal error without having 'compiler' in scope?

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart
(right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:420:
//statements.add(new jsAst.Return(receiver[target](arguments)));
On 2014/04/22 16:11:18, floitsch wrote:
> dead code.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:484:
$defPropName(Object.prototype, key, table[key]);
On 2014/04/22 16:11:18, floitsch wrote:
> We could make defPropAccess a template, and thus avoid a template that depends
> on string-interpolation.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:639:
//return js(namer.isolateAccess(backend.getImplementationClass(element)));
On 2014/04/22 16:11:18, floitsch wrote:
> dead code.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:875:
parameters.toList().map(declaration).toList();
On 2014/04/22 16:11:18, floitsch wrote:
> toList is necessary?

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart
(right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:76:
js('function(#) { #; }',
On 2014/04/22 16:11:18, floitsch wrote:
> not correctly indented.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:77:
[fields, fields.map((name) => js('this.# = #', [name, name]))])));
On 2014/04/22 16:11:18, floitsch wrote:
> split arguments and body in separate lines.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:78: //
TODO(sra): Implement placeholders in VariableDeclaration position:
On 2014/04/22 16:11:18, floitsch wrote:
> Move TODO before the 'new'.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:88:
js.statement('''{
On 2014/04/22 16:11:18, floitsch wrote:
> make raw string?

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:158:
if (fieldInformation.length > 1) reflectable = true;
On 2014/04/22 16:11:18, floitsch wrote:
> var reflectable = fieldInformation.length > 1;

I agree, but the process for this change it to change the generated output as
little as possible.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:311:
if (typeof dart_precompiled == "function") {
On 2014/04/22 16:11:18, floitsch wrote:
> Not this CL, but I think this is a compile-time constant too ("are we in csp
> mode?").

I'm not sure because the csp stuff is appended.
It would certainly be nice to get rid of it on none-csp programs.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:604:
return [js('# = #', [js(lazyInitializerName), lazyInitializerFunction])];
On 2014/04/22 16:11:18, floitsch wrote:
> lazyInitializerName is a string. Do you have to run it through "js" first?

It is a String, but it is not an identifier: "Isolate.$lazy"
We should probably replace lazyInitializerName with lazyInitializerFunction (an
Expression).

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:736:
return js.statement('''
On 2014/04/22 16:11:18, floitsch wrote:
> This should not be cached.
> Write TODO, that this should only be cached when declaration names are allowed
> with #.

The name is dart_precompiled, from the previous line :-)
I'll substitute to make it obvious, since it is written out in other places.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:862:
[js(lazyInitializerName),
On 2014/04/22 16:11:18, floitsch wrote:
> lazyInitializerName is a String. Why do we need to run it through "js" ?

It is "Isolate.$lazy"

Only strings in the form of an identifier are allowed.  We don't want the
template substitution to have to decide if a complex expression should be
cached.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:272:
//return js('this')[getterName](<jsAst.Expression>[receiver]);
On 2014/04/22 16:11:18, floitsch wrote:
> dead code.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:275:
//return receiver[getterName](<jsAst.Expression>[]);
On 2014/04/22 16:11:18, floitsch wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart
(right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart:95: return
js(r'function($receiver, #) { return # }', [parameterNames, expression]);
On 2014/04/22 16:11:18, floitsch wrote:
> long line.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3332: canThrow:
true));
On 2014/04/22 16:11:18, floitsch wrote:
> Unless I'm missing something, this is an optimization.
> If yes, I would prefer to do this change in a separate CL.

It is not an optimization.  It is getting JS('','throw xxx') to be correctly
identified as a JS statement so it is not emitted as a subexpression.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1652:
push(js.js('#.prototype.#.call(#)', [
On 2014/04/22 16:11:18, floitsch wrote:
> can we import with show 'js', so that we don't have these "js.js" ?

Then we would have to change the current import name 'js' to something else. 
I'll think about it for another CL.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1728: //if
(!inputs.isEmpty) {
On 2014/04/22 16:11:18, floitsch wrote:
> dead code.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/lib/js_mirrors.dart:1137: var body = "(function
$className\$$name(o){return i.$name(o)})";
On 2014/04/22 16:11:18, floitsch wrote:
> dead code.

Done.

https://chromiumcodereview.appspot.com/237583014/diff/250001/tests/compiler/d...
File tests/compiler/dart2js/js_parser_statements_test.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/250001/tests/compiler/d...
tests/compiler/dart2js/js_parser_statements_test.dart:18:
Expect.stringEquals(expect.trim(), jsText.trim());
On 2014/04/22 16:11:18, floitsch wrote:
> for simplicity I would just remove all whitespace for both sides.
> "for (var x in foo)" -> "for(varxinfoo)"
> 
> But I have to admit that the error-messages are not as nice then.

I will leave it alone for now, but replacing multiple whitespaces with space
characters on both would be reasonably pleasant.

https://chromiumcodereview.appspot.com/237583014/diff/250001/tests/compiler/d...
tests/compiler/dart2js/js_parser_statements_test.dart:116: // Maybe we should
make this work?
On 2014/04/22 16:11:18, floitsch wrote:
> I don't think it will be necessary. but we will see.

I agree. It was not necessary.

[sra1, 2014-04-23 03:33:15]

Committed patchset #6 manually as r35296 (presubmit successful).

[floitsch, 2014-04-23 07:29:41]

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js/template.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:610: throw 'Should
not get here'; // Handled in visitArrayInitializer.
On 2014/04/23 02:33:50, sra1 wrote:
> On 2014/04/22 16:11:18, floitsch wrote:
> > make this an internal error.
> 
> Is there a way to signal an internal error without having 'compiler' in scope?

Apparently not :(

If you have a location you could throw a new SpannableAssertionFailure.

[ahe, 2014-04-23 08:38:14]

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/implementation/js/template.dart (right):

https://chromiumcodereview.appspot.com/237583014/diff/230001/sdk/lib/_interna...
sdk/lib/_internal/compiler/implementation/js/template.dart:610: throw 'Should
not get here'; // Handled in visitArrayInitializer.
On 2014/04/23 07:29:41, floitsch wrote:
> On 2014/04/23 02:33:50, sra1 wrote:
> > On 2014/04/22 16:11:18, floitsch wrote:
> > > make this an internal error.
> > 
> > Is there a way to signal an internal error without having 'compiler' in
scope?
> 
> Apparently not :(
> 
> If you have a location you could throw a new SpannableAssertionFailure.

If you don't have a location, just throw an exception.

[kevmoo, 2014-05-12 04:19:33]

DBC

https://codereview.chromium.org/237583014/diff/210001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js/builder.dart (right):

https://codereview.chromium.org/237583014/diff/210001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:201: Expression
call(String source, [var arguments]) {
var is unneeded here

https://codereview.chromium.org/237583014/diff/210001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:207: Statement
statement(String source, [var arguments]) {
var is unneeded here

https://codereview.chromium.org/237583014/diff/210001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js/template.dart (right):

https://codereview.chromium.org/237583014/diff/210001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/template.dart:33: var n =
((statementTemplates.length == prev) ? '-' : '${statementTemplates.length}')
long line