fixes #173, ability to use native JS indexers

fixes #173, ability to use native JS indexers
dom_experimental could opt-in to use this

R=jacobr@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/2317e72cd276230b92e104519f3be1019cee07d8

[Jennifer Messerly, 2015-05-19 23:03:08]

jmesserly@google.com changed reviewers:
+ jacobr@google.com, vsm@google.com

[Jennifer Messerly, 2015-05-19 23:03:09]

Thoughts?

[vsm, 2015-05-20 19:15:30]

General q: should we patch dindex and dsetindex to somehow recognize these
types?

https://codereview.chromium.org/1145833004/diff/1/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1145833004/diff/1/lib/src/utils.dart#newcode353
lib/src/utils.dart:353: AnnotatedNode node, bool test(DartObjectImpl value)) {
Could this just call findElementAnnotation(node.element)?

[Jennifer Messerly, 2015-05-20 19:25:32]

> General q: should we patch dindex and dsetindex to somehow recognize these
types?

yep, I think so. If type looks like not a Dart type, and doesn't have an
extension `[]` operator, we should dispatch it to native JS indexing.

https://codereview.chromium.org/1145833004/diff/1/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1145833004/diff/1/lib/src/utils.dart#newcode353
lib/src/utils.dart:353: AnnotatedNode node, bool test(DartObjectImpl value)) {
On 2015/05/20 19:15:29, vsm wrote:
> Could this just call findElementAnnotation(node.element)?

you'd think so, but AnnotatedNode doesn't have .element.
Of its three subtypes, Declaration and Directive both have a .element. However
VariableDeclarationList does not. I guess because of cases like:

    @deprecated
    var foo, bar, baz;

yet more reasons VariableDeclarationList is the most annoying thing ever :)
anyway, it does look like we could convert existing usage over to
findElementAnnotation, and just `.element` on the outside. does that work for
you?

[vsm, 2015-05-20 19:39:50]

https://codereview.chromium.org/1145833004/diff/1/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1145833004/diff/1/lib/src/utils.dart#newcode353
lib/src/utils.dart:353: AnnotatedNode node, bool test(DartObjectImpl value)) {
On 2015/05/20 19:25:32, John Messerly wrote:
> On 2015/05/20 19:15:29, vsm wrote:
> > Could this just call findElementAnnotation(node.element)?
> 
> you'd think so, but AnnotatedNode doesn't have .element.
> Of its three subtypes, Declaration and Directive both have a .element. However
> VariableDeclarationList does not. I guess because of cases like:
> 
>     @deprecated
>     var foo, bar, baz;
> 
> yet more reasons VariableDeclarationList is the most annoying thing ever :)
> anyway, it does look like we could convert existing usage over to
> findElementAnnotation, and just `.element` on the outside. does that work for
> you?

sgtm

[Jacob, 2015-05-20 19:55:22]

looks good. some minor nits.

https://codereview.chromium.org/1145833004/diff/1/test/codegen/domtest.dart
File test/codegen/domtest.dart (right):

https://codereview.chromium.org/1145833004/diff/1/test/codegen/domtest.dart#n...
test/codegen/domtest.dart:14: print(nodes[i] = null);
nodes[i] = null
is a bit odd as it won't do anything.
perhaps
var old = nodes[i];
nodes[i] = document.createElement('div')
print(nodes[i] == old)
which will print true.

https://codereview.chromium.org/1145833004/diff/1/test/codegen/sunflower/dom....
File test/codegen/sunflower/dom.dart (right):

https://codereview.chromium.org/1145833004/diff/1/test/codegen/sunflower/dom....
test/codegen/sunflower/dom.dart:44: @JsIndexer()
Seems a bit strange to have this annotation on the class. Shouldn't it just be
implied by a
@JsName class with an 
operator []
?

[Jennifer Messerly, 2015-05-20 20:13:31]

https://codereview.chromium.org/1145833004/diff/1/test/codegen/sunflower/dom....
File test/codegen/sunflower/dom.dart (right):

https://codereview.chromium.org/1145833004/diff/1/test/codegen/sunflower/dom....
test/codegen/sunflower/dom.dart:44: @JsIndexer()
On 2015/05/20 19:55:21, Jacob wrote:
> Seems a bit strange to have this annotation on the class. Shouldn't it just be
> implied by a
> @JsName class with an 
> operator []
> ?

Hah, yeah, I wanted it that way too :). But we can't get to that easily because
https://github.com/dart-lang/dev_compiler/issues/132. I could try looking up the
indexer again, essentially "resolve" it from js_codegen, but didn't seem worth
it.

[Jennifer Messerly, 2015-06-01 15:46:29]

Committed patchset #1 (id:1) manually as
8dc1eeeb4dcc4d186f59513abd18f98e67c36339 (presubmit successful).

[Jennifer Messerly, 2015-06-01 16:29:43]

FYI ... landed the wrong branch by accident =(.
Reverting @ https://codereview.chromium.org/1160343003

[Jennifer Messerly, 2015-06-01 17:36:58]

Hey Jacob, question about one of your comments. I totally misunderstood it the
first time.

https://codereview.chromium.org/1145833004/diff/1/test/codegen/sunflower/dom....
File test/codegen/sunflower/dom.dart (right):

https://codereview.chromium.org/1145833004/diff/1/test/codegen/sunflower/dom....
test/codegen/sunflower/dom.dart:44: @JsIndexer()
On 2015/05/20 20:13:30, John Messerly wrote:
> On 2015/05/20 19:55:21, Jacob wrote:
> > Seems a bit strange to have this annotation on the class. Shouldn't it just
be
> > implied by a
> > @JsName class with an 
> > operator []
> > ?
> 
> Hah, yeah, I wanted it that way too :). But we can't get to that easily
because
> https://github.com/dart-lang/dev_compiler/issues/132. I could try looking up
the
> indexer again, essentially "resolve" it from js_codegen, but didn't seem worth
> it.

