A few API changes:

A few API changes:
Remove Callback in favor of directly using closures or JsFunction.withThis().
Move jsify() to a named constructor on JsObject.
 - Throw on types other than Map and Iterable.
 - Handle cycles
Add JsObject.fromDartObject() which force a transferrable native to be proxied.

Still to do: Reach a conclusion on Serializable

BUG=
R=vsm@google.com

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

[justinfagnani, 2013-10-16 18:36:13]

This CL corresponds to the Dartium CL here:
https://chromiumcodereview.appspot.com/26729002/

It's important we keep the APIs in sync.

Also, the Dartium CL support transferring of certain native types, which isn't
implemented here yet.

[alexandre.ardhuin, 2013-10-16 19:32:11]

We should add a test for `new JsObject.fromBrowserObject`.

About API changes to be implemented in native, we should also consider
https://codereview.chromium.org/23291005/ that brings some API changes :
- add JsArray and JsObject.asDartMap()
- add automatic convertion of Iterables and Maps in _convertToJS

https://codereview.chromium.org/27514003/diff/1/sdk/lib/js/dart2js/js_dart2js...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/27514003/diff/1/sdk/lib/js/dart2js/js_dart2js...
sdk/lib/js/dart2js/js_dart2js.dart:50: factory JsObject.fromDartObject(Object
object) {
Rename to `fromBrowserObject`.

https://codereview.chromium.org/27514003/diff/1/sdk/lib/js/dart2js/js_dart2js...
sdk/lib/js/dart2js/js_dart2js.dart:90: // print("UnknownJavaScriptObject:
${jsObj is UnknownJavaScriptObject}");
Remove these comments ?

[justinfagnani, 2013-10-18 03:19:08]

PTAL

Added tests for illegal arguments and for JsObject.fromBrowserObject.

Alexandre, I think in general the approach of returning a JsArray is still fine,
since it's just a subclass of JsObject, but I don't think that automatically
calling jsify() for Dart Maps and Iterables is what we want since it silently
copies where  other operations proxy.

https://codereview.chromium.org/27514003/diff/1/sdk/lib/js/dart2js/js_dart2js...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/27514003/diff/1/sdk/lib/js/dart2js/js_dart2js...
sdk/lib/js/dart2js/js_dart2js.dart:50: factory JsObject.fromDartObject(Object
object) {
On 2013/10/16 19:32:11, alexandre.ardhuin wrote:
> Rename to `fromBrowserObject`.

Done.

https://codereview.chromium.org/27514003/diff/1/sdk/lib/js/dart2js/js_dart2js...
sdk/lib/js/dart2js/js_dart2js.dart:90: // print("UnknownJavaScriptObject:
${jsObj is UnknownJavaScriptObject}");
On 2013/10/16 19:32:11, alexandre.ardhuin wrote:
> Remove these comments ?

Done.

[alexandre.ardhuin, 2013-10-18 07:03:44]

>Alexandre, I think in general the approach of returning a JsArray is still fine
>since it's just a subclass of JsObject, but I don't think that automatically
>calling jsify() for Dart Maps and Iterables is what we want since it silently
>copies where  other operations proxy.

I agree. So we should provide a similar automatic proxification for Iterable and
Map. I've never seen a Iterable used as parameter without a call on
jsify(js.array).

https://codereview.chromium.org/27514003/diff/16001/sdk/lib/js/dart2js/js_dar...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/27514003/diff/16001/sdk/lib/js/dart2js/js_dar...
sdk/lib/js/dart2js/js_dart2js.dart:270: return
JS('Blob|DateTime|KeyRange|ImageData|Node', '#', o);
Is the JS call needed ? Isn't `return o` enough ?

https://codereview.chromium.org/27514003/diff/18001/tests/html/js_test.dart
File tests/html/js_test.dart (right):

https://codereview.chromium.org/27514003/diff/18001/tests/html/js_test.dart#n...
tests/html/js_test.dart:238: final date = new DateTime.now();
You can replace `new DateTime.now()` with `new Object()`.

https://codereview.chromium.org/27514003/diff/18001/tests/html/js_test.dart#n...
tests/html/js_test.dart:395: // all this tests is that null doesn't have the
method apply?
This test can be removed.

https://codereview.chromium.org/27514003/diff/18001/tests/html/js_test.dart#n...
tests/html/js_test.dart:481: group('JsObject.jsify()', () {
Add a test for cycle handling ?

[alexandre.ardhuin, 2013-10-18 08:09:36]

https://codereview.chromium.org/27514003/diff/18001/sdk/lib/js/dart2js/js_dar...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/27514003/diff/18001/sdk/lib/js/dart2js/js_dar...
sdk/lib/js/dart2js/js_dart2js.dart:7: import 'dart:html' show Blob, ImageData,
Node;
When we started dart:js, we saw a big impact on js generated code size when we
removed the handling of html.Element . Should the code size be taken in account
here ?

[justinfagnani, 2013-10-18 17:43:49]

On 2013/10/18 08:09:36, alexandre.ardhuin wrote:
>
https://codereview.chromium.org/27514003/diff/18001/sdk/lib/js/dart2js/js_dar...
> File sdk/lib/js/dart2js/js_dart2js.dart (right):
> 
>
https://codereview.chromium.org/27514003/diff/18001/sdk/lib/js/dart2js/js_dar...
> sdk/lib/js/dart2js/js_dart2js.dart:7: import 'dart:html' show Blob, ImageData,
> Node;
> When we started dart:js, we saw a big impact on js generated code size when we
> removed the handling of html.Element . Should the code size be taken in
account
> here ?

I'll do a new comparison today, but it shouldn't be much worse than the first
use of query() in an application, and it's critical functionality with shadow
DOM getting in the way of the previous node transfer approach.

[vsm, 2013-10-18 21:34:16]

lgtm

It'd be good to know the impact on code size of bringing in dart:html, etc., and
letting the dart2js team know if it's significant.

https://chromiumcodereview.appspot.com/27514003/diff/138001/tests/html/js_tes...
File tests/html/js_test.dart (right):

https://chromiumcodereview.appspot.com/27514003/diff/138001/tests/html/js_tes...
tests/html/js_test.dart:427: var node = new JsObject.fromBrowserObject(new
DivElement());
Test that you can get to a JS expando?

https://chromiumcodereview.appspot.com/27514003/diff/138001/tests/html/js_tes...
tests/html/js_test.dart:662: expect(list is Int32List, isTrue);
Consider tests for indexed access and write:
- list[3] == 4
- list[2] = 32
- list[2.5] = ... ; (should this add an expando?)
- list[-1] = ...; (ditto)
- x = list[34]; (unknown/null or bounds exception?)

[Jacob, 2013-10-18 22:03:43]

https://chromiumcodereview.appspot.com/27514003/diff/138001/sdk/lib/js/dart2j...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://chromiumcodereview.appspot.com/27514003/diff/138001/sdk/lib/js/dart2j...
sdk/lib/js/dart2js/js_dart2js.dart:196: /*
nit: /* --> /**

https://chromiumcodereview.appspot.com/27514003/diff/138001/sdk/lib/js/dart2j...
sdk/lib/js/dart2js/js_dart2js.dart:207: dynamic apply([thisArg, List args]) =>
change to 
apply(List args, {thisArg})

https://chromiumcodereview.appspot.com/27514003/diff/138001/tests/html/js_tes...
File tests/html/js_test.dart (right):

https://chromiumcodereview.appspot.com/27514003/diff/138001/tests/html/js_tes...
tests/html/js_test.dart:662: expect(list is Int32List, isTrue);
On 2013/10/18 21:34:16, vsm wrote:
> Consider tests for indexed access and write:
> - list[3] == 4
> - list[2] = 32
> - list[2.5] = ... ; (should this add an expando?)
> - list[-1] = ...; (ditto)
> - x = list[34]; (unknown/null or bounds exception?)

// should add an expando
list[1000000000000] = 1
x = list[34] should return null

[justinfagnani, 2013-10-18 22:38:40]

one more time...

https://codereview.chromium.org/27514003/diff/18001/sdk/lib/js/dart2js/js_dar...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/27514003/diff/18001/sdk/lib/js/dart2js/js_dar...
sdk/lib/js/dart2js/js_dart2js.dart:7: import 'dart:html' show Blob, ImageData,
Node;
On 2013/10/18 08:09:36, alexandre.ardhuin wrote:
> When we started dart:js, we saw a big impact on js generated code size when we
> removed the handling of html.Element . Should the code size be taken in
account
> here ?

Replied in CL-level comment, but I'm going to take a look at this more today.

https://codereview.chromium.org/27514003/diff/18001/tests/html/js_test.dart
File tests/html/js_test.dart (right):

https://codereview.chromium.org/27514003/diff/18001/tests/html/js_test.dart#n...
tests/html/js_test.dart:238: final date = new DateTime.now();
On 2013/10/18 07:03:45, alexandre.ardhuin wrote:
> You can replace `new DateTime.now()` with `new Object()`.

Done.

https://codereview.chromium.org/27514003/diff/18001/tests/html/js_test.dart#n...
tests/html/js_test.dart:395: // all this tests is that null doesn't have the
method apply?
On 2013/10/18 07:03:45, alexandre.ardhuin wrote:
> This test can be removed.

Done.

https://codereview.chromium.org/27514003/diff/138001/sdk/lib/js/dart2js/js_da...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/27514003/diff/138001/sdk/lib/js/dart2js/js_da...
sdk/lib/js/dart2js/js_dart2js.dart:196: /*
On 2013/10/18 22:03:43, Jacob wrote:
> nit: /* --> /**

Done.

https://codereview.chromium.org/27514003/diff/138001/sdk/lib/js/dart2js/js_da...
sdk/lib/js/dart2js/js_dart2js.dart:207: dynamic apply([thisArg, List args]) =>
On 2013/10/18 22:03:43, Jacob wrote:
> change to 
> apply(List args, {thisArg})

Done.

https://codereview.chromium.org/27514003/diff/138001/tests/html/js_test.dart
File tests/html/js_test.dart (right):

https://codereview.chromium.org/27514003/diff/138001/tests/html/js_test.dart#...
tests/html/js_test.dart:427: var node = new JsObject.fromBrowserObject(new
DivElement());
On 2013/10/18 21:34:16, vsm wrote:
> Test that you can get to a JS expando?

Done.

https://codereview.chromium.org/27514003/diff/138001/tests/html/js_test.dart#...
tests/html/js_test.dart:662: expect(list is Int32List, isTrue);
On 2013/10/18 21:34:16, vsm wrote:
> Consider tests for indexed access and write:
> - list[3] == 4
> - list[2] = 32
> - list[2.5] = ... ; (should this add an expando?)
> - list[-1] = ...; (ditto)
> - x = list[34]; (unknown/null or bounds exception?)

[2.5], [-1], and [34] all should throw because this is the dart Int32List object

https://codereview.chromium.org/27514003/diff/138001/tests/html/js_test.dart#...
tests/html/js_test.dart:662: expect(list is Int32List, isTrue);
On 2013/10/18 22:03:43, Jacob wrote:
> On 2013/10/18 21:34:16, vsm wrote:
> > Consider tests for indexed access and write:
> > - list[3] == 4
> > - list[2] = 32
> > - list[2.5] = ... ; (should this add an expando?)
> > - list[-1] = ...; (ditto)
> > - x = list[34]; (unknown/null or bounds exception?)
> 
> // should add an expando
> list[1000000000000] = 1
> x = list[34] should return null

they should throw, correct? should I use fromBrowserObject() for in this test
some reason? that's already covered with node.

[justinfagnani, 2013-10-19 04:18:46]

my comments on Jacob's patchset 9. Jacob, your Dart side is lgtm

https://codereview.chromium.org/27514003/diff/248001/tests/html/js_test.dart
File tests/html/js_test.dart (right):

https://codereview.chromium.org/27514003/diff/248001/tests/html/js_test.dart#...
tests/html/js_test.dart:545: test('handles cycles', () {
I think you missed my last patchset

https://codereview.chromium.org/27514003/diff/268001/tests/html/js_test.dart
File tests/html/js_test.dart (right):

https://codereview.chromium.org/27514003/diff/268001/tests/html/js_test.dart#...
tests/html/js_test.dart:247: expect(foo2, same(context['foo2']));
if you change same() to equals() this should still pass, and was an old test
before the referential integrity change

https://codereview.chromium.org/27514003/diff/268001/tests/html/js_test.dart#...
tests/html/js_test.dart:607: /* TODO(jacobr): remove this test as it is a dupe
and not the correct checked
what's it a dupe of?

[Jacob, 2013-10-19 04:35:54]

https://codereview.chromium.org/27514003/diff/248001/tests/html/js_test.dart
File tests/html/js_test.dart (right):

https://codereview.chromium.org/27514003/diff/248001/tests/html/js_test.dart#...
tests/html/js_test.dart:545: test('handles cycles', () {
On 2013/10/19 04:18:46, justinfagnani wrote:
> I think you missed my last patchset

I patched when you told me to. If you have newer patches can you rebase to this
CL and send them for review?
git cl patch is horrible about handling conflicting cls so I don't know a good
way of merging this cl to that cl without committing.

https://codereview.chromium.org/27514003/diff/268001/tests/html/js_test.dart
File tests/html/js_test.dart (right):

https://codereview.chromium.org/27514003/diff/268001/tests/html/js_test.dart#...
tests/html/js_test.dart:607: /* TODO(jacobr): remove this test as it is a dupe
and not the correct checked
On 2013/10/19 04:18:46, justinfagnani wrote:
> what's it a dupe of?

nevermind... there were other tests verifying that methods threw ArgumentErrors
but not for this method.

[Jacob, 2013-10-19 04:36:16]

Committed patchset #10 manually as r28902 (presubmit successful).

[alexandre.ardhuin, 2013-10-20 07:17:00]

A little late on this :(

Moreover with the last continuous build and the following test has 2 problems.

  final JsFunction s = context['String'];
  print(s.apply(['a'])); // display "" instead of "a"
  print(s.apply(['a'], thisArg: 'b')); // fail with the following error

NoSuchMethodError: incorrect number of arguments passed to method named 'apply'
Receiver: Instance of 'JsFunction'
Tried calling: apply(_GrowableList len:1, thisArg: "b")
Found: apply(thisArg, args)

https://codereview.chromium.org/27514003/diff/338001/sdk/lib/js/dart2js/js_da...
File sdk/lib/js/dart2js/js_dart2js.dart (right):

https://codereview.chromium.org/27514003/diff/338001/sdk/lib/js/dart2js/js_da...
sdk/lib/js/dart2js/js_dart2js.dart:172: bool instanceof(type) {
`type` should be JsFunction

https://codereview.chromium.org/27514003/diff/338001/sdk/lib/js/dartium/js_da...
File sdk/lib/js/dartium/js_dartium.dart (right):

https://codereview.chromium.org/27514003/diff/338001/sdk/lib/js/dartium/js_da...
sdk/lib/js/dartium/js_dartium.dart:72: bool hasProperty(String property) native
"JsObject_hasProperty";
`property` should be dynamic like in js_dart2js.dart

https://codereview.chromium.org/27514003/diff/338001/sdk/lib/js/dartium/js_da...
sdk/lib/js/dartium/js_dartium.dart:74: void deleteProperty(JsFunction name)
native "JsObject_deleteProperty";
`name` should be dynamic like in js_dart2js.dart