bindings: Make the sequence conversion code more complaint with WebIDL.

third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h

 // Copyright (c) 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NativeValueTraitsImpl_h
 #define NativeValueTraitsImpl_h
 
 #include "bindings/core/v8/IDLTypes.h"
 #include "bindings/core/v8/NativeValueTraits.h"
 #include "bindings/core/v8/V8Binding.h"
@@ skipping 265 matching lines @@
   }
 };
 
 // Sequences
 template <typename T>
 struct NativeValueTraits<IDLSequence<T>>
     : public NativeValueTraitsBase<IDLSequence<T>> {
   // Nondependent types need to be explicitly qualified to be accessible.
   using typename NativeValueTraitsBase<IDLSequence<T>>::ImplType;
 
+  // https://heycam.github.io/webidl/#es-sequence
   static ImplType NativeValue(v8::Isolate* isolate,
                               v8::Local<v8::Value> value,
                               ExceptionState& exception_state) {
-    return NativeValue(isolate, value, exception_state, 0);
+    if (!value->IsObject()) {
+      exception_state.ThrowTypeError(
+          "The provided value cannot be converted to a sequence.");
+      return ImplType();
+    }
+
+    ImplType result;
+    // TODO(rakuco): Checking for IsArray() may not be enough. Other engines
+    // also prefer regular array iteration over a custom @@iterator when the
+    // latter is defined, but it is not clear if this is a valid optimization.
+    if (value->IsArray()) {

[bashi, 2017/04/11 00:34:00]

We may want to ask v8 team this?

[Yuki, 2017/04/11 07:17:10]

I think that this should be:

  if (value->Get(v8::Symbol::GetIterator) == v8::kArrayProto_values)

because I guess that author script can do the following:

  a = new Array(1);
  a[Symbol.iterator] = function myIter() { ... }

Then, we'd like to fall back to iteratorForEachSlow path.

+      result.ReserveInitialCapacity(value.As<v8::Array>()->Length());
+      iteratorForEachFast(
+          isolate, value.As<v8::Object>(), exception_state,
+          [&isolate, &result, &exception_state](v8::Local<v8::Value> element) {
+            result.UncheckedAppend(NativeValueTraits<T>::NativeValue(
+                isolate, element, exception_state));
+            return !exception_state.HadException();
+          });
+    } else {
+      iteratorForEachSlow(
+          isolate, value.As<v8::Object>(), exception_state,
+          [&isolate, &result, &exception_state](v8::Local<v8::Value> element) {
+            result.emplace_back(NativeValueTraits<T>::NativeValue(
+                isolate, element, exception_state));
+            return !exception_state.HadException();
+          });
+    }
+
+    if (exception_state.HadException())
+      return ImplType();
+    return result;
   }
 
-  static ImplType NativeValue(v8::Isolate* isolate,
-                              v8::Local<v8::Value> value,
-                              ExceptionState& exception_state,
-                              int index) {
-    return ToImplArray<ImplType, T>(value, index, isolate, exception_state);
+ private:
+  // Fast case: we're interating over an Array that adheres to
+  // %ArrayIteratorPrototype%'s protocol.
+  template <typename CallbackFunction>
+  static void iteratorForEachFast(v8::Isolate* isolate,
+                                  v8::Local<v8::Object> v8Object,
+                                  ExceptionState& exception_state,
+                                  CallbackFunction&& callback) {
+    DCHECK(v8Object->IsArray());
+    v8::TryCatch block(isolate);
+    const uint32_t length = v8Object.As<v8::Array>()->Length();
+    for (uint32_t i = 0; i < length; ++i) {
+      v8::Local<v8::Value> element;
+      if (!V8Call(v8Object->Get(isolate->GetCurrentContext(), i), element,

[Yuki, 2017/04/11 07:17:10]

nit: V8Call is deprecated.  Please use V8's ToLocal() or To() instead.

+                  block)) {
+        exception_state.RethrowV8Exception(block.Exception());
+        return;
+      }
+      if (!callback(element))

[bashi, 2017/04/11 00:34:00]

Help me understand: Why does the callback need to return bool? Isn't it enough
to check exception_state has an exception?

+        return;
+    }
+  }
+
+  // Slow case: follow WebIDL's "Creating a sequence from an iterable" steps to
+  // iterate through each element.
+  // https://heycam.github.io/webidl/#create-sequence-from-iterable
+  template <typename CallbackFunction>
+  static void iteratorForEachSlow(v8::Isolate* isolate,
+                                  v8::Local<v8::Object> v8Object,
+                                  ExceptionState& exception_state,
+                                  CallbackFunction&& callback) {
+    v8::TryCatch block(isolate);
+
+    v8::Local<v8::Object> iterator =
+        GetEsIterator(isolate, v8Object, exception_state);
+    if (exception_state.HadException())
+      return;
+
+    v8::Local<v8::String> next_key = V8String(isolate, "next");
+    v8::Local<v8::String> value_key = V8String(isolate, "value");
+    v8::Local<v8::String> done_key = V8String(isolate, "done");
+    v8::Local<v8::Context> context = isolate->GetCurrentContext();
+    while (true) {
+      v8::Local<v8::Value> next;
+      if (!iterator->Get(context, next_key).ToLocal(&next)) {
+        exception_state.RethrowV8Exception(block.Exception());
+        return;
+      }
+      // TODO(bashi): Support callable objects.

[bashi, 2017/04/11 00:34:00]

nit: My vague recollection is that yukishiino@ said that IsFunction() is always
true when it is callable. Maybe we can remove this comment.

[Yuki, 2017/04/11 07:17:10]

Yes, a callable object is a function by definition.

You can simply write:
  if (!next->IsFunction())

+      if (!next->IsObject() || !next.As<v8::Object>()->IsFunction()) {
+        exception_state.ThrowTypeError("Iterator.next should be callable.");
+        return;
+      }
+      v8::Local<v8::Value> nextResult;

[Yuki, 2017/04/11 07:17:10]

nit: s/nextResult/next_result/

Last week end, Blink finally changed the naming convention to Chromium's style. 
(Maybe you already noticed.  Other names look good.)

+      if (!V8ScriptRunner::CallFunction(next.As<v8::Function>(),
+                                        ToExecutionContext(context), iterator,
+                                        0, nullptr, isolate)
+               .ToLocal(&nextResult)) {
+        exception_state.RethrowV8Exception(block.Exception());
+        return;
+      }
+      if (!nextResult->IsObject()) {
+        exception_state.ThrowTypeError(
+            "Iterator.next() did not return an object.");
+        return;
+      }
+      v8::Local<v8::Object> result_object = nextResult.As<v8::Object>();
+      v8::Local<v8::Value> element;
+      v8::Local<v8::Value> done;
+      if (!result_object->Get(context, value_key).ToLocal(&element) ||
+          !result_object->Get(context, done_key).ToLocal(&done)) {
+        exception_state.RethrowV8Exception(block.Exception());
+        return;
+      }
+      v8::Local<v8::Boolean> done_boolean;
+      if (!done->ToBoolean(context).ToLocal(&done_boolean)) {

[Yuki, 2017/04/11 07:17:10]

nit: You can use done->BooleanValue(context).To(&done_boolean).

+        exception_state.RethrowV8Exception(block.Exception());
+        return;
+      }
+      if (done_boolean->Value())
+        break;
+      if (!callback(element))
+        return;
+    }
   }
 };
 
 // Records
 template <typename K, typename V>
 struct NativeValueTraits<IDLRecord<K, V>>
     : public NativeValueTraitsBase<IDLRecord<K, V>> {
   // Nondependent types need to be explicitly qualified to be accessible.
   using typename NativeValueTraitsBase<IDLRecord<K, V>>::ImplType;
 
@@ skipping 113 matching lines @@
       }
     }
     // "5. Return result."
     return result;
   }
 };
 
 }  // namespace blink
 
 #endif  // NativeValueTraitsImpl_h