Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26092003:
Maintain referential integrity of proxy instances in dart:js (Closed)
|
Created:
7 years, 2 months ago by justinfagnani Modified:
7 years, 2 months ago CC:
reviews_dartlang.org, Jacob Visibility:
Public. |
DescriptionMaintain referential integrity of proxy instances in dart:js
Rename DartProxy to DartObject in JavaScript to be symmetrical with JsObject
R=ahe@google.com, alexandre.ardhuin@gmail.com
Committed: https://code.google.com/p/dart/source/detail?r=28686
Patch Set 1 : #
Total comments: 9
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : Add proxy stability for Dart->JS #
Total comments: 12
Patch Set 5 : #Patch Set 6 : #
Total comments: 8
Patch Set 7 : #Patch Set 8 : fix for safari and firefox #Patch Set 9 : #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dart2js/js_dart... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dart2js/js_dart... sdk/lib/js/dart2js/js_dart2js.dart:170: const _DART_OBJECT_PROPERTY_NAME = r'_$dart_dartObject'; Peter, how do I generate a unique name per-isolate? I couldn't find the example browsing though the _internal/lib code.
https://codereview.chromium.org/26092003/diff/9001/pkg/browser/lib/interop.js File pkg/browser/lib/interop.js (right): https://codereview.chromium.org/26092003/diff/9001/pkg/browser/lib/interop.js... pkg/browser/lib/interop.js:262: this.map[ref] = obj; Move this line in the above "if" block. https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dart2js/js_dart... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dart2js/js_dart... sdk/lib/js/dart2js/js_dart2js.dart:203: var dartClosure = JS('', '#[#]', o, _DART_CLOSURE_PROPERTY_NAME); Extract a method for this case and default case ? dynamic _bindDartOnJsIfAbsent(dynamic o, String propertyName, JsObject createDart(o)) { var dartProxy = JS('', '#[#]', o, propertyName); if (dartProxy == null) { dartProxy = createDart(o); JS('void', 'Object.defineProperty(#, #, { value: #})', o, propertyName, dartProxy); } return dartProxy; } https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dart2js/js_dart... sdk/lib/js/dart2js/js_dart2js.dart:217: dartProxy = new JsObject._fromJs(JS('=Object', '#', o)); I don't remember why we used `JS('=Object', '#', o)` instead of `o` but having a look now I find that really strange. The tests are passing with `o`. https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dartium/js_dart... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dartium/js_dart... sdk/lib/js/dartium/js_dartium.dart:350: // Remote function. _proxiedObjectTable currently contains only Dart local objects that are proxyfied in JS. If we have to store local and remote references we should perhaps change its name and/or its doc (see line 245).
Thanks! PTAL https://codereview.chromium.org/26092003/diff/9001/pkg/browser/lib/interop.js File pkg/browser/lib/interop.js (right): https://codereview.chromium.org/26092003/diff/9001/pkg/browser/lib/interop.js... pkg/browser/lib/interop.js:262: this.map[ref] = obj; On 2013/10/07 08:56:54, alexandre.ardhuin wrote: > Move this line in the above "if" block. Done. https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dart2js/js_dart... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dart2js/js_dart... sdk/lib/js/dart2js/js_dart2js.dart:203: var dartClosure = JS('', '#[#]', o, _DART_CLOSURE_PROPERTY_NAME); On 2013/10/07 08:56:54, alexandre.ardhuin wrote: > Extract a method for this case and default case ? > > dynamic _bindDartOnJsIfAbsent(dynamic o, String propertyName, > JsObject createDart(o)) { > var dartProxy = JS('', '#[#]', o, propertyName); > if (dartProxy == null) { > dartProxy = createDart(o); > JS('void', 'Object.defineProperty(#, #, { value: #})', o, propertyName, > dartProxy); > } > return dartProxy; > } Good idea. Done. https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dart2js/js_dart... sdk/lib/js/dart2js/js_dart2js.dart:217: dartProxy = new JsObject._fromJs(JS('=Object', '#', o)); On 2013/10/07 08:56:54, alexandre.ardhuin wrote: > I don't remember why we used `JS('=Object', '#', o)` instead of `o` but having a > look now I find that really strange. The tests are passing with `o`. Peter can shed light here, but I've seen similar things in dart2js code as a way to indicate the correct type, which in this case is not a Dart object. I'm about to send out another CL which only changes JS() expressions for Peter to take a closer look at. https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dartium/js_dart... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/26092003/diff/9001/sdk/lib/js/dartium/js_dart... sdk/lib/js/dartium/js_dartium.dart:350: // Remote function. On 2013/10/07 08:56:54, alexandre.ardhuin wrote: > _proxiedObjectTable currently contains only Dart local objects that are > proxyfied in JS. If we have to store local and remote references we should > perhaps change its name and/or its doc (see line 245). I'm ok with the name, since it's either holding proxied dart objects, or proxies to proxied JS objects. Updated docs.
https://codereview.chromium.org/26092003/diff/18001/sdk/lib/js/dart2js/js_dar... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/26092003/diff/18001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:204: (o) => JsFunction._fromJs(o)); "new" is missing. https://codereview.chromium.org/26092003/diff/18001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:209: (o) => JsObject._fromJs(o)); "new" is missing. https://codereview.chromium.org/26092003/diff/18001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:214: var dartProxy = JS('', '#[#]', o, propertyName); Indentation size.
https://codereview.chromium.org/26092003/diff/18001/sdk/lib/js/dart2js/js_dar... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/26092003/diff/18001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:204: (o) => JsFunction._fromJs(o)); On 2013/10/07 19:41:30, alexandre.ardhuin wrote: > "new" is missing. Done. https://codereview.chromium.org/26092003/diff/18001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:209: (o) => JsObject._fromJs(o)); On 2013/10/07 19:41:30, alexandre.ardhuin wrote: > "new" is missing. Done. https://codereview.chromium.org/26092003/diff/18001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:214: var dartProxy = JS('', '#[#]', o, propertyName); On 2013/10/07 19:41:30, alexandre.ardhuin wrote: > Indentation size. Done.
The tests are failling on dartium. We have to keep the same kind of directionnal
binding between js and dart to make the tests pass. BTW I wonder when a native
dart:js dartium implementation will land. Is Jacob waiting for a stabilization
of the dart:js dart2js implementation before ?
It could be worth to have an additionnal test like :
test('identical Dart closures should have identical proxies', () {
var c1 = () => 1;
context['c'] = c1;
var c2 = context['c'];
expect(identical(c1, c2), isTrue);
});
In js_dart2js, what about introduce a "_getJsProxy" similar to "_getDartProxy" ?
Something like this :
dynamic _convertToJS(dynamic o) {
if (o == null) {
return null;
} else if (o is String || o is num || o is bool) {
return o;
} else if (o is JsObject) {
return o._jsObject;
} else if (o is Serializable) {
return _convertToJS(o.toJs());
} else if (o is Function) {
return _getJsProxy(o, (o) => _convertDartFunction(o));
} else {
return _getJsProxy(o, (o) => JS('', 'new DartObject(#)', o));
}
}
// converts a Dart object to a reference to a native JS object
// which might be a DartObject JS->Dart proxy
dynamic _convertToDart(dynamic o) {
if (JS('bool', '# == null', o)) {
return null;
} else if (JS('bool', 'typeof # == "string" || # instanceof String', o, o)
||
JS('bool', 'typeof # == "number" || # instanceof Number', o, o) ||
JS('bool', 'typeof # == "boolean" || # instanceof Boolean', o, o)) {
return o;
} else if (JS('bool', '# instanceof Function', o)) {
return _getDartProxy(o, (o) => new JsFunction._fromJs(o));
} else if (JS('bool', '# instanceof DartObject', o)) {
return JS('var', '#.o', o);
} else {
return _getDartProxy(o, (o) => new JsObject._fromJs(o));
}
}
// property added to a Dart object referencing its JS-side DartObject proxy
const _DART_OBJECT_PROPERTY_NAME = r'_$dart_dartObject';
// property added to a JS object referencing its Dart-side JsObject proxy
const _JS_OBJECT_PROPERTY_NAME = r'_$dart_jsObject';
dynamic _getJsProxy(o, createProxy(o)) {
var jsProxy = JS('', '#[#]', o, _JS_OBJECT_PROPERTY_NAME);
if (jsProxy == null) {
jsProxy = createProxy(JS('=Object', '#', o));
_bindJsAndDart(jsProxy, o);
}
return jsProxy;
}
dynamic _getDartProxy(o, createProxy(o)) {
var dartProxy = JS('', '#[#]', o, _DART_OBJECT_PROPERTY_NAME);
if (dartProxy == null) {
dartProxy = createProxy(JS('=Object', '#', o));
_bindJsAndDart(o, dartProxy);
}
return dartProxy;
}
void _bindJsAndDart(jsObject, dartObject) {
JS('void', 'Object.defineProperty(#, #, { value: #})', dartObject,
_JS_OBJECT_PROPERTY_NAME, jsObject);
JS('void', 'Object.defineProperty(#, #, { value: #})', jsObject,
_DART_OBJECT_PROPERTY_NAME, dartObject);
}
With this code the tests (with the above additionnal one) are passing in
dart2js.
https://codereview.chromium.org/26092003/diff/27001/sdk/lib/js/dart2js/js_dar...
File sdk/lib/js/dart2js/js_dart2js.dart (right):
https://codereview.chromium.org/26092003/diff/27001/sdk/lib/js/dart2js/js_dar...
sdk/lib/js/dart2js/js_dart2js.dart:58: '}(#, #, #)',
DART_CLOSURE_TO_JS(_callDartFunction), f, _captureThis);
"captureThis" instead of "_captureThis"
https://codereview.chromium.org/26092003/diff/27001/sdk/lib/js/dart2js/js_dar...
sdk/lib/js/dart2js/js_dart2js.dart:215: var jsFunction = JS('', '#[#]', o,
_JS_FUNCTION_PROPERTY_NAME);
1. Use _getDartProxy ?
2. _DART_CLOSURE_PROPERTY_NAME instead of _JS_FUNCTION_PROPERTY_NAME.
https://codereview.chromium.org/26092003/diff/27001/sdk/lib/js/dart2js/js_dar...
sdk/lib/js/dart2js/js_dart2js.dart:219: JS('void', 'Object.defineProperty(#, #,
{ value: #})', jsFunction,
_JS_FUNCTION_PROPERTY_NAME instead of _DART_CLOSURE_PROPERTY_NAME
https://codereview.chromium.org/26092003/diff/27001/sdk/lib/js/dart2js/js_dar...
sdk/lib/js/dart2js/js_dart2js.dart:222: JS('', '#[#] = #', o,
_JS_FUNCTION_PROPERTY_NAME, jsFunction);
1. Use Object.defineProperty.
2. _DART_CLOSURE_PROPERTY_NAME instead of _JS_FUNCTION_PROPERTY_NAME.
https://codereview.chromium.org/26092003/diff/27001/sdk/lib/js/dart2js/js_dar...
sdk/lib/js/dart2js/js_dart2js.dart:226: var jsProxy = JS('', '#[#]', o,
_DART_OBJECT_PROPERTY_NAME);
Use _getDartProxy ?
https://codereview.chromium.org/26092003/diff/27001/sdk/lib/js/dart2js/js_dar...
sdk/lib/js/dart2js/js_dart2js.dart:229: JS('', '#[#] = #', o,
_DART_OBJECT_PROPERTY_NAME, jsProxy);
Use Object.defineProperty.
Thanks! I fixed proxy identity in Dartium in patch set 5. I'll address the other comments next.
Addressed Alexandre's comments https://chromiumcodereview.appspot.com/26092003/diff/27001/sdk/lib/js/dart2js... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://chromiumcodereview.appspot.com/26092003/diff/27001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:58: '}(#, #, #)', DART_CLOSURE_TO_JS(_callDartFunction), f, _captureThis); On 2013/10/08 08:36:44, alexandre.ardhuin wrote: > "captureThis" instead of "_captureThis" Done. https://chromiumcodereview.appspot.com/26092003/diff/27001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:215: var jsFunction = JS('', '#[#]', o, _JS_FUNCTION_PROPERTY_NAME); On 2013/10/08 08:36:44, alexandre.ardhuin wrote: > 1. Use _getDartProxy ? > 2. _DART_CLOSURE_PROPERTY_NAME instead of _JS_FUNCTION_PROPERTY_NAME. _getDartProxy could maybe work if we used the same property names for referencing a dart proxy from a JS object as for referencing a dart object from a JS proxy. I like the separation right now. Also, this block is setting both the JS->dart reference and the dart->JS reference. For objects we don't set the dart->JS reference because DartObject already has it. _JS_FUNCTION_PROPERTY_NAME is used because it's referencing the JS function proxy from the dart closure https://chromiumcodereview.appspot.com/26092003/diff/27001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:219: JS('void', 'Object.defineProperty(#, #, { value: #})', jsFunction, On 2013/10/08 08:36:44, alexandre.ardhuin wrote: > _JS_FUNCTION_PROPERTY_NAME instead of _DART_CLOSURE_PROPERTY_NAME same response/reasoning as above https://chromiumcodereview.appspot.com/26092003/diff/27001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:222: JS('', '#[#] = #', o, _JS_FUNCTION_PROPERTY_NAME, jsFunction); On 2013/10/08 08:36:44, alexandre.ardhuin wrote: > 1. Use Object.defineProperty. > 2. _DART_CLOSURE_PROPERTY_NAME instead of _JS_FUNCTION_PROPERTY_NAME. Done for defineProperty https://chromiumcodereview.appspot.com/26092003/diff/27001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:226: var jsProxy = JS('', '#[#]', o, _DART_OBJECT_PROPERTY_NAME); On 2013/10/08 08:36:44, alexandre.ardhuin wrote: > Use _getDartProxy ? used _getJsProxy https://chromiumcodereview.appspot.com/26092003/diff/27001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:229: JS('', '#[#] = #', o, _DART_OBJECT_PROPERTY_NAME, jsProxy); On 2013/10/08 08:36:44, alexandre.ardhuin wrote: > Use Object.defineProperty. Done.
dart2js changes LGTM https://codereview.chromium.org/26092003/diff/40001/pkg/browser/lib/interop.js File pkg/browser/lib/interop.js (right): https://codereview.chromium.org/26092003/diff/40001/pkg/browser/lib/interop.j... pkg/browser/lib/interop.js:265: Object.defineProperty(obj, _dartRefPropertyName, { value: ref }); Why are you using Object.defineProperty? https://codereview.chromium.org/26092003/diff/40001/sdk/lib/js/dart2js/js_dar... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://codereview.chromium.org/26092003/diff/40001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:65: var dartArgs = arguments.map(_convertToDart).toList(growable: false); Setting growable to false doesn't win you anything, and it is infinitesimally slower. https://codereview.chromium.org/26092003/diff/40001/sdk/lib/js/dart2js/js_dar... sdk/lib/js/dart2js/js_dart2js.dart:266: dartProxy = createProxy(JS('=Object', '#', o)); Is JS('=Object', '#', o) necessary?
lgtm https://codereview.chromium.org/26092003/diff/40001/pkg/browser/lib/interop.js File pkg/browser/lib/interop.js (right): https://codereview.chromium.org/26092003/diff/40001/pkg/browser/lib/interop.j... pkg/browser/lib/interop.js:265: Object.defineProperty(obj, _dartRefPropertyName, { value: ref }); On 2013/10/11 12:54:40, ahe wrote: > Why are you using Object.defineProperty? To avoid that this property shows up during property enumeration on obj. https://codereview.chromium.org/26092003/diff/40001/tests/html/js_test.dart File tests/html/js_test.dart (right): https://codereview.chromium.org/26092003/diff/40001/tests/html/js_test.dart#n... tests/html/js_test.dart:106: console.log('result = ' + result); To remove ?
https://chromiumcodereview.appspot.com/26092003/diff/40001/sdk/lib/js/dart2js... File sdk/lib/js/dart2js/js_dart2js.dart (right): https://chromiumcodereview.appspot.com/26092003/diff/40001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:65: var dartArgs = arguments.map(_convertToDart).toList(growable: false); On 2013/10/11 12:54:40, ahe wrote: > Setting growable to false doesn't win you anything, and it is infinitesimally > slower. Done. https://chromiumcodereview.appspot.com/26092003/diff/40001/sdk/lib/js/dart2js... sdk/lib/js/dart2js/js_dart2js.dart:266: dartProxy = createProxy(JS('=Object', '#', o)); On 2013/10/11 12:54:40, ahe wrote: > Is JS('=Object', '#', o) necessary? This existed before, but if you don't think it is, then it's not https://chromiumcodereview.appspot.com/26092003/diff/40001/tests/html/js_test... File tests/html/js_test.dart (right): https://chromiumcodereview.appspot.com/26092003/diff/40001/tests/html/js_test... tests/html/js_test.dart:106: console.log('result = ' + result); On 2013/10/11 13:52:53, alexandre.ardhuin wrote: > To remove ? Done.
I had to add a workaround for objects that aren't extensible, like some native objects and sealed objects. These won't have the same proxy for the same instance. That situation will improve when we start transferring some native types. We really want WeakMaps... See the diff in js_dart2js.dart between patch set 8 and 7
On 2013/10/12 03:53:09, justinfagnani wrote: > I had to add a workaround for objects that aren't extensible, like some native > objects and sealed objects. These won't have the same proxy for the same > instance. That situation will improve when we start transferring some native > types. We really want WeakMaps... > > See the diff in js_dart2js.dart between patch set 8 and 7 Shouldn't you do the same handling of unextensible objects in interop.js ?
On 2013/10/12 11:47:53, alexandre.ardhuin wrote: > On 2013/10/12 03:53:09, justinfagnani wrote: > > I had to add a workaround for objects that aren't extensible, like some native > > objects and sealed objects. These won't have the same proxy for the same > > instance. That situation will improve when we start transferring some native > > types. We really want WeakMaps... > > > > See the diff in js_dart2js.dart between patch set 8 and 7 > > Shouldn't you do the same handling of unextensible objects in interop.js ? Yep, though Chrome behaves differently so it didn't trigger in the tests.
Message was sent while issue was closed.
Committed patchset #9 manually as r28686 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
