Support user generated custom native JS classes.

Support user generated custom native JS classes.

BUG=
R=jmesserly@google.com, sra@google.com

Committed: https://github.com/dart-lang/sdk/commit/cd2752431e41c33c7ae85e0d0d26e5261f168930

[Jacob, 2015-09-02 22:03:27]

jacobr@google.com changed reviewers:
+ jmesserly@google.com, sra@chromium.org

[Jacob, 2015-09-02 22:03:27]

Essentially this is an implementation of JS interfaces using "some such method"
stubs added to the core Interceptor class that call directly through to the JS
method with the same method. Uses new JsNative collection of static methods
rather than existing heavy weight JsObject and friends interop classes.

This works assuming wrapper free interop where all objects are passed directly
between dart and js.
Closures must be manually wrapped with calls to 
dart:js allowInterop to be passed from Dart to JS. JS closures can be passed to
Dart without any wrapping due to monkey patching of Function.prototype

Added jmesserly for review of 
js_dart2js.dart for feedback on the verbose but efficient in dart2js JS API.
Once that API looks reasonable I will implement in ddc and dartium as well.
ddc impl should be trivial.
Basically, this API is a workaround for avoiding exposing the JS(...) API for
review.

[Jacob, 2015-09-02 22:07:28]

Known issue with the CL:
in a number of places I check for both isJsInterop and isNative even though I
elsewhere set isNative to true if isJsInterop is true.
Let me know whether it makes sense to try to set isNative to false for these
classes. I wasn't able to shake out bugs I was hitting if I didn't set isNative
to true for these not quite really native classes.

[Jennifer Messerly, 2015-09-02 22:29:12]

I looked at js_dart2js.dart, some questions/comments

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:101: export 'dart:_interceptors' show
Interceptor, JavaScriptObject, JSArray;
now that these are public, how are they intended to be used?

IMO it's worth checking if their documentation and public methods reflect their
intended public use case.

for what it's worth, DDC does not have an "Interceptor" or "JavaScriptObject"
... we do have a JSArray though.

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:109: 'return function() {'
style suggestion: add spaces inside the string literals. keeps the indentation
looking nice while still being able to use dart_style formatter.

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:685: dest[name] = src[name];
fyi, this won't work with all props. you'd want
Object.defineProperty/Object.getOwnProperty
but I guess you only care about value properties? In that case it works.

possibly change the method name?

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:791: /// Zero overhead, unchecked, and
verbose low level JavaScript interop API.
should this say "not verbose" ?

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:798: // WARNING: behavior for UNDEFINED is
undefined for the following cases:
did you mean for this to be a doc comment?

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:806: // Helpers for significantly more
efficient JavaScript object literal creation that jsify.
long line

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:817: static newLiteral1(String n0, v0) =>
JS('JavaScriptObject', '{#:#}', n0, v0);
are these intended for end users? how many do we need?
past a certain number, it might be easier to convert a Dart map, or assign the
additional properties one by one...

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:1113: '{#:#, #:#, #:#, #:#, #:#, #:#, #:#,
#:#, #:#, #:#, #:#, #:#, #:#, #:#}',
long line

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:2701: // These helpers all assume that
optional arguments are specified by UNDEFINED.
long line

[Jacob, 2015-09-02 22:49:56]

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:101: export 'dart:_interceptors' show
Interceptor, JavaScriptObject, JSArray;
On 2015/09/02 22:29:12, John Messerly wrote:
> now that these are public, how are they intended to be used?
> 
> IMO it's worth checking if their documentation and public methods reflect
their
> intended public use case.
> 
> for what it's worth, DDC does not have an "Interceptor" or "JavaScriptObject"
> ... we do have a JSArray though.

Having both Interceptor and JavaScriptObject be exposed seems like an error. I
think I should remove Interceptor assuming that doesn't break anything.

The intended usage is users write.
@JsName()
class MyJsClass extends Interceptor {
  someMethod() => ...;
  get someProperty => ...; 
}

and then those methods can be called on any JS object in Dart2JS because we add
the methods to the root Interceptor.
You could also add them to other JS classes. E.g.
@JsName()
class MyEvent extends html.Event {
  get awesomeProperty => JsNative.getProperty(this, 'awesomeProperty'); 
}
technically you could still get awesomeProperty on any JS object but in the
future we could choose to only add it to the Interceptor for Event instead of to
all interceptors.

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:109: 'return function() {'
On 2015/09/02 22:29:12, John Messerly wrote:
> style suggestion: add spaces inside the string literals. keeps the indentation
> looking nice while still being able to use dart_style formatter.

what about just using """
so that you can have both... or getting the formatter to leave you alone if you
have text formatted with this multi-line string format.

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:685: dest[name] = src[name];
On 2015/09/02 22:29:12, John Messerly wrote:
> fyi, this won't work with all props. you'd want
> Object.defineProperty/Object.getOwnProperty
> but I guess you only care about value properties? In that case it works.
> 
> possibly change the method name?

yep. I only care about value properties. Got an idea for a name? This method is
a private helper so I don't think the name has to be that precise and the
current method does copy all properties specified by getOwnPropertyNames it will
just not make a live copy if some happen to be complex properties.

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:791: /// Zero overhead, unchecked, and
verbose low level JavaScript interop API.
On 2015/09/02 22:29:12, John Messerly wrote:
> should this say "not verbose" ?

this API is extremely verbose for the person using it. Modified the comment to
clarify that it is verbose for the user.

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:798: // WARNING: behavior for UNDEFINED is
undefined for the following cases:
On 2015/09/02 22:29:12, John Messerly wrote:
> did you mean for this to be a doc comment?

Done.

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:806: // Helpers for significantly more
efficient JavaScript object literal creation that jsify.
On 2015/09/02 22:29:12, John Messerly wrote:
> long line

Done.

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:817: static newLiteral1(String n0, v0) =>
JS('JavaScriptObject', '{#:#}', n0, v0);
On 2015/09/02 22:29:12, John Messerly wrote:
> are these intended for end users? how many do we need?
> past a certain number, it might be easier to convert a Dart map, or assign the
> additional properties one by one...

These are for code generators.
In dart2js they are awesome in terms of code size. The output is a terse JS
literal.
30 was used because the maximum I needed for some large test JS code bases was
22 so I went up to 30 to keep a bit of headroom.
This is much more efficient than using a Dart map and then copying and is also
better (or was a few years ago) for the JS VMs than assigning additional
properties 1 by 1.

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:1113: '{#:#, #:#, #:#, #:#, #:#, #:#, #:#,
#:#, #:#, #:#, #:#, #:#, #:#, #:#}',
On 2015/09/02 22:29:12, John Messerly wrote:
> long line
I see your long line comment and I don't care in this case. :)
This code is auto generated. If you would like these lines broken, file a
dartfmt bug. :)

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:2701: // These helpers all assume that
optional arguments are specified by UNDEFINED.
On 2015/09/02 22:29:12, John Messerly wrote:
> long line

Done.

[Jennifer Messerly, 2015-09-02 22:54:56]

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:109: 'return function() {'
On 2015/09/02 22:49:55, Jacob wrote:
> On 2015/09/02 22:29:12, John Messerly wrote:
> > style suggestion: add spaces inside the string literals. keeps the
indentation
> > looking nice while still being able to use dart_style formatter.
> 
> what about just using """
> so that you can have both... or getting the formatter to leave you alone if
you
> have text formatted with this multi-line string format.

