Add @jsify annotation. Support automatically proxying Lists and Maps to Dart in Proxies. Support co…

Add @jsify annotation. Support automatically proxying Lists and Maps to Dart in Proxies. Support copying List and Maps to JS in Proxies.

BUG=

[justinfagnani, 2014-10-01 19:57:35]

justinfagnani@google.com changed reviewers:
+ alexandre.ardhuin@gmail.com, sigmund@google.com

[justinfagnani, 2014-10-01 19:57:35]

This CL adds List and Map support to JsProxies. I still need to add it to
exported classes.

[alexandre.ardhuin, 2014-10-01 20:36:52]

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/js_impl.dart
File lib/src/js_impl.dart (left):

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/js_impl.dart#...
lib/src/js_impl.dart:77: 
Should a test that o is a JsObject be added ?
If not a JsObject should "o" be returned to handle transferable types and raw
dart object ?

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/js_impl.dart
File lib/src/js_impl.dart (right):

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/js_impl.dart#...
lib/src/js_impl.dart:91: 
We should have a JsList that would be a JsArray like but with toDart/toJs
everywhere. ( reminder: https://gist.github.com/a14n/2bc9d2d0be9e5e6b1592 :D )

Without that the user will get a List<JsObject> instead of List<JsInterface>.

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/js_object_map...
File lib/src/js_object_map.dart (right):

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/js_object_map...
lib/src/js_object_map.dart:15: class JsObjectMap<V> implements Map<String,V> {
You can use dart:collection.MapMixin to get rid of Maps.*

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/mirrors.dart
File lib/src/mirrors.dart (right):

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/mirrors.dart#...
lib/src/mirrors.dart:383: jsArgs[i] = new js.JsObject.jsify(arg);
Even if @jsify is used we shouldn't jsify an JsObjectMap or JsList.

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/transformer/j...
File lib/src/transformer/js_proxy_generator.dart (right):

https://chromiumcodereview.appspot.com/615913004/diff/1/lib/src/transformer/j...
lib/src/transformer/js_proxy_generator.dart:204: 
Same thing about @jsify for JsObjectMap/JsList.

[Siggi Cherem (dart-lang), 2014-10-01 22:21:19]

could you try reuploading the CL? for some reason the side-by-side diff appears
broken :(

[justinfagnani, 2014-10-02 18:18:12]

On 2014/10/01 22:21:19, Siggi Cherem (dart-lang) wrote:
> could you try reuploading the CL? for some reason the side-by-side diff
appears
> broken :(

Uploaded new patch set.

[justinfagnani, 2014-10-07 20:32:05]

Patchset #3 (id:40001) has been deleted

[justinfagnani, 2014-10-07 20:32:24]

Thanks for the thoughtful reviews. PTAL

https://codereview.chromium.org/615913004/diff/1/lib/src/js_impl.dart
File lib/src/js_impl.dart (left):

https://codereview.chromium.org/615913004/diff/1/lib/src/js_impl.dart#oldcode77
lib/src/js_impl.dart:77: 
On 2014/10/01 20:36:51, alexandre.ardhuin wrote:
> Should a test that o is a JsObject be added ?
> If not a JsObject should "o" be returned to handle transferable types and raw
> dart object ?

Yes, I'll add that. We'll want to handle all the transferrable types, or just
let them pass through. I've just been conservative so far.

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

https://codereview.chromium.org/615913004/diff/1/lib/src/js_impl.dart#newcode91
lib/src/js_impl.dart:91: 
On 2014/10/01 20:36:51, alexandre.ardhuin wrote:
> We should have a JsList that would be a JsArray like but with toDart/toJs
> everywhere. ( reminder: https://gist.github.com/a14n/2bc9d2d0be9e5e6b1592 :D )
> 
> Without that the user will get a List<JsObject> instead of List<JsInterface>.

Of course. Adding that. thanks :)

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

https://codereview.chromium.org/615913004/diff/1/lib/src/js_object_map.dart#n...
lib/src/js_object_map.dart:15: class JsObjectMap<V> implements Map<String,V> {
On 2014/10/01 20:36:51, alexandre.ardhuin wrote:
> You can use dart:collection.MapMixin to get rid of Maps.*

Done.

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

https://codereview.chromium.org/615913004/diff/1/lib/src/mirrors.dart#newcode383
lib/src/mirrors.dart:383: jsArgs[i] = new js.JsObject.jsify(arg);
On 2014/10/01 20:36:52, alexandre.ardhuin wrote:
> Even if @jsify is used we shouldn't jsify an JsObjectMap or JsList.

Done.

https://codereview.chromium.org/615913004/diff/1/lib/src/transformer/js_proxy...
File lib/src/transformer/js_proxy_generator.dart (right):

https://codereview.chromium.org/615913004/diff/1/lib/src/transformer/js_proxy...
lib/src/transformer/js_proxy_generator.dart:204: 
On 2014/10/01 20:36:52, alexandre.ardhuin wrote:
> Same thing about @jsify for JsObjectMap/JsList.

Done.

[Siggi Cherem (dart-lang), 2014-10-07 20:41:41]

lgtm

https://codereview.chromium.org/615913004/diff/60001/lib/src/js_list.dart
File lib/src/js_list.dart (right):

https://codereview.chromium.org/615913004/diff/60001/lib/src/js_list.dart#new...
lib/src/js_list.dart:19: class JsList<E> extends ListBase {
ListBase<E>

https://codereview.chromium.org/615913004/diff/60001/lib/src/mirrors.dart
File lib/src/mirrors.dart (right):

https://codereview.chromium.org/615913004/diff/60001/lib/src/mirrors.dart#new...
lib/src/mirrors.dart:385: var hasJsify = param.metadata.any((m) => m.reflectee
== metadata.jsify);
80

[justinfagnani, 2014-10-08 06:30:02]

Committed in
https://github.com/dart-lang/js-interop/commit/1efa2768cdb366932b3d6b7b09d47a...

https://codereview.chromium.org/615913004/diff/60001/lib/src/mirrors.dart
File lib/src/mirrors.dart (right):

https://codereview.chromium.org/615913004/diff/60001/lib/src/mirrors.dart#new...
lib/src/mirrors.dart:385: var hasJsify = param.metadata.any((m) => m.reflectee
== metadata.jsify);
On 2014/10/07 20:41:41, Siggi Cherem (dart-lang) wrote:
> 80

Done.

[alexandre.ardhuin, 2014-10-08 09:00:50]

lgtm

https://codereview.chromium.org/615913004/diff/60001/lib/src/js_list.dart
File lib/src/js_list.dart (right):

https://codereview.chromium.org/615913004/diff/60001/lib/src/js_list.dart#new...
lib/src/js_list.dart:19: class JsList<E> extends ListBase {
On 2014/10/07 20:41:41, Siggi Cherem (dart-lang) wrote:
> ListBase<E>
+1
The commit in master does not contain this change.

https://codereview.chromium.org/615913004/diff/60001/lib/src/js_list.dart#new...
lib/src/js_list.dart:71: void setRange(int start, int end, List<E> iterable,
[int startFrom = 0]) {
"iterable" is an List<E> here but it is a Iterable<E> in List interface.
It should be Iterable<E> here.