Doh, I misunderstood your question.

Restating the Q: why can't @JsName imply @JsIndexable?
A: It could. But we'd have to do something different for JsArray, to indicate it
should *not* be JS-Indexable.

Based on further work in https://codereview.chromium.org/1153003003/, I think we
have a few kinds of JS types.

A. ones like Array, Number, Boolean, String, where we want to pretend they are
entirely Dart types and hide all the JS methods. ("dart:core types")
B. transparent-JS ones, where we want to expose all of the JS methods.
("dart:dom types", or "JS interop types")
C. In the future, we might have types that expose most of JS methods, but add
some Dart extension methods, and maybe subtract some JS methods. ("dart:html
types", "Dartified JS interop types")


Anyway, I'm tempted to say category A uses @JsPeerInterface, category B and C
use @JsName. Then we can have @JsIndexable be implied and remove it. Does that
sound good to you?

[Jacob, 2015-06-01 17:53:24]

On 2015/06/01 17:36:58, John Messerly wrote:
> Hey Jacob, question about one of your comments. I totally misunderstood it the
> first time.
> 
>
https://codereview.chromium.org/1145833004/diff/1/test/codegen/sunflower/dom....
> File test/codegen/sunflower/dom.dart (right):
> 
>
https://codereview.chromium.org/1145833004/diff/1/test/codegen/sunflower/dom....
> test/codegen/sunflower/dom.dart:44: @JsIndexer()
> On 2015/05/20 20:13:30, John Messerly wrote:
> > On 2015/05/20 19:55:21, Jacob wrote:
> > > Seems a bit strange to have this annotation on the class. Shouldn't it
just
> be
> > > implied by a
> > > @JsName class with an 
> > > operator []
> > > ?
> > 
> > Hah, yeah, I wanted it that way too :). But we can't get to that easily
> because
> > https://github.com/dart-lang/dev_compiler/issues/132. I could try looking up
> the
> > indexer again, essentially "resolve" it from js_codegen, but didn't seem
worth
> > it.
> 
> Doh, I misunderstood your question.
> 
> Restating the Q: why can't @JsName imply @JsIndexable?
> A: It could. But we'd have to do something different for JsArray, to indicate
it
> should *not* be JS-Indexable.
> 
> Based on further work in https://codereview.chromium.org/1153003003/, I think
we
> have a few kinds of JS types.
> 
> A. ones like Array, Number, Boolean, String, where we want to pretend they are
> entirely Dart types and hide all the JS methods. ("dart:core types")
> B. transparent-JS ones, where we want to expose all of the JS methods.
> ("dart:dom types", or "JS interop types")
> C. In the future, we might have types that expose most of JS methods, but add
> some Dart extension methods, and maybe subtract some JS methods. ("dart:html
> types", "Dartified JS interop types")
> 
> 
> Anyway, I'm tempted to say category A uses @JsPeerInterface, category B and C
> use @JsName. Then we can have @JsIndexable be implied and remove it. Does that
> sound good to you?
sgtm

[Jennifer Messerly, 2015-06-04 22:16:20]

Hey Vijay and/or Jacob, would you mind taking another look?

I think I've addressed all comments, and ready to submit FOR REALZ this time.

[Jennifer Messerly, 2015-06-04 22:17:16]

On 2015/06/04 22:16:20, John Messerly wrote:
> Hey Vijay and/or Jacob, would you mind taking another look?
> 
> I think I've addressed all comments, and ready to submit FOR REALZ this time.

oops, except for actually removing the (now unused JSIndexable) type.

[Jennifer Messerly, 2015-06-04 22:20:14]

test case fixed :)

[Jacob, 2015-06-05 18:28:48]

lgtm

[Jennifer Messerly, 2015-06-05 19:14:13]

Committed patchset #6 (id:100001) manually as
2317e72cd276230b92e104519f3be1019cee07d8 (presubmit successful).

[vsm, 2015-06-05 19:22:06]

https://codereview.chromium.org/1145833004/diff/100001/test/codegen/expect/su...
File test/codegen/expect/sunflower/dom.js (right):

https://codereview.chromium.org/1145833004/diff/100001/test/codegen/expect/su...
test/codegen/expect/sunflower/dom.js:22: let EventListener =
dart.typedef('EventListener', () => dart.functionType(dart.void, [Event]));
Event is gone, but it's referred to right here still.

[Jennifer Messerly, 2015-06-05 19:24:37]

https://codereview.chromium.org/1145833004/diff/100001/test/codegen/expect/su...
File test/codegen/expect/sunflower/dom.js (right):

https://codereview.chromium.org/1145833004/diff/100001/test/codegen/expect/su...
test/codegen/expect/sunflower/dom.js:22: let EventListener =
dart.typedef('EventListener', () => dart.functionType(dart.void, [Event]));
On 2015/06/05 19:22:05, vsm wrote:
> Event is gone, but it's referred to right here still.

just chatted ... I think what happened here is Event is now @JsName, so we don't
generate it, on the theory that it's already a global.