dart2js: Refactoring, documentation, and a few bugfixes in Namer class.

dart2js: Refactoring, documentation, and a few bugfixes in Namer class.

See long doc comment in namer.dart.

BUG=
R=floitsch@google.com

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

Reverted: https://code.google.com/p/dart/source/detail?r=43590
(to avoid rebase conflicts for people pushing to trunk)

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

Reverted: https://code.google.com/p/dart/source/detail?r=43739
(red bots)

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

[asgerf, 2015-01-30 15:20:28]

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

[asgerf, 2015-01-30 15:20:29]

https://codereview.chromium.org/891673003/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/minify_namer.dart (right):

https://codereview.chromium.org/891673003/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/minify_namer.dart:52: if
(!_hasBannedPrefix(name)) return name;
I believe this was a bug, although I admit I was never able to reproduce it on
master.

https://codereview.chromium.org/891673003/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/minify_namer.dart:154: } while
(used.contains(letter) || _hasBannedPrefix(letter));
Same as above, it can clash with the getter for the 'call' property.

I believe this was also a bug. It caused problems on my branch, but again, for
some reason I could not produce it on master.

https://codereview.chromium.org/891673003/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/namer.dart (left):

https://codereview.chromium.org/891673003/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/namer.dart:427: if (name ==
closureInvocationSelectorName) return '$callPrefix\$$arity';
The closureInvocationSelectorName logic has been isolated in disambiguateMember
(line 708).

[floitsch, 2015-01-30 21:03:38]

LGTM.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:741: String name =
namer.getInterceptorName(classes);
We need to change this name.
nameForGetInterceptorFunction ?

