Should we really be calling the serialized script value logic when getting elements out of Lists re…

sdk/lib/html/html_common/conversions_dartium.dart

 part of html_common;
 
 convertDartToNative_PrepareForStructuredClone(value) =>
     new _StructuredCloneDartium().convertDartToNative_PrepareForStructuredClone(value);
 
 convertNativeToDart_AcceptStructuredClone(object, {mustCopy: false}) =>
     new _AcceptStructuredCloneDartium().convertNativeToDart_AcceptStructuredClone(object, mustCopy: mustCopy);
 
 class _StructuredCloneDartium extends _StructuredClone {
   newJsMap() => new js.JsObject(js.context["Object"]);
@@ skipping 218 matching lines @@
     }
 
     // TODO(alanknight): With upgraded custom elements this causes a failure because
     // we need a new wrapper after the type changes. We could possibly invalidate this
     // if the constructor name didn't match?
     var wrapper = js.getDartHtmlWrapperFor(jsObject);
     if (wrapper != null) {
       return wrapper;
     }
 
+    // TODO(jacobr): auomatically wrapping JsArray here is fundamentally broken
+    // as it hijacks adding custom methods on JS Array classes as part of the
+    // new typed DartJsInterop.
+    // To make this work we really need to make DartHtmlWrappingList extend
+    // JsArrayImpl. Fixing this issue needs to be part of a broader refactor
+    // that allows calling custom typed JS interop methods on all dart:html
+    // classes.

[Alan Knight, 2015/11/09 20:46:07]

I think the largest issue here is that we call wrap_jso without any information
about context. So all we know is that we have some object, not where it's coming
from or what the expected conversion is. Dart2js has better support for this,
and I added some of their code for dealing with input parameters when calling
JS, where this happens more often. So far we haven't had a pressing need to do
it for return values/callback parameters, but it's definitely something we ought
to do.

     if (jsObject is js.JsArray) {
       var wrappingList = new DartHtmlWrappingList(jsObject);
       js.setDartHtmlWrapperFor(jsObject, wrappingList);
       return wrappingList;
     }
 
     var constructor = js.JsNative.getProperty(jsObject, 'constructor');
     if (constructor == null) {
       // Perfectly valid case for JavaScript objects where __proto__ has
       // intentionally been set to null.
@@ skipping 124 matching lines @@
 }
 
 /**
  * Wraps a JsArray and will call wrap_jso on its entries.
  */
 class DartHtmlWrappingList extends ListBase implements NativeFieldWrapperClass2 {
   DartHtmlWrappingList(this.blink_jsObject);
 
   final js.JsArray blink_jsObject;
 
-  operator [](int index) => wrap_jso(js.JsNative.getArrayIndex(blink_jsObject, index));
+  operator [](int index) => wrap_jso_no_SerializedScriptvalue(js.JsNative.getArrayIndex(blink_jsObject, index));

[Alan Knight, 2015/11/09 20:46:07]

I *think* this might be ok, but should look at the uses more carefully. But
basically if we did a wrap_jso to get here, then if we ought to be treating
something as a serialized script value it would be the whole list, not each item
individually. And since structured clone is trying to preserve identity,
re-cloning each list element might give incorrect answers.

And trying to do structured clone on every value probably doesn't help our perf.

 
   operator []=(int index, value) => blink_jsObject[index] = value;
 
   int get length => blink_jsObject.length;
   int set length(int newLength) => blink_jsObject.length = newLength;
 }