Update toNativeArray() / toRefPtrNativeArray() do not match Web IDL specification

Source/bindings/v8/V8Binding.h

 /*
 * Copyright (C) 2009 Google Inc. All rights reserved.
 * Copyright (C) 2012 Ericsson AB. All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions are
 * met:
 *
 *     * Redistributions of source code must retain the above copyright
 * notice, this list of conditions and the following disclaimer.
@@ skipping 57 matching lines @@
 
     // A helper for throwing JavaScript TypeError.
     v8::Handle<v8::Value> throwTypeError(v8::Isolate*);
     v8::Handle<v8::Value> throwTypeError(const String&, v8::Isolate*);
 
     // A helper for throwing JavaScript TypeError for not enough arguments.
     v8::Handle<v8::Value> throwNotEnoughArgumentsError(v8::Isolate*);
 
     v8::ArrayBuffer::Allocator* v8ArrayBufferAllocator();
 
+    v8::Handle<v8::Value> toV8Sequence(v8::Handle<v8::Value>, uint32_t& length, v8::Isolate*);
+
     inline v8::Handle<v8::Value> argumentOrNull(const v8::FunctionCallbackInfo<v8::Value>& args, int index)
     {
         return index >= args.Length() ? v8::Local<v8::Value>() : args[index];
     }
 
     // Since v8::Null(isolate) crashes if we pass a null isolate,
     // we need to use v8NullWithCheck(isolate) if an isolate can be null.
     //
     // FIXME: Remove all null isolates from V8 bindings, and remove v8NullWithCheck(isolate).
     inline v8::Handle<v8::Value> v8NullWithCheck(v8::Isolate* isolate)
@@ skipping 307 matching lines @@
     };
 
     template<>
     struct NativeValueTraits<double> {
         static inline double nativeValue(const v8::Handle<v8::Value>& value)
         {
             return static_cast<double>(value->NumberValue());
         }
     };
 
+    // Converts a JavaScript value to an array as per the Web IDL specification:
+    // http://www.w3.org/TR/2012/CR-WebIDL-20120419/#es-array
     template <class T, class V8T>
     Vector<RefPtr<T> > toRefPtrNativeArray(v8::Handle<v8::Value> value, v8::Isolate* isolate, bool* success = 0)
     {
         if (success)
             *success = true;
 
-        if (!value->IsArray())
-            return Vector<RefPtr<T> >();
+        v8::Local<v8::Value> v8Value(v8::Local<v8::Value>::New(value));

[haraken, 2013/07/24 15:59:30]

Please pass an Isolate to Local::New.

It's sad that we have to allocate a new Local handle here. Currently dcarney@
and marja@ are working on deprecating Handles. In that world, this method will
receive a Local, and we can use the Local without creating a new one.
(Accurately, they are (1) deprecating Handles, and then (2) renaming "Local" to
"Handle". In other words, we will have only Handle and Persistent in the end.)

[do-not-use, 2013/07/25 08:01:26]

Ok, I forgot about this is the preferred way now.

+        uint32_t length = 0;
+        if (value->IsArray()) {
+            length = v8::Local<v8::Array>::Cast(v8Value)->Length();
+        } else {

[arv (Not doing code reviews), 2013/07/24 16:38:13]

else if

+            if (toV8Sequence(value, length, isolate).IsEmpty())

[arv (Not doing code reviews), 2013/07/24 16:38:13]

Are we converting this twice. Once to see if it is empty and once more below?

+                return Vector<RefPtr<T> >();
+        }
 
         Vector<RefPtr<T> > result;
-        v8::Local<v8::Value> v8Value(v8::Local<v8::Value>::New(value));
-        v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(v8Value);
-        size_t length = array->Length();
-        for (size_t i = 0; i < length; ++i) {
-            v8::Handle<v8::Value> element = array->Get(i);
+        v8::Local<v8::Object> object = v8::Local<v8::Object>::Cast(v8Value);
+        for (uint32_t i = 0; i < length; ++i) {

[haraken, 2013/07/24 15:59:30]

I'm just curious about why you choose uint32_t instead of size_t. We normally
use size_t to iterate an array.

[arv (Not doing code reviews), 2013/07/24 16:38:13]

I feel like I'm missing something here. Where is the [[Get]]("length") done? In
the case where this is not an array we should get the length of the object as
far as I understand.

[do-not-use, 2013/07/25 08:01:26]

It is done in the pre-existing toV8Sequence() function that I call in the
beginning on this function. toV8Sequence() is defined in the same file.

[do-not-use, 2013/07/25 08:01:26]

The reason is that:
- v8::Array::Length() returns a uint32_t
- toV8Sequence() also returns a uint32_t because the Web IDL spec says to call
ToUint32(length).
- size_t is potentially larger than uint32_t and I got a compilation error if I
kept length variable as a size_t but passed it as uint32_t& to toV8Sequence().

+            v8::Handle<v8::Value> element = object->Get(i);
 
             if (V8T::HasInstance(element, isolate, worldType(isolate))) {
-                v8::Handle<v8::Object> object = v8::Handle<v8::Object>::Cast(element);
-                result.append(V8T::toNative(object));
+                v8::Handle<v8::Object> elementObject = v8::Handle<v8::Object>::Cast(element);
+                result.append(V8T::toNative(elementObject));
             } else {
                 if (success)
                     *success = false;
                 throwTypeError("Invalid Array element type", isolate);
                 return Vector<RefPtr<T> >();
             }
         }
         return result;
     }
 
+    // Converts a JavaScript value to an array as per the Web IDL specification:
+    // http://www.w3.org/TR/2012/CR-WebIDL-20120419/#es-array
     template <class T>
-    Vector<T> toNativeArray(v8::Handle<v8::Value> value)
+    Vector<T> toNativeArray(v8::Handle<v8::Value> value, v8::Isolate* isolate)
     {
-        if (!value->IsArray())
-            return Vector<T>();
+        v8::Local<v8::Value> v8Value(v8::Local<v8::Value>::New(value));

[haraken, 2013/07/24 15:59:30]

Please pass an Isolate to Local::New.

[do-not-use, 2013/07/25 08:01:26]

Ok.

+        uint32_t length = 0;
+        if (value->IsArray()) {
+            length = v8::Local<v8::Array>::Cast(v8Value)->Length();
+        } else {
+            if (toV8Sequence(value, length, isolate).IsEmpty())
+                return Vector<T>();
+        }
 
         Vector<T> result;
         typedef NativeValueTraits<T> TraitsType;
-        v8::Local<v8::Value> v8Value(v8::Local<v8::Value>::New(value));
-        v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(v8Value);
-        size_t length = array->Length();
-        for (size_t i = 0; i < length; ++i)
-            result.append(TraitsType::nativeValue(array->Get(i)));
+        v8::Local<v8::Object> object = v8::Local<v8::Object>::Cast(v8Value);
+        for (uint32_t i = 0; i < length; ++i)
+            result.append(TraitsType::nativeValue(object->Get(i)));
         return result;
     }
 
     template <class T>
     Vector<T> toNativeArguments(const v8::FunctionCallbackInfo<v8::Value>& args, int startIndex)
     {
         ASSERT(startIndex <= args.Length());
         Vector<T> result;
         typedef NativeValueTraits<T> TraitsType;
         int length = args.Length();
         for (int i = startIndex; i < length; ++i)
             result.append(TraitsType::nativeValue(args[i]));
         return result;
     }
 
     Vector<v8::Handle<v8::Value> > toVectorOfArguments(const v8::FunctionCallbackInfo<v8::Value>& args);
 
     // Validates that the passed object is a sequence type per WebIDL spec
-    // http://www.w3.org/TR/2012/WD-WebIDL-20120207/#es-sequence
+    // http://www.w3.org/TR/2012/CR-WebIDL-20120419/#es-sequence
     inline v8::Handle<v8::Value> toV8Sequence(v8::Handle<v8::Value> value, uint32_t& length, v8::Isolate* isolate)
     {
-        if (!value->IsObject()) {
+        // Attempt converting to a sequence if the value is not already an array but is
+        // any kind of object except for a native Date object or a native RegExp object.
+        ASSERT(!value->IsArray());
+        if (!value->IsObject() || value->IsDate() || value->IsRegExp()) {

[haraken, 2013/07/24 15:59:30]

Would you add a test case about this? We might want to have a thorough test that
covers which type can/cannot converted to what.

[arv (Not doing code reviews), 2013/07/24 16:38:13]

Why are you special casing Date and RegExp? What about all the other object
type?

[do-not-use, 2013/07/25 08:01:26]

Yes, I will work on a test case for these.

[do-not-use, 2013/07/25 08:01:26]

Because the Web IDL specification is special casing those 2 types:
http://dev.w3.org/2006/webapi/WebIDL/#es-sequence

"""
Any kind of object except for a native Date object or a native RegExp object.
"""

I am not sure why the spec is doing these but I thought it was safer the comply
with the spec here.

[arv (Not doing code reviews), 2013/07/25 18:24:41]

Please link to WebIDL bug that might clarify why/if this is needed.

WebIDL bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22806

             throwTypeError(isolate);
             return v8Undefined();
         }
 
         v8::Local<v8::Value> v8Value(v8::Local<v8::Value>::New(value));
         v8::Local<v8::Object> object = v8::Local<v8::Object>::Cast(v8Value);
 
+        // FIXME: The specification states that the length property should be used as fallback, if value
+        // is not a platform object that supports indexed properties. If it supports indexed properties,
+        // length should actually be one greater than value’s maximum indexed property index.
         V8TRYCATCH(v8::Local<v8::Value>, lengthValue, object->Get(v8::String::NewSymbol("length")));
 
         if (lengthValue->IsUndefined() || lengthValue->IsNull()) {
             throwTypeError(isolate);
             return v8Undefined();
         }
 
         V8TRYCATCH(uint32_t, sequenceLength, lengthValue->Int32Value());
         length = sequenceLength;
 
@@ skipping 109 matching lines @@
     inline v8::Local<v8::Function> createClosure(v8::FunctionCallback function, v8::Handle<v8::Value> environment)
     {
         return v8::FunctionTemplate::New(function, environment)->GetFunction();
     }
 
     v8::Local<v8::Value> getHiddenValueFromMainWorldWrapper(v8::Isolate*, ScriptWrappable*, v8::Handle<v8::String> key);
 
 } // namespace WebCore
 
 #endif // V8Binding_h