Add TypeVariable object on runtime to support reflection on type variables.

Add TypeVariable object on runtime to support reflection on type variables.

R=ahe@google.com, karlklose@google.com

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

[zarah, 2013-10-14 09:55:16]

Ping.

[karlklose, 2013-10-14 10:54:43]

LGTM.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:341:
.map((DartType type) => rti.getTypeRepresentationWithHashes(type, (_){}));
Long line.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:532:
String getTypeRepresentationWithHashes(DartType type, OnVariableCallback
onVariable) {
Long line.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:543:
OnVariableCallback onVariable) {
Indentation is off.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:11:

Remove line.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:18:

Perhaps add a bit of documentation for what classes you collect in the
collections in this handler.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:20:
Map<ClassElement, List<int>> typeVars = new Map();
Add type arguments in new expression - otherwise the object will be a raw Map.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:27:
typeVarClass =
Indentation.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:53:
Constant name =  backend.constantSystem.createString(
Remove one space.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:62:
List<int> constants = [];
'[]' -> '<int>[]'

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:95:
emitter.globalMetadata.add('DUMMY');
Is this debug code? Shouldn't you assert that the constant is emitted?

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1718:
if (typeVarHandler.typeVars[classElement] != null)
Use a {} block even for one statement unless it is a short return statement.

[ahe, 2013-10-14 13:07:07]

Mostly nits below, but I would like to see an updated version before I approve
this CL.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:409:
TypeVariableConstantHandler typeVarHandler;
Rename to typeVariableHandler (please don't abbreviate).

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:670:
typeVarHandler.initializeHelperClass();
I think I would prefer if the helper class was initialized here. This makes it
easier to find helper classes.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:1:
// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
The file naming convention is:

foo_bar_baz.dart, not fooBarBaz.dart.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:8:
* Handles construction of TypeVariable constants needed on runtime.
on -> at.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:10:
class TypeVariableConstantHandler {
The name "TypeVariableConstantHandler" is suboptimal. We already have the
concept of "ConstantHandler" in the compiler, and this class does not extend
that class. In other words, "TypeVariableConstantHandler" is not a
"ConstantHandler".

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:13:
ClassElement typeVarClass;
Try to avoid abbreviating typeVariable as typeVar.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:15:
Compiler get compiler => backend.compiler;
Please don't mix getters with fields. Follow this guide line for class members:

fields (static or not)
constructors
accessors (static or not)
methods (static or not)

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:37:
typeVarConstructor = typeVarClass.constructors.head;
report an internal error if typeVarClass.constructors doesn't contain exactly
one element.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:52:
List<Constant> createArguments(_) {
What is the argument to createArguments, and why do you ignore it?

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:62:
List<int> constants = [];
I actually think it would be better to use a Link here.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:64:
currentTypeVar = typeVar;
You should be able to write:

for (currentTypeVar in cls.typeVariables) {
...

Eliminating the need to assign currentTypeVar = typeVar.

However, given that createArguments close over typeVar/currentTypeVar, it would
be cleaner if you defined the function inside this loop.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:95:
emitter.globalMetadata.add('DUMMY');
Please document the relationship between reifyTypeVar and reifyTypeVarConstant.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1718:
if (typeVarHandler.typeVars[classElement] != null)
Please keep abstractions in place.

typeVarHandler is an abstraction, but it is broken down when you directly access
the "typeVars" map.

You should have a method like:

typeVarHandler.typeVariablesOf(classElement);

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1720:
.map((s) => js.toExpression(s)));
This should probably be:

properties = typeVarHandler.typeVars[classElement].map((s) =>
js.toExpression(s));

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:3060:
});
Weird indentation.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/lib/js_mirr...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/lib/js_mirr...
sdk/lib/_internal/lib/js_mirrors.dart:1330: new
JsTypeVariableMirror(s(typeVar.name), upperBound, this);
Why isn't this simply:

TypeVariable typeVar = JS('', 'init.metadata[#]', typeVars[i]);
result.add(new JsTypeVariableMirror(typeVar, this))

https://codereview.chromium.org/26472002/diff/1/tests/lib/mirrors/class_mirro...
File tests/lib/mirrors/class_mirror_type_variables_test.dart (right):

https://codereview.chromium.org/26472002/diff/1/tests/lib/mirrors/class_mirro...
tests/lib/mirrors/class_mirror_type_variables_test.dart:18: main() {
Please move main to the end of the file.

[zarah, 2013-10-15 14:20:02]

PTAL.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:409:
TypeVariableConstantHandler typeVarHandler;
On 2013/10/14 13:07:07, ahe wrote:
> Rename to typeVariableHandler (please don't abbreviate).

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:670:
typeVarHandler.initializeHelperClass();
On 2013/10/14 13:07:07, ahe wrote:
> I think I would prefer if the helper class was initialized here. This makes it
> easier to find helper classes.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:341:
.map((DartType type) => rti.getTypeRepresentationWithHashes(type, (_){}));
On 2013/10/14 10:54:43, karlklose wrote:
> Long line.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:532:
String getTypeRepresentationWithHashes(DartType type, OnVariableCallback
onVariable) {
On 2013/10/14 10:54:43, karlklose wrote:
> Long line.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:543:
OnVariableCallback onVariable) {
On 2013/10/14 10:54:43, karlklose wrote:
> Indentation is off.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:1:
// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
On 2013/10/14 13:07:07, ahe wrote:
> The file naming convention is:
> 
> foo_bar_baz.dart, not fooBarBaz.dart.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:8:
* Handles construction of TypeVariable constants needed on runtime.
On 2013/10/14 13:07:07, ahe wrote:
> on -> at.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:10:
class TypeVariableConstantHandler {
On 2013/10/14 13:07:07, ahe wrote:
> The name "TypeVariableConstantHandler" is suboptimal. We already have the
> concept of "ConstantHandler" in the compiler, and this class does not extend
> that class. In other words, "TypeVariableConstantHandler" is not a
> "ConstantHandler".

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:11:

On 2013/10/14 10:54:43, karlklose wrote:
> Remove line.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:13:
ClassElement typeVarClass;
On 2013/10/14 13:07:07, ahe wrote:
> Try to avoid abbreviating typeVariable as typeVar.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:15:
Compiler get compiler => backend.compiler;
On 2013/10/14 13:07:07, ahe wrote:
> Please don't mix getters with fields. Follow this guide line for class
members:
> 
> fields (static or not)
> constructors
> accessors (static or not)
> methods (static or not)

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:18:

On 2013/10/14 10:54:43, karlklose wrote:
> Perhaps add a bit of documentation for what classes you collect in the
> collections in this handler.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:20:
Map<ClassElement, List<int>> typeVars = new Map();
On 2013/10/14 10:54:43, karlklose wrote:
> Add type arguments in new expression - otherwise the object will be a raw Map.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:27:
typeVarClass =
On 2013/10/14 10:54:43, karlklose wrote:
> Indentation.

Removed from here.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:37:
typeVarConstructor = typeVarClass.constructors.head;
On 2013/10/14 13:07:07, ahe wrote:
> report an internal error if typeVarClass.constructors doesn't contain exactly
> one element.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:52:
List<Constant> createArguments(_) {
On 2013/10/14 13:07:07, ahe wrote:
> What is the argument to createArguments, and why do you ignore it?

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:53:
Constant name =  backend.constantSystem.createString(
On 2013/10/14 10:54:43, karlklose wrote:
> Remove one space.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:62:
List<int> constants = [];
On 2013/10/14 10:54:43, karlklose wrote:
> '[]' -> '<int>[]'

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:64:
currentTypeVar = typeVar;
On 2013/10/14 13:07:07, ahe wrote:
> You should be able to write:
> 
> for (currentTypeVar in cls.typeVariables) {
> ...
> 
> Eliminating the need to assign currentTypeVar = typeVar.
> 
> However, given that createArguments close over typeVar/currentTypeVar, it
would
> be cleaner if you defined the function inside this loop.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/typeVariableConstantHandler.dart:95:
emitter.globalMetadata.add('DUMMY');
On 2013/10/14 13:07:07, ahe wrote:
> Please document the relationship between reifyTypeVar and
reifyTypeVarConstant.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1718:
if (typeVarHandler.typeVars[classElement] != null)
On 2013/10/14 10:54:43, karlklose wrote:
> Use a {} block even for one statement unless it is a short return statement.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1718:
if (typeVarHandler.typeVars[classElement] != null)
On 2013/10/14 13:07:07, ahe wrote:
> Please keep abstractions in place.
> 
> typeVarHandler is an abstraction, but it is broken down when you directly
access
> the "typeVars" map.
> 
> You should have a method like:
> 
> typeVarHandler.typeVariablesOf(classElement);

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1720:
.map((s) => js.toExpression(s)));
On 2013/10/14 13:07:07, ahe wrote:
> This should probably be:
> 
> properties = typeVarHandler.typeVars[classElement].map((s) =>
> js.toExpression(s));

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:3060:
});
On 2013/10/14 13:07:07, ahe wrote:
> Weird indentation.

Done.

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/lib/js_mirr...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/26472002/diff/1/sdk/lib/_internal/lib/js_mirr...
sdk/lib/_internal/lib/js_mirrors.dart:1330: new
JsTypeVariableMirror(s(typeVar.name), upperBound, this);
On 2013/10/14 13:07:07, ahe wrote:
> Why isn't this simply:
> 
> TypeVariable typeVar = JS('', 'init.metadata[#]', typeVars[i]);
> result.add(new JsTypeVariableMirror(typeVar, this))

Done.

https://codereview.chromium.org/26472002/diff/1/tests/lib/mirrors/class_mirro...
File tests/lib/mirrors/class_mirror_type_variables_test.dart (right):

https://codereview.chromium.org/26472002/diff/1/tests/lib/mirrors/class_mirro...
tests/lib/mirrors/class_mirror_type_variables_test.dart:18: main() {
On 2013/10/14 13:07:07, ahe wrote:
> Please move main to the end of the file.

Done.

[ahe, 2013-10-15 14:56:31]

Comments so far.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:15:
ClassElement get typeVariableClass => backend.typeVariableClass;
Please declare getters after the constructor.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart
(right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:8: 
Extra line.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:297:
.map((s) => js.toExpression(s));
"(s) => js.toExpression(s)" is equivalent to "js.toExpression" due to tear-off
closures.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1627:
return js.toExpression(typeVariableHandler.reifyTypeVariable(variable));
Long line.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:210: return _cachedUpperBound = 
typeMirrorFromRuntimeTypeRepresentation(
Extra space after =.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1325: result.add( new
JsTypeVariableMirror(typeVariable, this));
Extra space after (.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_rti.dart:68: /**
Missing newline.

[ahe, 2013-10-15 14:59:56]

LGTM!

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:71:
if (constructor != typeVariableConstructor)
Braces.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:122:
* the global metadata list and the index pointing to this entry is returned.
Add a reference to [emitter.globalMetadata].

[zarah, 2013-10-16 07:00:47]

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:15:
ClassElement get typeVariableClass => backend.typeVariableClass;
On 2013/10/15 14:56:31, ahe wrote:
> Please declare getters after the constructor.

Done.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:71:
if (constructor != typeVariableConstructor)
On 2013/10/15 14:59:56, ahe wrote:
> Braces.

Done.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:122:
* the global metadata list and the index pointing to this entry is returned.
On 2013/10/15 14:59:56, ahe wrote:
> Add a reference to [emitter.globalMetadata].

Done.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart
(right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:8: 
On 2013/10/15 14:56:31, ahe wrote:
> Extra line.

Done.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:297:
.map((s) => js.toExpression(s));
On 2013/10/15 14:56:31, ahe wrote:
> "(s) => js.toExpression(s)" is equivalent to "js.toExpression" due to tear-off
> closures.

Done.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1627:
return js.toExpression(typeVariableHandler.reifyTypeVariable(variable));
On 2013/10/15 14:56:31, ahe wrote:
> Long line.

Done.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:210: return _cachedUpperBound = 
typeMirrorFromRuntimeTypeRepresentation(
On 2013/10/15 14:56:31, ahe wrote:
> Extra space after =.

Done.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1325: result.add( new
JsTypeVariableMirror(typeVariable, this));
On 2013/10/15 14:56:31, ahe wrote:
> Extra space after (.

Done.

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/26472002/diff/25001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_rti.dart:68: /**
On 2013/10/15 14:56:31, ahe wrote:
> Missing newline.

Done.

[zarah, 2013-10-16 08:17:32]

Committed patchset #3 manually as r28708 (presubmit successful).