Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15782009:
RFC: introduce dart:js (Closed)
|
Created:
7 years, 6 months ago by alexandre.ardhuin Modified:
7 years, 5 months ago Reviewers:
vsm CC:
reviews_dartlang.org, justinfagnani Visibility:
Public. |
DescriptionRFC: introduce dart:js
This is a port of js-interop with an optimized dart2js compilation.
It also gets rid of noSuchMethod.
I also made some renamings inspired by https://github.com/dart-lang/js-interop/issues/68
BUG=
R=vsm@google.com
Committed: https://code.google.com/p/dart/source/detail?r=25018
Patch Set 1 #
Total comments: 27
Patch Set 2 : fix vsm comments #Patch Set 3 : fix review comments #Patch Set 4 : without html and without scopes #Patch Set 5 : remove Element handling + fix js_dartium for passing tests #
Total comments: 18
Patch Set 6 : additional fixes #Patch Set 7 : remove all scope handlings #
Total comments: 8
Patch Set 8 : fix comment on patch set 7 #Patch Set 9 : homogeneous hashCode and operator==(other) + dead code elimination #
Total comments: 10
Patch Set 10 : fix comments on patch 9 #
Messages
Total messages: 27 (0 generated)
Nice start! https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/_internal/libr... File sdk/lib/_internal/libraries.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/_internal/libr... sdk/lib/_internal/libraries.dart:73: dart2jsPath: "js/dart2js/js.dart"), We probably should name this js_dart2js.dart and js_dartium.dart for consistency with (most) other libraries that follow this pattern. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js.... File sdk/lib/js/dart2js/js.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js.... sdk/lib/js/dart2js/js.dart:13: _enterScopeIfNeeded(); Shall we make scopes no-ops here and get rid of this? It would avoid pulling in dart:async altogether. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... File sdk/lib/js/dartium/js.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:79: // this bootstrap string. We'll need an alternate solution here. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:489: // generic way that may not work, particulary when the constructor does not particulary -> particularly https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:759: /// WARNING: This API is experimental and may be removed. We can get rid of this. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:768: /// WARNING: This API is experimental and may be removed. This too. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:795: * Converts a json-like [data] to a JavaScript map and return a [JsObject] to it. JavaScript map or array line length just over 80 https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:927: bool hasProperty(String property) => Needs a comment. Should we make these top-level instead of instance methods? If we ever add NSM back, these may shadow. https://chromiumcodereview.appspot.com/15782009/diff/1/tools/create_sdk.py File tools/create_sdk.py (right): https://chromiumcodereview.appspot.com/15782009/diff/1/tools/create_sdk.py#ne... tools/create_sdk.py:221: 'js', 'json', 'math', 'mdv_observe_impl', 'mirrors', 'typed_data', line length
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/_internal/libr... File sdk/lib/_internal/libraries.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/_internal/libr... sdk/lib/_internal/libraries.dart:73: dart2jsPath: "js/dart2js/js.dart"), On 2013/05/29 04:55:39, vsm wrote: > We probably should name this js_dart2js.dart and js_dartium.dart for consistency > with (most) other libraries that follow this pattern. Done. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js.... File sdk/lib/js/dart2js/js.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js.... sdk/lib/js/dart2js/js.dart:13: _enterScopeIfNeeded(); On 2013/05/29 04:55:39, vsm wrote: > Shall we make scopes no-ops here and get rid of this? It would avoid pulling in > dart:async altogether. I just made some tests. On js_tests.dart we loose only 7% of size. So, we should perhaps keep it like that. What do you think ? https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... File sdk/lib/js/dartium/js.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:79: // this bootstrap string. On 2013/05/29 04:55:39, vsm wrote: > We'll need an alternate solution here. Agree but I have no answer :/ Perhaps in a package like "browser/dart.js" ? https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:489: // generic way that may not work, particulary when the constructor does not On 2013/05/29 04:55:39, vsm wrote: > particulary -> particularly Done. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:759: /// WARNING: This API is experimental and may be removed. On 2013/05/29 04:55:39, vsm wrote: > We can get rid of this. Done. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:768: /// WARNING: This API is experimental and may be removed. On 2013/05/29 04:55:39, vsm wrote: > This too. Done. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:795: * Converts a json-like [data] to a JavaScript map and return a [JsObject] to it. On 2013/05/29 04:55:39, vsm wrote: > JavaScript map or array > > line length just over 80 Done. https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dartium/js.... sdk/lib/js/dartium/js.dart:927: bool hasProperty(String property) => On 2013/05/29 04:55:39, vsm wrote: > Needs a comment. > > Should we make these top-level instead of instance methods? If we ever add NSM > back, these may shadow. Commented. Perhaps we could avoid NSM here and keep the js package on top of it or something like Proxy that provides NSM and makes its usage simpler.
Some more comments:
(1) Can you try getting rid of dart:async and dart:html deps?
(2) If you do that, how small can you make:
import 'dart:js';
main() => context.console.log('Hello from JS');
My sense is that we should be get this quite small if there is *no* dart:html
anywhere in the app.
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
File sdk/lib/js/dart2js/js.dart (right):
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:8: import 'dart:html' show Element, document;
Can you try killing the two imports above and testing code size? (See comments
below for eliminating deps.)
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:13: _enterScopeIfNeeded();
Did you eliminate dart:async above as well?
On 2013/05/31 19:11:13, alexandre.ardhuin wrote:
> On 2013/05/29 04:55:39, vsm wrote:
> > Shall we make scopes no-ops here and get rid of this? It would avoid
pulling
> in
> > dart:async altogether.
>
> I just made some tests. On js_tests.dart we loose only 7% of size.
>
> So, we should perhaps keep it like that. What do you think ?
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:54: int $experimentalEnterScope() {
These can go.
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:91: final bool _manualDispose;
We can try getting rid of _manualDispose and related logic as well.
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:94: Object _jsFunction;
This should probably be typed dynamic. That is what we do elsewhere for raw JS
pointers with no corresponding Dart type.
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:273: dynamic _serialize(dynamic o) {
Is this really serializing? Perhaps call _convertToJavaScript?
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:278: } else if (o is Element && (o.document == null
|| o.document == document)) {
Let's try removing the dependence on dart:html.
Can you replace with your check below:
if (JS('bool', '# instanceof Element && '
'(#.ownerDocument == null || #.ownerDocument === document)', o, o, o))
This should do the same without pulling in dart:html.
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:291: return o;
Does this mean we're passing a raw Dart object to JS? We probably need to wrap
this as a DartProxy.
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:294: dynamic _deserialize(dynamic o) {
_convertToDart?
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
sdk/lib/js/dart2js/js.dart:313: final js = !dartOnly;
Note, if you're getting rid of scopes, you can try returning 0 here and really
scrubbing out all this logic.
One more:
Can you try replacing all occurrences of JS('Object', ...) and JS('dynamic',
...) and replace them with JS('=Object')? This appears (for now) to be the best
type to give to any raw JS object that is only access itself via JS. Using
Object or dynamic instead makes the type analysis very conservative as the
system assumes every type in the program is possible.
thanks!
On 2013/06/06 17:06:41, vsm wrote:
> Some more comments:
>
> (1) Can you try getting rid of dart:async and dart:html deps?
>
> (2) If you do that, how small can you make:
>
> import 'dart:js';
>
> main() => context.console.log('Hello from JS');
>
> My sense is that we should be get this quite small if there is *no* dart:html
> anywhere in the app.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> File sdk/lib/js/dart2js/js.dart (right):
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:8: import 'dart:html' show Element, document;
> Can you try killing the two imports above and testing code size? (See
comments
> below for eliminating deps.)
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:13: _enterScopeIfNeeded();
> Did you eliminate dart:async above as well?
>
> On 2013/05/31 19:11:13, alexandre.ardhuin wrote:
> > On 2013/05/29 04:55:39, vsm wrote:
> > > Shall we make scopes no-ops here and get rid of this? It would avoid
> pulling
> > in
> > > dart:async altogether.
> >
> > I just made some tests. On js_tests.dart we loose only 7% of size.
> >
> > So, we should perhaps keep it like that. What do you think ?
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:54: int $experimentalEnterScope() {
> These can go.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:91: final bool _manualDispose;
> We can try getting rid of _manualDispose and related logic as well.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:94: Object _jsFunction;
> This should probably be typed dynamic. That is what we do elsewhere for raw
JS
> pointers with no corresponding Dart type.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:273: dynamic _serialize(dynamic o) {
> Is this really serializing? Perhaps call _convertToJavaScript?
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:278: } else if (o is Element && (o.document == null
> || o.document == document)) {
> Let's try removing the dependence on dart:html.
>
> Can you replace with your check below:
> if (JS('bool', '# instanceof Element && '
> '(#.ownerDocument == null || #.ownerDocument === document)', o, o, o))
>
> This should do the same without pulling in dart:html.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:291: return o;
> Does this mean we're passing a raw Dart object to JS? We probably need to
wrap
> this as a DartProxy.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:294: dynamic _deserialize(dynamic o) {
> _convertToDart?
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:313: final js = !dartOnly;
> Note, if you're getting rid of scopes, you can try returning 0 here and really
> scrubbing out all this logic.
I think you have reviewed the old version. I have renamed js.dart to
js_dart2js.dart in patch set 2.
I can't get rid of 'dart:html' otherwise I get the following error using dart2js
:
Type 'Element' not found
return JS('Element', '#', o);
However I can get rid of 'dart:html' if I change this into :
return JS('=Object', '#', o);
but I don't know what will be the consequences (although the tests pass)
For the following code :
import 'dart:js';
void main() {
context['console'].callMethod('log', ['Hello from JS']);
}
here are the compilation sizes (with r23302) :
- actual : 333876
- without scope : 322641 (but breaks some tests)
- without html : 133875
Without 'dart:html' the compilation is really smaller ! But I don't know what
'=Object' instead of 'Element' will bring.
On 2013/06/06 17:06:41, vsm wrote:
> Some more comments:
>
> (1) Can you try getting rid of dart:async and dart:html deps?
>
> (2) If you do that, how small can you make:
>
> import 'dart:js';
>
> main() => context.console.log('Hello from JS');
>
> My sense is that we should be get this quite small if there is *no* dart:html
> anywhere in the app.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> File sdk/lib/js/dart2js/js.dart (right):
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:8: import 'dart:html' show Element, document;
> Can you try killing the two imports above and testing code size? (See
comments
> below for eliminating deps.)
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:13: _enterScopeIfNeeded();
> Did you eliminate dart:async above as well?
>
> On 2013/05/31 19:11:13, alexandre.ardhuin wrote:
> > On 2013/05/29 04:55:39, vsm wrote:
> > > Shall we make scopes no-ops here and get rid of this? It would avoid
> pulling
> > in
> > > dart:async altogether.
> >
> > I just made some tests. On js_tests.dart we loose only 7% of size.
> >
> > So, we should perhaps keep it like that. What do you think ?
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:54: int $experimentalEnterScope() {
> These can go.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:91: final bool _manualDispose;
> We can try getting rid of _manualDispose and related logic as well.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:94: Object _jsFunction;
> This should probably be typed dynamic. That is what we do elsewhere for raw
JS
> pointers with no corresponding Dart type.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:273: dynamic _serialize(dynamic o) {
> Is this really serializing? Perhaps call _convertToJavaScript?
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:278: } else if (o is Element && (o.document == null
> || o.document == document)) {
> Let's try removing the dependence on dart:html.
>
> Can you replace with your check below:
> if (JS('bool', '# instanceof Element && '
> '(#.ownerDocument == null || #.ownerDocument === document)', o, o, o))
>
> This should do the same without pulling in dart:html.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:291: return o;
> Does this mean we're passing a raw Dart object to JS? We probably need to
wrap
> this as a DartProxy.
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:294: dynamic _deserialize(dynamic o) {
> _convertToDart?
>
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> sdk/lib/js/dart2js/js.dart:313: final js = !dartOnly;
> Note, if you're getting rid of scopes, you can try returning 0 here and really
> scrubbing out all this logic.
On 2013/06/06 19:58:24, alexandre.ardhuin wrote:
> I think you have reviewed the old version. I have renamed js.dart to
> js_dart2js.dart in patch set 2.
>
> I can't get rid of 'dart:html' otherwise I get the following error using
dart2js
> :
> Type 'Element' not found
> return JS('Element', '#', o);
> However I can get rid of 'dart:html' if I change this into :
> return JS('=Object', '#', o);
> but I don't know what will be the consequences (although the tests pass)
Hmm - this could be problematic. Can you test something like this with
'dart:html' removed?
import 'dart:js';
void main() {
var canvas = context.canvasElementDefinedInJs;
print(canvas.height);
}
E.g., don't bring in the definition of Element (or CanvasElement) in - let's see
if this breaks canvas.height.
>
>
> For the following code :
>
> import 'dart:js';
> void main() {
> context['console'].callMethod('log', ['Hello from JS']);
> }
>
> here are the compilation sizes (with r23302) :
> - actual : 333876
> - without scope : 322641 (but breaks some tests)
> - without html : 133875
>
> Without 'dart:html' the compilation is really smaller ! But I don't know what
> '=Object' instead of 'Element' will bring.
Nice! We might have to jump another hoop or so, but I think we can get this to
work.
>
>
>
> On 2013/06/06 17:06:41, vsm wrote:
> > Some more comments:
> >
> > (1) Can you try getting rid of dart:async and dart:html deps?
> >
> > (2) If you do that, how small can you make:
> >
> > import 'dart:js';
> >
> > main() => context.console.log('Hello from JS');
> >
> > My sense is that we should be get this quite small if there is *no*
dart:html
> > anywhere in the app.
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > File sdk/lib/js/dart2js/js.dart (right):
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:8: import 'dart:html' show Element, document;
> > Can you try killing the two imports above and testing code size? (See
> comments
> > below for eliminating deps.)
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:13: _enterScopeIfNeeded();
> > Did you eliminate dart:async above as well?
> >
> > On 2013/05/31 19:11:13, alexandre.ardhuin wrote:
> > > On 2013/05/29 04:55:39, vsm wrote:
> > > > Shall we make scopes no-ops here and get rid of this? It would avoid
> > pulling
> > > in
> > > > dart:async altogether.
> > >
> > > I just made some tests. On js_tests.dart we loose only 7% of size.
> > >
> > > So, we should perhaps keep it like that. What do you think ?
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:54: int $experimentalEnterScope() {
> > These can go.
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:91: final bool _manualDispose;
> > We can try getting rid of _manualDispose and related logic as well.
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:94: Object _jsFunction;
> > This should probably be typed dynamic. That is what we do elsewhere for raw
> JS
> > pointers with no corresponding Dart type.
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:273: dynamic _serialize(dynamic o) {
> > Is this really serializing? Perhaps call _convertToJavaScript?
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:278: } else if (o is Element && (o.document ==
null
> > || o.document == document)) {
> > Let's try removing the dependence on dart:html.
> >
> > Can you replace with your check below:
> > if (JS('bool', '# instanceof Element && '
> > '(#.ownerDocument == null || #.ownerDocument === document)', o, o, o))
> >
> > This should do the same without pulling in dart:html.
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:291: return o;
> > Does this mean we're passing a raw Dart object to JS? We probably need to
> wrap
> > this as a DartProxy.
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:294: dynamic _deserialize(dynamic o) {
> > _convertToDart?
> >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > sdk/lib/js/dart2js/js.dart:313: final js = !dartOnly;
> > Note, if you're getting rid of scopes, you can try returning 0 here and
really
> > scrubbing out all this logic.
On 2013/06/06 20:08:57, vsm wrote:
> On 2013/06/06 19:58:24, alexandre.ardhuin wrote:
> > I think you have reviewed the old version. I have renamed js.dart to
> > js_dart2js.dart in patch set 2.
> >
> > I can't get rid of 'dart:html' otherwise I get the following error using
> dart2js
> > :
> > Type 'Element' not found
> > return JS('Element', '#', o);
> > However I can get rid of 'dart:html' if I change this into :
> > return JS('=Object', '#', o);
> > but I don't know what will be the consequences (although the tests pass)
>
> Hmm - this could be problematic. Can you test something like this with
> 'dart:html' removed?
>
> import 'dart:js';
>
> void main() {
> var canvas = context.canvasElementDefinedInJs;
> print(canvas.height);
> }
>
> E.g., don't bring in the definition of Element (or CanvasElement) in - let's
see
> if this breaks canvas.height.
Without 'dart:html' compilation works but execution fails :
Uncaught TypeError: Object #<HTMLCanvasElement> has no method 'get$height'
But the test works if I change a little the test with :
import 'dart:js';
import 'dart:html';
void main() {
var canvas = context['canvasElementDefinedInJs'] as CanvasElement;
print(canvas.height);
}
However, the code size is now the same as with the html version of
js_dart2js.dart !
Is it really important to get rid of 'dart:html' ? I know that the code size for
this simple test is really smaller but the majority of dart scripts will import
'dart:html' and their code size will not be reduce a lot with that.
> >
> >
> > For the following code :
> >
> > import 'dart:js';
> > void main() {
> > context['console'].callMethod('log', ['Hello from JS']);
> > }
> >
> > here are the compilation sizes (with r23302) :
> > - actual : 333876
> > - without scope : 322641 (but breaks some tests)
> > - without html : 133875
> >
> > Without 'dart:html' the compilation is really smaller ! But I don't know
what
> > '=Object' instead of 'Element' will bring.
>
> Nice! We might have to jump another hoop or so, but I think we can get this
to
> work.
>
> >
> >
> >
> > On 2013/06/06 17:06:41, vsm wrote:
> > > Some more comments:
> > >
> > > (1) Can you try getting rid of dart:async and dart:html deps?
> > >
> > > (2) If you do that, how small can you make:
> > >
> > > import 'dart:js';
> > >
> > > main() => context.console.log('Hello from JS');
> > >
> > > My sense is that we should be get this quite small if there is *no*
> dart:html
> > > anywhere in the app.
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > File sdk/lib/js/dart2js/js.dart (right):
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:8: import 'dart:html' show Element, document;
> > > Can you try killing the two imports above and testing code size? (See
> > comments
> > > below for eliminating deps.)
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:13: _enterScopeIfNeeded();
> > > Did you eliminate dart:async above as well?
> > >
> > > On 2013/05/31 19:11:13, alexandre.ardhuin wrote:
> > > > On 2013/05/29 04:55:39, vsm wrote:
> > > > > Shall we make scopes no-ops here and get rid of this? It would avoid
> > > pulling
> > > > in
> > > > > dart:async altogether.
> > > >
> > > > I just made some tests. On js_tests.dart we loose only 7% of size.
> > > >
> > > > So, we should perhaps keep it like that. What do you think ?
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:54: int $experimentalEnterScope() {
> > > These can go.
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:91: final bool _manualDispose;
> > > We can try getting rid of _manualDispose and related logic as well.
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:94: Object _jsFunction;
> > > This should probably be typed dynamic. That is what we do elsewhere for
raw
> > JS
> > > pointers with no corresponding Dart type.
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:273: dynamic _serialize(dynamic o) {
> > > Is this really serializing? Perhaps call _convertToJavaScript?
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:278: } else if (o is Element && (o.document ==
> null
> > > || o.document == document)) {
> > > Let's try removing the dependence on dart:html.
> > >
> > > Can you replace with your check below:
> > > if (JS('bool', '# instanceof Element && '
> > > '(#.ownerDocument == null || #.ownerDocument === document)', o, o, o))
> > >
> > > This should do the same without pulling in dart:html.
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:291: return o;
> > > Does this mean we're passing a raw Dart object to JS? We probably need to
> > wrap
> > > this as a DartProxy.
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:294: dynamic _deserialize(dynamic o) {
> > > _convertToDart?
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15782009/diff/1/sdk/lib/js/dart2js/js....
> > > sdk/lib/js/dart2js/js.dart:313: final js = !dartOnly;
> > > Note, if you're getting rid of scopes, you can try returning 0 here and
> really
> > > scrubbing out all this logic.
Your last comments are fixed (except for the removing of dart:async, I have the CL locally)
> > > import 'dart:js';
> > > void main() {
> > > context['console'].callMethod('log', ['Hello from JS']);
> > > }
> > >
> > > here are the compilation sizes (with r23302) :
> > > - actual : 333876
> > > - without scope : 322641 (but breaks some tests)
> > > - without html : 133875
> > >
> > > Without 'dart:html' the compilation is really smaller ! But I don't know
> what
> > > '=Object' instead of 'Element' will bring.
> >
> > Nice! We might have to jump another hoop or so, but I think we can get this
> to
> > work.
> >
Here are some additionnal numbers after merging the trunk.
Compilation sizes with r23742 :
- with patch set 3 : 153038
- with patch set 3 + without html (no Element handling) : 124435
- with patch set 3 + without scope : 71044
- with patch set 3 + without html (no Element handling) + without scope : 42728
I tend to like your proposition 'don't "serialize" Element here. Handle it in
package:js instead' . This could also provide a way to make monkeypatching on
html elements as described in http://dartbug.com/9991
I have to make a version of package:js built on dart:js to see how we can handle
html Elements.
On the scope issue, it's tempting to remove it for the dart2js compilation but
this will lead to different behaviour between dart and js versions. Have you
some feedback from the Dart team about this concern ?
On 2013/06/07 14:19:12, alexandre.ardhuin wrote:
> > > > import 'dart:js';
> > > > void main() {
> > > > context['console'].callMethod('log', ['Hello from JS']);
> > > > }
> > > >
> > > > here are the compilation sizes (with r23302) :
> > > > - actual : 333876
> > > > - without scope : 322641 (but breaks some tests)
> > > > - without html : 133875
> > > >
> > > > Without 'dart:html' the compilation is really smaller ! But I don't know
> > what
> > > > '=Object' instead of 'Element' will bring.
> > >
> > > Nice! We might have to jump another hoop or so, but I think we can get
this
> > to
> > > work.
> > >
>
> Here are some additionnal numbers after merging the trunk.
> Compilation sizes with r23742 :
> - with patch set 3 : 153038
> - with patch set 3 + without html (no Element handling) : 124435
> - with patch set 3 + without scope : 71044
> - with patch set 3 + without html (no Element handling) + without scope :
42728
That looks pretty good. Can you upload the version without html and without
scope?
> I tend to like your proposition 'don't "serialize" Element here. Handle it in
> package:js instead' . This could also provide a way to make monkeypatching on
> html elements as described in http://dartbug.com/9991
>
> I have to make a version of package:js built on dart:js to see how we can
handle
> html Elements.
>
> On the scope issue, it's tempting to remove it for the dart2js compilation but
> this will lead to different behaviour between dart and js versions. Have you
> some feedback from the Dart team about this concern ?
I am comfortable with this as long as the APIs are compatible, and I have not
gotten objections. We'll need to update the docs to add flexibility over when
non-retained proxies may be invalidated and remove tests that expect them to be
eagerly invalidated.
On 2013/06/10 15:55:14, vsm wrote:
> On 2013/06/07 14:19:12, alexandre.ardhuin wrote:
> > > > > import 'dart:js';
> > > > > void main() {
> > > > > context['console'].callMethod('log', ['Hello from JS']);
> > > > > }
> > > > >
> > > > > here are the compilation sizes (with r23302) :
> > > > > - actual : 333876
> > > > > - without scope : 322641 (but breaks some tests)
> > > > > - without html : 133875
> > > > >
> > > > > Without 'dart:html' the compilation is really smaller ! But I don't
know
> > > what
> > > > > '=Object' instead of 'Element' will bring.
> > > >
> > > > Nice! We might have to jump another hoop or so, but I think we can get
> this
> > > to
> > > > work.
> > > >
> >
> > Here are some additionnal numbers after merging the trunk.
> > Compilation sizes with r23742 :
> > - with patch set 3 : 153038
> > - with patch set 3 + without html (no Element handling) : 124435
> > - with patch set 3 + without scope : 71044
> > - with patch set 3 + without html (no Element handling) + without scope :
> 42728
>
> That looks pretty good. Can you upload the version without html and without
> scope?
Done !
But js_tests.dart will fail (in dart2js version) on tests about scopes and
Elements. Should I remove them ?
>
> > I tend to like your proposition 'don't "serialize" Element here. Handle it
in
> > package:js instead' . This could also provide a way to make monkeypatching
on
> > html elements as described in http://dartbug.com/9991
> >
> > I have to make a version of package:js built on dart:js to see how we can
> handle
> > html Elements.
> >
> > On the scope issue, it's tempting to remove it for the dart2js compilation
but
> > this will lead to different behaviour between dart and js versions. Have you
> > some feedback from the Dart team about this concern ?
>
> I am comfortable with this as long as the APIs are compatible, and I have not
> gotten objections. We'll need to update the docs to add flexibility over when
> non-retained proxies may be invalidated and remove tests that expect them to
be
> eagerly invalidated.
Tests are updated. js_dart2js and js_dartium pass them.
Nice, some more comments. This is getting quite a bit cleaner and simpler! https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:106: ret = constructor.apply(instance, #); 'constructor' isn't in scope here. I think you need to pass constr in here again. Can you add a test for this path? https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:130: operator[](key) => _convertToDart(JS('=Object', '#[#]', _convertToJS(this), key)); Nit: line length > 80 https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:191: } Nit: space between functions. https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:198: return new JsFunction._fromJs(JS('Function', '#', o)); I think this should be JS('=Object', ...) as well. It's not a Dart Function. https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dartium... File sdk/lib/js/dartium/js_dartium.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dartium... sdk/lib/js/dartium/js_dartium.dart:79: // this bootstrap string. I just landed my CL with interop.js. Shall we move this bootstrap code there as well and remove from here? https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dartium... sdk/lib/js/dartium/js_dartium.dart:557: void _inject(code) { We can also eliminate this. https://chromiumcodereview.appspot.com/15782009/diff/29001/tests/html/js_test... File tests/html/js_tests.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/29001/tests/html/js_test... tests/html/js_tests.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file 2013 :-) https://chromiumcodereview.appspot.com/15782009/diff/29001/tests/html/js_test... tests/html/js_tests.dart:5: library js_tests; Can you rename this file to js_test.dart (removing the 's')? That should trigger the automated tests to pick it up. You can run: ./tools/test.py -m release -c dart2js -r drt html/js_test to verify before and after. https://chromiumcodereview.appspot.com/15782009/diff/29001/tests/html/js_test... tests/html/js_tests.dart:17: Foo(num a) : this._proxy = new JsObject(context['Foo'], [a]); The automated test framework needs the test self-contained. Can we move the JS definition of Foo, etc., here? See js_interop_2_test.dart for an example.
The comments have been addressed. What is the way to test "js_test" against dartium ? I couldn't find. Should I inject a <script src="..../interop.js"> in "js_test" to make it testable in dartium ? https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:106: ret = constructor.apply(instance, #); On 2013/06/11 16:17:44, vsm wrote: > 'constructor' isn't in scope here. I think you need to pass constr in here > again. > > Can you add a test for this path? Done. https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:130: operator[](key) => _convertToDart(JS('=Object', '#[#]', _convertToJS(this), key)); On 2013/06/11 16:17:44, vsm wrote: > Nit: line length > 80 Done. https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:191: } On 2013/06/11 16:17:44, vsm wrote: > Nit: space between functions. Done. https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:198: return new JsFunction._fromJs(JS('Function', '#', o)); On 2013/06/11 16:17:44, vsm wrote: > I think this should be JS('=Object', ...) as well. It's not a Dart Function. Done. https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dartium... File sdk/lib/js/dartium/js_dartium.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dartium... sdk/lib/js/dartium/js_dartium.dart:79: // this bootstrap string. On 2013/06/11 16:17:44, vsm wrote: > I just landed my CL with interop.js. Shall we move this bootstrap code there as > well and remove from here? Done. https://chromiumcodereview.appspot.com/15782009/diff/29001/sdk/lib/js/dartium... sdk/lib/js/dartium/js_dartium.dart:557: void _inject(code) { On 2013/06/11 16:17:44, vsm wrote: > We can also eliminate this. Done. https://chromiumcodereview.appspot.com/15782009/diff/29001/tests/html/js_test... File tests/html/js_tests.dart (right): https://chromiumcodereview.appspot.com/15782009/diff/29001/tests/html/js_test... tests/html/js_tests.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file On 2013/06/11 16:17:44, vsm wrote: > 2013 :-) Done. https://chromiumcodereview.appspot.com/15782009/diff/29001/tests/html/js_test... tests/html/js_tests.dart:5: library js_tests; On 2013/06/11 16:17:44, vsm wrote: > Can you rename this file to js_test.dart (removing the 's')? That should > trigger the automated tests to pick it up. > > You can run: > > ./tools/test.py -m release -c dart2js -r drt html/js_test > > to verify before and after. Done. https://chromiumcodereview.appspot.com/15782009/diff/29001/tests/html/js_test... tests/html/js_tests.dart:17: Foo(num a) : this._proxy = new JsObject(context['Foo'], [a]); On 2013/06/11 16:17:44, vsm wrote: > The automated test framework needs the test self-contained. Can we move the JS > definition of Foo, etc., here? > > See js_interop_2_test.dart for an example. Done.
The patch 7 removes the scope handlings.
https://codereview.chromium.org/15782009/diff/45001/pkg/browser/lib/interop.js File pkg/browser/lib/interop.js (right): https://codereview.chromium.org/15782009/diff/45001/pkg/browser/lib/interop.j... pkg/browser/lib/interop.js:315: return [ 'return', serialize(receiver.apply(args[0], args.slice(1))) ]; nit: line length https://codereview.chromium.org/15782009/diff/45001/sdk/lib/js/dart2js/js_dar... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/15782009/diff/45001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:166: class _DartProxy { This might need to be defined in JS, as a JS type. https://codereview.chromium.org/15782009/diff/45001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:183: return new _DartProxy(o); It feels dangerous to pass out a direct pointer to a Dart instance here. https://codereview.chromium.org/15782009/diff/45001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:187: dynamic _convertToDart(dynamic o) { I think we need to always access "o" via JS notation to avoid confusing the compiler. E.g., based on the calls to this method, it looks like it only ever gets passed an "Object". Given that, dart2js might incorrectly tree-shake the Dart type tests below. Using JS - as you are doing for function - should be safe.
https://codereview.chromium.org/15782009/diff/45001/pkg/browser/lib/interop.js File pkg/browser/lib/interop.js (right): https://codereview.chromium.org/15782009/diff/45001/pkg/browser/lib/interop.j... pkg/browser/lib/interop.js:315: return [ 'return', serialize(receiver.apply(args[0], args.slice(1))) ]; On 2013/06/20 18:43:58, vsm wrote: > nit: line length Done. https://codereview.chromium.org/15782009/diff/45001/sdk/lib/js/dart2js/js_dar... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/15782009/diff/45001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:166: class _DartProxy { On 2013/06/20 18:43:58, vsm wrote: > This might need to be defined in JS, as a JS type. Done. I have added a DartProxy javascript object. https://codereview.chromium.org/15782009/diff/45001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:183: return new _DartProxy(o); On 2013/06/20 18:43:58, vsm wrote: > It feels dangerous to pass out a direct pointer to a Dart instance here. Done. https://codereview.chromium.org/15782009/diff/45001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:187: dynamic _convertToDart(dynamic o) { On 2013/06/20 18:43:58, vsm wrote: > I think we need to always access "o" via JS notation to avoid confusing the > compiler. E.g., based on the calls to this method, it looks like it only ever > gets passed an "Object". Given that, dart2js might incorrectly tree-shake the > Dart type tests below. Using JS - as you are doing for function - should be > safe. Done.
lgtm!
Last update (I hope) on this CL to fix https://github.com/dart-lang/js-interop/pull/93 . Basically : - in dart2js : hashCode returns always 0 and operator==(other) compares the js objects with === - in dartium : I delegate hashCode and operator==(other) to _id. To make that work, I return the same _id for the same js object (a side effect is that the current leak will be smaller)
https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:126: int get hashCode => 0; Would it make sense to invoke toString on the underlying JS object and hash that? If not, can you add a TODO here - I think we'll want to figure out a proper hash. https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... sdk/lib/js/dartium/js_dartium.dart:165: int get hashCode => _id.hashCode; Will this work? Do we really cache and reuse ids or just generate new ones? E.g., can this fail now? context.foo.bar == context.foo.bar https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... sdk/lib/js/dartium/js_dartium.dart:274: // TODO(vsm): Cache x and reuse id. E.g., We're possibly generating multiple ids for the same object here.
https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:126: int get hashCode => 0; On 2013/07/11 19:49:26, vsm wrote: > Would it make sense to invoke toString on the underlying JS object and hash > that? > > If not, can you add a TODO here - I think we'll want to figure out a proper > hash. I don't think this makes sense to call toString. For example if JsObject wrap an Array. The result of toString will change if the content of the Array changes. So it could lead to issues. hashCode=0 is not very optimized but it works. A solution could be to define a "__dart_id" on every proxify object and use the hash of this id. Something like this : int _dart_id = 0; // top level JsObject._fromJs(this._jsObject) { if (JS('bool', '!("__dart_id" in #)', _convertToJS(this))) { JS('void', 'Object.defineProperty(#, "__dart_id", {value: #});', _convertToJS(this), _dart_id++); } } int get hashCode => JS('int', '#["__dart_id"]', _convertToJS(this)).hashCode; operator==(other) => other is JsObject && JS('bool', '# === #', _convertToJS(this), _convertToJS(other)); I have a CL ready with that change. What do you think ? https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... sdk/lib/js/dartium/js_dartium.dart:165: int get hashCode => _id.hashCode; On 2013/07/11 19:49:26, vsm wrote: > Will this work? Do we really cache and reuse ids or just generate new ones? > > E.g., can this fail now? > > context.foo.bar == context.foo.bar Yes, it works. I have modified interop.js to cache _ids. Thus _id will always be the same for each context['Object'] call. I also added a test https://codereview.chromium.org/15782009/diff2/57001:61001/tests/html/js_test... https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... sdk/lib/js/dartium/js_dartium.dart:274: // TODO(vsm): Cache x and reuse id. On 2013/07/11 19:49:26, vsm wrote: > E.g., We're possibly generating multiple ids for the same object here. In this CL, this "id" doesn't need to be unique because it's only used to deserialize local objects.
On 2013/07/12 19:03:41, alexandre.ardhuin wrote: > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... > File sdk/lib/js/dart2js/js_dart2js.dart (right): > > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... > sdk/lib/js/dart2js/js_dart2js.dart:126: int get hashCode => 0; > On 2013/07/11 19:49:26, vsm wrote: > > Would it make sense to invoke toString on the underlying JS object and hash > > that? > > > > If not, can you add a TODO here - I think we'll want to figure out a proper > > hash. > > I don't think this makes sense to call toString. > For example if JsObject wrap an Array. The result of toString will change if the > content of the Array changes. So it could lead to issues. > > hashCode=0 is not very optimized but it works. > > A solution could be to define a "__dart_id" on every proxify object and use the > hash of this id. Something like this : > > int _dart_id = 0; // top level > > JsObject._fromJs(this._jsObject) { > if (JS('bool', '!("__dart_id" in #)', _convertToJS(this))) { > JS('void', 'Object.defineProperty(#, "__dart_id", {value: #});', > _convertToJS(this), _dart_id++); > } > } > > int get hashCode => JS('int', '#["__dart_id"]', _convertToJS(this)).hashCode; > > operator==(other) => other is JsObject && > JS('bool', '# === #', _convertToJS(this), _convertToJS(other)); > > I have a CL ready with that change. > > What do you think ? > > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... > File sdk/lib/js/dartium/js_dartium.dart (right): > > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... > sdk/lib/js/dartium/js_dartium.dart:165: int get hashCode => _id.hashCode; > On 2013/07/11 19:49:26, vsm wrote: > > Will this work? Do we really cache and reuse ids or just generate new ones? > > > > E.g., can this fail now? > > > > context.foo.bar == context.foo.bar > > Yes, it works. I have modified interop.js to cache _ids. Thus _id will always be > the same for each context['Object'] call. I also added a test > https://codereview.chromium.org/15782009/diff2/57001:61001/tests/html/js_test... > > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... > sdk/lib/js/dartium/js_dartium.dart:274: // TODO(vsm): Cache x and reuse id. > On 2013/07/11 19:49:26, vsm wrote: > > E.g., We're possibly generating multiple ids for the same object here. > > In this CL, this "id" doesn't need to be unique because it's only used to > deserialize local objects. LGTM. Let's hold off on the __dart_id. That might be the right approach, but a couple things to consider: 1. What if there is more than one dart isolate? 2. Not all JS objects are amenable to expandos.
Still LGTM with minor fixes https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:193: return JS('Object', '#.o', o); sra pointed me to the docs here: https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/sdk/... We should replace 'Object' with 'var' in the line above. This should reduce code size safely: return JS('var', '#.o', o); https://codereview.chromium.org/15782009/diff/61001/tests/html/js_test.dart File tests/html/js_test.dart (right): https://codereview.chromium.org/15782009/diff/61001/tests/html/js_test.dart#n... tests/html/js_test.dart:171: expect(o1.hashCode == d.hashCode, isFalse); Note: this is breaking with your latest. It's not really valid to assert that hashCode must be different.
On 2013/07/15 13:25:53, vsm wrote: > On 2013/07/12 19:03:41, alexandre.ardhuin wrote: > > > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... > > File sdk/lib/js/dart2js/js_dart2js.dart (right): > > > > > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... > > sdk/lib/js/dart2js/js_dart2js.dart:126: int get hashCode => 0; > > On 2013/07/11 19:49:26, vsm wrote: > > > Would it make sense to invoke toString on the underlying JS object and hash > > > that? > > > > > > If not, can you add a TODO here - I think we'll want to figure out a proper > > > hash. > > > > I don't think this makes sense to call toString. > > For example if JsObject wrap an Array. The result of toString will change if > the > > content of the Array changes. So it could lead to issues. > > > > hashCode=0 is not very optimized but it works. > > > > A solution could be to define a "__dart_id" on every proxify object and use > the > > hash of this id. Something like this : > > > > int _dart_id = 0; // top level > > > > JsObject._fromJs(this._jsObject) { > > if (JS('bool', '!("__dart_id" in #)', _convertToJS(this))) { > > JS('void', 'Object.defineProperty(#, "__dart_id", {value: #});', > > _convertToJS(this), _dart_id++); > > } > > } > > > > int get hashCode => JS('int', '#["__dart_id"]', > _convertToJS(this)).hashCode; > > > > operator==(other) => other is JsObject && > > JS('bool', '# === #', _convertToJS(this), _convertToJS(other)); > > > > I have a CL ready with that change. > > > > What do you think ? > > > > > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... > > File sdk/lib/js/dartium/js_dartium.dart (right): > > > > > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... > > sdk/lib/js/dartium/js_dartium.dart:165: int get hashCode => _id.hashCode; > > On 2013/07/11 19:49:26, vsm wrote: > > > Will this work? Do we really cache and reuse ids or just generate new ones? > > > > > > E.g., can this fail now? > > > > > > context.foo.bar == context.foo.bar > > > > Yes, it works. I have modified interop.js to cache _ids. Thus _id will always > be > > the same for each context['Object'] call. I also added a test > > > https://codereview.chromium.org/15782009/diff2/57001:61001/tests/html/js_test... > > > > > https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dartium/js_dar... > > sdk/lib/js/dartium/js_dartium.dart:274: // TODO(vsm): Cache x and reuse id. > > On 2013/07/11 19:49:26, vsm wrote: > > > E.g., We're possibly generating multiple ids for the same object here. > > > > In this CL, this "id" doesn't need to be unique because it's only used to > > deserialize local objects. > > LGTM. > > Let's hold off on the __dart_id. That might be the right approach, but a couple > things to consider: > 1. What if there is more than one dart isolate? Right ! I haven't considered this issue. It could make sense to use a key by isolate "__dart_id_${isolateID}" but I don't know if such an ID is available. > 2. Not all JS objects are amenable to expandos. I wasn't aware of this kind of objects. Have you an example in mind ?
https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/15782009/diff/61001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:193: return JS('Object', '#.o', o); On 2013/07/15 13:58:45, vsm wrote: > sra pointed me to the docs here: > https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/sdk/... > > We should replace 'Object' with 'var' in the line above. This should reduce > code size safely: > return JS('var', '#.o', o); Done. https://codereview.chromium.org/15782009/diff/61001/tests/html/js_test.dart File tests/html/js_test.dart (right): https://codereview.chromium.org/15782009/diff/61001/tests/html/js_test.dart#n... tests/html/js_test.dart:171: expect(o1.hashCode == d.hashCode, isFalse); On 2013/07/15 13:58:45, vsm wrote: > Note: this is breaking with your latest. It's not really valid to assert that > hashCode must be different. Good catch ! Removed.
Message was sent while issue was closed.
Committed patchset #10 manually as r25018 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
