Reapply class/method/field minification

Reapply class/method/field minification


BUG=

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

[sra1, 2012-12-07 06:03:18]

lgtm

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:843:
namer.setterNameFromAccessorName(accessorName);
Single line?

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:216: // minfiied
versions of the class and instance names, but the result is
minfiied -> minified

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

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:543:
String hashCodeName = backend.namer.publicGetterName(new
SourceString('hashCode'));
const SourceString(...)

[kasperl, 2012-12-07 10:37:45]

LGTM.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1150: new
ClosureInvocationElement(namer.CLOSURE_INVOCATION_NAME, element);
It looks a bit fishy to try to grab a constant (we only use ALL_CAPS_NAMES for
constants) from an object that varies. Change CLOSURE_INVOCATION_NAME to a field
or a method and call it closureInvocationName instead.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/native_helper.dart (left):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:121:
propertyGet(var object, String property) {
Why did this disappear? No remaining uses? I'd probably leave it in -- and start
using it in more places instead.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/native_helper.dart (right):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:216: if
(JS('bool', '#.call(#, #)', hasOwnProperty, entry._set, tag)) {
Consider adding a named helper for the hasOwnProperty cleverness. Our compiler
should inline such top-level helpers that return a single expression nicely.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:262: JS('var',
'#[#]', methods, className) :
Use propertyGet here?

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:294: JS('var',
'Object.getPrototypeOf(#)[#]', const Object(), name);
Add helper for getPrototypeOf stuff?

[floitsch, 2012-12-07 12:01:01]

LGTM.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:790: ? 
member.nativeName()
accidental space?

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1695: if
(typeof document !== 'undefined' && document.readyState != 'complete') {
not sure that's needed. In theory it should be clear to all engines that both
sides are strings. but fine with both.
However, I would actually prefer the document.readyState !== 'complete'.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:86: // The
minifier never returns the same string as we suggested so we
The minifier always constructs a new name, using the argument as input to its
hashing algorithm. The given name does not need to be valid.

No need to talk about the tendency to have similar names. That should be a
comment on the minifier, but not here.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:95: longName =
"CTC";
We could maybe switch to "CONSTANT" now? (but no strong opinion).

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:135:
LibraryElement owner =
I don't think you need to fill the shortPrivateNameOwners if you are minifying.
You should probably guard this with the 'shouldMinify' flag.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:201: if (name ==
CLOSURE_INVOCATION_NAME) {
Copy comment from above. (saying that closure invoking function name is not
mangled because of "applyFunction").

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:417: if
(identical(kind, ElementKind.CLASS)) {
use "=="

[erikcorry, 2012-12-10 08:36:00]

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:790: ? 
member.nativeName()
On 2012/12/07 12:01:01, floitsch wrote:
> accidental space?

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:843:
namer.setterNameFromAccessorName(accessorName);
On 2012/12/07 06:03:18, sra1 wrote:
> Single line?

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1150: new
ClosureInvocationElement(namer.CLOSURE_INVOCATION_NAME, element);
On 2012/12/07 10:37:45, kasperl wrote:
> It looks a bit fishy to try to grab a constant (we only use ALL_CAPS_NAMES for
> constants) from an object that varies. Change CLOSURE_INVOCATION_NAME to a
field
> or a method and call it closureInvocationName instead.

Done and changed:

ISOLATE -> isolateName
ISOLATE_PROPERTIES -> isolatePropertiesName

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1695: if
(typeof document !== 'undefined' && document.readyState != 'complete') {
On 2012/12/07 12:01:01, floitsch wrote:
> not sure that's needed. In theory it should be clear to all engines that both
> sides are strings. but fine with both.
> However, I would actually prefer the document.readyState !== 'complete'.

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:86: // The
minifier never returns the same string as we suggested so we
On 2012/12/07 12:01:01, floitsch wrote:
> The minifier always constructs a new name, using the argument as input to its
> hashing algorithm. The given name does not need to be valid.
> 
> No need to talk about the tendency to have similar names. That should be a
> comment on the minifier, but not here.

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:95: longName =
"CTC";
On 2012/12/07 12:01:01, floitsch wrote:
> We could maybe switch to "CONSTANT" now? (but no strong opinion).

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:135:
LibraryElement owner =
On 2012/12/07 12:01:01, floitsch wrote:
> I don't think you need to fill the shortPrivateNameOwners if you are
minifying.
> You should probably guard this with the 'shouldMinify' flag.

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:201: if (name ==
CLOSURE_INVOCATION_NAME) {
On 2012/12/07 12:01:01, floitsch wrote:
> Copy comment from above. (saying that closure invoking function name is not
> mangled because of "applyFunction").

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:216: // minfiied
versions of the class and instance names, but the result is
On 2012/12/07 06:03:18, sra1 wrote:
> minfiied -> minified

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:417: if
(identical(kind, ElementKind.CLASS)) {
On 2012/12/07 12:01:01, floitsch wrote:
> use "=="

Done.

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

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:543:
String hashCodeName = backend.namer.publicGetterName(new
SourceString('hashCode'));
On 2012/12/07 06:03:18, sra1 wrote:
> const SourceString(...)

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/native_helper.dart (left):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:121:
propertyGet(var object, String property) {
On 2012/12/07 10:37:45, kasperl wrote:
> Why did this disappear? No remaining uses? I'd probably leave it in -- and
start
> using it in more places instead.

It's not getting inlined, but I'll put it back and we can fix the inlining
separately.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/native_helper.dart (right):

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:216: if
(JS('bool', '#.call(#, #)', hasOwnProperty, entry._set, tag)) {
On 2012/12/07 10:37:45, kasperl wrote:
> Consider adding a named helper for the hasOwnProperty cleverness. Our compiler
> should inline such top-level helpers that return a single expression nicely.

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:262: JS('var',
'#[#]', methods, className) :
On 2012/12/07 10:37:45, kasperl wrote:
> Use propertyGet here?

Done.

https://codereview.chromium.org/11453032/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:294: JS('var',
'Object.getPrototypeOf(#)[#]', const Object(), name);
On 2012/12/07 10:37:45, kasperl wrote:
> Add helper for getPrototypeOf stuff?

Done.

[ngeoffray, 2012-12-10 08:55:42]

LGTM

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:70: SourceString
get closureInvocationSelector => Compiler.CALL_OPERATOR_NAME;
We have the Selector class in the compiler, so I'd prefer keeping
closureInvocationName. Or closureInvocationSelectorName.

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:104:
instanceMethodInvocationName(null, closureInvocationSelector, selector);
The style we usually use is arguments on the new line.

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:203: //
generated in applyFunction.
generated or used? What is applyFunction? Dart's Function.apply?

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:340: // the
important thing is that it is a unique name.
the -> The

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:420:
ClassElement class_elt = element;
class_elt -> classElement

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:984: *
TODO(erikcorry): Fix this to produce consistent result regardless of
What's 'this' here?

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/native_helper.dart (right):

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:218: //
getTypeNameOf to getTypeTag.
What's making it difficult today to have it in this CL?

[erikcorry, 2012-12-10 12:37:40]

Now continued in https://codereview.chromium.org/11492006

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:70: SourceString
get closureInvocationSelector => Compiler.CALL_OPERATOR_NAME;
On 2012/12/10 08:55:42, ngeoffray wrote:
> We have the Selector class in the compiler, so I'd prefer keeping
> closureInvocationName. Or closureInvocationSelectorName.

We already have something else called closureInvocationName in this class, so
closureInvocationSelectorName it is, though it reminds me a little of
http://en.wikipedia.org/wiki/Haddocks'_Eyes

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:104:
instanceMethodInvocationName(null, closureInvocationSelector, selector);
On 2012/12/10 08:55:42, ngeoffray wrote:
> The style we usually use is arguments on the new line.

Done.

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:203: //
generated in applyFunction.
On 2012/12/10 08:55:42, ngeoffray wrote:
> generated or used? What is applyFunction? Dart's Function.apply?

The name is generated (wiith string addition), then it gets used.  The
applyFunction function is found in js_helper.dart.  Comment clarified here and
below.

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:340: // the
important thing is that it is a unique name.
On 2012/12/10 08:55:42, ngeoffray wrote:
> the -> The

Done.

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:420:
ClassElement class_elt = element;
On 2012/12/10 08:55:42, ngeoffray wrote:
> class_elt -> classElement

Done.

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:984: *
TODO(erikcorry): Fix this to produce consistent result regardless of
On 2012/12/10 08:55:42, ngeoffray wrote:
> What's 'this' here?

I no longer recall, so I deleted the comment.

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/native_helper.dart (right):

https://codereview.chromium.org/11453032/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/native_helper.dart:218: //
getTypeNameOf to getTypeTag.
On 2012/12/10 08:55:42, ngeoffray wrote:
> What's making it difficult today to have it in this CL?

I don't like to mix renaming changes with real changes as they obscure the real
changes making it hard to review.