Issue 26729002: dartium_js_interop changes

Created:
7 years, 2 months ago by Jacob

Modified:
6 years, 6 months ago

Reviewers:
vsm, justinfagnani

CC:
reviews_dartlang.org, vsm

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Patch Set 1 : ready for review #

Total comments: 18

Patch Set 4 : Rebased and replied to comments. PTAL #

Created: 7 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+53 lines, -378 lines)			Patch
M	sdk/lib/js/dartium/js_dartium.dart	View	1 2 3	1 chunk	+51 lines, -374 lines	0 comments	Download
M	tools/dom/scripts/systemnative.py	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	tools/dom/src/native_DOMImplementation.dart	View	1 2 3	1 chunk	+0 lines, -4 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

justinfagnani

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium/js_dartium.dart File sdk/lib/js/dartium/js_dartium.dart (right): https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium/js_dartium.dart#newcode15 sdk/lib/js/dartium/js_dartium.dart:15: JsObject jsify(dynamic data) native "Js_jsify"; I think we should ...

7 years, 2 months ago (2013-10-11 18:30:59 UTC) #2

Jennifer Messerly

DBC https://codereview.chromium.org/26729002/diff/10001/sdk/lib/js/dartium/js_dartium.dart File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/26729002/diff/10001/sdk/lib/js/dartium/js_dartium.dart#newcode5 sdk/lib/js/dartium/js_dartium.dart:5: library dart.Js; did you mean to uppercase the ...

7 years, 2 months ago (2013-10-11 19:21:09 UTC) #3

vsm

DBC https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium/js_dartium.dart File sdk/lib/js/dartium/js_dartium.dart (right): https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium/js_dartium.dart#newcode36 sdk/lib/js/dartium/js_dartium.dart:36: factory JsObject.fromDartObject(var object) { Perhaps fromBrowserObject? Will this ...

7 years, 2 months ago (2013-10-15 21:05:33 UTC) #4

Jacob

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium/js_dartium.dart File sdk/lib/js/dartium/js_dartium.dart (right): https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium/js_dartium.dart#newcode5 sdk/lib/js/dartium/js_dartium.dart:5: library dart.Js; On 2013/10/11 19:21:09, John Messerly wrote: > ...

7 years, 2 months ago (2013-10-15 22:39:43 UTC) #5

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

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:5: library dart.Js;
On 2013/10/11 19:21:09, John Messerly wrote:
> did you mean to uppercase the "J"?
Done.
lowercased.

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:15: JsObject jsify(dynamic data) native
"Js_jsify";
On 2013/10/11 18:30:59, justinfagnani wrote:
> I think we should move this to a constructor on JsObject that has a name to
> indicate the deep-conversion behavior

Done. Although we still need a clearer name.

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:23: class JsObject extends
NativeFieldWrapperClass1 {
On 2013/10/11 18:30:59, justinfagnani wrote:
> will NativeFieldWrapperClass1 be visible in the objects superclass chain? How
> can we make this and the dart2js JsObject be the same? Should JsObject just be
> an interface?
Discussed offline.
I wouldn't worry too much about the extends NativeFieldWrapperClass1
All classes in dart:html suffer from the same wart and so far it hasn't been an
issue as far as I know. That class doesn't have any public methods and cannot be
imported when using dart2js.

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:36: factory JsObject.fromDartObject(var
object) {
On 2013/10/15 21:05:34, vsm wrote:
> Perhaps fromBrowserObject?  Will this throw if object isn't a native type? 
> Maybe check that object is NativeFieldWrapper*?

one step ahead of me. :) just wrote a comment that we will do exactly that. I
also prefer the name fromBrowserObject. I disliked the existing name.

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:37: if (object is num || object is String ||
object == null) {
On 2013/10/11 18:30:59, justinfagnani wrote:
> add bool
synced offline.  Instead restricting further and throwing if object isn't one of
the small list of types that we perform auto-conversions on. Those are the only
types where this method makes any sense.

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:37: if (object is num || object is String ||
object == null) {
On 2013/10/11 18:30:59, justinfagnani wrote:
> add bool

Synced offline. We'll restrict which types can be passed to this method further.
Only types where we can get a true JS peer will be allowed. All others will
throw.  This means only the subset of browser native types that we allow
auto-conversion for.

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:38: throw new
IllegalArgumentException("object cannot be a num, string, or null");
On 2013/10/11 18:30:59, justinfagnani wrote:
> what about returning a proxy to the boxed primitive?

same note as previous comment.

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:48: int get hashCode => 0;
On 2013/10/11 18:30:59, justinfagnani wrote:
> we talked about an identity-based hashCode

added that to this cl.

https://chromiumcodereview.appspot.com/26729002/diff/10001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:73: // String get jsType native
"JsObject_jsType";
On 2013/10/11 18:30:59, justinfagnani wrote:
> yes, remove it, it's not in current dart:js anyway

yay :) done.

justinfagnani

https://chromiumcodereview.appspot.com/26729002/diff/20001/sdk/lib/js/dartium/js_dartium.dart File sdk/lib/js/dartium/js_dartium.dart (right): https://chromiumcodereview.appspot.com/26729002/diff/20001/sdk/lib/js/dartium/js_dartium.dart#newcode24 sdk/lib/js/dartium/js_dartium.dart:24: JsFunction captureThis(Function f) native "Js_captureThis"; This has moved to ...

7 years, 2 months ago (2013-10-18 01:05:47 UTC) #6

Jacob

7 years, 2 months ago (2013-10-18 21:56:24 UTC) #7

PTAL

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

https://chromiumcodereview.appspot.com/26729002/diff/20001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:24: JsFunction captureThis(Function f) native
"Js_captureThis";
On 2013/10/18 01:05:47, justinfagnani wrote:
> This has moved to the named constructor JsFunction.captureThis in my CL

Done.

https://chromiumcodereview.appspot.com/26729002/diff/20001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:43: * TODO(jacobr): support Event and
NodeList as well.
On 2013/10/18 01:05:47, justinfagnani wrote:
> Window too, if we figure out what to do with other frames

Done.

https://chromiumcodereview.appspot.com/26729002/diff/20001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:43: * TODO(jacobr): support Event and
NodeList as well.
On 2013/10/18 01:05:47, justinfagnani wrote:
> Window too, if we figure out what to do with other frames

Done.

https://chromiumcodereview.appspot.com/26729002/diff/20001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:90: callMethod(String name, [List args])
native "JsObject_callMethod";
On 2013/10/18 01:05:47, justinfagnani wrote:
> I wonder if name should have the same type restrictions (String or num) as
other
> property names

I think name should be a string.
You can't write a JavaScript expression like

foo.3.someMethod()

so there is no need to support that here.
If you really want to call a property that is a number you call always write
foo[3].apply()
just as in JS you would have to write
foo[3]()
or foo[3].apply(...)

https://chromiumcodereview.appspot.com/26729002/diff/20001/sdk/lib/js/dartium...
sdk/lib/js/dartium/js_dartium.dart:94: apply([thisArg, List args]) native
"JsFunction_apply";
On 2013/10/18 01:05:47, justinfagnani wrote:
> I think (possibly) thisArg should either come second, or both args should be
> named. It should be very rare to use thisArg

synced offline. Made thisArg named and moved it to the second position.
Made args required.

Expand Messages | Collapse Messages