Generate tear-off closures dynamically.

Generate tear-off closures dynamically.

R=johnniwinther@google.com

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

[ahe, 2013-10-16 17:18:50]

This is not ready to review or submit yet, but I'd still like to solicit
comments.

Test status:

dart2js_extra: [00:41 | 100% | +  200 | -   11]
language: [04:30 | 100% | + 1963 | -   52]

[kasperl, 2013-10-21 10:25:44]

The approach basically looks fine to me.

https://codereview.chromium.org/27524003/diff/1/dart/sdk/lib/_internal/compil...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:171:
namer.closureInvocationSelectorName];
Cache namer.closureInvocationSelectorName in a local?

https://codereview.chromium.org/27524003/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:177:
selectors = new Set<Selector>.from(selectors)..addAll(callSelectors);
Can you use Set.union here?

https://codereview.chromium.org/27524003/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:541:
bool canTearOff = false;
Consider only using tearOffName (null could represent canTearOff = false). Not
sure if it's a good idea.

[ahe, 2013-10-22 10:52:33]

Thank you, Kasper!

https://codereview.chromium.org/27524003/diff/1/dart/sdk/lib/_internal/compil...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:177:
selectors = new Set<Selector>.from(selectors)..addAll(callSelectors);
On 2013/10/21 10:25:44, kasperl wrote:
> Can you use Set.union here?

Oh yeah! I didn't know about that method :-)

[ahe, 2013-11-29 08:53:37]

Sorry, this CL contains a lot more junk that I'll normally include in an initial
review request.

[kasperl, 2013-11-29 10:10:55]

I'll continue try to digest the new reflection data. New round of comments:

https://codereview.chromium.org/27524003/diff/33001/dart/failing_tests.txt
File dart/failing_tests.txt (right):