''' would work for me too

https://codereview.chromium.org/1318043005/diff/60001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:1113: '{#:#, #:#, #:#, #:#, #:#, #:#, #:#,
#:#, #:#, #:#, #:#, #:#, #:#, #:#}',
On 2015/09/02 22:49:55, Jacob wrote:
> On 2015/09/02 22:29:12, John Messerly wrote:
> > long line
> I see your long line comment and I don't care in this case. :)
> This code is auto generated. If you would like these lines broken, file a
> dartfmt bug. :)

I did, but it was wontfix'd
https://github.com/dart-lang/dart_style/issues/74

[Jennifer Messerly, 2015-09-03 19:54:17]

fwiw, js_dart2js LGTM

[alexandre.ardhuin, 2015-09-03 20:14:11]

alexandre.ardhuin@gmail.com changed reviewers:
+ alexandre.ardhuin@gmail.com

[alexandre.ardhuin, 2015-09-03 20:14:12]

Do you plan to handle abstract members of objects inheriting from
JavaScriptObject  (as in https://codereview.chromium.org/1194643002/) ?

https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:1280: if (type.element != null &&
(type.element.isNative || type.element.isJsInterop)) {
nit: line length

https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:1476: return (element.isNative ||
element.isJsInterop) ? jsInterceptorClass : compiler.objectClass;
nit: line length

https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:3011: if (type.element != null &&
(type.element.isNative || type.element.isJsInterop)) {
nit: line length

https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:476: new
ClassStubGenerator(_compiler, namer, backend);
There are some strange indentations in the rest of this file.

https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:804: static identical(a, b) => JS('bool',
'#===#', a, b);
return a bool

https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:810: static hasOwnProperty(object, name) =>
return a bool

https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:811: JS('', '#.hasOwnProperty(#)', object,
name);
JS('bool', .... )  ?

https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
sdk/lib/js/dart2js/js_dart2js.dart:812: static hasProperty(object, name) =>
JS('', '# in #', object, name);
return a bool and JS('bool',...) ?

[Jacob, 2015-09-03 23:48:44]

On 2015/09/03 20:14:12, alexandre.ardhuin wrote:
> Do you plan to handle abstract members of objects inheriting from
> JavaScriptObject  (as in https://codereview.chromium.org/1194643002/) ?
> 
>
https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
> File pkg/compiler/lib/src/js_backend/backend.dart (right):
> 
>
https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js_backend/backend.dart:1280: if (type.element != null &&
> (type.element.isNative || type.element.isJsInterop)) {
> nit: line length
> 
>
https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js_backend/backend.dart:1476: return (element.isNative ||
> element.isJsInterop) ? jsInterceptorClass : compiler.objectClass;
> nit: line length
> 
>
https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js_backend/backend.dart:3011: if (type.element != null &&
> (type.element.isNative || type.element.isJsInterop)) {
> nit: line length
> 
>
https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
> File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
> (right):
> 
>
https://codereview.chromium.org/1318043005/diff/80001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:476: new
> ClassStubGenerator(_compiler, namer, backend);
> There are some strange indentations in the rest of this file.
> 
>
https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
> File sdk/lib/js/dart2js/js_dart2js.dart (right):
> 
>
https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
> sdk/lib/js/dart2js/js_dart2js.dart:804: static identical(a, b) => JS('bool',
> '#===#', a, b);
> return a bool
> 
>
https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
> sdk/lib/js/dart2js/js_dart2js.dart:810: static hasOwnProperty(object, name) =>
> return a bool
> 
>
https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
> sdk/lib/js/dart2js/js_dart2js.dart:811: JS('', '#.hasOwnProperty(#)', object,
> name);
> JS('bool', .... )  ?
> 
>
https://codereview.chromium.org/1318043005/diff/80001/sdk/lib/js/dart2js/js_d...
> sdk/lib/js/dart2js/js_dart2js.dart:812: static hasProperty(object, name) =>
> JS('', '# in #', object, name);
> return a bool and JS('bool',...) ?

The members of objects inheriting from JavaScriptObject using JsName are all
essentially abstract. I'll be cleaning up this CL specifying the class members
and object literal constructors using extern so the intent is clearer and the
implementation is simpler. The current stub methods are there just so that
Dart2Js doesn't generate errors and doesn't perform incorrect dead code
elimination due to concluding that methods will throw as a result of whole
program analysis.

[Jacob, 2015-09-17 00:58:18]

jacobr@google.com changed reviewers:
+ sra@google.com
- sra@chromium.org

[sra1, 2015-09-17 19:52:57]

What to others think about the design of the 'annotation language'?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/js_t...
File tests/html/js_typed_interop_test.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/js_t...
tests/html/js_typed_interop_test.dart:5: @JsName()
It looks odd to me when there is no name in @JsName()

The annotation expresses different things - that something is implemented in
JavaScript and certain attributes of that implementation.
Perhaps there should be an annotation for each thing.

@JsLibrary()   
@JsName('JSON')  // optional
library ...

maybe a short annotation with named optional arguments would be more expressive
and also extensible. (e.g. Maybe we will want to add a way inform the compiler
how to call, say Math.max, with multiple arguments.)

@Js()

@Js(name: 'JSON')

@Js(access: "JSON.stringify")
external String stringify(obj);

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/json...
File tests/html/json_helper.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/json...
tests/html/json_helper.dart:8: // TODO(jacobr): depend on package:js instead of
defining this annotation here.
dart2js needs to find a known definition that is maintained in step with 

We could have a 'js interop annotations' library in js_runtime that contains
just the declaration of the annotation.  dart2js can find this definition just
like it does for other 'special' elements.

This can be re-exported by package:js

[Siggi Cherem (dart-lang), 2015-09-18 20:34:09]

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

[Siggi Cherem (dart-lang), 2015-09-18 20:34:10]

This is very exciting Jacob

Some preliminary comments below. I still didn't get to do a full review of the
compiler changes though (I didn't get a chance to look in detail at the emitter
or ssa builder, for instance).

https://chromiumcodereview.appspot.com/1318043005/diff/60001/sdk/lib/js/dart2...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/60001/sdk/lib/js/dart2...
sdk/lib/js/dart2js/js_dart2js.dart:797: class JsNative {
I couldn't find this class in the latest patchset, is it gone?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
File pkg/compiler/lib/src/elements/modelx.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/elements/modelx.dart:254: if (!isConstructor)
sb.write(nameHelper(this));
are named constructors not supported?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/elements/modelx.dart:1840: if (super.isJsInterop) return
true;
are there any cases where this is true if the two conditions below are false? or
can we delete this line?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/elements/modelx.dart:1841: if (isClassMember) return
contextClass.isJsInterop && isExternal;
it might be worth checking for isExternal upfront, that way we skip all
non-external class members and top-levels

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:138: needed = true;  //
TODO(jacobr): we don't need all interop classes.
what are your thoughts on determining what is needed?

- types that are allocated from Dart
- types returned from interop-functions (using trust-type-annotations, or using
metadata to declare whether the return type of functions includes a new type not
seen in the Dart code?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:330: js.statement('return
#(#)', [js(target), arguments]));
style nit: might be nice to keep the statements.add at the end and only massage
the receiver part a bit. For example, could we turn the #.# into a js('#.#',
receiver, target) for the other cases?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
File pkg/compiler/lib/src/patch_parser.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/patch_parser.dart:410: if
(compiler.enqueuer.resolution.queueIsClosed) {
this is probably breaking the current enqueuer invariant, since calling
ensureResolved may add more work to the queue :-/

In what cases do you hit this?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/patch_parser.dart:413:
compiler.enqueuer.resolution.addDeferredAction(element, actionCallback);
nit: long line

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/patch_parser.dart:470: String
dietResolveStringToken(StringToken token) {
could we switch to use the constant evaluation instead? That way you don't have
to assume that the string token is in the annotation.

For example, this should be OK:

  const _foo = 'my.name';

  @JsName(_foo)
  class MyName extends JavascriptObject {
    get jsName => _foo;
  }

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/ssa/builder.dart:1514: * interop methods to match
JavaScript semantics for ommitted arguments.
completely interop => completely from interop?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/ssa/builder.dart:1617:
push(invokeJsInteropFunction(functionElement, parameters.values.toList(),
sourceInformationBuilder.buildGeneric(function)));
long lines (here and below)

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/ssa/builder.dart:1620:
sourceInformationBuilder.buildReturn(functionElement.node)));
indent

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/_intern...
File sdk/lib/_internal/js_runtime/lib/interceptors.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/_intern...
sdk/lib/_internal/js_runtime/lib/interceptors.dart:419: } catch(e) {
under what circumstances do you expect an exception?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/js/dart...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/js/dart...
sdk/lib/js/dart2js/js_dart2js.dart:108: 'function(_call, f, captureThis) {'
+1 on using ''' to restore indentation

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/js/dart...
sdk/lib/js/dart2js/js_dart2js.dart:695: // libraries are imported. Remove after
Dartium is modified to function without
would it make sense to use tree-shakeable lookup-maps to describe dependencies
between jsinterop types instead?

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/js/dart...
sdk/lib/js/dart2js/js_dart2js.dart:697: void registerJsInterfaces([List<Type>
types]) {
I seem to be missing something, is `types` used anywhere? Only in the dartium
implementation?

You could also change this implementation to make it easier for dart2js to
inline it and decide that `types` is not used. For example, just doing a
forwarding call like this:

  registerJsInterfaces([types]) => _registerFunctionHacks();
  _registerFunctionHacks() {
   ...
  }

might be enough.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/js_t...
File tests/html/js_typed_interop_test.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/js_t...
tests/html/js_typed_interop_test.dart:5: @JsName()
On 2015/09/17 19:52:57, sra1 wrote:
> It looks odd to me when there is no name in @JsName()
> 
> The annotation expresses different things - that something is implemented in
> JavaScript and certain attributes of that implementation.
> Perhaps there should be an annotation for each thing.
> 
> @JsLibrary()   
> @JsName('JSON')  // optional
> library ...
> 
> maybe a short annotation with named optional arguments would be more
expressive
> and also extensible. (e.g. Maybe we will want to add a way inform the compiler
> how to call, say Math.max, with multiple arguments.)
> 
> @Js()
> 
> @Js(name: 'JSON')
> 
> @Js(access: "JSON.stringify")
> external String stringify(obj);
> 

I had a similar reaction - @JsName() seems like it's missing something.

One question: what other annotations do we expect to have in addition to this
one? Are we planning to have code-gen of stubs like Justin and Alexander did a
while back, or does that become completely unnecessary in the future?

That might have an influence on the names we pick, to make sure we clearly
distinguish between those and the interop annotations here.

I like @Js() and @Js('name.in.a.string'), but I'd also be OK with different
annotations for different kind of elements:

@JsLibrary('JSON')
library ...

@JsClass('Foo')
class A {}

@JsFunction('foo.bar')
external _fooBar();

If `Js` seems too generic, other ideas are: @JsApi(), @FromJs()

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/json...
File tests/html/json_helper.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/json...
tests/html/json_helper.dart:8: // TODO(jacobr): depend on package:js instead of
defining this annotation here.
On 2015/09/17 19:52:57, sra1 wrote:
> dart2js needs to find a known definition that is maintained in step with 
> 
> We could have a 'js interop annotations' library in js_runtime that contains
> just the declaration of the annotation.  dart2js can find this definition just
> like it does for other 'special' elements.
> 
> This can be re-exported by package:js

how can package:js access this internal library? would it have to be a
configuration-specific package that only works in dart2js?

What if we do something similar to what we did for package:lookup_map?

[Jacob, 2015-10-01 00:47:34]

ptal

https://chromiumcodereview.appspot.com/1318043005/diff/60001/sdk/lib/js/dart2...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/60001/sdk/lib/js/dart2...
sdk/lib/js/dart2js/js_dart2js.dart:797: class JsNative {
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> I couldn't find this class in the latest patchset, is it gone?

It is gone. JsNative isn't needed now that extern constructors and statics are
handled directly by the compiler.
What motivated that change was Sra pointed out how a lot of the infrastructure
was already in place (coincidentally existing annotation name JSName) and that
newLiteralN as written only worked if the code was inlined by dart2js.

https://chromiumcodereview.appspot.com/1318043005/diff/80001/sdk/lib/js/dart2...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/80001/sdk/lib/js/dart2...
sdk/lib/js/dart2js/js_dart2js.dart:804: static identical(a, b) => JS('bool',
'#===#', a, b);
On 2015/09/03 20:14:11, alexandre.ardhuin wrote:
> return a bool

Done.

https://chromiumcodereview.appspot.com/1318043005/diff/80001/sdk/lib/js/dart2...
sdk/lib/js/dart2js/js_dart2js.dart:810: static hasOwnProperty(object, name) =>
On 2015/09/03 20:14:11, alexandre.ardhuin wrote:
> return a bool

Done.

https://chromiumcodereview.appspot.com/1318043005/diff/80001/sdk/lib/js/dart2...
sdk/lib/js/dart2js/js_dart2js.dart:811: JS('', '#.hasOwnProperty(#)', object,
name);
On 2015/09/03 20:14:11, alexandre.ardhuin wrote:
> JS('bool', .... )  ?

Done.

https://chromiumcodereview.appspot.com/1318043005/diff/80001/sdk/lib/js/dart2...
sdk/lib/js/dart2js/js_dart2js.dart:812: static hasProperty(object, name) =>
JS('', '# in #', object, name);
On 2015/09/03 20:14:11, alexandre.ardhuin wrote:
> return a bool and JS('bool',...) ?

Done.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
File pkg/compiler/lib/src/elements/modelx.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/elements/modelx.dart:254: if (!isConstructor)
sb.write(nameHelper(this));
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> are named constructors not supported?

they are supported.
Suppose you have a JavaScript class GoogleMap.

you could write
@Js
class GoogleMap {
  external GoogleMap(num lat, num long);
  external GoogleMap.fromPoint(Point p);
}

Both external constructors will compile down to calls to JS
new GoogleMap(someArgs...);

For factory constructors it could be reasonable to allow specifying a custom JS
name for the JavaScript static method that is invoked but I don't think that is
really worth it. Currently factory constructors are being interpreted as
generating a JS literal but it is a bit adhoc.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/elements/modelx.dart:1840: if (super.isJsInterop) return
true;
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> are there any cases where this is true if the two conditions below are false?
or
> can we delete this line?

yes. super.isJsInterop checks whether the member itself has a @Js annotation.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/elements/modelx.dart:1841: if (isClassMember) return
contextClass.isJsInterop && isExternal;
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> it might be worth checking for isExternal upfront, that way we skip all
> non-external class members and top-levels 

 done.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:138: needed = true;  //
TODO(jacobr): we don't need all interop classes.
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> what are your thoughts on determining what is needed?
> 
> - types that are allocated from Dart
> - types returned from interop-functions (using trust-type-annotations, or
using
> metadata to declare whether the return type of functions includes a new type
not
> seen in the Dart code?

types allocated from Dart isn't adequate
types returned from interop-functions could work as long as we also include
types returned from "as" checks. That still might not be sufficient. BTW, while
the JS interop classes are "needed" we don't really need to generate metadata
for them as at the end of the day all that matters is that the correct
interceptors are added to JavaScriptObject.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:330: js.statement('return
#(#)', [js(target), arguments]));
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> style nit: might be nice to keep the statements.add at the end and only
massage
> the receiver part a bit. For example, could we turn the #.# into a js('#.#',
> receiver, target) for the other cases?

yeah this code was ugly. done.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
File pkg/compiler/lib/src/patch_parser.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/patch_parser.dart:410: if
(compiler.enqueuer.resolution.queueIsClosed) {
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> this is probably breaking the current enqueuer invariant, since calling
> ensureResolved may add more work to the queue :-/
> 
> In what cases do you hit this?

this is somewhat horrible but somewhat acceptable according to sra.
The reason is calling annotation.ensureResolved(compiler) should essentially act
as an assert if we are calling this too late.

here's the stack trace I get when I tear out hack.
#0      ResolutionEnqueuer.addDeferredAction
(package:compiler/src/enqueue.dart:883:7)
#1      EagerAnnotationHandler.checkAnnotation
(package:compiler/src/patch_parser.dart:406:38)
#2      checkJsInteropAnnotation (package:compiler/src/patch_parser.dart:366:26)
#3      NativeEnqueuerBase.processJsInteropAnnotationsInLibrary
(package:compiler/src/native/enqueue.dart:151:5)
#4      Iterable.forEach (dart:core/iterable.dart:217)
#5      NativeEnqueuerBase.processNativeClasses
(package:compiler/src/native/enqueue.dart:142:15)
#6      NativeCodegenEnqueuer.processNativeClasses
(package:compiler/src/native/enqueue.dart:653:31)
#7      Compiler.processQueue (package:compiler/src/compiler.dart:1221:26)
#8      Compiler.compileLoadedLibraries
(package:compiler/src/compiler.dart:1165:5)
#9      Compiler.runCompiler.<anonymous closure>
(package:compiler/src/compiler.dart:979:7)
#10     _RootZone.runUnary (dart:async/zone.dart:1166)
#11     _Future._propagateToListeners.handleValueCallback
(dart:async/future_impl.dart:494)
#12     _Future._propagateToListeners (dart:async/future_impl.dart:577)
#13     _Future._completeWithValue (dart:async/future_impl.dart:368)
#14     _Future._asyncComplete.<anonymous closure>
(dart:async/future_impl.dart:422)
#15     _microtaskLoop (dart:async/schedule_microtask.dart:43)
#16     _microtaskLoopEntry (dart:async/schedule_microtask.dart:52)
#17     _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:96)
#18     _RawReceivePortImpl._handleMessage
(dart:isolate-patch/isolate_patch.dart:149)


Added a TODO that annotation resolution needs to be refactored.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/patch_parser.dart:413:
compiler.enqueuer.resolution.addDeferredAction(element, actionCallback);
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> nit: long line

Done.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/patch_parser.dart:470: String
dietResolveStringToken(StringToken token) {
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> could we switch to use the constant evaluation instead? That way you don't
have
> to assume that the string token is in the annotation.
> 
> For example, this should be OK:
> 
>   const _foo = 'my.name';
> 
>   @JsName(_foo)
>   class MyName extends JavascriptObject {
>     get jsName => _foo;
>   }
> 
The trouble is getJsNameAnnotation is currently called before const evaluation
is possible unless I'm missing something.
I could modify the ordering so we fill in the value for jsName on the later call
to validate. Let me know if you think that is worth it and you give me a
breadcrumb pointing at the const evaluation code.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/ssa/builder.dart:1514: * interop methods to match
JavaScript semantics for ommitted arguments.
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> completely interop => completely from interop?

Done.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/ssa/builder.dart:1617:
push(invokeJsInteropFunction(functionElement, parameters.values.toList(),
sourceInformationBuilder.buildGeneric(function)));
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> long lines (here and below)

Done.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/pkg/compiler/li...
pkg/compiler/lib/src/ssa/builder.dart:1620:
sourceInformationBuilder.buildReturn(functionElement.node)));
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/_intern...
File sdk/lib/_internal/js_runtime/lib/interceptors.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/_intern...
sdk/lib/_internal/js_runtime/lib/interceptors.dart:419: } catch(e) {
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> under what circumstances do you expect an exception?

Just matching the toString() method that was in dart:js
Removed this as it really isn't neeeded as far as we can figure out.
looks like this code was originally copied from 
https://github.com/dart-lang/js-interop-experimental/commit/fcdcbdd2ddb06a4e9...
maybe there is an edge case we are forgetting but I doubt it.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/js/dart...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/js/dart...
sdk/lib/js/dart2js/js_dart2js.dart:108: 'function(_call, f, captureThis) {'
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> +1 on using ''' to restore indentation

this file is now automatically formatted so please send your whitespace style
complaints to Bob :)

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/js/dart...
sdk/lib/js/dart2js/js_dart2js.dart:697: void registerJsInterfaces([List<Type>
types]) {
This was used in Dartium. I've ripped it out there and just traversed all
libraries using mirrors. I think this method should get removed completely and
this code should be emitted directly in codegen if the new JS interop is used at
all. Sound reasonable?
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> I seem to be missing something, is `types` used anywhere? Only in the dartium
> implementation?
> 
> You could also change this implementation to make it easier for dart2js to
> inline it and decide that `types` is not used. For example, just doing a
> forwarding call like this:
> 
>   registerJsInterfaces([types]) => _registerFunctionHacks();
>   _registerFunctionHacks() {
>    ...
>   }
> 
> might be enough.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/js_t...
File tests/html/js_typed_interop_test.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/js_t...
tests/html/js_typed_interop_test.dart:5: @JsName()
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> On 2015/09/17 19:52:57, sra1 wrote:
> > It looks odd to me when there is no name in @JsName()
> > 
> > The annotation expresses different things - that something is implemented in
> > JavaScript and certain attributes of that implementation.
> > Perhaps there should be an annotation for each thing.
> > 
> > @JsLibrary()   
> > @JsName('JSON')  // optional
> > library ...
> > 
> > maybe a short annotation with named optional arguments would be more
> expressive
> > and also extensible. (e.g. Maybe we will want to add a way inform the
compiler
> > how to call, say Math.max, with multiple arguments.)
> > 
> > @Js()
> > 
> > @Js(name: 'JSON')
> > 
> > @Js(access: "JSON.stringify")
> > external String stringify(obj);
> > 
> 
> I had a similar reaction - @JsName() seems like it's missing something.
> 
> One question: what other annotations do we expect to have in addition to this
> one? Are we planning to have code-gen of stubs like Justin and Alexander did a
> while back, or does that become completely unnecessary in the future?
> 
> That might have an influence on the names we pick, to make sure we clearly
> distinguish between those and the interop annotations here.
> 
> I like @Js() and @Js('name.in.a.string'), but I'd also be OK with different
> annotations for different kind of elements:
> 
> @JsLibrary('JSON')
> library ...
> 
> @JsClass('Foo')
> class A {}
> 
> @JsFunction('foo.bar')
> external _fooBar();
> 
> If `Js` seems too generic, other ideas are: @JsApi(), @FromJs()
switched to @Js

there will probably be other annotations but not many. 

Modifying the behavior of a JS class is now an explicit non-goal as it seems
hard to implement efficiently without wrappers in Dart2Js and the goal is to
expose existing JS libraries not to beautify them.

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/json...
File tests/html/json_helper.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/json...
tests/html/json_helper.dart:8: // TODO(jacobr): depend on package:js instead of
defining this annotation here.
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> On 2015/09/17 19:52:57, sra1 wrote:
> > dart2js needs to find a known definition that is maintained in step with 
> > 
> > We could have a 'js interop annotations' library in js_runtime that contains
> > just the declaration of the annotation.  dart2js can find this definition
just
> > like it does for other 'special' elements.
> > 
> > This can be re-exported by package:js
> 
> how can package:js access this internal library? would it have to be a
> configuration-specific package that only works in dart2js?
> 
> What if we do something similar to what we did for package:lookup_map?

thanks for the link to what you did for lookup_map! I've duplicated what you did
for that package and moved "0.6.0" of package:js into the repo. I didn't bother
with a lot of complex versioning checks as the package:js only contains
annotations and not actual implementation.

[alexandre.ardhuin, 2015-10-01 08:51:05]

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/js_t...
File tests/html/js_typed_interop_test.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/js_t...
tests/html/js_typed_interop_test.dart:5: @JsName()
> Modifying the behavior of a JS class is now an explicit non-goal as it seems
> hard to implement efficiently without wrappers in Dart2Js and the goal is to
> expose existing JS libraries not to beautify them.

:( That means the dart libs that proxified Js lib will not be able to follow the
Dart style guide without adding dart wrappers.

For instance in
https://developers.google.com/maps/documentation/javascript/3.exp/reference#Map
you can see a lot of `getXxx()` that would be more darty with `get xxx;`
(https://www.dartlang.org/articles/style-guide/#do-use-a-getter-for-operations...).

https://chromiumcodereview.appspot.com/1318043005/diff/140001/tests/html/js_a...
File tests/html/js_array_test.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/140001/tests/html/js_a...
tests/html/js_array_test.dart:167: class SimpleJsLiteralClass extends
JavaScriptObject {
I can't find where this class is used.

https://chromiumcodereview.appspot.com/1318043005/diff/140001/tests/html/js_t...
File tests/html/js_typed_interop_test.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/140001/tests/html/js_t...
tests/html/js_typed_interop_test.dart:91: class Foo extends JavaScriptObject {
I didn't understand quickly why some objects were extending `JavaScriptObject`.
Could you add a comment to explain that extending JavaScriptObject allows to
make any dynamic calls with `(foo as dynamic).xxx` or by adding `@proxy` on the
class.

[sra1, 2015-10-01 17:34:30]

https://codereview.chromium.org/1318043005/diff/120001/pkg/compiler/lib/src/s...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1318043005/diff/120001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:5763: var templateString;
The rest of this file uses local types.
We have found it useful for catching errors.
Please do the same.

https://codereview.chromium.org/1318043005/diff/120001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:5771: FunctionSignature params = (element
as ConstructorElement).functionSignature;
The dart2js way is to use a local declaration: 

ConstructorElement constructorElement = element;

https://codereview.chromium.org/1318043005/diff/120001/sdk/lib/js/dart2js/js_...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/120001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:108: 'function(_call, f, captureThis) {'
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> +1 on using ''' to restore indentation

When this was originally written JS didn't parse functions so the indented
strings reduced space in the output.

Now we parse the whole thing, so use whitespace liberally to make it look nice.

[Siggi Cherem (dart-lang), 2015-10-01 18:21:28]

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/js/dart...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/sdk/lib/js/dart...
sdk/lib/js/dart2js/js_dart2js.dart:108: 'function(_call, f, captureThis) {'
On 2015/10/01 17:34:30, sra1 wrote:
> On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> > +1 on using ''' to restore indentation
> 
> When this was originally written JS didn't parse functions so the indented
> strings reduced space in the output.
> 
> Now we parse the whole thing, so use whitespace liberally to make it look
nice.

to clarify, what I meant was to use the triple-' going forward, which dartfmt
leaves alone :)

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/json...
File tests/html/json_helper.dart (right):

https://chromiumcodereview.appspot.com/1318043005/diff/120001/tests/html/json...
tests/html/json_helper.dart:8: // TODO(jacobr): depend on package:js instead of
defining this annotation here.
On 2015/10/01 00:47:34, Jacob wrote:
> On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> > On 2015/09/17 19:52:57, sra1 wrote:
> > > dart2js needs to find a known definition that is maintained in step with 
> > > 
> > > We could have a 'js interop annotations' library in js_runtime that
contains
> > > just the declaration of the annotation.  dart2js can find this definition
> just
> > > like it does for other 'special' elements.
> > > 
> > > This can be re-exported by package:js
> > 
> > how can package:js access this internal library? would it have to be a
> > configuration-specific package that only works in dart2js?
> > 
> > What if we do something similar to what we did for package:lookup_map?
> 
> thanks for the link to what you did for lookup_map! I've duplicated what you
did
> for that package and moved "0.6.0" of package:js into the repo. I didn't
bother
> with a lot of complex versioning checks as the package:js only contains
> annotations and not actual implementation.

Makes sense you don't need to validate the version of the entire package. Let's
see if there is a way you can use the constant rather than trying to parse out
the annotation from the syntax. If we can manage that, it would make sense to
also add some logic to validate that the annotation looks correct (e.g. check
that the `name` field exists, otherwise, issue an error).

[sra1, 2015-10-01 20:55:29]

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/e...
File pkg/compiler/lib/src/elements/modelx.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:237: String _jsNameHelper(e) {
Element e
or some more specific type.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:251: String get fixedBackendReceiver {
All this 'backend' logic must be in the js_backend.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:262: return sb.toString();
So what is this computing?

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
File pkg/compiler/lib/src/inferrer/node_tracer.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/node_tracer.dart:190: bailout('Used in JS Interop
${info.call}');
Seems harsh.
Describe what we want to happen here in a TODO.

I am worried that this is 'covering over' a lot of problems.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
File pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:1615: if
(element.isJsInterop) return types.dynamicType;
Maybe explain why.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:2102: 
Remove this empty line.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:2104: 
You could keep this line or remove it so there is no gratuitous edit to this
method

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:2134: 
Remove added empty line

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:1479: if (element.isJsInterop)
return jsPlainJavaScriptObjectClass;
jsJavaScriptObjectClass

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/interceptor_stub_generator.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/interceptor_stub_generator.dart:144: && typeof
receiver != "function" ) return receiver;
This is a hot path. Make the most of every test.

Can we avoid the very common instanceof test if js functions are never normal
Dart objects?

    if (typeof receiver != "object") {
      if (typeof receiver == "function") return #;
      return receiver;
    }

The additional # placeholder gets

  interceptorFor(jsJavaScriptFunctionClass)

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:329: receiver =
js(member.fixedBackendReceiver);
Use uncached version.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:206:
_buildJsInteropStubs(librariesMap);
_addJsInteropStubs()
By convention _buildXXX returns the thing, and does minimal damage.

Add the stubs before calling _buildMainFragment.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:403: }
You can just use strings of names.

var parameters = new List<String>.generate(argumentCount,
  (i) => 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLOMOPQRSTUVWXYZ'[i]);

This will crash rather than generate bogus names for too many names.

----

Iterable<String> interpolates to multiple Parameters in parameter lists and
multiple VariableUse in argument lists.

(js.Parameter is a 'declaration' so should really only appear in function
parameter lists).

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:406:
js.js('function(obj, #) { return obj.#(#) }',
obj -> receiver for consistency with other stubs.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:408:
element: member));
indentation is off

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:476: new
ClassStubGenerator(_compiler, namer, backend);
repair indentation

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:478: new
RuntimeTypeGenerator(_compiler, _task, namer);
ditto

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:495:
classStubGenerator.generateCallStubsForGetter(member, selectors);
ditto

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:504:
runtimeTypeGenerator.generateTypeVariableReaderStubs(element);
ditto

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:510:
classStubGenerator.computeSelectorsForNsmHandlers();
ditto

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:520:
selector));
ditto

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:541:
onlyForRti ? const <Field>[] : _buildFields(element, false);
ditto

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:568: if
(element.isJsInterop) {
use one of element.isJsInterop or onlyJsInterop consistently.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:569: //
TODO(jacobr): make sure we aren't adding the same is checks multiple times..
line lengyh

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/n...
File pkg/compiler/lib/src/native/native.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/n...
pkg/compiler/lib/src/native/native.dart:36: checkJsInteropAnnotation;
Seems like the wrong place for this.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/t...
File pkg/compiler/lib/src/types/types.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/t...
pkg/compiler/lib/src/types/types.dart:298: if (element.isJsInterop) {
This is a complete hack.
This code is only called to query the inference result and not to generate it.
We should do this in a way that allows us to believe some of the types.

If you want to leave this for a while, it needs a TODO(issueNumber)

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/t...
pkg/compiler/lib/src/types/types.dart:314: }
ditto

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/LICENSE
File pkg/js/LICENSE (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/LICENSE#newcode1
pkg/js/LICENSE:1: Copyright 2012, the Dart project authors. All rights reserved.
Really?

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/lib/js.dart
File pkg/js/lib/js.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/lib/js.dart#new...
pkg/js/lib/js.dart:17: /// instance members.
Comment needs examples.

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/lib/js.dart#new...
pkg/js/lib/js.dart:24: /// object. That is it is created with `new Object()`.
Say why this is needed.
Add example.

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:107: '',
fix indentation

  return JS('', r'''
      function(...) {
        ...
      }(#, #, #)''',
      foo,
      ...);

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:662: [a = const JS_CONST('void 0'),
probably best to give this a name

const _UNDEFINED = const JS_CONST('void 0');

  call([a = _UNDEFINED, b = _UNDEFINED, ...])

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:674: _stripUndefinedArgs([a, b, c, d, e, f,
g, h, i, j]));
I can't find this function.

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:680: '',
Why line-break?

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:723: _convertDartFunctionFast(Function f,
{bool captureThis: false}) {
captureThis is unused

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:728: 'function(_call, f) {'
Use multiline string to fix indentation

[Jacob, 2015-10-02 20:08:16]

https://codereview.chromium.org/1318043005/diff/120001/pkg/compiler/lib/src/s...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1318043005/diff/120001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:5763: var templateString;
On 2015/10/01 17:34:30, sra1 wrote:
> The rest of this file uses local types.
> We have found it useful for catching errors.
> Please do the same.

Done. Tried to match the general style of builder.dart

https://codereview.chromium.org/1318043005/diff/120001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:5771: FunctionSignature params = (element
as ConstructorElement).functionSignature;
On 2015/10/01 17:34:30, sra1 wrote:
> The dart2js way is to use a local declaration: 
> 
> ConstructorElement constructorElement = element;

Done.

https://codereview.chromium.org/1318043005/diff/120001/sdk/lib/js/dart2js/js_...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/120001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:108: 'function(_call, f, captureThis) {'
On 2015/10/01 18:21:28, Siggi Cherem (dart-lang) wrote:
> On 2015/10/01 17:34:30, sra1 wrote:
> > On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> > > +1 on using ''' to restore indentation
> > 
> > When this was originally written JS didn't parse functions so the indented
> > strings reduced space in the output.
> > 
> > Now we parse the whole thing, so use whitespace liberally to make it look
> nice.
> 
> to clarify, what I meant was to use the triple-' going forward, which dartfmt
> leaves alone :)
fine. I switched to ''' and did a bunch of white space deck chair moving.

https://codereview.chromium.org/1318043005/diff/120001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:695: // libraries are imported. Remove after
Dartium is modified to function without
On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> would it make sense to use tree-shakeable lookup-maps to describe dependencies
> between jsinterop types instead?

could be. moved away from requiring this method instead.

https://codereview.chromium.org/1318043005/diff/120001/tests/html/js_typed_in...
File tests/html/js_typed_interop_test.dart (right):

https://codereview.chromium.org/1318043005/diff/120001/tests/html/js_typed_in...
tests/html/js_typed_interop_test.dart:5: @JsName()
On 2015/10/01 08:51:05, alexandre.ardhuin wrote:
> > Modifying the behavior of a JS class is now an explicit non-goal as it seems
> > hard to implement efficiently without wrappers in Dart2Js and the goal is to
> > expose existing JS libraries not to beautify them.
> 
> :( That means the dart libs that proxified Js lib will not be able to follow
the
> Dart style guide without adding dart wrappers.
> 
> For instance in
>
https://developers.google.com/maps/documentation/javascript/3.exp/reference#Map
> you can see a lot of `getXxx()` that would be more darty with `get xxx;`
>
(https://www.dartlang.org/articles/style-guide/#do-use-a-getter-for-operations...).

There are some significant tradeoffs at play that lead to this design. Mapping
get xxx to getXxx for some JS classes comes with a real performance and/or code
size hit. In addition, this is a feature needed largely for older JS libraries
designed before ES5 was widely available. I think it is better to provide fast
and automate-able importing of JS libraries to Dart rather than trying to make
JS libraries look like they were written by Dart users.

Even for libraries with "ugly" getXxx members some end users will prefer keeping
the methods as getXxx as that makes it much easier to reuse JS samples. For
dart:html we found that ugly is often better than pretty and harder to search
for/less familiar.


The Maps API looks that way likely because they needed to support ES3.  If you
want to make a version of the API designed with ES5+ in mind, those methods
would likely be getters.
If you want the Maps API to have a more modern shape, use the same technique you
would writing regular JavaScript and monkey patch the JavaScript properties you
want to add. 

If a particular JS library really needs to be more darty to be productively used
from Dart, monkey patch it on the JS side to be more darty the same way you
would modernize the JS library for JS users.
Note: in the future there may be an option to do this monkey patching in a
typesafe way using ES6 symbols in the way the Dart Dev Compiler currently makes
JS Array support the Dart List API.  At some point we may expose a similar
mechanism in Dart2Js to enable the case you describe. In any case, there is
still the real cost of adding these unlikely to be treeshaken methods. The
material difference in system behavior is really just whether the new properties
are visible from JS or hidden from JS users.

https://codereview.chromium.org/1318043005/diff/120001/tests/html/json_helper...
File tests/html/json_helper.dart (right):

https://codereview.chromium.org/1318043005/diff/120001/tests/html/json_helper...
tests/html/json_helper.dart:8: // TODO(jacobr): depend on package:js instead of
defining this annotation here.
On 2015/10/01 18:21:28, Siggi Cherem (dart-lang) wrote:
> On 2015/10/01 00:47:34, Jacob wrote:
> > On 2015/09/18 20:34:10, Siggi Cherem (dart-lang) wrote:
> > > On 2015/09/17 19:52:57, sra1 wrote:
> > > > dart2js needs to find a known definition that is maintained in step with

> > > > 
> > > > We could have a 'js interop annotations' library in js_runtime that
> contains
> > > > just the declaration of the annotation.  dart2js can find this
definition
> > just
> > > > like it does for other 'special' elements.
> > > > 
> > > > This can be re-exported by package:js
> > > 
> > > how can package:js access this internal library? would it have to be a
> > > configuration-specific package that only works in dart2js?
> > > 
> > > What if we do something similar to what we did for package:lookup_map?
> > 
> > thanks for the link to what you did for lookup_map! I've duplicated what you
> did
> > for that package and moved "0.6.0" of package:js into the repo. I didn't
> bother
> > with a lot of complex versioning checks as the package:js only contains
> > annotations and not actual implementation.
> 
> Makes sense you don't need to validate the version of the entire package.
Let's
> see if there is a way you can use the constant rather than trying to parse out
> the annotation from the syntax. If we can manage that, it would make sense to
> also add some logic to validate that the annotation looks correct (e.g. check
> that the `name` field exists, otherwise, issue an error).

got any pointers for how that should be done? There are other annotations such
as @Native that it would be better if dart2js evaluated instead of just parsing
so I'd rather not block this CL on that. After this lands I can start working on
a followup CL to reduce the technical debt processing annotations.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/e...
File pkg/compiler/lib/src/elements/modelx.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:237: String _jsNameHelper(e) {
On 2015/10/01 20:55:27, sra1 wrote:
> Element e
> or some more specific type.

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:251: String get fixedBackendReceiver {
On 2015/10/01 20:55:27, sra1 wrote:
> All this 'backend' logic must be in the js_backend.

moved to js_backend/namer.dart

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:262: return sb.toString();
On 2015/10/01 20:55:27, sra1 wrote:
> So what is this computing?

discussed offline and changed name to 
fixedBackendPath

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
File pkg/compiler/lib/src/inferrer/node_tracer.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/node_tracer.dart:190: bailout('Used in JS Interop
${info.call}');
On 2015/10/01 20:55:27, sra1 wrote:
> Seems harsh.
> Describe what we want to happen here in a TODO.
> 
> I am worried that this is 'covering over' a lot of problems.
Removed obsolete bailout hack.
This is a bailout I added when I was banging my head against why js interop type
inference was going wrong. Either this hack was never needed or I fixed
something along the way that made it obsolete. My tests till pass with minified
code without this check.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
File pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:1615: if
(element.isJsInterop) return types.dynamicType;
On 2015/10/01 20:55:27, sra1 wrote:
> Maybe explain why.

Hmm. I don't think this still made sense. Reverted all changes in this file.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:2104: 
On 2015/10/01 20:55:27, sra1 wrote:
> You could keep this line or remove it so there is no gratuitous edit to this
> method

Reverted accidental whitespace edits.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:2134: 
On 2015/10/01 20:55:27, sra1 wrote:
> Remove added empty line

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:1479: if (element.isJsInterop)
return jsPlainJavaScriptObjectClass;
On 2015/10/01 20:55:28, sra1 wrote:
> jsJavaScriptObjectClass

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/interceptor_stub_generator.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/interceptor_stub_generator.dart:144: && typeof
receiver != "function" ) return receiver;
On 2015/10/01 20:55:28, sra1 wrote:
> This is a hot path. Make the most of every test.
> 
> Can we avoid the very common instanceof test if js functions are never normal
> Dart objects?
> 
>     if (typeof receiver != "object") {
>       if (typeof receiver == "function") return #;
>       return receiver;
>     }
> 
> The additional # placeholder gets
> 
>   interceptorFor(jsJavaScriptFunctionClass)

I was concerned about this code as well. Added your optimization.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:329: receiver =
js(member.fixedBackendReceiver);
On 2015/10/01 20:55:28, sra1 wrote:
> Use uncached version.

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:206:
_buildJsInteropStubs(librariesMap);
On 2015/10/01 20:55:28, sra1 wrote:
> _addJsInteropStubs()
> By convention _buildXXX returns the thing, and does minimal damage.
> 
> Add the stubs before calling _buildMainFragment.

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:403: }
On 2015/10/01 20:55:28, sra1 wrote:
> You can just use strings of names.
> 
> var parameters = new List<String>.generate(argumentCount,
>   (i) => 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLOMOPQRSTUVWXYZ'[i]);
> 
> This will crash rather than generate bogus names for too many names.
> 
> ----
> 
> Iterable<String> interpolates to multiple Parameters in parameter lists and
> multiple VariableUse in argument lists.
> 
> (js.Parameter is a 'declaration' so should really only appear in function
> parameter lists).

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:406:
js.js('function(obj, #) { return obj.#(#) }',
On 2015/10/01 20:55:28, sra1 wrote:
> obj -> receiver for consistency with other stubs.

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:408:
element: member));
On 2015/10/01 20:55:28, sra1 wrote:
> indentation is off

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:476: new
ClassStubGenerator(_compiler, namer, backend);
On 2015/10/01 20:55:28, sra1 wrote:
> repair indentation

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:478: new
RuntimeTypeGenerator(_compiler, _task, namer);
On 2015/10/01 20:55:28, sra1 wrote:
> ditto

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:495:
classStubGenerator.generateCallStubsForGetter(member, selectors);
On 2015/10/01 20:55:28, sra1 wrote:
> ditto

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:504:
runtimeTypeGenerator.generateTypeVariableReaderStubs(element);
On 2015/10/01 20:55:28, sra1 wrote:
> ditto

done... arghhh... intelij

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:510:
classStubGenerator.computeSelectorsForNsmHandlers();
On 2015/10/01 20:55:28, sra1 wrote:
> ditto

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:520:
selector));
On 2015/10/01 20:55:28, sra1 wrote:
> ditto

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:541:
onlyForRti ? const <Field>[] : _buildFields(element, false);
On 2015/10/01 20:55:28, sra1 wrote:
> ditto

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:568: if
(element.isJsInterop) {
On 2015/10/01 20:55:28, sra1 wrote:
> use one of element.isJsInterop or onlyJsInterop consistently.

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:569: //
TODO(jacobr): make sure we aren't adding the same is checks multiple times..
On 2015/10/01 20:55:28, sra1 wrote:
> line lengyh

fixedh

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/n...
File pkg/compiler/lib/src/native/native.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/n...
pkg/compiler/lib/src/native/native.dart:36: checkJsInteropAnnotation;
On 2015/10/01 20:55:28, sra1 wrote:
> Seems like the wrong place for this.

agreed. this place was bad.
Discussed offline and relaxed when missing external method errors are reported
which allows this check to be moved to js_backend after resolution is done...
pending a rebase to include sigmund's similar changes to provide the right
callback. I'm uploading a patch with XXX at the right spot and then rebasing.

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/t...
File pkg/compiler/lib/src/types/types.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/compiler/lib/src/t...
pkg/compiler/lib/src/types/types.dart:298: if (element.isJsInterop) {
On 2015/10/01 20:55:28, sra1 wrote:
> This is a complete hack.
> This code is only called to query the inference result and not to generate it.
> We should do this in a way that allows us to believe some of the types.
> 
> If you want to leave this for a while, it needs a TODO(issueNumber)

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/LICENSE
File pkg/js/LICENSE (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/LICENSE#newcode1
pkg/js/LICENSE:1: Copyright 2012, the Dart project authors. All rights reserved.
On 2015/10/01 20:55:28, sra1 wrote:
> Really?

really... because this is copied from the previous location of the package in
github.
Ignore the diff.. git guessed a bogus source location.

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/lib/js.dart
File pkg/js/lib/js.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/lib/js.dart#new...
pkg/js/lib/js.dart:17: /// instance members.
On 2015/10/01 20:55:28, sra1 wrote:
> Comment needs examples.

Done.

https://codereview.chromium.org/1318043005/diff/140001/pkg/js/lib/js.dart#new...
pkg/js/lib/js.dart:24: /// object. That is it is created with `new Object()`.
On 2015/10/01 20:55:28, sra1 wrote:
> Say why this is needed.
> Add example.

Removed as we aren't using it yet.

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:107: '',
On 2015/10/01 20:55:29, sra1 wrote:
> fix indentation
> 
>   return JS('', r'''
>       function(...) {
>         ...
>       }(#, #, #)''',
>       foo,
>       ...);
made the indentation beautiful

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:674: _stripUndefinedArgs([a, b, c, d, e, f,
g, h, i, j]));
On 2015/10/01 20:55:29, sra1 wrote:
> I can't find this function.
arg. clearly I don't have a test case that uses more than 4 arguments.

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:680: '',
On 2015/10/01 20:55:29, sra1 wrote:
> Why line-break?

reran the formatter.

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:723: _convertDartFunctionFast(Function f,
{bool captureThis: false}) {
On 2015/10/01 20:55:29, sra1 wrote:
> captureThis is unused

removed

https://codereview.chromium.org/1318043005/diff/140001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:728: 'function(_call, f) {'
On 2015/10/01 20:55:29, sra1 wrote:
> Use multiline string to fix indentation

Done.

https://codereview.chromium.org/1318043005/diff/140001/tests/html/js_array_te...
File tests/html/js_array_test.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/tests/html/js_array_te...
tests/html/js_array_test.dart:167: class SimpleJsLiteralClass extends
JavaScriptObject {
On 2015/10/01 08:51:05, alexandre.ardhuin wrote:
> I can't find where this class is used.

There is a JS literal with a getter foo

see Line 38
  concatValues: function(obj) {
    return obj.concat("a", "b", ["c", "d"], 42, {foo: 10});
  },

[kevmoo, 2015-10-02 20:20:12]

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

[kevmoo, 2015-10-02 20:20:13]

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/lib/js.dart
File pkg/js/lib/js.dart (right):

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/lib/js.dart#new...
pkg/js/lib/js.dart:11: /// A metadata annotation that indicates that a Library,
Class, or member is
Also, expose `dart:js` `wrapClosure` – although I wouldn't export it directly.
Wrap it in another top-level method

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/pubspec.yaml
File pkg/js/pubspec.yaml (right):

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/pubspec.yaml#ne...
pkg/js/pubspec.yaml:9: sdk: '>=1.0.0 <2.0.0'
Should be >= 1.13.0

[alexandre.ardhuin, 2015-10-02 20:29:56]

https://codereview.chromium.org/1318043005/diff/120001/tests/html/js_typed_in...
File tests/html/js_typed_interop_test.dart (right):

https://codereview.chromium.org/1318043005/diff/120001/tests/html/js_typed_in...
tests/html/js_typed_interop_test.dart:5: @JsName()
On 2015/10/02 20:08:15, Jacob wrote:
> On 2015/10/01 08:51:05, alexandre.ardhuin wrote:
> > > Modifying the behavior of a JS class is now an explicit non-goal as it
seems
> > > hard to implement efficiently without wrappers in Dart2Js and the goal is
to
> > > expose existing JS libraries not to beautify them.
> > 
> > :( That means the dart libs that proxified Js lib will not be able to follow
> the
> > Dart style guide without adding dart wrappers.
> > 
> > For instance in
> >
>
https://developers.google.com/maps/documentation/javascript/3.exp/reference#Map
> > you can see a lot of `getXxx()` that would be more darty with `get xxx;`
> >
>
(https://www.dartlang.org/articles/style-guide/#do-use-a-getter-for-operations...).
> 
> There are some significant tradeoffs at play that lead to this design. Mapping
> get xxx to getXxx for some JS classes comes with a real performance and/or
code
> size hit. In addition, this is a feature needed largely for older JS libraries
> designed before ES5 was widely available. I think it is better to provide fast
> and automate-able importing of JS libraries to Dart rather than trying to make
> JS libraries look like they were written by Dart users.
> 
> Even for libraries with "ugly" getXxx members some end users will prefer
keeping
> the methods as getXxx as that makes it much easier to reuse JS samples. For
> dart:html we found that ugly is often better than pretty and harder to search
> for/less familiar.
> 
> 
> The Maps API looks that way likely because they needed to support ES3.  If you
> want to make a version of the API designed with ES5+ in mind, those methods
> would likely be getters.
> If you want the Maps API to have a more modern shape, use the same technique
you
> would writing regular JavaScript and monkey patch the JavaScript properties
you
> want to add. 
> 
> If a particular JS library really needs to be more darty to be productively
used
> from Dart, monkey patch it on the JS side to be more darty the same way you
> would modernize the JS library for JS users.
> Note: in the future there may be an option to do this monkey patching in a
> typesafe way using ES6 symbols in the way the Dart Dev Compiler currently
makes
> JS Array support the Dart List API.  At some point we may expose a similar
> mechanism in Dart2Js to enable the case you describe. In any case, there is
> still the real cost of adding these unlikely to be treeshaken methods. The
> material difference in system behavior is really just whether the new
properties
> are visible from JS or hidden from JS users.

Thanks for this explanation. It makes sense.
I'll take a look at monkey patching (with symbols) and ES6 proxies (once the
implementation in the main browsers has improved).

https://codereview.chromium.org/1318043005/diff/140001/tests/html/js_array_te...
File tests/html/js_array_test.dart (right):

https://codereview.chromium.org/1318043005/diff/140001/tests/html/js_array_te...
tests/html/js_array_test.dart:167: class SimpleJsLiteralClass extends
JavaScriptObject {
On 2015/10/02 20:08:16, Jacob wrote:
> On 2015/10/01 08:51:05, alexandre.ardhuin wrote:
> > I can't find where this class is used.
> 
> There is a JS literal with a getter foo
> 
> see Line 38
>   concatValues: function(obj) {
>     return obj.concat("a", "b", ["c", "d"], 42, {foo: 10});
>   },

I saw this JS line but on the dart side there's only a `ret[7].foo` at L534 and
I thought `ret[7]` was a JavaScriptObject (not a SimpleJsLiteralClass that is
not explicitly used here)

[Jacob, 2015-10-03 01:01:09]

Also rebased to tip of dart repo trunk, implemented siggi's suggestion of using
the techniques from lookup_map_analysis to remove the horrible hacks to parse
annotation values.
due to a lack of creativity, my matching class is called js_interop_analysis.

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/lib/js.dart
File pkg/js/lib/js.dart (right):

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/lib/js.dart#new...
pkg/js/lib/js.dart:11: /// A metadata annotation that indicates that a Library,
Class, or member is
On 2015/10/02 20:20:13, kevmoo wrote:
> Also, expose `dart:js` `wrapClosure` – although I wouldn't export it
directly.
> Wrap it in another top-level method

exported it anyway to avoid extra overhead.
This method "should" be in dart:js anyway.. we're putting it here really just to
avoid confusion as the rest of dart:js is essentially deprecated.

[sra1, 2015-10-06 21:42:18]

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/lib/js.dart
File pkg/js/lib/js.dart (right):

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/lib/js.dart#new...
pkg/js/lib/js.dart:22: ///     library maps
semicolon

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/common/backend_api.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/common/backend_api.dart:302: if (element.isClass) {
Is there a way to defer the member checking?
If the class has no annotation, are annotations on the members meaningful?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
File pkg/compiler/lib/src/elements/modelx.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:277: 'computed.'));
I'd try and indent this like
 message:
     ' cccc'
     'ccc'

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:916: if (cls.isJsInterop) {
Ideally this should be done earlier.
Add a TODO to move this checking earlier.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:1267: 
remove extra line

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:345: var
interceptorClass = _classes[backend.jsInterceptorClass];
Add them to jsJavaScriptObjectClass.
Otherwise you are adding these methods to bool, String, int.

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:668: args.takeWhile((i) => JS('bool', '# !==
#', i, _UNDEFINED)).toList();
could also use
JS('bool', 'typeof # != "undefined"', i)

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:743: return _call(f,
Array.prototype.slice.apply(arguments));
.apply -> .call ?

Add small targeted test for this.

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:762: return _call(f,
this,Array.prototype.slice.apply(arguments));
'this,' -> 'this, '

.apply -> .call ?

Add test for this path.

https://codereview.chromium.org/1318043005/diff/180001/tests/compiler/dart2js...
File tests/compiler/dart2js_extra/js_interop_error_test.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/tests/compiler/dart2js...
tests/compiler/dart2js_extra/js_interop_error_test.dart:6: import
"package:js/js.dart";
Add a few more tests in the dart2js_native directory.
You can execute some setup code in that directory to create global JS things.

1. Minimal test that JS calling toString via a coercion calls Dart's toString.

2. Methods on @Js classes are not visible on Dart corelib intercepted classes.
You should not be able to call charCodeAt.

    @Js()
    class Foo {
      external charCodeAt(int i);
    }

    main() {
      var s = confuse('Hello');
      Expect.throws(() => s.charCodeAt(0));
    }

[Siggi Cherem (dart-lang), 2015-10-06 22:38:02]

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/d...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:2138: "#{cls} cannot extend a
normal Dart class as it is annoted with"
nit: "#{cls} ... " => "Class '#{cls}' ..."

or:

  'Js-interop class '#{cls}' cannot extend from the non js-interop class
'#{superclass}'.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:2140: howToFix: "Add @Js
annotation to super class",
nits:

1.   add => add the
     super class => superclass

   or change the sentence to:

     Annotate the superclass with @Js.

2. end with a "."

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:2152: "Js interop class #{cls}
member #{member} is not external.",
nit: maybe change to:
   "Member '#{member}' in js-interop class '#{cls}' is not external."

(minor grammar change, add ' ' around names, end with . )

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
File pkg/compiler/lib/src/elements/modelx.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:246: bool get isJsInterop =>
_isJsInterop;
+ dartdoc

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:247: String get jsInteropName =>
_jsInteropName;
+ dartdoc

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:249: void setHasJsInterop() {
setIsJsInterop? (to match the field name)

markAsJsInterop()

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:260: bool get isNative => _isNative ||
isJsInterop;
questions/comments here:
 - now that this is more than _isNative, maybe it needs a dartdoc? (same below)
 - I noticed that you commonly make the distinction between the two in the rest
of the compiler, so I'm not sure how much are you gaining from adding `||
isJsInterop` here. I wonder if we can remove it here and explicitly check for it
where we are missing the check (maybe in type-inference?)

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:264: if (e.jsInteropName != null &&
e.jsInteropName.isNotEmpty)
style nit: add braces

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:275: !(_isJsInterop &&  _jsInteropName
== null),
should the check here also involve isConstructor like below? (or move the
invariant check to _jsNameHelper?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:278: return
_jsNameHelper(isConstructor ? enclosingClass : this);
could we memoize the result by writting _fixedBackendName here?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:2025: /**
nit, let's use /// (dart2js has both, but I'm trying to start moving towards
///)

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:2028: * JsInterop.
consider splitting the sentence into a set of bullets with each possible
scenario?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:2658: bool get isNative =>
nativeTagInfo != null || isJsInterop;
same here

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/i...
File pkg/compiler/lib/src/inferrer/type_graph_nodes.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/type_graph_nodes.dart:407: if
(inferrer.isNativeElement(element) && !element.isConstructor) {
is this change still needed?

If so, maybe we need to change it a bit to make it more clear that this is about
js-interop?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/custom_elements_analysis.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/custom_elements_analysis.dart:157: if
(classElement.isJsInterop) continue;
can this happen given the change you have above in line 74?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/namer.dart:643: String fixedBackendPath(Element
element) {
+ dartdoc?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/namer.dart:788: // TODO(jacobr): we need to
relax this constraint.
please file a bug to refer to it here.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/patch_resolver.dart (left):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/patch_resolver.dart:41: element,
MessageKind.PATCH_EXTERNAL_WITHOUT_IMPLEMENTATION);
rather than completely removing this, maybe we should report it when
element.isJsInterop is false?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:329: receiver =
js.uncachedExpressionTemplate(
might be worth explaining why does this need to be uncached

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:579: bool
isInstantiated = element.isJsInterop ? false :
  x ? false : y 
=>
  !x && y

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/p...
File pkg/compiler/lib/src/patch_parser.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/patch_parser.dart:417: actionCallback();
I was hoping that now that you moved the logic to `onCodegenStartup`, we should
be able to revert this change

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/patch_parser.dart:486: return hasJsInterop == true ? true :
null;
might be worth a tiny comment on why not return false (probably something to do
with the semantics of apply in the baseclass?)

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:1525: null;
do we need `null`, or the null-constant object? (graph.addNullConstant(...))?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:1633: function,
MessageKind.PATCH_EXTERNAL_WITHOUT_IMPLEMENTATION);
it would be nice to produce these errors elsewhere - otherwise we need to
replicate the logic in the cps-builder too. Maybe add TODO?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:3958: );
style nit: move this back to the line above?

eventually we'll run the formatter here, but let's not do it yet (trying to
avoid crazy conflicts for some time)

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:5845: {'text': 'Error: all arguments to
external constructors of'
nit: add trailing spaces here and below (to avoid getting 'ofJavaScript' instead
of 'of JavaScript')

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:661: // Start of methods for new style
Dart-JS interop.
nit: maybe add a // ---------------------------------------------- :)

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:690: '',
eeek - maybe file a dartfmt bug, this format seems very odd to me, maybe because
the function name is short. I'd prefer:
JS('', r'''
       ...
     ''', src, dest);

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:709: void registerJsInterfaces([List<Type>
types]) {
I thought you said this was no longer used?

https://codereview.chromium.org/1318043005/diff/180001/tests/html/js_typed_in...
File tests/html/js_typed_interop_test.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/tests/html/js_typed_in...
tests/html/js_typed_interop_test.dart:213: /*     
expect(Foo.multiplyDefault2(6, 7), equals(42));
uncomment?

[Siggi Cherem (dart-lang), 2015-10-06 22:39:37]

oh, I forgot to ask: I couldn't find the js_interop_analysis.dart file, maybe
missing a `git add`?

[Jacob, 2015-10-13 01:19:24]

PTAL
Added a few more files to fix failing tests.

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/lib/js.dart
File pkg/js/lib/js.dart (right):

https://codereview.chromium.org/1318043005/diff/160001/pkg/js/lib/js.dart#new...
pkg/js/lib/js.dart:22: ///     library maps
On 2015/10/06 21:42:17, sra1 wrote:
> semicolon

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/common/backend_api.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/common/backend_api.dart:302: if (element.isClass) {
On 2015/10/06 21:42:17, sra1 wrote:
> Is there a way to defer the member checking?
> If the class has no annotation, are annotations on the members meaningful?

They aren't. Added a short circuit based on 
element.isJsInterop

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/d...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:2138: "#{cls} cannot extend a
normal Dart class as it is annoted with"
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> nit: "#{cls} ... " => "Class '#{cls}' ..."
> 
> or:
> 
>   'Js-interop class '#{cls}' cannot extend from the non js-interop class
> '#{superclass}'.
much better. done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:2140: howToFix: "Add @Js
annotation to super class",
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> nits:
> 
> 1.   add => add the
>      super class => superclass
> 
>    or change the sentence to:
> 
>      Annotate the superclass with @Js.
> 
> 2. end with a "."

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:2152: "Js interop class #{cls}
member #{member} is not external.",
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> nit: maybe change to:
>    "Member '#{member}' in js-interop class '#{cls}' is not external."
> 
> (minor grammar change, add ' ' around names, end with . )

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
File pkg/compiler/lib/src/elements/modelx.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:246: bool get isJsInterop =>
_isJsInterop;
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> + dartdoc

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:247: String get jsInteropName =>
_jsInteropName;
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> + dartdoc

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:249: void setHasJsInterop() {
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> setIsJsInterop? (to match the field name)
> 
> markAsJsInterop()

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:260: bool get isNative => _isNative ||
isJsInterop;
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> questions/comments here:
>  - now that this is more than _isNative, maybe it needs a dartdoc? (same
below)
>  - I noticed that you commonly make the distinction between the two in the
rest
> of the compiler, so I'm not sure how much are you gaining from adding `||
> isJsInterop` here. I wonder if we can remove it here and explicitly check for
it
> where we are missing the check (maybe in type-inference?)
> 

There are a ton of places where the code paths are consistent. if you look
through the previous patches in this CL I started out with a top of checks for
isNative || isJsInterop but I reverted them as one point as I didn't hit all the
edge cases and for the most part they were treated the same. The main difference
is the name is the new js interop allows a bit more complex names. E.g.
"foo.bar.baz" rather than just "baz"

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:264: if (e.jsInteropName != null &&
e.jsInteropName.isNotEmpty)
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> style nit: add braces

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:275: !(_isJsInterop &&  _jsInteropName
== null),
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> should the check here also involve isConstructor like below? (or move the
> invariant check to _jsNameHelper?

done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:277: 'computed.'));
On 2015/10/06 21:42:17, sra1 wrote:
> I'd try and indent this like
>  message:
>      ' cccc'
>      'ccc'

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:278: return
_jsNameHelper(isConstructor ? enclosingClass : this);
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> could we memoize the result by writting _fixedBackendName here?

might as well. done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:2025: /**
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> nit, let's use /// (dart2js has both, but I'm trying to start moving towards
> ///)

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:2028: * JsInterop.
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> consider splitting the sentence into a set of bullets with each possible
> scenario?

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/e...
pkg/compiler/lib/src/elements/modelx.dart:2658: bool get isNative =>
nativeTagInfo != null || isJsInterop;
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> same here

The comment for the superclass still applies here so not sure it should be
commented here?

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/i...
File pkg/compiler/lib/src/inferrer/type_graph_nodes.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/inferrer/type_graph_nodes.dart:407: if
(inferrer.isNativeElement(element) && !element.isConstructor) {
On 2015/10/06 22:38:01, Siggi Cherem (dart-lang) wrote:
> is this change still needed?
> 
> If so, maybe we need to change it a bit to make it more clear that this is
about
> js-interop?

removed the .isConstructor check here and added
|| element.isConstructor
to the assert bellow.
Not really sure if this is right and don't have enough context on this code to
really judge it. Let me know if this seems reasonable or if we should handle
this some other way.
My guess is that we simply never had to worry about constructors that were
native elements.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:916: if (cls.isJsInterop) {
On 2015/10/06 21:42:17, sra1 wrote:
> Ideally this should be done earlier.
> Add a TODO to move this checking earlier.

done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:1267: 
On 2015/10/06 21:42:17, sra1 wrote:
> remove extra line

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/custom_elements_analysis.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/custom_elements_analysis.dart:157: if
(classElement.isJsInterop) continue;
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> can this happen given the change you have above in line 74?

nope. Removed and moved comment to the other isJsInterop case

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/namer.dart:643: String fixedBackendPath(Element
element) {
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> + dartdoc?

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/namer.dart:788: // TODO(jacobr): we need to
relax this constraint.
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> please file a bug to refer to it here.

synced with sra and we decided this assert can be ignored now. it made sense a
couple years ago but doesn't now that we are less concerned about fully hiding
the fact that we are compiling to JS.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/patch_resolver.dart (left):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/patch_resolver.dart:41: element,
MessageKind.PATCH_EXTERNAL_WITHOUT_IMPLEMENTATION);
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> rather than completely removing this, maybe we should report it when
> element.isJsInterop is false?

discussed offline. added comment explaining why we are now lazier about
reporting this error.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:329: receiver =
js.uncachedExpressionTemplate(
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> might be worth explaining why does this need to be uncached

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:345: var
interceptorClass = _classes[backend.jsInterceptorClass];
On 2015/10/06 21:42:17, sra1 wrote:
> Add them to jsJavaScriptObjectClass.
> Otherwise you are adding these methods to bool, String, int.

I thought you would complain about that. done :)

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:579: bool
isInstantiated = element.isJsInterop ? false :
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
>   x ? false : y 
> =>
>   !x && y

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/p...
File pkg/compiler/lib/src/patch_parser.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/patch_parser.dart:417: actionCallback();
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> I was hoping that now that you moved the logic to `onCodegenStartup`, we
should
> be able to revert this change

me too.. reverted.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/patch_parser.dart:486: return hasJsInterop == true ? true :
null;
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> might be worth a tiny comment on why not return false (probably something to
do
> with the semantics of apply in the baseclass?)

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:1525: null;
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> do we need `null`, or the null-constant object? (graph.addNullConstant(...))?

this is intentional and not something I'm particularly proud of. We need actual
"null" to determine which parameters were unspecified versus which parameters
were specified but had a value of null.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:1633: function,
MessageKind.PATCH_EXTERNAL_WITHOUT_IMPLEMENTATION);
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> it would be nice to produce these errors elsewhere - otherwise we need to
> replicate the logic in the cps-builder too. Maybe add TODO?

added TODO

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:3958: );
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> style nit: move this back to the line above?
> 
> eventually we'll run the formatter here, but let's not do it yet (trying to
> avoid crazy conflicts for some time)

Done.

https://codereview.chromium.org/1318043005/diff/180001/pkg/compiler/lib/src/s...
pkg/compiler/lib/src/ssa/builder.dart:5845: {'text': 'Error: all arguments to
external constructors of'
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> nit: add trailing spaces here and below (to avoid getting 'ofJavaScript'
instead
> of 'of JavaScript')

Done.

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:661: // Start of methods for new style
Dart-JS interop.
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> nit: maybe add a // ---------------------------------------------- :)

Done.

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:668: args.takeWhile((i) => JS('bool', '# !==
#', i, _UNDEFINED)).toList();
On 2015/10/06 21:42:17, sra1 wrote:
> could also use
> JS('bool', 'typeof # != "undefined"', i)

done. is that faster?

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:690: '',
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> eeek - maybe file a dartfmt bug, this format seems very odd to me, maybe
because
> the function name is short. I'd prefer:
> JS('', r'''
>        ...
>      ''', src, dest);

i think the existing is easier to read although less dense.
I find it annoying that with the other option, the first '' is 1 character off
from the '''

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:709: void registerJsInterfaces([List<Type>
types]) {
On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> I thought you said this was no longer used?

I need a home for the fnProto munging.
We also need to figure out what the solution is for cross frames.
Perhaps this method needs to be
allowInteropOnFrame(Window window)
?

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:743: return _call(f,
Array.prototype.slice.apply(arguments));
On 2015/10/06 21:42:17, sra1 wrote:
> .apply -> .call ?
> 
> Add small targeted test for this.

I have tests for this.
Array.prototype.slice.apply is just a cargo cult JS tactic to convert Arguments
to a valid Array. I did a double take when I saw it originally. Notice that
_convertDartFunction
is using the same pattern. 
If there is a faster pattern I can switch to it.

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:762: return _call(f,
this,Array.prototype.slice.apply(arguments));
On 2015/10/06 21:42:17, sra1 wrote:
> 'this,' -> 'this, '
> 
> .apply -> .call ?
> 
> Add test for this path.

see comment for previous case.

https://codereview.chromium.org/1318043005/diff/180001/tests/compiler/dart2js...
File tests/compiler/dart2js_extra/js_interop_error_test.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/tests/compiler/dart2js...
tests/compiler/dart2js_extra/js_interop_error_test.dart:6: import
"package:js/js.dart";
On 2015/10/06 21:42:18, sra1 wrote:
> Add a few more tests in the dart2js_native directory.
> You can execute some setup code in that directory to create global JS things.
> 
> 1. Minimal test that JS calling toString via a coercion calls Dart's toString.
> 
> 2. Methods on @Js classes are not visible on Dart corelib intercepted classes.
> You should not be able to call charCodeAt.
> 
>     @Js()
>     class Foo {
>       external charCodeAt(int i);
>     }
> 
>     main() {
>       var s = confuse('Hello');
>       Expect.throws(() => s.charCodeAt(0));
>     }
>       
Done. However put tests in test/html with the other interop tests as the tests
should run in dartium and ddc as well.
Put the confuse test in the same file as the existing test as that should just
increase the "confusion" :)
Kept the toString test separate to ensure we are retaining toString, etc.

[Siggi Cherem (dart-lang), 2015-10-13 02:08:53]

It's looking really nice! I just added a few small comments below, besides that
looks good to me (but you should wait for Stephen comments though).

I'm cc'ing Johnni in case he has suggestions for how to work with the @Js
annotation earlier during resolution, or if what you have waiting for
onQueueClosed is the best we can do. I'm OK submitting what you have and
improving that as a follow up later as well.

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:709: void registerJsInterfaces([List<Type>
types]) {
On 2015/10/13 01:19:24, Jacob wrote:
> On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> > I thought you said this was no longer used?
> 
> I need a home for the fnProto munging.
> We also need to figure out what the solution is for cross frames.
> Perhaps this method needs to be
> allowInteropOnFrame(Window window)
> ?

sounds good - btw, this code disappeared, did it move somewhere else?

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/d...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:2139: "class #{superclass}.",
nit: add missing ' ' around #{superclass}

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:2696:
jsInteropAnalysis.onQueueClosed();
should we check that we only run this logic once? `onQueueClosed` is called
twice, at the end of resolution and at the end of codegen. We could either move
this to onCodegenStart, or keep a boolean in jsInteropAnalysis to check if
you've already ran that code.

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/js_interop_analysis.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:55:
backend.compiler.constants.getConstantValue(annotation.constant);
check first for the type of constant since some annotations might be something
other than ConstructedConstantValues, for example:

  const foo = 1;
  @foo
  fElement(){}

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:56: if (constant !=
null &&
since you'll have more than one check, consider doing:

if (constant == null || constant is! ConstructedConstantValue) continue;

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:57:
constant.getType(backend.compiler.coreTypes).element ==
or simply `constant.type.element` (I believe ConstructedConstantValue has a
.type getter)

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:77: if
(element.isClass) {
nit: to keep the nesting level below smaller, we could here just do:

if (!element.isClass || !element.isJsInterop) return;
...

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:81:
processJsInteropAnnotation(memberElement);
could we move this down and combine with the body of the other forEachMember?

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/patch_resolver.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/patch_resolver.dart:39: } else if
(!element.isJsInterop){
nit: add space after )

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/p...
File pkg/compiler/lib/src/patch_parser.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/patch_parser.dart:481: return hasJsInterop == true ? true :
null;
nit: `return hasJsInterop ? true : null` (just realized hasJsInterop can't be
null, so == true is not needed)

https://codereview.chromium.org/1318043005/diff/200001/tests/html/js_typed_in...
File tests/html/js_typed_interop_test.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/tests/html/js_typed_in...
tests/html/js_typed_interop_test.dart:233: /*     
expect(Foo.multiplyDefault2(6, 7), equals(42));
remove commented code?

[Jacob, 2015-10-13 03:10:43]

ptal

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/1318043005/diff/180001/sdk/lib/js/dart2js/js_...
sdk/lib/js/dart2js/js_dart2js.dart:709: void registerJsInterfaces([List<Type>
types]) {
On 2015/10/13 02:08:52, Siggi Cherem (dart-lang) wrote:
> On 2015/10/13 01:19:24, Jacob wrote:
> > On 2015/10/06 22:38:02, Siggi Cherem (dart-lang) wrote:
> > > I thought you said this was no longer used?
> > 
> > I need a home for the fnProto munging.
> > We also need to figure out what the solution is for cross frames.
> > Perhaps this method needs to be
> > allowInteropOnFrame(Window window)
> > ?
> 
> sounds good - btw, this code disappeared, did it move somewhere else?

Yep. I now generate the .call$N methods through the correct (i think) in
js_interop_analysis.dart 
see buildJsInteropBootstrap

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/d...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/d...
pkg/compiler/lib/src/diagnostics/messages.dart:2139: "class #{superclass}.",
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> nit: add missing ' ' around #{superclass}

Done.

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:2696:
jsInteropAnalysis.onQueueClosed();
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> should we check that we only run this logic once? `onQueueClosed` is called
> twice, at the end of resolution and at the end of codegen. We could either
move
> this to onCodegenStart, or keep a boolean in jsInteropAnalysis to check if
> you've already ran that code.

Added a bool in JsInteropAnalysis to track whether we are in codegen yet.
Unfortunately onCodegenStart is no good as some tests only run resolution and I
need to have the errors reported early enough.

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/js_interop_analysis.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:55:
backend.compiler.constants.getConstantValue(annotation.constant);
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> check first for the type of constant since some annotations might be something
> other than ConstructedConstantValues, for example:
> 
>   const foo = 1;
>   @foo
>   fElement(){}

Done.

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:56: if (constant !=
null &&
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> since you'll have more than one check, consider doing:
> 
> if (constant == null || constant is! ConstructedConstantValue) continue;

done

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:57:
constant.getType(backend.compiler.coreTypes).element ==
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> or simply `constant.type.element` (I believe ConstructedConstantValue has a
> .type getter)

Done.

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:77: if
(element.isClass) {
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> nit: to keep the nesting level below smaller, we could here just do:
> 
> if (!element.isClass || !element.isJsInterop) return;
> ...

Done.

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:81:
processJsInteropAnnotation(memberElement);
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> could we move this down and combine with the body of the other forEachMember?

moved it anyway.

i think this is slightly as the rest of the code is error checking while this
code is actually about processing the annotations.
it would be reasonable to move the error checking into its own method, etc.. Not
sure this class is the right place for it. moved it here at the last minute as
the original location was causing errors to be reported multiple times.

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/patch_resolver.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/patch_resolver.dart:39: } else if
(!element.isJsInterop){
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> nit: add space after )

Done.

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/p...
File pkg/compiler/lib/src/patch_parser.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/pkg/compiler/lib/src/p...
pkg/compiler/lib/src/patch_parser.dart:481: return hasJsInterop == true ? true :
null;
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> nit: `return hasJsInterop ? true : null` (just realized hasJsInterop can't be
> null, so == true is not needed)

done

https://codereview.chromium.org/1318043005/diff/200001/tests/html/js_typed_in...
File tests/html/js_typed_interop_test.dart (right):

https://codereview.chromium.org/1318043005/diff/200001/tests/html/js_typed_in...
tests/html/js_typed_interop_test.dart:233: /*     
expect(Foo.multiplyDefault2(6, 7), equals(42));
On 2015/10/13 02:08:53, Siggi Cherem (dart-lang) wrote:
> remove commented code?

removed comments.

[sra1, 2015-10-13 03:31:36]

lgtm

https://codereview.chromium.org/1318043005/diff/220001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1318043005/diff/220001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:917: // TODO(jacobr): this is the
wrong place to be reporting errors.
Since there is already a test that shows this problem, can you open an issue and
make the TODO reference it?

https://codereview.chromium.org/1318043005/diff/220001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/js_interop_analysis.dart (right):

https://codereview.chromium.org/1318043005/diff/220001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/js_interop_analysis.dart:73:
assert(value.isNull);

    @Js(1)
    external method();

Should generate an assignability warning, but is still valid 'Dart' code.
Should probably generate another warning or error and not assert.

https://codereview.chromium.org/1318043005/diff/220001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_emitter/full_emitter/setup_program_builder.dart
(right):

https://codereview.chromium.org/1318043005/diff/220001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_emitter/full_emitter/setup_program_builder.dart:33: 
line?

[Jacob, 2015-10-13 20:15:32]

Committed patchset #15 (id:280001) manually as
cd2752431e41c33c7ae85e0d0d26e5261f168930 (presubmit successful).