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

Index: sdk/lib/html/html_common/conversions_dartium.dart
diff --git a/sdk/lib/html/html_common/conversions_dartium.dart b/sdk/lib/html/html_common/conversions_dartium.dart
index 22738eae6fa2a001fc93c350f58c0890ffb0fd5b..2403387b4e608e457e43e88cf080c08d76d2b535 100644
--- a/sdk/lib/html/html_common/conversions_dartium.dart
+++ b/sdk/lib/html/html_common/conversions_dartium.dart
@@ -236,6 +236,13 @@ wrap_jso_no_SerializedScriptvalue(jsObject) {
       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);
@@ -380,7 +387,7 @@ class DartHtmlWrappingList extends ListBase implements NativeFieldWrapperClass2
 
   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;