add JsArray and JsObject.asJsMap()

add JsArray and JsObject.asJsMap()

[alexandre.ardhuin, 2013-08-20 15:19:02]

Patch Set 2 : fix dart2js and add automatic convertion of Iterables and Maps

[vsm, 2013-08-20 15:44:13]

cool!

https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
sdk/lib/js/dart2js/js_dart2js.dart:182: if (index is int && (index < 0 || index
>= this.length)) {
Should JsArray throw on non-int indices?

https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
sdk/lib/js/dart2js/js_dart2js.dart:220: JsObject _jsObject;
It seems odd that Array extends JsObject while Map wraps it.  What's the reason
for the difference?

https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
sdk/lib/js/dart2js/js_dart2js.dart:288: return new JsArray._fromJs(JS('=Object',
'#', o));
After talking to sra, I think the right to put here (and 2 lines above) instead
of "=Object" is "var".  Though you may want to sanity check code size.

[alexandre.ardhuin, 2013-08-20 20:58:55]

https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
sdk/lib/js/dart2js/js_dart2js.dart:182: if (index is int && (index < 0 || index
>= this.length)) {
On 2013/08/20 15:44:13, vsm wrote:
> Should JsArray throw on non-int indices?

In Js an array is an object. So it's possible to set a property even if it's not
a Number. I have kept this semantic.

I hesitate to get rid of this Range check because Js don't have it.

https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
sdk/lib/js/dart2js/js_dart2js.dart:220: JsObject _jsObject;
On 2013/08/20 15:44:13, vsm wrote:
> It seems odd that Array extends JsObject while Map wraps it.  What's the
reason
> for the difference?

Ideally I would have made JsObject implementing Map<String, dynamic>. This is
not possible if JsArray (that implements List) extends JsObject because Map and
List have some incompatible functions (for instance list.forEach(void f(E
element)) vs. map.forEach(void f(K key, V value)))

Moreover in Js there is not really maps.

So I had decided to reflect the Js types by making JsArray extends JsObject (in
Js Array "extends" Object) and provide a way (with .asDartMap()) to handle
JsObject as a Map.

https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
sdk/lib/js/dart2js/js_dart2js.dart:288: return new JsArray._fromJs(JS('=Object',
'#', o));
On 2013/08/20 15:44:13, vsm wrote:
> After talking to sra, I think the right to put here (and 2 lines above)
instead
> of "=Object" is "var".  Though you may want to sanity check code size.

Done

[vsm, 2013-08-21 15:47:19]

On 2013/08/20 20:58:55, alexandre.ardhuin wrote:
>
https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
> File sdk/lib/js/dart2js/js_dart2js.dart (right):
> 
>
https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
> sdk/lib/js/dart2js/js_dart2js.dart:182: if (index is int && (index < 0 ||
index
> >= this.length)) {
> On 2013/08/20 15:44:13, vsm wrote:
> > Should JsArray throw on non-int indices?
> 
> In Js an array is an object. So it's possible to set a property even if it's
not
> a Number. I have kept this semantic.
> 
> I hesitate to get rid of this Range check because Js don't have it.
> 
>
https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
> sdk/lib/js/dart2js/js_dart2js.dart:220: JsObject _jsObject;
> On 2013/08/20 15:44:13, vsm wrote:
> > It seems odd that Array extends JsObject while Map wraps it.  What's the
> reason
> > for the difference?
> 
> Ideally I would have made JsObject implementing Map<String, dynamic>. This is
> not possible if JsArray (that implements List) extends JsObject because Map
and
> List have some incompatible functions (for instance list.forEach(void f(E
> element)) vs. map.forEach(void f(K key, V value)))

Ahh, right that makes sense.  How about wrapping with JsArray as well?  JsArray
could provide the same error semantics as regular Dart arrays that way.

> 
> Moreover in Js there is not really maps.
> 
> So I had decided to reflect the Js types by making JsArray extends JsObject
(in
> Js Array "extends" Object) and provide a way (with .asDartMap()) to handle
> JsObject as a Map.
> 
>
https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
> sdk/lib/js/dart2js/js_dart2js.dart:288: return new
JsArray._fromJs(JS('=Object',
> '#', o));
> On 2013/08/20 15:44:13, vsm wrote:
> > After talking to sra, I think the right to put here (and 2 lines above)
> instead
> > of "=Object" is "var".  Though you may want to sanity check code size.
> 
> Done

[alexandre.ardhuin, 2013-08-21 21:20:11]

On 2013/08/21 15:47:19, vsm wrote:
> On 2013/08/20 20:58:55, alexandre.ardhuin wrote:
> >
>
https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
> > File sdk/lib/js/dart2js/js_dart2js.dart (right):
> > 
> >
>
https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
> > sdk/lib/js/dart2js/js_dart2js.dart:182: if (index is int && (index < 0 ||
> index
> > >= this.length)) {
> > On 2013/08/20 15:44:13, vsm wrote:
> > > Should JsArray throw on non-int indices?
> > 
> > In Js an array is an object. So it's possible to set a property even if it's
> not
> > a Number. I have kept this semantic.
> > 
> > I hesitate to get rid of this Range check because Js don't have it.
> > 
> >
>
https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
> > sdk/lib/js/dart2js/js_dart2js.dart:220: JsObject _jsObject;
> > On 2013/08/20 15:44:13, vsm wrote:
> > > It seems odd that Array extends JsObject while Map wraps it.  What's the
> > reason
> > > for the difference?
> > 
> > Ideally I would have made JsObject implementing Map<String, dynamic>. This
is
> > not possible if JsArray (that implements List) extends JsObject because Map
> and
> > List have some incompatible functions (for instance list.forEach(void f(E
> > element)) vs. map.forEach(void f(K key, V value)))
> 
> Ahh, right that makes sense.  How about wrapping with JsArray as well? 
JsArray
> could provide the same error semantics as regular Dart arrays that way.
> 

The advantages of inheritance are :
1. provide the same "inheritance" structure as in JS (a JS Array is a JS Object
: `[] instanceof Object == true`)
2. directly type the results without having to test if it's an Array or not
before wrapping with JsArray.

For the error handling I'd prefer to have the semantic of Js arrays rather than
Dart List. This library is here to deal with JS so I find that would make sense
to keep the JS behaviour.
For instance in JS : 

  var a = [];
  a["custom"] = "test"; // custom attribute
  a.length == 0;
  a[2] = 1;
  a.length == 3;
  a["5"] = 1;
  a.length == 6;
  a.myCustomMethod = function(){return "custom";}; // custom method

JsArray should allow the same behaviour. This almost works (need to get rid of
some Range checks).

What do you thing ?

> > 
> > Moreover in Js there is not really maps.
> > 
> > So I had decided to reflect the Js types by making JsArray extends JsObject
> (in
> > Js Array "extends" Object) and provide a way (with .asDartMap()) to handle
> > JsObject as a Map.
> > 
> >
>
https://codereview.chromium.org/23291005/diff/9001/sdk/lib/js/dart2js/js_dart...
> > sdk/lib/js/dart2js/js_dart2js.dart:288: return new
> JsArray._fromJs(JS('=Object',
> > '#', o));
> > On 2013/08/20 15:44:13, vsm wrote:
> > > After talking to sra, I think the right to put here (and 2 lines above)
> > instead
> > > of "=Object" is "var".  Though you may want to sanity check code size.
> > 
> > Done

[vsm, 2013-08-22 15:36:41]

lgtm

The asymmetry between list and map seems unfortunate though I see why you did it
this way.

[justinfagnani, 2013-08-22 18:29:36]

Sorry for the delay, I've been in a firedrill this week.

I think the implementation of some of the methods needs looking at to make sure
you're not unnecessarily transferring too much data in the Dartium
implementation.

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

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dart2js...
sdk/lib/js/dart2js/js_dart2js.dart:175: /*@override*/ int get length =>
super['length'];
The /*@override*/ part is visually noisy. Can you remove it or import the actual
annotation from dart:meta? AFIK, we don't really use those annotations much.

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dart2js...
sdk/lib/js/dart2js/js_dart2js.dart:247: /*@override*/ void forEach(void f(String
key, value)) => Maps.forEach(this, f);
long line

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
File sdk/lib/js/dartium/js_dartium.dart (right):

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:236: /*@override*/ void add(value) {
callMethod('push', [value]); }
I think you should override addAll() also. The default implementation loops and
calls add() which will result in a lot of messages.

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:295: /*@override*/ bool containsValue(value)
=> Maps.containsValue(this, value);
I think containsValue should be implemented in JS

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:296: /*@override*/ bool containsKey(String
key) => Maps.containsKey(this, key);
same here, but much less important. rather than transfer the key, you'd just
transfer a bool

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:299: /*@override*/ void addAll(Map<String,
dynamic> other) {
same here, but more important so that conversion / messaging is done once

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:305: /*@override*/ void forEach(void f(String
key, value)) => Maps.forEach(this, f);
long line

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:306: /*@override*/ Iterable get values =>
Maps.getValues(this);
also implement in JS

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:307: /*@override*/ int get length =>
Maps.length(this);
these are especially important to implement in JS or with super['length'], etc.,
because Maps uses keys.length, keys.isEmpty, etc., and I believe that will
transfer the entire key list.

[alexandre.ardhuin, 2013-08-23 21:08:32]

The optimisations for _JsObjectAsMap lead to some refactorings and I would
prefer to make them in an other CL.

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

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dart2js...
sdk/lib/js/dart2js/js_dart2js.dart:175: /*@override*/ int get length =>
super['length'];
On 2013/08/22 18:29:36, justinfagnani wrote:
> The /*@override*/ part is visually noisy. Can you remove it or import the
actual
> annotation from dart:meta? AFIK, we don't really use those annotations much.

Unfortunately there's no library 'dart:meta' there's only a package (meta) but I
don't know if it is legal to use "import 'package:meta/meta.dart'" in /sdk/lib .
So I have removed this comment.
BTW I like this @override because it allows to prevent errors (if the Editor
handles this metadata correctly). For instance if I had put "lenght" instead of
"length" the tools could notify that something is wrong if @override is present
and that the annotated method does not override a method for parent class. Same
thing if the parent class renames an overriden method.

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dart2js...
sdk/lib/js/dart2js/js_dart2js.dart:247: /*@override*/ void forEach(void f(String
key, value)) => Maps.forEach(this, f);
On 2013/08/22 18:29:36, justinfagnani wrote:
> long line

Done.

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
File sdk/lib/js/dartium/js_dartium.dart (right):

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:236: /*@override*/ void add(value) {
callMethod('push', [value]); }
On 2013/08/22 18:29:36, justinfagnani wrote:
> I think you should override addAll() also. The default implementation loops
and
> calls add() which will result in a lot of messages.

Done.

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:305: /*@override*/ void forEach(void f(String
key, value)) => Maps.forEach(this, f);
On 2013/08/22 18:29:36, justinfagnani wrote:
> long line

Done.

https://chromiumcodereview.appspot.com/23291005/diff/15001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:307: /*@override*/ int get length =>
Maps.length(this);
On 2013/08/22 18:29:36, justinfagnani wrote:
> these are especially important to implement in JS or with super['length'],
etc.,
> because Maps uses keys.length, keys.isEmpty, etc., and I believe that will
> transfer the entire key list.

The result of "get keys" is not the entire key list but it's a JsArray. So
there's only the concern about several message passing.

[alexandre.ardhuin, 2013-08-24 19:01:34]

I have added some optimizations.