dart2js: Use an abstract Name class for names in the generated JavaScript ast.

dart2js: Use an abstract Name class for names in the generated JavaScript ast.

BUG=
R=asgerf@google.com, floitsch@google.com, sigurdm@google.com, sra@google.com

Committed: https://github.com/dart-lang/sdk/commit/dd41983945b4ccbae5f4670c6235265444a16c31

[herhut, 2015-06-22 11:46:12]

herhut@google.com changed reviewers:
+ floitsch@google.com, sigurdm@google.com, sra@google.com

[herhut, 2015-06-22 11:47:14]

PTAL

sra: the js_ast package
sigurdm: the async translation
floitsch: rest?

[herhut, 2015-06-22 11:47:25]

Patchset #1 (id:1) has been deleted

[sigurdm, 2015-06-22 11:52:51]

Async-part: LGTM

[floitsch, 2015-06-22 17:43:45]

LGTM.

@Peter: could you please have a look at nsm_emitter.dart?

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:237: return
js.propertyCall(interceptorLibrary, js.string(selector.name),
why is this just a string?

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart:46: Selector
selector = new Selector.call("getInterceptor", null, 1);
There should be a better way to get the name of this function.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/frequency_namer.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/frequency_namer.dart:1: // Copyright (c) 2011,
the Dart project authors.  Please see the AUTHORS file
2015

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/frequency_namer.dart:9: FrequencyNamer(Compiler
compiler) : super(compiler);
indentation.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:567:
_proposeNameForConstructorBody(ConstructorBodyElement method) {
returns a String.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:588: int arity) {
indentation.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1285: // Why do we need to
disambiguate here?
Maybe I'm missing something, but I think you can have:

    // on top-level:
    var get$myglobal = 0;
    var myglobal = new Foo();

the get$myglobal will be disambiguated to "get$myglobal".
if we just went ahead and derived the getter-name from myglobal it would clash
with the other variable.

This wouldn't be the case for instance variables where we pay more attention.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1374: /// If [name] is a setter or
getter name, the corresponding [GetterName] or
That shouldn't be necessary for static names. (unless mirrors depend on it).

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1545: operator==(other) {
bool

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/runtime_types.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/runtime_types.dart:404: if (type.isDynamic)
return "dynamic";
too hard-coded.
With what parts of the compiler/runtime does this name need to agree with?

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:7: const USE_NEW_EMITTER
= true; //const bool.fromEnvironment("dart2js.use.new.emitter");
debug.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:330:
classBuilder.addPropertyByName('static', new jsAst.ObjectInitializer(statics));
long line.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/container_builder.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/container_builder.dart:111: //
TODO(sra): Don't use LiteralString for non-strings.
does this comment still apply?

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1068:
builder.addPropertyByName(embeddedNames.TYPEDEF_TYPE_PROPERTY_NAME, typeIndex);
long line.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1198: var keys =
mangledGlobalFieldNames.keys.toList()..sort();
I prefer it in a separate line.
Otherwise it's hard to see.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1198: var keys =
mangledGlobalFieldNames.keys.toList()..sort();
Please add types in this function.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/interceptor_emitter.dart
(right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/interceptor_emitter.dart:38:
List<jsAst.Name> names = specializedGetInterceptors.keys.toList()..sort();
I would prefer moving the sort into a separate line.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/interceptor_emitter.dart:54: =
backend.oneShotInterceptors.keys.toList()..sort();
I would prefer keeping the sort in a separate line.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/interceptor_emitter.dart:82:
interceptorInvocationNames.toList()..sort();;
ditto.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:36:
List<jsAst.Name> names = addedJsNames.keys.toList()..sort();
sort on a different line?

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:74: if
(backend.isInterceptedName(selector.name)) {
TBH I don't see why we do special casing for the intercepted names.
Maybe Peter remembers.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:157:
namer.invocationMirrorInternalName(selector));
indentation.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:283: /// of given
names.
Refer or copy the documentation from buildTrivialNsmHandler

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/nodes.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/nodes.dart:215: Compiler compiler) {
indentation.

[sra1, 2015-06-23 04:47:54]

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:782: return new SetterName(new
StringBackedName(setterPrefix),
The prefix should probably be cached.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1470: final int _kind = 1;
int get _kind => 1;

Otherwise you are still doing virtual dispatch to load a field.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1476: operator==(other) => (other is
StringBackedName) && other.name == name;
Is the purpose of  ==, hashCode and compareTo?
Are == and hashCode make a Name possible as a hash table key?

Is compareTo for a specific order or arbitrary order?

I think it must be an arbitrary but stable order since it is used to order the
assignments to getInterceptor specializations, which will happen before you can
do a frequency based name assignment.  You must assume that two names with
different derivations will have different final names.

Please document that compareTo is arbitrary but consistent.


The nsm delta table requires lexicographic ordering. This is is completely
different. This is the 'same fringe' problem which can be solved with a codeUnit
iterator.
At that point you might as well simply sort by full name.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1481: if (other is! _NamerName)
return -1;
Since you can't make the other implementation of Name honour this by returning
'1',
please make this an assertion or type assertion.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1490: int get _kind;
Somewhat redundant with _NamerName._kind.
Leave it here if you want.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:142: return
aName.compareTo(bName);
How does this work?
jsAst.Name does not implement Comparable or define compareTo.
Does the type need to be _NamerName?

For the delta table:

1. It is essential for the that the sort takes place after deferred names are
assigned.
2. short names need to precede longer names.
3. setters and getters need to be interleaved with regular names.

What we need is length-then-lexicographic ordering, not the crazy _NamerName
compareTo ordering.

You need to defer this sort until the string is generated. get the final name
and do the original comparison on the final name:

    if (aName.length != bName.length) return aName.length - bName.length;	
    return aName.compareTo(bName);

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:150:
firstNormalSelector = i;
You can find this without sorting by counting:

trivialNsmHandlers.where((e) => backend.isInterceptedName(e.name)).length;

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:167:
jsAst.joinLiterals(shorts, jsAst.stringPart(",")),
This also needs to be deferred, since the sort order depends on the deferred
names.

I think you need several deferred expressions which use the same object to do
the processing (sorting, returning concatenated strings etc).

Plus side is that we can just return a deferred string for the short names list
too.

https://codereview.chromium.org/1198293002/diff/40001/pkg/js_ast/lib/src/node...
File pkg/js_ast/lib/src/nodes.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/js_ast/lib/src/node...
pkg/js_ast/lib/src/nodes.dart:580: abstract class Name extends Literal
Please add a class comment.

[herhut, 2015-06-23 13:26:32]

sra@ PTAL

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:237: return
js.propertyCall(interceptorLibrary, js.string(selector.name),
On 2015/06/22 17:43:43, floitsch wrote:
> why is this just a string?

Because it is a broken abstraction. Selectors can only hold strings and the
cps_ir to tree converter puts the backend name of the interceptor to use into
the selector. However, selectors should only have front-end names in them.

asgerf@ was so nice to agree to fix the abstraction and then this will go away.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart:46: Selector
selector = new Selector.call("getInterceptor", null, 1);
On 2015/06/22 17:43:44, floitsch wrote:
> There should be a better way to get the name of this function.

You can ask the namer for a backend-name but that will not fit into a front-end
selector. This has to and will be fixed in the tree ir.

NB: This currently breaks tests, because "getInterceptor" is wrong :)

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/frequency_namer.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/frequency_namer.dart:1: // Copyright (c) 2011,
the Dart project authors.  Please see the AUTHORS file
On 2015/06/22 17:43:44, floitsch wrote:
> 2015

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/frequency_namer.dart:9: FrequencyNamer(Compiler
compiler) : super(compiler);
On 2015/06/22 17:43:44, floitsch wrote:
> indentation.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:567:
_proposeNameForConstructorBody(ConstructorBodyElement method) {
On 2015/06/22 17:43:44, floitsch wrote:
> returns a String.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:588: int arity) {
On 2015/06/22 17:43:44, floitsch wrote:
> indentation.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:782: return new SetterName(new
StringBackedName(setterPrefix),
On 2015/06/23 04:47:54, sra1 wrote:
> The prefix should probably be cached.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1285: // Why do we need to
disambiguate here?
On 2015/06/22 17:43:44, floitsch wrote:
> Maybe I'm missing something, but I think you can have:
> 
>     // on top-level:
>     var get$myglobal = 0;
>     var myglobal = new Foo();
> 
> the get$myglobal will be disambiguated to "get$myglobal".
> if we just went ahead and derived the getter-name from myglobal it would clash
> with the other variable.
> 
> This wouldn't be the case for instance variables where we pay more attention.

Good point. Fixed.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1374: /// If [name] is a setter or
getter name, the corresponding [GetterName] or
On 2015/06/22 17:43:44, floitsch wrote:
> That shouldn't be necessary for static names. (unless mirrors depend on it).

This is mostly used in testing (as otherwise one cannot test for the right names
to be emitted). By how == is defined, we have to construct 'getternames' in the
right way.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1470: final int _kind = 1;
On 2015/06/23 04:47:54, sra1 wrote:
> int get _kind => 1;
> 
> Otherwise you are still doing virtual dispatch to load a field.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1476: operator==(other) => (other is
StringBackedName) && other.name == name;
On 2015/06/23 04:47:54, sra1 wrote:
> Is the purpose of  ==, hashCode and compareTo?
> Are == and hashCode make a Name possible as a hash table key?

Yes. However, typically only instances of StingBackedName are used in HashMaps
and Sets, to the cost for CompoundName can be tolerated.

> Is compareTo for a specific order or arbitrary order?
> 
> I think it must be an arbitrary but stable order since it is used to order the
> assignments to getInterceptor specializations, which will happen before you
can
> do a frequency based name assignment.  You must assume that two names with
> different derivations will have different final names.

Yes, they are sorted in some consistent order. Which one does not really matter
in cases where it is about stability only. 

> Please document that compareTo is arbitrary but consistent.

Done in the js_ast package.

> The nsm delta table requires lexicographic ordering. This is is completely
> different. This is the 'same fringe' problem which can be solved with a
codeUnit
> iterator.
> At that point you might as well simply sort by full name.

While this namer could do full name sorting, a frequency based one cannot do it
on the name tokens. I wanted to have both behave the same way, so that I only
need to replace StringBackedName and keep all the rest as is.

In the nsm emitter, I now sort as it was before yet on the actual names when
printing.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1481: if (other is! _NamerName)
return -1;
On 2015/06/23 04:47:54, sra1 wrote:
> Since you can't make the other implementation of Name honour this by returning
> '1',
> please make this an assertion or type assertion.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1490: int get _kind;
On 2015/06/23 04:47:54, sra1 wrote:
> Somewhat redundant with _NamerName._kind.
> Leave it here if you want.

Removed.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/namer.dart:1545: operator==(other) {
On 2015/06/22 17:43:44, floitsch wrote:
> bool 

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/runtime_types.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/runtime_types.dart:404: if (type.isDynamic)
return "dynamic";
On 2015/06/22 17:43:44, floitsch wrote:
> too hard-coded.
> With what parts of the compiler/runtime does this name need to agree with?

None. This could be any string. It is only used to build up a suggestion for the
name of a type constant. So it needs to be somewhat human readable. No need to
pass this through the namer, as the namer will see the constructed name in the
end and possibly change that.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:7: const USE_NEW_EMITTER
= true; //const bool.fromEnvironment("dart2js.use.new.emitter");
On 2015/06/22 17:43:44, floitsch wrote:
> debug.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:330:
classBuilder.addPropertyByName('static', new jsAst.ObjectInitializer(statics));
On 2015/06/22 17:43:44, floitsch wrote:
> long line.

Thanks!

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/container_builder.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/container_builder.dart:111: //
TODO(sra): Don't use LiteralString for non-strings.
On 2015/06/22 17:43:44, floitsch wrote:
> does this comment still apply?

It did but to get rid of the TODO I have now replaced the LiteralString by a
LiteralNull.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1068:
builder.addPropertyByName(embeddedNames.TYPEDEF_TYPE_PROPERTY_NAME, typeIndex);
On 2015/06/22 17:43:44, floitsch wrote:
> long line.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1198: var keys =
mangledGlobalFieldNames.keys.toList()..sort();
On 2015/06/22 17:43:44, floitsch wrote:
> I prefer it in a separate line.
> Otherwise it's hard to see.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1198: var keys =
mangledGlobalFieldNames.keys.toList()..sort();
On 2015/06/22 17:43:44, floitsch wrote:
> Please add types in this function.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/interceptor_emitter.dart
(right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/interceptor_emitter.dart:38:
List<jsAst.Name> names = specializedGetInterceptors.keys.toList()..sort();
On 2015/06/22 17:43:44, floitsch wrote:
> I would prefer moving the sort into a separate line.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/interceptor_emitter.dart:54: =
backend.oneShotInterceptors.keys.toList()..sort();
On 2015/06/22 17:43:44, floitsch wrote:
> I would prefer keeping the sort in a separate line.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/interceptor_emitter.dart:82:
interceptorInvocationNames.toList()..sort();;
On 2015/06/22 17:43:44, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:36:
List<jsAst.Name> names = addedJsNames.keys.toList()..sort();
On 2015/06/22 17:43:45, floitsch wrote:
> sort on a different line?

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:74: if
(backend.isInterceptedName(selector.name)) {
On 2015/06/22 17:43:45, floitsch wrote:
> TBH I don't see why we do special casing for the intercepted names.
> Maybe Peter remembers.

This is probably an artifact. I checked the decode and I am sure it does the
right thing in both minified and non-minified mode.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:142: return
aName.compareTo(bName);
On 2015/06/23 04:47:54, sra1 wrote:
> How does this work?
> jsAst.Name does not implement Comparable or define compareTo.
> Does the type need to be _NamerName?
> 
> For the delta table:
> 
> 1. It is essential for the that the sort takes place after deferred names are
> assigned.
> 2. short names need to precede longer names.
> 3. setters and getters need to be interleaved with regular names.
> 
> What we need is length-then-lexicographic ordering, not the crazy _NamerName
> compareTo ordering.
> 
> You need to defer this sort until the string is generated. get the final name
> and do the original comparison on the final name:
> 
>     if (aName.length != bName.length) return aName.length - bName.length;	
>     return aName.compareTo(bName);
> 

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:150:
firstNormalSelector = i;
On 2015/06/23 04:47:54, sra1 wrote:
> You can find this without sorting by counting:
> 
> trivialNsmHandlers.where((e) => backend.isInterceptedName(e.name)).length;

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:157:
namer.invocationMirrorInternalName(selector));
On 2015/06/22 17:43:45, floitsch wrote:
> indentation.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:167:
jsAst.joinLiterals(shorts, jsAst.stringPart(",")),
On 2015/06/23 04:47:54, sra1 wrote:
> This also needs to be deferred, since the sort order depends on the deferred
> names.
> 
> I think you need several deferred expressions which use the same object to do
> the processing (sorting, returning concatenated strings etc).
> 
> Plus side is that we can just return a deferred string for the short names
list
> too.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:283: /// of given
names.
On 2015/06/22 17:43:45, floitsch wrote:
> Refer or copy the documentation from buildTrivialNsmHandler

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/nodes.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/nodes.dart:215: Compiler compiler) {
On 2015/06/22 17:43:45, floitsch wrote:
> indentation.

Done.

https://codereview.chromium.org/1198293002/diff/40001/pkg/js_ast/lib/src/node...
File pkg/js_ast/lib/src/nodes.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/js_ast/lib/src/node...
pkg/js_ast/lib/src/nodes.dart:580: abstract class Name extends Literal
On 2015/06/23 04:47:54, sra1 wrote:
> Please add a class comment.

Done.

[asgerf, 2015-06-23 13:34:53]

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

[asgerf, 2015-06-23 13:34:54]

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/1198293002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:237: return
js.propertyCall(interceptorLibrary, js.string(selector.name),
On 2015/06/23 13:26:30, herhut wrote:
> On 2015/06/22 17:43:43, floitsch wrote:
> > why is this just a string?
> 
> Because it is a broken abstraction. Selectors can only hold strings and the
> cps_ir to tree converter puts the backend name of the interceptor to use into
> the selector. However, selectors should only have front-end names in them.
> 
> asgerf@ was so nice to agree to fix the abstraction and then this will go
away.

Yeah that was broken. This CL should fix it: 
https://codereview.chromium.org/1204733002/

[floitsch, 2015-06-23 14:59:33]

https://codereview.chromium.org/1198293002/diff/60001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right):

https://codereview.chromium.org/1198293002/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:150: JsBuiltin.isGivenTypeRti,
Actually you might as well use rawRtiToJsConstructorName from above and write ==
yourself. Removes one builtin.

(Please add the necessary "effects:none;depends:none" in the rawRtiTo...
function)

[sra1, 2015-06-23 20:54:16]

lgtm

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart (right):

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:161:
diffEncodedString = sequence[0];
Move under 'if (sequence.length)'.
If sequence can be empty, this might be a deopt.

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:168:
objectClassObject = objectClassObject[1];
This should probably be hoisted out of the j loop.

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:184:
codes.unshift(${$HASH} + remaining % 88);
I'm curious of you noticed this showing up in startup profiling?
In one big app, a 13k string encodes 8k stubs.

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:217: : [js('(j < #)
? 1 : 0', js.number(specialSelectors.length))]);
I fixed a bug in this code, sorry that you will have to merge.
(https://chromiumcodereview.appspot.com/1181063005/)

[herhut, 2015-06-24 07:55:07]

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart (right):

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:161:
diffEncodedString = sequence[0];
On 2015/06/23 20:54:16, sra1 wrote:
> Move under 'if (sequence.length)'.
> If sequence can be empty, this might be a deopt.

Done.

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:168:
objectClassObject = objectClassObject[1];
On 2015/06/23 20:54:16, sra1 wrote:
> This should probably be hoisted out of the j loop.

Done.

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:184:
codes.unshift(${$HASH} + remaining % 88);
On 2015/06/23 20:54:16, sra1 wrote:
> I'm curious of you noticed this showing up in startup profiling?
> In one big app, a 13k string encodes 8k stubs.

No, I was not even aware this existed before working on the namer change.

https://codereview.chromium.org/1198293002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:217: : [js('(j < #)
? 1 : 0', js.number(specialSelectors.length))]);
On 2015/06/23 20:54:16, sra1 wrote:
> I fixed a bug in this code, sorry that you will have to merge.
>
(https://chromiumcodereview.appspot.com/1181063005/?_ga=1.83535574.1389110575....)

Acknowledged.

[herhut, 2015-06-24 14:11:22]

sigurdm@ PTAL patchset 6

[herhut, 2015-06-24 14:11:56]

On 2015/06/24 14:11:22, herhut wrote:
> sigurdm@ PTAL patchset 6

make that patchset 5...

[sigurdm, 2015-06-24 14:35:21]

Patch 5 async-related parts LGTM.
Thanks for the fix!

[sigurdm, 2015-06-24 14:35:24]

Patch 5 async-related parts LGTM.
Thanks for the fix!

[herhut, 2015-06-24 15:49:05]

asgerf@ PTAL the cps_ir changes due to merging. In essence I need to construct
the constant in a different way but that ties it to the JS backend.

[asgerf, 2015-06-24 15:59:55]

CPS builder and tree codegen LGTM.

https://codereview.chromium.org/1198293002/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/1198293002/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:211: arguments);
Don't worry about this code, I forgot to remove it in the CL that introduced
interceptors to the tree IR (visitInterceptor handles this now). I will remove
it.

[herhut, 2015-06-25 07:40:03]

Committed patchset #7 (id:140001) manually as
dd41983945b4ccbae5f4670c6235265444a16c31 (presubmit successful).