dart2js: Only emit constructors of the native class interceptors that are used.

pkg/compiler/lib/src/js_backend/native_emitter.dart

Index: pkg/compiler/lib/src/js_backend/native_emitter.dart
diff --git a/pkg/compiler/lib/src/js_backend/native_emitter.dart b/pkg/compiler/lib/src/js_backend/native_emitter.dart
index 60f11553430e71639cbfa93dd9780a1d1020a973..535aa12c2086c27db9471931ff478da850228a99 100644
--- a/pkg/compiler/lib/src/js_backend/native_emitter.dart
+++ b/pkg/compiler/lib/src/js_backend/native_emitter.dart
@@ -232,10 +232,27 @@ class NativeEmitter {
     for (ClassElement classElement in classes) {
       if (!classElement.isNative) continue;
       if (neededClasses.contains(classElement)) {
+        ClassBuilder builder = builders[classElement];
+
+        // In CSP mode [emitClassConstructor] and [emitClassGettersSetters] emit
+        // code outside the classBuilder. They are called here to avoid emitting
+        // code for unneeded classes.
+        //
+        // Having the calls here (instead of in the classBuilder) is safe:

[ahe, 2015/01/15 08:49:51]

I'm not sure this comment adds value long term. When reading this code, why
would you think that this code should be in a different place?

I think this comment should be sufficient:

/*
In CSP mode [emitClassConstructor] and [emitClassGettersSetters] have a
side-effect on "precompiled" functions in [OldEmitter]. For this reason, it is
important that we don't call these methods before we are certain that a class is
needed.
*/

[zarah, 2015/01/15 15:10:19]

Done.

+        //
+        // [emitClassConstructor] only affects the generation of constructors

[ahe, 2015/01/15 08:49:51]

Shouldn't this be part of emitClassConstructor's documentation comment?

[floitsch, 2015/01/15 14:37:20]

Probably both.

[zarah, 2015/01/15 15:10:19]

Added documentation of the function as well.

+        // in CSP mode.
+        //
+        // [emitClassGettersSetters] does not affect whether or not a class is

[ahe, 2015/01/15 08:49:51]

Ditto.

[floitsch, 2015/01/15 14:37:20]

No. this comment should be here.
It explains why we can delay the call to here, and don't need to run it below.

However it could/should also be in the documentation of the function.

[zarah, 2015/01/15 15:10:19]

Added documentation of the function as well.

+        // needed. If getters/setters are emitted, the class has fields and
+        // is therefore non-trivial.
+        emitterTask.oldEmitter.classEmitter.emitClassConstructor(
+            classElement, builder);
+        emitterTask.oldEmitter.classEmitter.emitClassGettersSetters(
+            classElement, builder);
         // Define interceptor class for [classElement].
         emitterTask.oldEmitter.classEmitter.emitClassBuilderWithReflectionData(
-            backend.namer.getNameOfClass(classElement),
-            classElement, builders[classElement],
+            backend.namer.getNameOfClass(classElement), classElement, builder,
             emitterTask.oldEmitter.getElementDescriptor(classElement));
         emitterTask.oldEmitter.needsClassSupport = true;
       }
@@ -301,13 +318,10 @@ class NativeEmitter {
     }
     builder.superName = superName;
 
-    emitterTask.oldEmitter.classEmitter.emitClassConstructor(
-        classElement, builder);
     bool hasFields = emitterTask.oldEmitter.classEmitter.emitFields(
         classElement, builder, classIsNative: true);
     int propertyCount = builder.properties.length;
-    emitterTask.oldEmitter.classEmitter.emitClassGettersSetters(
-        classElement, builder);
+
     emitterTask.oldEmitter.classEmitter.emitInstanceMembers(
         classElement, builder);
     emitterTask.oldEmitter.typeTestEmitter.emitIsTests(classElement, builder);