https://codereview.chromium.org/27524003/diff/33001/dart/failing_tests.txt#ne...
dart/failing_tests.txt:1:
language/(/closure7_test|/compile_time_constant10_test|/dynamic_test|/field_increment_bailout_test|/function_subtype0_test|/function_subtype_bound_closure0_test|/function_subtype_bound_closure1_test|/function_subtype_bound_closure2_test|/function_subtype_bound_closure4_test|/function_subtype_bound_closure3_test|/function_subtype_bound_closure6_test|/function_subtype_bound_closure5_test|/function_subtype_call0_test|/function_subtype_bound_closure7_test|/function_subtype_cast0_test|/function_subtype_cast1_test|/function_subtype_cast3_test|/function_subtype_cast2_test|/function_subtype_factory0_test|/function_subtype_local0_test|/function_subtype_local1_test|/function_subtype_local2_test|/function_subtype_local5_test|/function_subtype_named1_test|/function_subtype_not0_test|/function_subtype_not1_test|/function_subtype_not2_test|/function_subtype_not3_test|/function_subtype_optional1_test|/function_subtype_simple2_test|/function_subtype_top_level0_test|/function_subtype_top_level1_test|/function_subtype_simple0_test|/function_subtype_simple1_test|/function_subtype_typearg0_test|/function_type_alias2_test|/function_type_alias3_test|/function_type_alias4_test|/function_type_alias_test|/function_type_alias6_test|/invocation_mirror2_test|/invocation_mirror_invoke_on_test|/invocation_mirror_invoke_on2_test|/issue14014_3_test|/many_named_arguments_test|/many_overridden_no_such_method_test|/method_binding_test|/method_override_test|/named_parameters_passing_falsy_test|/named_parameters_with_conversions_test|/no_such_method_test|/overridden_no_such_method_test|/prefix16_test|/null_test|/super_call4_test|/super_getter_setter_test|/typedef_is_test)
I guess you're not planning on submitting this file, right?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/class_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/class_builder.dart:11:
// TODO (sigurdm) this is just a bag of properties, rename this class
Remove this TODO now?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/class_builder.dart:30:
fields.add(field);
Should we be asserting that we aren't adding the same field multiple times? Too
expensive?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/class_builder.dart:47:
if (fieldMetadata != null) {
Maybe add a comment here that explains how metadata is in encoded in the class
data by turning it into an array?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1216:
mainBuffer.add('function dart() {}');
Add a comment that explains what this is good for. If youu need to access it
from generated JS code you probably need to add this to the reserved helper
function list in the namer.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1280:
// addComment('Method closures', mainBuffer);
This is just throw away code, right?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:177:
Set<Selector> selectors = null;
Maybe use ?: so the comment about not needing stubs for static methods goes with
the null value.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:605:
bool needStructuredInfo = canTearOff || canBeReflected ||
compiler.enabledFunctionApply;
Long line.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:698:
if (canTearOff && callSelectors != null && callSelectors.contains(callSelector))
{
Long lines (there are a number of occurrences in this method).

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart:58:
// handlers we use addProperty for them all, rather than try to generate them
Long line.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart
(left):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:56:
function processStatics(descriptor) {
Maybe it would be easier to review this if you waited with the "use strict" and
the move of this function?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:97:
if (prop && elem != null && elem.constructor === Array && prop !== "<>") {
Long line.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:113:
function addStubs(descriptor, array, name, isStatic, originalDescriptor,
functions) {
Long lines.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:160:
var unmangledNameIndex = optionalParameterCount * 2 + requiredParameterCount +
3;
Long lines.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/scanner/parser.dart:1781: var
begin = token;
var -> BeginGroupToken?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
File dart/sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/isolate_helper.dart:561: if (f is! Closure) return
null;
?:

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
File dart/sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:202: if (JS('bool', 'typeof #  ==
"function"', method)) {
DANGER, WILL ROBINSON! There's too much whitespace after #.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:270: ReflectionInfo info = new
ReflectionInfo(jsFunction);
Could this be cached in this object? The function doesn't change.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:348:
ReflectionInfo.internal(this.jsFunction,
Nice use of an internal constructor.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:366: int optionalParametersInfo =
JS('int', '#[#]', data, OPTIONAL_PARAMETERS_INFO);
Long line.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:874: if (jsFunction == null) {
Get rid of this code -- it's already covered by lines 869-872.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:882: positionalArguments = [];
const []?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:924: return
functionNoSuchMethod(function, positionalArguments, namedArguments);
Long line.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1758: if (isStatic && JS('bool',
'"\$tearOff" in #', function)) {
Maybe use hasOwnProperty rather than in? Or just go ahead and fetch the tearOff
property and check if it is undefined?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1758: if (isStatic && JS('bool',
'"\$tearOff" in #', function)) {
Raw string? (there are quite a few places in this function where that might be
handy).

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1789: JS('', '#.\$tearOff = #',
function, closure);
Use raw string here too?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1799: && JS('bool', 'typeof # ==
"function"', functionType)) {
Should we have a helper for checking if a given object is a JS function?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1819: // index +=
requiredParameterCount;
What is this?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
File dart/sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1173: String reflectionName =
JS('String|Null', r'#.$reflectionName', jsFunction);
Long line.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1173: String reflectionName =
JS('String|Null', r'#.$reflectionName', jsFunction);
Would it make sense to add helpers for all the JS property reads you're
introducing? It might make the code a bit smaller and easier to grok.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1831: if (hasDefaultValue) return
reflect(getMetadata(_defaultValue));
?:

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:2029: if (JS('bool', '# in #',
r'$metadataIndex', victim)) {
In general, it would probably be safer to use hasOwnProperty rather than 'in' to
avoid getting in trouble if someone extends one of the builtin prototies (like
Object.prototype) with weird stuff.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:2119: final String _kind;
Wouldn't it be simpler to use an int for this? Are you planning on making the
kind string user-provideable?

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
File dart/sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_rti.dart:385: bool checkSubtypeOfRuntimeType(o,
var t) {
Get rid of 'var' for t too?

https://codereview.chromium.org/27524003/diff/33001/dart/start_here.txt
File dart/start_here.txt (right):

https://codereview.chromium.org/27524003/diff/33001/dart/start_here.txt#newcode1
dart/start_here.txt:1: FAILED: dart2js-d8 release_ia32 corelib/apply2_test #
NoSuchMethodError : method not found: 'Symbol("call")'
I guess you're not planning on submitting this file, right?

https://codereview.chromium.org/27524003/diff/33001/dart/test_failures.txt
File dart/test_failures.txt (right):

https://codereview.chromium.org/27524003/diff/33001/dart/test_failures.txt#ne...
dart/test_failures.txt:1: corelib/apply2_test
I guess you're not planning on submitting this file, right?

[ngeoffray, 2013-12-04 09:26:07]

On 2013/11/29 19:11:02, ahe wrote:

Hey Peter, is the CL ready to be reviewed, or are you still fixing things?

[ahe, 2013-12-04 09:54:35]

On 2013/12/04 09:26:07, ngeoffray wrote:
> On 2013/11/29 19:11:02, ahe wrote:
> 
> Hey Peter, is the CL ready to be reviewed, or are you still fixing things?

My intent was to say "please review now" in my message from Nov 19, I did
communicate this to both Johnni and Kasper in person.

But I did make a few tweaks after that, as I was able to extract a few unrelated
changes and found a few issues in unit tests.

Regardless, I'm done tinkering, and this is ready for review. Johnni and Kasper
are already looking, but your comments would be welcome as well (but feel free
to sit it out).

[ahe, 2013-12-04 09:55:53]

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:325:
void addMemberMethod(FunctionElement member, ClassBuilder builder) {
FYI: this is a good place to start your review.

[ngeoffray, 2013-12-04 14:38:28]

Haven't seen things to say so far, except way too many long lines :-) Will do a
full review later.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:354:
bool needStructuredInfo = canTearOff || canBeReflected ||
compiler.enabledFunctionApply;
Line too long.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart:58:
// handlers we use addProperty for them all, rather than try to generate them
Line too long.

[Johnni Winther, 2013-12-05 11:49:56]

LGTM

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:138:
compiler.internalErrorOnElement(cls.methodElement, 'Bound closure1.');
Better error message.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:141:
compiler.internalErrorOnElement(cls.methodElement, 'Bound closure2.');
Ditto.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:201:
Set<Selector> untypedSelectors = new Set();
`new Set()` -> `new Set<Selector>()`

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:354:
bool needStructuredInfo = canTearOff || canBeReflected ||
compiler.enabledFunctionApply;
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:417:
callSelector == null ? null : '"${namer.invocationName(callSelector)}"';
Compute this when [callSelector] is computed (in line 402).

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:419:
[new jsAst.LiteralString('$callSelectorString /* tearOffInfo */')];
Remove /* tearOffInfo */ (at least from minified code).

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:447:
if (canTearOff && callSelectors != null && callSelectors.contains(callSelector))
{
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:450:
tearOffInfo.add(new jsAst.LiteralString('$callSelectorString /* tearOffInfo
*/'));
Remove /* tearOffInfo */.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:455:
if ((canTearOff || canBeReflected) && !member.isGenerativeConstructorBody()) {
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:489:
'"new ${Elements.reconstructConstructorName(member)}" /* $reflectionNameString
*/');
Long line. Remove /* $reflectionNameString */ (at least from minified).

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart:58:
// handlers we use addProperty for them all, rather than try to generate them
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:29:
String reflectionInfoField = r'$reflectionInfo'; // MOVE TO NAMER
Add TODO for the comments.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:51:
if (firstChar === "+") {
It would be nice to have a comment that gives an example of the descriptor
element and the resulting modifications. For instance: If the descriptor
contains
  'foo': function ...,
  '+bar': 1,
we will set 
  mangledGlobalNames['foo'] = 'bar'
and
  descriptor['foo']['$reflectable'] = 1.

Similarly for the cases below.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:54:
'''descriptor[previousProperty].$reflectableField = 1;
Indent so `descriptor[previousProperty]` is below ` (descriptor[property]` to
make it more readable.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:56:
'''init.typeInformation[previousProperty] = element;
Ditto.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:85:
'''element[previousProp].$reflectableField = 1;
Ditto.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:97:
if (prop && elem != null && elem.constructor === Array && prop !== "<>") {
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:113:
function addStubs(descriptor, array, name, isStatic, originalDescriptor,
functions) {
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:114:
var f, funcs = [originalDescriptor[name] = descriptor[name] = f =
${readFunction("array", "$FUNCTION_INDEX")}];
Ditto.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:122:
// print("Stub for " + name + ": " + f.\$stubName);
Remove debug code.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:144:
f = tearOff(funcs, array, isStatic, name);${
Replace ${ /* ... */ ""} by ''' // ... '''.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:160:
var unmangledNameIndex = optionalParameterCount * 2 + requiredParameterCount +
3;
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:162:
var reflectionName = unmangledName + ":" + requiredParameterCount + ":" +
optionalParameterCount;
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:173:
// print(name + " is reflectable with reflection name \'" + reflectionName + "\'
" + isStatic);
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:232:
array, index, 'result != null && (typeof result != "number" || (result|0) !==
result)',
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:244:
array, index, 'result != null && (typeof result != "number" || (result|0) !==
result) && typeof result != "function"',
Ditto.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:250:
(function() {
Only return an applied function in checked mode?

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File dart/sdk/lib/_internal/compiler/implementation/universe/universe.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/universe/universe.dart:188: //
optional arguments.
How?

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
File dart/sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:364: int optionalParametersInfo =
JS('int', '#[#]', data, OPTIONAL_PARAMETERS_INFO);
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:930: return
functionNoSuchMethod(function, positionalArguments, namedArguments);
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:934: // [function].
How are default values for optional parameters handled?

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:1746: * This method assumes that
[functions] was created by JavaScript function
`JavaScript` -> `the JavaScript`

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:1750: * Further assumes that
[reflectionInfo] was is the end of the array created
`was is the end` -> `is at the end`

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:1769: // printString('Trying to tear
off $propertyName using '
Remove debug code.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
File dart/sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1182: int length = keys.length;
Remove [length] - it seems to be unused.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1190: String reflectionName =
JS('String|Null', r'#.$reflectionName', jsFunction);
Long line.

https://codereview.chromium.org/27524003/diff/243001/dart/tests/compiler/dart...
File dart/tests/compiler/dart2js/types_of_captured_variables_test.dart (right):

https://codereview.chromium.org/27524003/diff/243001/dart/tests/compiler/dart...
dart/tests/compiler/dart2js/types_of_captured_variables_test.dart:12: var f = ()
=> a + 87;
Why these changes?

[ahe, 2013-12-06 15:57:53]

Thank you!

https://codereview.chromium.org/27524003/diff/1/dart/sdk/lib/_internal/compil...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:171:
namer.closureInvocationSelectorName];
On 2013/10/21 10:25:44, kasperl wrote:
> Cache namer.closureInvocationSelectorName in a local?

The code is now so distant that I don't think it helps readability.

https://codereview.chromium.org/27524003/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:541:
bool canTearOff = false;
On 2013/10/21 10:25:44, kasperl wrote:
> Consider only using tearOffName (null could represent canTearOff = false). Not
> sure if it's a good idea.

I forgot about this comment, but I'm weary of changing this logic until after
this CL has landed.

https://codereview.chromium.org/27524003/diff/33001/dart/failing_tests.txt
File dart/failing_tests.txt (right):

https://codereview.chromium.org/27524003/diff/33001/dart/failing_tests.txt#ne...
dart/failing_tests.txt:1:
language/(/closure7_test|/compile_time_constant10_test|/dynamic_test|/field_increment_bailout_test|/function_subtype0_test|/function_subtype_bound_closure0_test|/function_subtype_bound_closure1_test|/function_subtype_bound_closure2_test|/function_subtype_bound_closure4_test|/function_subtype_bound_closure3_test|/function_subtype_bound_closure6_test|/function_subtype_bound_closure5_test|/function_subtype_call0_test|/function_subtype_bound_closure7_test|/function_subtype_cast0_test|/function_subtype_cast1_test|/function_subtype_cast3_test|/function_subtype_cast2_test|/function_subtype_factory0_test|/function_subtype_local0_test|/function_subtype_local1_test|/function_subtype_local2_test|/function_subtype_local5_test|/function_subtype_named1_test|/function_subtype_not0_test|/function_subtype_not1_test|/function_subtype_not2_test|/function_subtype_not3_test|/function_subtype_optional1_test|/function_subtype_simple2_test|/function_subtype_top_level0_test|/function_subtype_top_level1_test|/function_subtype_simple0_test|/function_subtype_simple1_test|/function_subtype_typearg0_test|/function_type_alias2_test|/function_type_alias3_test|/function_type_alias4_test|/function_type_alias_test|/function_type_alias6_test|/invocation_mirror2_test|/invocation_mirror_invoke_on_test|/invocation_mirror_invoke_on2_test|/issue14014_3_test|/many_named_arguments_test|/many_overridden_no_such_method_test|/method_binding_test|/method_override_test|/named_parameters_passing_falsy_test|/named_parameters_with_conversions_test|/no_such_method_test|/overridden_no_such_method_test|/prefix16_test|/null_test|/super_call4_test|/super_getter_setter_test|/typedef_is_test)
On 2013/11/29 10:10:55, kasperl wrote:
> I guess you're not planning on submitting this file, right?

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/class_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/class_builder.dart:11:
// TODO (sigurdm) this is just a bag of properties, rename this class
On 2013/11/29 10:10:55, kasperl wrote:
> Remove this TODO now?

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/class_builder.dart:30:
fields.add(field);
On 2013/11/29 10:10:55, kasperl wrote:
> Should we be asserting that we aren't adding the same field multiple times?
Too
> expensive?

We could do that, but it does require an amount of programming that I'm too lazy
to do now.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/class_builder.dart:47:
if (fieldMetadata != null) {
On 2013/11/29 10:10:55, kasperl wrote:
> Maybe add a comment here that explains how metadata is in encoded in the class
> data by turning it into an array?

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1216:
mainBuffer.add('function dart() {}');
On 2013/11/29 10:10:55, kasperl wrote:
> Add a comment that explains what this is good for. If youu need to access it
> from generated JS code you probably need to add this to the reserved helper
> function list in the namer.

This gives slightly better names in stack traces.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1280:
// addComment('Method closures', mainBuffer);
On 2013/11/29 10:10:55, kasperl wrote:
> This is just throw away code, right?

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:177:
Set<Selector> selectors = null;
On 2013/11/29 10:10:55, kasperl wrote:
> Maybe use ?: so the comment about not needing stubs for static methods goes
with
> the null value.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:605:
bool needStructuredInfo = canTearOff || canBeReflected ||
compiler.enabledFunctionApply;
On 2013/11/29 10:10:55, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:698:
if (canTearOff && callSelectors != null && callSelectors.contains(callSelector))
{
On 2013/11/29 10:10:55, kasperl wrote:
> Long lines (there are a number of occurrences in this method).

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart:58:
// handlers we use addProperty for them all, rather than try to generate them
On 2013/11/29 10:10:55, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart
(left):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:56:
function processStatics(descriptor) {
On 2013/11/29 10:10:55, kasperl wrote:
> Maybe it would be easier to review this if you waited with the "use strict"
and
> the move of this function?

Yes. Sorry about that.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart
(right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:97:
if (prop && elem != null && elem.constructor === Array && prop !== "<>") {
On 2013/11/29 10:10:55, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:113:
function addStubs(descriptor, array, name, isStatic, originalDescriptor,
functions) {
On 2013/11/29 10:10:55, kasperl wrote:
> Long lines.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:160:
var unmangledNameIndex = optionalParameterCount * 2 + requiredParameterCount +
3;
On 2013/11/29 10:10:55, kasperl wrote:
> Long lines.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/scanner/parser.dart:1781: var
begin = token;
On 2013/11/29 10:10:55, kasperl wrote:
> var -> BeginGroupToken?

That breaks the code. We don't know that token is a BeginGroupToken until
calling expect. This change has already been submitted in a separate CL, and I
added a comment.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
File dart/sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/isolate_helper.dart:561: if (f is! Closure) return
null;
On 2013/11/29 10:10:55, kasperl wrote:
> ?:

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
File dart/sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:202: if (JS('bool', 'typeof #  ==
"function"', method)) {
On 2013/11/29 10:10:55, kasperl wrote:
> DANGER, WILL ROBINSON! There's too much whitespace after #. 

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:270: ReflectionInfo info = new
ReflectionInfo(jsFunction);
On 2013/11/29 10:10:55, kasperl wrote:
> Could this be cached in this object? The function doesn't change.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:348:
ReflectionInfo.internal(this.jsFunction,
On 2013/11/29 10:10:55, kasperl wrote:
> Nice use of an internal constructor.

Agreed :-)

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:366: int optionalParametersInfo =
JS('int', '#[#]', data, OPTIONAL_PARAMETERS_INFO);
On 2013/11/29 10:10:55, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:874: if (jsFunction == null) {
On 2013/11/29 10:10:55, kasperl wrote:
> Get rid of this code -- it's already covered by lines 869-872.

Actually, the code was wrong. It should check if info is null.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:882: positionalArguments = [];
On 2013/11/29 10:10:55, kasperl wrote:
> const []?

No, I add to them below.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:924: return
functionNoSuchMethod(function, positionalArguments, namedArguments);
On 2013/11/29 10:10:55, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1758: if (isStatic && JS('bool',
'"\$tearOff" in #', function)) {
On 2013/11/29 10:10:55, kasperl wrote:
> Raw string? (there are quite a few places in this function where that might be
> handy).

I've added this TODO:

    // TODO(ahe): All the place below using \$ should be rewritten to go
    // through the namer.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1758: if (isStatic && JS('bool',
'"\$tearOff" in #', function)) {
On 2013/11/29 10:10:55, kasperl wrote:
> Maybe use hasOwnProperty rather than in? Or just go ahead and fetch the
tearOff
> property and check if it is undefined?

I've added a TODO. Should investigate performance, and be consistent.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1789: JS('', '#.\$tearOff = #',
function, closure);
On 2013/11/29 10:10:55, kasperl wrote:
> Use raw string here too?

Should the the namer, really.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1799: && JS('bool', 'typeof # ==
"function"', functionType)) {
On 2013/11/29 10:10:55, kasperl wrote:
> Should we have a helper for checking if a given object is a JS function?

In my opinion, those helper functions makes the code harder to read. For
example, I'm having a hard time reading the code in js_rti.dart, which I think
is a good example of following this principle to its logical conclusion.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_helper.dart:1819: // index +=
requiredParameterCount;
On 2013/11/29 10:10:55, kasperl wrote:
> What is this?

Something I implemented using a different mechanism. I have removed it.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
File dart/sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1173: String reflectionName =
JS('String|Null', r'#.$reflectionName', jsFunction);
On 2013/11/29 10:10:55, kasperl wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1173: String reflectionName =
JS('String|Null', r'#.$reflectionName', jsFunction);
On 2013/11/29 10:10:55, kasperl wrote:
> Would it make sense to add helpers for all the JS property reads you're
> introducing? It might make the code a bit smaller and easier to grok.

To me it makes the code less readable.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1831: if (hasDefaultValue) return
reflect(getMetadata(_defaultValue));
On 2013/11/29 10:10:55, kasperl wrote:
> ?:

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:2029: if (JS('bool', '# in #',
r'$metadataIndex', victim)) {
On 2013/11/29 10:10:55, kasperl wrote:
> In general, it would probably be safer to use hasOwnProperty rather than 'in'
to
> avoid getting in trouble if someone extends one of the builtin prototies (like
> Object.prototype) with weird stuff.

OK.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:2119: final String _kind;
On 2013/11/29 10:10:55, kasperl wrote:
> Wouldn't it be simpler to use an int for this? Are you planning on making the
> kind string user-provideable?

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
File dart/sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/27524003/diff/33001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_rti.dart:385: bool checkSubtypeOfRuntimeType(o,
var t) {
On 2013/11/29 10:10:55, kasperl wrote:
> Get rid of 'var' for t too?

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/start_here.txt
File dart/start_here.txt (right):

https://codereview.chromium.org/27524003/diff/33001/dart/start_here.txt#newcode1
dart/start_here.txt:1: FAILED: dart2js-d8 release_ia32 corelib/apply2_test #
NoSuchMethodError : method not found: 'Symbol("call")'
On 2013/11/29 10:10:55, kasperl wrote:
> I guess you're not planning on submitting this file, right?

Done.

https://codereview.chromium.org/27524003/diff/33001/dart/test_failures.txt
File dart/test_failures.txt (right):

https://codereview.chromium.org/27524003/diff/33001/dart/test_failures.txt#ne...
dart/test_failures.txt:1: corelib/apply2_test
On 2013/11/29 10:10:55, kasperl wrote:
> I guess you're not planning on submitting this file, right?

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:138:
compiler.internalErrorOnElement(cls.methodElement, 'Bound closure1.');
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Better error message.

I'm not sure what you suggest would be better. I only added the number to make
sure that we have enough debugging information if a user reports a crash.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:141:
compiler.internalErrorOnElement(cls.methodElement, 'Bound closure2.');
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Ditto.

Ditto ditto :-)

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:201:
Set<Selector> untypedSelectors = new Set();
On 2013/12/05 11:49:56, Johnni Winther wrote:
> `new Set()` -> `new Set<Selector>()`

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:354:
bool needStructuredInfo = canTearOff || canBeReflected ||
compiler.enabledFunctionApply;
On 2013/12/04 14:38:28, ngeoffray wrote:
> Line too long.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:354:
bool needStructuredInfo = canTearOff || canBeReflected ||
compiler.enabledFunctionApply;
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:417:
callSelector == null ? null : '"${namer.invocationName(callSelector)}"';
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Compute this when [callSelector] is computed (in line 402).

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:419:
[new jsAst.LiteralString('$callSelectorString /* tearOffInfo */')];
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Remove /* tearOffInfo */ (at least from minified code).

Added TODO.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:447:
if (canTearOff && callSelectors != null && callSelectors.contains(callSelector))
{
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:450:
tearOffInfo.add(new jsAst.LiteralString('$callSelectorString /* tearOffInfo
*/'));
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Remove /* tearOffInfo */.

TODO...

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:455:
if ((canTearOff || canBeReflected) && !member.isGenerativeConstructorBody()) {
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:489:
'"new ${Elements.reconstructConstructorName(member)}" /* $reflectionNameString
*/');
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line. Remove /* $reflectionNameString */ (at least from minified).

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart:58:
// handlers we use addProperty for them all, rather than try to generate them
On 2013/12/04 14:38:28, ngeoffray wrote:
> Line too long.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/nsm_emitter.dart:58:
// handlers we use addProperty for them all, rather than try to generate them
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:29:
String reflectionInfoField = r'$reflectionInfo'; // MOVE TO NAMER
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Add TODO for the comments.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:51:
if (firstChar === "+") {
On 2013/12/05 11:49:56, Johnni Winther wrote:
> It would be nice to have a comment that gives an example of the descriptor
> element and the resulting modifications. For instance: If the descriptor
> contains
>   'foo': function ...,
>   '+bar': 1,
> we will set 
>   mangledGlobalNames['foo'] = 'bar'
> and
>   descriptor['foo']['$reflectable'] = 1.
> 
> Similarly for the cases below.

https://code.google.com/p/dart/issues/detail?id=15492

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:54:
'''descriptor[previousProperty].$reflectableField = 1;
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Indent so `descriptor[previousProperty]` is below ` (descriptor[property]` to
> make it more readable.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:56:
'''init.typeInformation[previousProperty] = element;
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Ditto.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:85:
'''element[previousProp].$reflectableField = 1;
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Ditto.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:97:
if (prop && elem != null && elem.constructor === Array && prop !== "<>") {
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:113:
function addStubs(descriptor, array, name, isStatic, originalDescriptor,
functions) {
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:114:
var f, funcs = [originalDescriptor[name] = descriptor[name] = f =
${readFunction("array", "$FUNCTION_INDEX")}];
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Ditto.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:122:
// print("Stub for " + name + ": " + f.\$stubName);
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Remove debug code.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:144:
f = tearOff(funcs, array, isStatic, name);${
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Replace ${ /* ... */ ""} by ''' // ... '''.

OK, but I don't think the code is more readable (or the generated output
suffers).

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:160:
var unmangledNameIndex = optionalParameterCount * 2 + requiredParameterCount +
3;
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:162:
var reflectionName = unmangledName + ":" + requiredParameterCount + ":" +
optionalParameterCount;
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:173:
// print(name + " is reflectable with reflection name \'" + reflectionName + "\'
" + isStatic);
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:232:
array, index, 'result != null && (typeof result != "number" || (result|0) !==
result)',
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:244:
array, index, 'result != null && (typeof result != "number" || (result|0) !==
result) && typeof result != "function"',
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Ditto.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:250:
(function() {
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Only return an applied function in checked mode?

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
File dart/sdk/lib/_internal/compiler/implementation/universe/universe.dart
(right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/universe/universe.dart:188: //
optional arguments.
On 2013/12/05 11:49:56, Johnni Winther wrote:
> How?

I think I fixed that below :-)

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
File dart/sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:364: int optionalParametersInfo =
JS('int', '#[#]', data, OPTIONAL_PARAMETERS_INFO);
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:930: return
functionNoSuchMethod(function, positionalArguments, namedArguments);
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:934: // [function].
On 2013/12/05 11:49:56, Johnni Winther wrote:
> How are default values for optional parameters handled?

Poorly. We used to ensure all stubs where created. Now we don't.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:1746: * This method assumes that
[functions] was created by JavaScript function
On 2013/12/05 11:49:56, Johnni Winther wrote:
> `JavaScript` -> `the JavaScript`

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:1750: * Further assumes that
[reflectionInfo] was is the end of the array created
On 2013/12/05 11:49:56, Johnni Winther wrote:
> `was is the end` -> `is at the end`

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:1769: // printString('Trying to tear
off $propertyName using '
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Remove debug code.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
File dart/sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1182: int length = keys.length;
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Remove [length] - it seems to be unused.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1190: String reflectionName =
JS('String|Null', r'#.$reflectionName', jsFunction);
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Long line.

Done.

https://codereview.chromium.org/27524003/diff/243001/dart/tests/compiler/dart...
File dart/tests/compiler/dart2js/types_of_captured_variables_test.dart (right):

https://codereview.chromium.org/27524003/diff/243001/dart/tests/compiler/dart...
dart/tests/compiler/dart2js/types_of_captured_variables_test.dart:12: var f = ()
=> a + 87;
On 2013/12/05 11:49:56, Johnni Winther wrote:
> Why these changes?

The code in ReflectionInfo contains "+ 3", so I needed another value.

[ahe, 2013-12-06 17:42:15]

Committed patchset #13 manually as r30955 (presubmit successful).

[ngeoffray, 2013-12-09 11:15:58]

Post-commit comments.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:138:
compiler.internalErrorOnElement(cls.methodElement, 'Bound closure1.');
Maybe add a better error message.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:141:
compiler.internalErrorOnElement(cls.methodElement, 'Bound closure2.');
Ditto.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:170:
Set<Selector> selectors = member.isInstanceMember()
For readability, I would not initialize this set here, but instead do it in the
if/else line 186.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:337:
// Careful with operators.
I don't understand this comment.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:436:
// data, we can eliminate this element.
What does eliminate mean here?

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:453:
!member.isGenerativeConstructorBody()) {
A generative constructor body can be reflected?

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/metadata_emitter.dart
(right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/metadata_emitter.dart:47:
if (signature.optionalParameterCount == 0) return const [];
const <int>[]

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart
(right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:50:

Remove extra line.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
File dart/sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:183: bool isCatchAll = false;
Please add a comment on what 'catchAll' means.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:1788: // Create a closure and "monkey"
patch it with call stubs.
The monkey patching happens later in this code. I would move the comment there.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
File dart/sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:828: // TODO(ahe): Handle intercepted
methods.
Does it crash if it's intercepted?

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1235: continue;
This is weird. Why not putting the code below in the if?

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1635: //= JS('JsMethodMirror|Null',
r'#.constructor[#]', reflectee, cacheName);
Why can't you cache anymore?

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1771: if (functionType is int) {
Please add a comment what it means to be int.

[ahe, 2013-12-09 17:06:47]

Comments addressed in CL 104723004.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:138:
compiler.internalErrorOnElement(cls.methodElement, 'Bound closure1.');
On 2013/12/09 11:15:58, ngeoffray wrote:
> Maybe add a better error message.

Johnni said the same, so I'm going to repeat my answer :-)

I'm not sure what you suggest would be better. I only added the number to make
sure that we have enough debugging information if a user reports a crash.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:141:
compiler.internalErrorOnElement(cls.methodElement, 'Bound closure2.');
On 2013/12/09 11:15:58, ngeoffray wrote:
> Ditto.

Ditto :-)

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:170:
Set<Selector> selectors = member.isInstanceMember()
On 2013/12/09 11:15:58, ngeoffray wrote:
> For readability, I would not initialize this set here, but instead do it in
the
> if/else line 186.

Done.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:337:
// Careful with operators.
On 2013/12/09 11:15:58, ngeoffray wrote:
> I don't understand this comment.

Changed it to:

        // TODO(ahe): What happens if an operator is passed to getterName?

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:436:
// data, we can eliminate this element.
On 2013/12/09 11:15:58, ngeoffray wrote:
> What does eliminate mean here?

That it doesn't need to be added to the reflective data.

Changed comment to:

          // TOOD(ahe): Since we know when reading static data versus instance
          // data, we can eliminate this element from the emitted array. In
          // other words, instead of generating [..., foo, null, bar, ...], we
          // can generate [..., foo, bar, ...] and adjust the index differently
          // when reading this information from a static function.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:453:
!member.isGenerativeConstructorBody()) {
On 2013/12/09 11:15:58, ngeoffray wrote:
> A generative constructor body can be reflected?

Yeah. It is kind of a hack, but these end up being generated as the static
methods I can safely call to implement ClassMirror.newInstance.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/metadata_emitter.dart
(right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/metadata_emitter.dart:47:
if (signature.optionalParameterCount == 0) return const [];
On 2013/12/09 11:15:58, ngeoffray wrote:
> const <int>[]

Done.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart
(right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/reflection_data_parser.dart:50:

On 2013/12/09 11:15:58, ngeoffray wrote:
> Remove extra line.

Done.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
File dart/sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:183: bool isCatchAll = false;
On 2013/12/09 11:15:58, ngeoffray wrote:
> Please add a comment on what 'catchAll' means.

Done.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_helper.dart:1788: // Create a closure and "monkey"
patch it with call stubs.
On 2013/12/09 11:15:58, ngeoffray wrote:
> The monkey patching happens later in this code. I would move the comment
there.

Done.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
File dart/sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:828: // TODO(ahe): Handle intercepted
methods.
On 2013/12/09 11:15:58, ngeoffray wrote:
> Does it crash if it's intercepted?

Nope, just bad undefined stuff.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1235: continue;
On 2013/12/09 11:15:58, ngeoffray wrote:
> This is weird. Why not putting the code below in the if?

Yeah. Historical reasons.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1635: //= JS('JsMethodMirror|Null',
r'#.constructor[#]', reflectee, cacheName);
On 2013/12/09 11:15:58, ngeoffray wrote:
> Why can't you cache anymore?

Because there is only one constructor that is shared by all bound closures.
Update the comment.

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/l...
dart/sdk/lib/_internal/lib/js_mirrors.dart:1771: if (functionType is int) {
On 2013/12/09 11:15:58, ngeoffray wrote:
> Please add a comment what it means to be int.

Done.

[ngeoffray, 2013-12-09 17:14:54]

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart
(right):

https://codereview.chromium.org/27524003/diff/283001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/container_builder.dart:138:
compiler.internalErrorOnElement(cls.methodElement, 'Bound closure1.');
On 2013/12/09 17:06:47, ahe wrote:
> On 2013/12/09 11:15:58, ngeoffray wrote:
> > Maybe add a better error message.
> 
> Johnni said the same, so I'm going to repeat my answer :-)
> 
> I'm not sure what you suggest would be better. I only added the number to make
> sure that we have enough debugging information if a user reports a crash.

BoundClosure1 may be enough for you, but besides the location of the error, I
can't come up with a reason it should crash. Maybe then add a comment on why
this is an error to have a supertype being the bound closure class, and same
below.