otherwise it reads way too much like we are getting something.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/minify_namer.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:34: {bool ensureSafe, bool
sanitizeForAnnotations}) {
assert that ensureSafe and sanitizeForAnnotations are not null ?

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:69: * 0. The disambiguated name can
itself be seen as a annotated name.
as an annotated name.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:81: *    hardcoded into other
components, such as [Element] and [Selector].
:(

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:333: * Map from top-level or static
elements to their unique identifiers provided
This comment is on the wrong field.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:334: * by [getName].
`getName` doesn't exist anymore. Was replaced with `getNameX` which you just
removed as well...

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:344: final Map<String, String>
internGlobals;
add comment.
internGlobals -> internalGlobals?

Many other fields have "Names" as suffix. Not this one...
Unify?

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:345: final Map<String, String>
suggestedGlobalNames;
I would group the fields:

globals
usedGlobalNames
suggestedGlobalNames

instanceMembers
usedInstanceNames
suggestedInstanceNames

Add comments to all fields that are not completely obvious.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:346: final Map<LibraryElement,
Map<String, String>> instanceMembers;
Add a comment explaining that `null` is used as key for public members.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:348: final Map<Element, String>
directAccessMap = <Element, String>{};
comment.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:362: : compiler = compiler,
nit: Namer(this.compiler);

You could move all assignments (except the constantHasher and functionTypeNamer)
to the field-declarations. Since they would almost never fit on one line anyway,
I'm ok with leaving them here.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:408: /// Unique within the
global-member namespace.
Is that necessary?
After all, we always put constants on the C object where they don't conflict
with anything else.

If in doubt, just leave a TODO.
See also comment in disambiguateGlobal.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:425: String
constantLongName(ConstantValue constant) {
This looks like a function that shouldn't be called from the outside. Make it
private?

Same for all other private methods.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:480: return
'_$libraryName\$${originalName}';
Add comment why this is not unique.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:494: /// and [arity] and no named
parameters.
Maybe mention that it handles operators?

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:511: // TODO(asgerf): Avoid clash
when named parameters contain '$' symbols.
Create a failing test.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:541: default: throw 'Unexpected
selector kind: ${selector.kind}';
throw an internal error. (we tend to avoid throwing strings).

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:546: * Returns the internal name used
for an invocation mirror of this selector.
What's an "internal" name?

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:573: * variable.
Also mention that this is inside a class and thus at the same level as instance
members. (if I'm right).

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:575: String
readTypeVariableName(TypeVariableElement element) {
Why is this prefixed "read" ?

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:595: return element.fixedBackendName;
Does this potentially lead to clashes?

What if there is another member that ends up being assigned the
fixedBackendName.

If it's not possible, please comment (and/or assert).

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:604: () =>
'${element.enclosingClass.name}_${element.name}');
Is this enough?
What if I have a mixin class A_b with field c and then another mixin class A
with field "b_c", and both have their fields shadowed by a class that mixes in A
and A_b.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:607: // No inheritance-related name
clashes exist, so the accessor and property
I don't understand this comment.
Do you mean: no inheritance related clashes exist, so accessors may use the same
naming algorithm as instance members?

If yes, why is that the case?

From what I can tell we can still have a subclass that overrides the field's
name with a getter or method. I'm not sure if that's an issue or not...

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:613: bool
shadowingAnotherField(Element element) {
isShadowingASuperField ?

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:640: String getterForElement(Element
element) {
nit: either put getter and setter next to each other, or getterForElement and
deriveGetterName (in the same order). That is:

getterForElement
setterForElement
deriveGetterName
deriveSetterName

or
getterForElement
deriveGetterName
setterForElement
deriveSetterName

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:659: String
disambiguateInternGlobal(String name, {bool ensureSafe: true}) {
Comment what "ensureSafe" means.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:659: String
disambiguateInternGlobal(String name, {bool ensureSafe: true}) {
disambiguateInternalGlobal ?
disambiguateCompilerGlobal ?
disambiguateSynthesizedGlobal ?

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:674: /// identifying the global. The
[name] should not contain `$` symbols, since
Make this a "must" and assert it.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:678: /// globals, although the
callers of must still take care not to accidentally
-of-

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:680: String internGlobal(String name,
{bool ensureSafe: true}) {
internGlobal -> internalGlobal ?

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:686: /// The resulting name is unique
within the global-member namespace.
Same as for the constant: we might be able to only disambiguate within the
element's holder.

I know that classes need to be unique (for going from type to constructor) but
I'm not sure anything else needs to. (Unless, of course, reflection destroys
everything...)

Also note: in the new emitter we are considering removing the global objects.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:710: if (originalName ==
closureInvocationSelectorName) {
assert that library is not null if the originalName is not public.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:737: String
disambiguateDirectAccess(Element element, String proposeName()) {
Explain how this is different from disambiguateMember.
Basically: two elements could have the same name (even private name) and still
get different disambiguated names here, whereas disambiguateMember only looks at
the name.
Maybe best explained with an example.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:794: int i = counter == null ? 0 :
counter;
(counter == null)

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:811: /// cannot clash with the getter
for `x`.
Add '_isAnnotated' method and assert that every annotated method satisfies it,
and the result of this function doesn't.

You could then also add:
String input = name;
...
assert(_isAnnotated(input) || input == name);
return name;

This would make sure that we don't strip too much.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:916: /// Property name used for
`getInterceptor` or one of its specializations.
as written in another file:
"nameOfGetInterceptorFunctionFor" (or similar).

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:949: if (selector.isGetter ||
selector.isSetter) root = '$root\$';
Why does a getter and setter need the "$" ?
The old comment explained the reasoning, although it didn't make it clear why
operators and methods couldn't clash.
I would have interpreted this as 'all other internal globals, except getters and
setters, contain a "$"', but that doesn't seem to be true.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:965: /// from the type name.
Yeah for weird arbitrary rules...

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:989: /// without clashing with
another JS property name.
Might help to give a small example when this is necessary:

foo(x, [y]) has a stub method foo$1(x) that redirects to this.foo$2(x, null);

However, if the method is overridden by another foo, method we must make sure
that we reach the right `foo$2` method. By giving it an alias we can redirect to
`aliased$2(x, null)`.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1001: /// may share its name with an
instance method.
But is it unique within their respective categories?
Is the returned name a proposed name or a disambiguated one?

(it looks like it's the disambiguated one, but add comment).

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1002: String getNameOfMethod(Element
method) {
Why is this prefixed with "get"?

Most of the other methods in this class have: "methodName" or similar naming
conventions. Some drop the "name" suffix (although it feels inconsistent).

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1043: return
disambiguateInternGlobal("$getterPrefix${globalPropertyName(element)}");
long line.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1108: * and also ensures it won't
clash with other identifiers.
with other reserved identifiers (like "__proto__").

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1119: // TODO(ahe): Creating a string
here is unfortunate. It is slow (due to
I don't know what Peter means.
The cost in string concatenation for substitutions should be noise.
I don't see what "segmentation of '$'" means.

Unless you think there is a reason we should keep the TODO, please delete it.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:50: String name =
namer.getterForMember(null, Compiler.CALL_OPERATOR_NAME);
maybe add a namer.getterForPublicMember(...)
Otherwise it's hard to know what the `null` at the call-sites are.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:163: /// All the global
state can be passed around with this variable.
Please update comment:
Contains the global state that is needed to initialize and load a deferred
library.

[sra1, 2015-01-30 22:48:26]

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

[sra1, 2015-01-30 22:48:27]

DBC

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/minify_namer.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:52: if
(!_hasBannedPrefix(name)) return name;
Where are 'g' and 's' defined?
Is is 'accidental' that these names correspond to the getter/setter prefix?

Are you sure this is a problem - AFAIK this function names the fields of a scope
'box' which can never be treated as an object, so there should be no collision.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:56: return '${basename}_$id';
So will we get something like 'xy_6' where previously we got 'g'?

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:154: } while
(used.contains(letter) || _hasBannedPrefix(letter));
Was this a problem?  How did it trigger?
It seems a shame to lose 'g' just because 'gX' means something.  It is probably
worthwhile strengthening the prefix check in runtime code to permit some common
method to be minified to 'g'.

I don't think 'g' a problem for a global name.

https://codereview.chromium.org/891673003/diff/20001/tests/compiler/dart2js/p...
File tests/compiler/dart2js/pretty_parameter_test.dart (right):

https://codereview.chromium.org/891673003/diff/20001/tests/compiler/dart2js/p...
tests/compiler/dart2js/pretty_parameter_test.dart:82:
Expect.isTrue(generated.contains(r"function($eval, $eval0) {"));
Can we do something to keep the original names where possible?
(eval, $eval)
-->
($eval, $eval0)
This is confusing when you have to step through the generated code.

[asgerf, 2015-02-03 17:39:14]

Thanks for the great reviews!

PTAL as there are major changes. 

Sorry about the mega patch, btw.
I'll try to summarize the changes here:

- Methods other than call were disambiguated before adding the suffix, so
minification did not work well. They are now disambiguated and minified
properly.

- Try to retain the names that the previous version of the namer would produce.
This helps measure potential regression since it stabilizes the diff between
versions.

- Some methods and fields were renamed to follow a more consistent naming
convention.

- Avoid clash with native fields that have fixed backend names.

- Avoid clash between named parameters.

- Various changes in response to comments.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:741: String name =
namer.getInterceptorName(classes);
On 2015/01/30 21:03:36, floitsch wrote:
> We need to change this name.
> nameForGetInterceptorFunction ?
> 
> otherwise it reads way too much like we are getting something.

Yeah it was bothering me too. Renamed to nameForGetInterceptor.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/minify_namer.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:34: {bool ensureSafe, bool
sanitizeForAnnotations}) {
On 2015/01/30 21:03:36, floitsch wrote:
> assert that ensureSafe and sanitizeForAnnotations are not null ?

Since the minifying namer ignores these, I don't see why.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:52: if
(!_hasBannedPrefix(name)) return name;
On 2015/01/30 22:48:27, sra1 wrote:
> Where are 'g' and 's' defined?
> Is is 'accidental' that these names correspond to the getter/setter prefix?
> 
> Are you sure this is a problem - AFAIK this function names the fields of a
scope
> 'box' which can never be treated as an object, so there should be no
collision.

I was concerned because this is also used to name closure fields (as well as box
fields), and these fields can clash with regular property names.

It turns out this method doesn't actually decide the field name --
instanceFieldPropertyName does, and that will only issue 1-letter names if the
proposed name is Q, a, b, c, d, e, f, r, x, y, or z.

Anyways, after some other changes in how methods are disambiguated, the check is
certainly not necessary anymore and has been removed again.

I added some documentation to getClosureVariableName in the Namer class (which
is overridden here) which should clarify get the role of this method.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:56: return '${basename}_$id';
On 2015/01/30 22:48:27, sra1 wrote:
> So will we get something like 'xy_6' where previously we got 'g'?

As it turns out we never got 'g' because it's not a "reserved backend name".
It's pretty counter-intuitive that "reserved" names are used, while unreserved
names are not, but apparently that's how minifying namer work at the moment.

It can certainly be improved, but I think this CL is big enough already.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:154: } while
(used.contains(letter) || _hasBannedPrefix(letter));
On 2015/01/30 22:48:27, sra1 wrote:
> Was this a problem?  How did it trigger?
> It seems a shame to lose 'g' just because 'gX' means something.  It is
probably
> worthwhile strengthening the prefix check in runtime code to permit some
common
> method to be minified to 'g'.
> 
> I don't think 'g' a problem for a global name.

Done. The check is no longer necessary.

It was a problem because 'g' was also the getter for 'call', so an expression "x
+ y" would return x if x was a closure. But 'g' should not actually be the
getter for 'call', that was a misunderstanding on my part, which has been fixed
now.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:69: * 0. The disambiguated name can
itself be seen as a annotated name.
On 2015/01/30 21:03:38, floitsch wrote:
> as an annotated name.

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:81: *    hardcoded into other
components, such as [Element] and [Selector].
On 2015/01/30 21:03:36, floitsch wrote:
> :(

Clarified that it's just the ordering of parameters that's hardcoded this way. I
made it sound worse than it was.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:333: * Map from top-level or static
elements to their unique identifiers provided
On 2015/01/30 21:03:38, floitsch wrote:
> This comment is on the wrong field.

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:334: * by [getName].
On 2015/01/30 21:03:37, floitsch wrote:
> `getName` doesn't exist anymore. Was replaced with `getNameX` which you just
> removed as well...

disambiguateGlobal is the closest we have now

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:344: final Map<String, String>
internGlobals;
On 2015/01/30 21:03:36, floitsch wrote:
> add comment.
> internGlobals -> internalGlobals?
> 
> Many other fields have "Names" as suffix. Not this one...
> Unify?

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:345: final Map<String, String>
suggestedGlobalNames;
On 2015/01/30 21:03:38, floitsch wrote:
> I would group the fields:
> 
> globals
> usedGlobalNames
> suggestedGlobalNames
> 
> instanceMembers
> usedInstanceNames
> suggestedInstanceNames
> 
> Add comments to all fields that are not completely obvious.

Grouped and renamed, to be more streamlined. I added a comment on the usedX set
for each group and hope it is clear the following maps all are part of the
mapping into that namespace.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:346: final Map<LibraryElement,
Map<String, String>> instanceMembers;
On 2015/01/30 21:03:37, floitsch wrote:
> Add a comment explaining that `null` is used as key for public members.

The map works differently now, see disambiguateMember.

I decided to omit declaration-site comments for these maps since they are
repetitive and verbose, and most of them are only referenced from one method, so
it should be easy to determine their use if it is not clear from the name and
grouping.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:348: final Map<Element, String>
directAccessMap = <Element, String>{};
On 2015/01/30 21:03:37, floitsch wrote:
> comment.

see above.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:362: : compiler = compiler,
On 2015/01/30 21:03:36, floitsch wrote:
> nit: Namer(this.compiler);
> 
> You could move all assignments (except the constantHasher and
functionTypeNamer)
> to the field-declarations. Since they would almost never fit on one line
anyway,
> I'm ok with leaving them here.

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:408: /// Unique within the
global-member namespace.
On 2015/01/30 21:03:37, floitsch wrote:
> Is that necessary?
> After all, we always put constants on the C object where they don't conflict
> with anything else.
> 
> If in doubt, just leave a TODO.
> See also comment in disambiguateGlobal.

You are right, but this is how it worked to begin with. We can do more
fine-grained disambiguation of globals, but I deliberately left it this way to
limit the scope of the CL.

I left a TODO to clarfy that this is just how the implementation works atm.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:425: String
constantLongName(ConstantValue constant) {
On 2015/01/30 21:03:37, floitsch wrote:
> This looks like a function that shouldn't be called from the outside. Make it
> private?
> 
> Same for all other private methods.

The emitters currently call it for some sort of compression optimization.

I've made some other methods private, though, in particular the disambiguateXXX
methods.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:480: return
'_$libraryName\$${originalName}';
On 2015/01/30 21:03:38, floitsch wrote:
> Add comment why this is not unique.

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:494: /// and [arity] and no named
parameters.
On 2015/01/30 21:03:36, floitsch wrote:
> Maybe mention that it handles operators?

Not anymore.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:511: // TODO(asgerf): Avoid clash
when named parameters contain '$' symbols.
On 2015/01/30 21:03:37, floitsch wrote:
> Create a failing test.

Done. And the issue has been fixed.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:541: default: throw 'Unexpected
selector kind: ${selector.kind}';
On 2015/01/30 21:03:37, floitsch wrote:
> throw an internal error. (we tend to avoid throwing strings).

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:546: * Returns the internal name used
for an invocation mirror of this selector.
On 2015/01/30 21:03:38, floitsch wrote:
> What's an "internal" name?

I don't know what this function is used for.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:573: * variable.
On 2015/01/30 21:03:38, floitsch wrote:
> Also mention that this is inside a class and thus at the same level as
instance
> members. (if I'm right).

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:575: String
readTypeVariableName(TypeVariableElement element) {
On 2015/01/30 21:03:37, floitsch wrote:
> Why is this prefixed "read" ?

Don't know. Changed to typeVariableName.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:595: return element.fixedBackendName;
On 2015/01/30 21:03:36, floitsch wrote:
> Does this potentially lead to clashes?
> 
> What if there is another member that ends up being assigned the
> fixedBackendName.
> 
> If it's not possible, please comment (and/or assert).

Turns out clashes are possible. Added custom_element_name_clash_test.dart.

I couldn't find a way to get a list of all fixed backend names, so I gave up on
just blacklisting them.

Instead, we now take extra care for property names on classes that extend native
classes. We prepend a $ in front of such names, and assert that fixed backend
names cannot contain a $.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:604: () =>
'${element.enclosingClass.name}_${element.name}');
On 2015/01/30 21:03:37, floitsch wrote:
> Is this enough?
> What if I have a mixin class A_b with field c and then another mixin class A
> with field "b_c", and both have their fields shadowed by a class that mixes in
A
> and A_b.

The string we're building is only a proposed name. The disambiguated name is
still guaranteed to be unique to the element we're passing in.

I've rewritten it a bit to make it more clear at call-site that the string is
only proposed.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:607: // No inheritance-related name
clashes exist, so the accessor and property
On 2015/01/30 21:03:37, floitsch wrote:
> I don't understand this comment.
> Do you mean: no inheritance related clashes exist, so accessors may use the
same
> naming algorithm as instance members?
> 
> If yes, why is that the case?
> 
> From what I can tell we can still have a subclass that overrides the field's
> name with a getter or method. I'm not sure if that's an issue or not...

Done following offline discussion.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:613: bool
shadowingAnotherField(Element element) {
On 2015/01/30 21:03:37, floitsch wrote:
> isShadowingASuperField ?

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:640: String getterForElement(Element
element) {
On 2015/01/30 21:03:38, floitsch wrote:
> nit: either put getter and setter next to each other, or getterForElement and
> deriveGetterName (in the same order). That is:
> 
> getterForElement
> setterForElement
> deriveGetterName
> deriveSetterName
> 
> or
> getterForElement
> deriveGetterName
> setterForElement
> deriveSetterName

I agree. I kept it like this to avoid messing up the diff. Will reorder when
getting close to a final version.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:674: /// identifying the global. The
[name] should not contain `$` symbols, since
On 2015/01/30 21:03:36, floitsch wrote:
> Make this a "must" and assert it.

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:678: /// globals, although the
callers of must still take care not to accidentally
On 2015/01/30 21:03:36, floitsch wrote:
> -of-

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:680: String internGlobal(String name,
{bool ensureSafe: true}) {
On 2015/01/30 21:03:37, floitsch wrote:
> internGlobal -> internalGlobal ?

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:686: /// The resulting name is unique
within the global-member namespace.
On 2015/01/30 21:03:37, floitsch wrote:
> Same as for the constant: we might be able to only disambiguate within the
> element's holder.
> 
> I know that classes need to be unique (for going from type to constructor) but
> I'm not sure anything else needs to. (Unless, of course, reflection destroys
> everything...)
> 
> Also note: in the new emitter we are considering removing the global objects.

Again, probably yes. But this CL is big enough already.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:710: if (originalName ==
closureInvocationSelectorName) {
On 2015/01/30 21:03:36, floitsch wrote:
> assert that library is not null if the originalName is not public.

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:737: String
disambiguateDirectAccess(Element element, String proposeName()) {
On 2015/01/30 21:03:38, floitsch wrote:
> Explain how this is different from disambiguateMember.
> Basically: two elements could have the same name (even private name) and still
> get different disambiguated names here, whereas disambiguateMember only looks
at
> the name.
> Maybe best explained with an example.

I've changed it to disambiguateInternalMember. I hope this makes it clear that
it just generates names that don't clash with anything else.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:794: int i = counter == null ? 0 :
counter;
On 2015/01/30 21:03:36, floitsch wrote:
> (counter == null)

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:811: /// cannot clash with the getter
for `x`.
On 2015/01/30 21:03:37, floitsch wrote:
> Add '_isAnnotated' method and assert that every annotated method satisfies it,
> and the result of this function doesn't.
> 
> You could then also add:
> String input = name;
> ...
> assert(_isAnnotated(input) || input == name);
> return name;
> 
> This would make sure that we don't strip too much.

I don't think it's worth it anymore, since it would now just be a duplicate of
the method body, which is now just four checks.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:916: /// Property name used for
`getInterceptor` or one of its specializations.
On 2015/01/30 21:03:37, floitsch wrote:
> as written in another file:
> "nameOfGetInterceptorFunctionFor" (or similar).

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:949: if (selector.isGetter ||
selector.isSetter) root = '$root\$';
On 2015/01/30 21:03:37, floitsch wrote:
> Why does a getter and setter need the "$" ?
> The old comment explained the reasoning, although it didn't make it clear why
> operators and methods couldn't clash.
> I would have interpreted this as 'all other internal globals, except getters
and
> setters, contain a "$"', but that doesn't seem to be true.

Rephrased comment.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:965: /// from the type name.
On 2015/01/30 21:03:37, floitsch wrote:
> Yeah for weird arbitrary rules...

Still better than undocumented arbitrary rules...

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:989: /// without clashing with
another JS property name.
On 2015/01/30 21:03:36, floitsch wrote:
> Might help to give a small example when this is necessary:
> 
> foo(x, [y]) has a stub method foo$1(x) that redirects to this.foo$2(x, null);
> 
> However, if the method is overridden by another foo, method we must make sure
> that we reach the right `foo$2` method. By giving it an alias we can redirect
to
> `aliased$2(x, null)`.

Done, although I don't understand your example, so I chose a different one.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1001: /// may share its name with an
instance method.
On 2015/01/30 21:03:37, floitsch wrote:
> But is it unique within their respective categories?
> Is the returned name a proposed name or a disambiguated one?
> 
> (it looks like it's the disambiguated one, but add comment).

Ideally, the users of Namer shouldn't have to know about the different kinds of
names (proposed, disambiguated, annotated).

This is why it just says "property name". It's what the caller should care
about.

If we were to go completely formal on this, I'd say some annotated names are
just a disambiguated name with an empty annotation. This is one of those names.
But I think writing this would cause more confusion than clarity.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1002: String getNameOfMethod(Element
method) {
On 2015/01/30 21:03:38, floitsch wrote:
> Why is this prefixed with "get"?
> 
> Most of the other methods in this class have: "methodName" or similar naming
> conventions. Some drop the "name" suffix (although it feels inconsistent).

I agree. I've changed it to "methodName".

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1043: return
disambiguateInternGlobal("$getterPrefix${globalPropertyName(element)}");
On 2015/01/30 21:03:36, floitsch wrote:
> long line.

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1108: * and also ensures it won't
clash with other identifiers.
On 2015/01/30 21:03:38, floitsch wrote:
> with other reserved identifiers (like "__proto__").

Actually I think that was a stray comment from when _safeName also was the guard
against clashes with annotated names.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:1119: // TODO(ahe): Creating a string
here is unfortunate. It is slow (due to
On 2015/01/30 21:03:38, floitsch wrote:
> I don't know what Peter means.
> The cost in string concatenation for substitutions should be noise.
> I don't see what "segmentation of '$'" means.
> 
> Unless you think there is a reason we should keep the TODO, please delete it.

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:50: String name =
namer.getterForMember(null, Compiler.CALL_OPERATOR_NAME);
On 2015/01/30 21:03:38, floitsch wrote:
> maybe add a namer.getterForPublicMember(...)
> Otherwise it's hard to know what the `null` at the call-sites are.

Done.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:163: /// All the global
state can be passed around with this variable.
On 2015/01/30 21:03:38, floitsch wrote:
> Please update comment:
> Contains the global state that is needed to initialize and load a deferred
> library.

Done.

https://codereview.chromium.org/891673003/diff/20001/tests/compiler/dart2js/p...
File tests/compiler/dart2js/pretty_parameter_test.dart (right):

https://codereview.chromium.org/891673003/diff/20001/tests/compiler/dart2js/p...
tests/compiler/dart2js/pretty_parameter_test.dart:82:
Expect.isTrue(generated.contains(r"function($eval, $eval0) {"));
On 2015/01/30 22:48:27, sra1 wrote:
> Can we do something to keep the original names where possible?
> (eval, $eval)
> -->
> ($eval, $eval0)
> This is confusing when you have to step through the generated code.

Do you mean "original names" as in, the names from the source code? Or the names
that the Namer would use before the refactoring?

Previously it was (eval,$eval) -> ($eval, $$eval)
Now it's (eval,$eval) -> ($eval, $eval0)

Neither of them preserved the variable's original name.

If it's important I can make it use $$eval instead of $eval0, but it does not
seem that much better to me.

[floitsch, 2015-02-04 00:10:12]

Still LGTM.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/minify_namer.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:34: {bool ensureSafe, bool
sanitizeForAnnotations}) {
On 2015/02/03 17:39:11, asgerf wrote:
> On 2015/01/30 21:03:36, floitsch wrote:
> > assert that ensureSafe and sanitizeForAnnotations are not null ?
> 
> Since the minifying namer ignores these, I don't see why.

argh.
Of course.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:81: *    hardcoded into other
components, such as [Element] and [Selector].
On 2015/02/03 17:39:12, asgerf wrote:
> On 2015/01/30 21:03:36, floitsch wrote:
> > :(
> 
> Clarified that it's just the ordering of parameters that's hardcoded this way.
I
> made it sound worse than it was.

ok. that's not a big deal.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:408: /// Unique within the
global-member namespace.
On 2015/02/03 17:39:13, asgerf wrote:
> On 2015/01/30 21:03:37, floitsch wrote:
> > Is that necessary?
> > After all, we always put constants on the C object where they don't conflict
> > with anything else.
> > 
> > If in doubt, just leave a TODO.
> > See also comment in disambiguateGlobal.
> 
> You are right, but this is how it worked to begin with. We can do more
> fine-grained disambiguation of globals, but I deliberately left it this way to
> limit the scope of the CL.
> 
> I left a TODO to clarfy that this is just how the implementation works atm.

TODO is fine.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:425: String
constantLongName(ConstantValue constant) {
On 2015/02/03 17:39:13, asgerf wrote:
> On 2015/01/30 21:03:37, floitsch wrote:
> > This looks like a function that shouldn't be called from the outside. Make
it
> > private?
> > 
> > Same for all other private methods.
> 
> The emitters currently call it for some sort of compression optimization.
> 
> I've made some other methods private, though, in particular the
disambiguateXXX
> methods.

Acknowledged.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:686: /// The resulting name is unique
within the global-member namespace.
On 2015/02/03 17:39:12, asgerf wrote:
> On 2015/01/30 21:03:37, floitsch wrote:
> > Same as for the constant: we might be able to only disambiguate within the
> > element's holder.
> > 
> > I know that classes need to be unique (for going from type to constructor)
but
> > I'm not sure anything else needs to. (Unless, of course, reflection destroys
> > everything...)
> > 
> > Also note: in the new emitter we are considering removing the global
objects.
> 
> Again, probably yes. But this CL is big enough already.

Didn't expect more than a TODO.

https://codereview.chromium.org/891673003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:989: /// without clashing with
another JS property name.
On 2015/02/03 17:39:13, asgerf wrote:
> On 2015/01/30 21:03:36, floitsch wrote:
> > Might help to give a small example when this is necessary:
> > 
> > foo(x, [y]) has a stub method foo$1(x) that redirects to this.foo$2(x,
null);
> > 
> > However, if the method is overridden by another foo, method we must make
sure
> > that we reach the right `foo$2` method. By giving it an alias we can
redirect
> to
> > `aliased$2(x, null)`.
> 
> Done, although I don't understand your example, so I chose a different one.

Interesting. I didn't know that we added the alias for such simple cases (since
we could just grab the prototype and use `B.prototype.foo.call(this, ...)`.

My example is more complex (and was the reason we introduced aliases, since the
prototype approach wasn't working any more):

class A {
  foo([x]);
}

class B extends A {
  foo([x]) => super.foo();
}

Assume that A was compiled to:
A.prototype.foo$0 = function() { return this.foo$1(null); };
A.prototype.foo$1 = function(x) { ... };

with the prototype approach we would compile B.foo as follows:

B.prototype.foo$1 = function(x) { return A.prototype.foo$0.call(this); }

However, A.foo$0 redirects to this.foo$1 which would then again be B.foo$1. To
solve this issue one could either change foo$0 to:
A.prototype.foo$0 = function() { return A.prototype.foo$1.call(this, null); },
or simply give foo$1 another name.

Note: no need to add this as comment. Just FYI.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:542: String name =
namer.nameForOneShotInterceptor(selector, classes);
This is not really accurate.
The `getOneShotInterceptor` function returns the one-shot interceptor. This is
the name for the `getOneShotInterceptor` function.
Should be nameForGetOneShotInterceptor

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

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:79: *    The same convention is used
for other methods (in non-minified mode),
Keep the "Multiple annotated names exist per method name, ...

And then emphasize that `call` has these names computed at runtime.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:339: * Map from top-level or static
elements to their unique identifiers provided
Comment is still on the wrong field (or should just be removed).

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:368: = <ConstantValue, String>{};
equal sign should be on the previous line.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:374: = <String, LibraryElement>{};
ditto.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:379: /// it will try to the a
suggested name instead of producing a minified name.
it tries to ?? a suggested name...

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:492: // name and originalName contain
$ symbols.
could contain $ symbols.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:501: // but for now we include it to
ensure that we produce the same names as
but this way it produces the same names ...

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:526: ///    
call$<N>$namedParam1...$namedParam<M>
How is this unique?
Does it need to be unique?

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:667: String proposeName() =>
'${enclosingClass.name}_${element.name}';
I believe (would need to check), that native fields are stored on the native
object, while their methods are on an interceptor. This means that, for native
classes, fields and methods don't share the same namespace.
This seems to be the reason for being able to only mangle fields but not methods
of subclasses.

Add comment.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:755: /// pass in the same [name] for
two different intern globals.
internal globals

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:795: String originalName,
nit: indentation.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:807: String key =
'$libraryKey@$originalName@${suffixes.join('@')}';
like.
This makes it very clear that clashes are not possible.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:857: // Ensure the name we propose
cannot clash with a native property.
This assumes that getFreshName doesn't remove the leading "$".
assert it.

https://codereview.chromium.org/891673003/diff/40001/tests/html/custom_elemen...
File tests/html/custom_element_name_clash_test.dart (right):

https://codereview.chromium.org/891673003/diff/40001/tests/html/custom_elemen...
tests/html/custom_element_name_clash_test.dart:15: var appendChild = 123;
Add small comment so that it is clear that this is the thing that could trip the
compiler.

https://codereview.chromium.org/891673003/diff/40001/tests/html/custom_elemen...
tests/html/custom_element_name_clash_test.dart:22: custom.children.add(new
DivElement()..text = 'Hello world!'); // Will call appendChild in JS.
Long line.

https://codereview.chromium.org/891673003/diff/40001/tests/language/named_par...
File tests/language/named_parameter_clash_test.dart (right):

https://codereview.chromium.org/891673003/diff/40001/tests/language/named_par...
tests/language/named_parameter_clash_test.dart:8: m({a, b, c}) {
bad indentation.

https://codereview.chromium.org/891673003/diff/40001/tests/language/named_par...
tests/language/named_parameter_clash_test.dart:32: Expect.throws(() =>
foo.m('z', a$b: r'a$b', c: 'c'), (e) => e is NoSuchMethodError);
long lines.

[asgerf, 2015-02-06 12:12:10]

New patchsets have been uploaded after l-g-t-m from floitsch@google.com

[asgerf, 2015-02-06 12:15:18]

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:542: String name =
namer.nameForOneShotInterceptor(selector, classes);
On 2015/02/04 00:10:11, floitsch wrote:
> This is not really accurate.
> The `getOneShotInterceptor` function returns the one-shot interceptor. This is
> the name for the `getOneShotInterceptor` function.
> Should be nameForGetOneShotInterceptor

Done.

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

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:79: *    The same convention is used
for other methods (in non-minified mode),
On 2015/02/04 00:10:11, floitsch wrote:
> Keep the "Multiple annotated names exist per method name, ...
> 
> And then emphasize that `call` has these names computed at runtime.

This goes against the notion of annotated names that I introduced. As it says
above:

Annotated names must be computable at runtime by prefixing/suffixing constant
strings onto the disambiguated name.

This is not the case for ordinary methods, where the suffix is part of the
disambiguated name (and hence can be minified).

I've rephrased the comment a bit, hopefully clarifying.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:339: * Map from top-level or static
elements to their unique identifiers provided
On 2015/02/04 00:10:11, floitsch wrote:
> Comment is still on the wrong field (or should just be removed).

Argh. Removed.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:368: = <ConstantValue, String>{};
On 2015/02/04 00:10:11, floitsch wrote:
> equal sign should be on the previous line.

Done.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:374: = <String, LibraryElement>{};
On 2015/02/04 00:10:11, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:379: /// it will try to the a
suggested name instead of producing a minified name.
On 2015/02/04 00:10:11, floitsch wrote:
> it tries to ?? a suggested name...

Rephrased. Hopefully more clear now.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:492: // name and originalName contain
$ symbols.
On 2015/02/04 00:10:11, floitsch wrote:
> could contain $ symbols.

Done.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:501: // but for now we include it to
ensure that we produce the same names as
On 2015/02/04 00:10:11, floitsch wrote:
> but this way it produces the same names ...

Done.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:526: ///    
call$<N>$namedParam1...$namedParam<M>
On 2015/02/04 00:10:11, floitsch wrote:
> How is this unique?
> Does it need to be unique?

Dammit. It can clash.

As we discussed offline, I experimented with making call with named arguments a
disambiguated name instead of annotated, but it turns out it doesn't work. The
code for noSuchMethod breaks.

So for now I just inserted a TODO.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:667: String proposeName() =>
'${enclosingClass.name}_${element.name}';
On 2015/02/04 00:10:11, floitsch wrote:
> I believe (would need to check), that native fields are stored on the native
> object, while their methods are on an interceptor. This means that, for native
> classes, fields and methods don't share the same namespace.
> This seems to be the reason for being able to only mangle fields but not
methods
> of subclasses.
> 
> Add comment.

Done, and added test case to verify.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:755: /// pass in the same [name] for
two different intern globals.
On 2015/02/04 00:10:12, floitsch wrote:
> internal globals

Done.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:795: String originalName,
On 2015/02/04 00:10:11, floitsch wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:807: String key =
'$libraryKey@$originalName@${suffixes.join('@')}';
On 2015/02/04 00:10:11, floitsch wrote:
> like.
> This makes it very clear that clashes are not possible.

Acknowledged.

https://codereview.chromium.org/891673003/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:857: // Ensure the name we propose
cannot clash with a native property.
On 2015/02/04 00:10:11, floitsch wrote:
> This assumes that getFreshName doesn't remove the leading "$".
> assert it.

It's hard to assert because the minifying namer might remove it, but it also
knows what names to avoid.

But it was a brittle way to do it. I added a sanitizeNatives parameter to
getFreshName so it's handled in there.

At the same time, I got rid of ensureSafe since it wasn't really needed anymore.

https://codereview.chromium.org/891673003/diff/40001/tests/html/custom_elemen...
File tests/html/custom_element_name_clash_test.dart (right):

https://codereview.chromium.org/891673003/diff/40001/tests/html/custom_elemen...
tests/html/custom_element_name_clash_test.dart:15: var appendChild = 123;
On 2015/02/04 00:10:12, floitsch wrote:
> Add small comment so that it is clear that this is the thing that could trip
the
> compiler.

Done.

https://codereview.chromium.org/891673003/diff/40001/tests/html/custom_elemen...
tests/html/custom_element_name_clash_test.dart:22: custom.children.add(new
DivElement()..text = 'Hello world!'); // Will call appendChild in JS.
On 2015/02/04 00:10:12, floitsch wrote:
> Long line.

I was under the impression that tests were exempt from that rule. But fixed
anyway.

https://codereview.chromium.org/891673003/diff/40001/tests/language/named_par...
File tests/language/named_parameter_clash_test.dart (right):

https://codereview.chromium.org/891673003/diff/40001/tests/language/named_par...
tests/language/named_parameter_clash_test.dart:8: m({a, b, c}) {
On 2015/02/04 00:10:12, floitsch wrote:
> bad indentation.

Done.

https://codereview.chromium.org/891673003/diff/40001/tests/language/named_par...
tests/language/named_parameter_clash_test.dart:32: Expect.throws(() =>
foo.m('z', a$b: r'a$b', c: 'c'), (e) => e is NoSuchMethodError);
On 2015/02/04 00:10:12, floitsch wrote:
> long lines.

Done.

[floitsch, 2015-02-06 12:59:55]

Still LGTM.

https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/minify_namer.dart (right):

https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:34: {bool sanitizeForNatives,
bool sanitizeForAnnotations}) {
Add comment that sanitizeForNatives/sanitizeForAnnotations is unused in the
minifying namer.
(Ideally with a small explanation why).

https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:71: *    named parameters wit the
pattern:
with

https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:627: String
nameForReadTypeVariable(TypeVariableElement element) {
aah :)

[asgerf, 2015-02-06 13:26:57]

New patchsets have been uploaded after l-g-t-m from floitsch@google.com

[asgerf, 2015-02-06 13:27:06]

https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/minify_namer.dart (right):

https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/minify_namer.dart:34: {bool sanitizeForNatives,
bool sanitizeForAnnotations}) {
On 2015/02/06 12:59:55, floitsch wrote:
> Add comment that sanitizeForNatives/sanitizeForAnnotations is unused in the
> minifying namer.
> (Ideally with a small explanation why).

Done.

https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/namer.dart:71: *    named parameters wit the
pattern:
On 2015/02/06 12:59:55, floitsch wrote:
> with

Done.

[asgerf, 2015-02-06 13:52:16]

On 2015/02/06 13:27:06, asgerf wrote:
>
https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
> File pkg/compiler/lib/src/js_backend/minify_namer.dart (right):
> 
>
https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_backend/minify_namer.dart:34: {bool
sanitizeForNatives,
> bool sanitizeForAnnotations}) {
> On 2015/02/06 12:59:55, floitsch wrote:
> > Add comment that sanitizeForNatives/sanitizeForAnnotations is unused in the
> > minifying namer.
> > (Ideally with a small explanation why).
> 
> Done.
> 
>
https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
> File pkg/compiler/lib/src/js_backend/namer.dart (right):
> 
>
https://codereview.chromium.org/891673003/diff/60001/pkg/compiler/lib/src/js_...
> pkg/compiler/lib/src/js_backend/namer.dart:71: *    named parameters wit the
> pattern:
> On 2015/02/06 12:59:55, floitsch wrote:
> > with
> 
> Done.

Commit will be postponed until Monday to avoid rebase-related stress on those
who want to push to trunk.

[asgerf, 2015-02-09 11:31:42]

Committed patchset #8 (id:140001) manually as 43588 (presubmit successful).

[asgerf, 2015-02-12 13:44:02]

Committed patchset #11 (id:200001) manually as 43724 (presubmit successful).

[floitsch, 2015-02-25 15:05:58]

Still LGTM.

https://chromiumcodereview.appspot.com/891673003/diff/240001/pkg/compiler/lib...
File pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart (right):

https://chromiumcodereview.appspot.com/891673003/diff/240001/pkg/compiler/lib...
pkg/compiler/lib/src/js_emitter/old_emitter/class_emitter.dart:57: /**
Please add a new line here and indent the comment correctly.

https://chromiumcodereview.appspot.com/891673003/diff/240001/pkg/compiler/lib...
File pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart (right):

https://chromiumcodereview.appspot.com/891673003/diff/240001/pkg/compiler/lib...
pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart:62: // '$receiver'
works because namer.safeVariableName used for getting parameter
long line.

https://chromiumcodereview.appspot.com/891673003/diff/240001/tests/language/n...
File tests/language/named_parameter_clash_test.dart (right):

https://chromiumcodereview.appspot.com/891673003/diff/240001/tests/language/n...
tests/language/named_parameter_clash_test.dart:22: inscrutable(xs, i) => i == 0
? xs[0] : inscrutable(xs.sublist(1), i-1);
Use the new annotations instead.

[asgerf, 2015-02-27 14:21:28]

Committed patchset #14 (id:260001) manually as 44092 (presubmit successful).