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

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

Update toNativeArray() / toRefPtrNativeArray() do not match the latest Web IDL
specification:
- http://dev.w3.org/2006/webapi/WebIDL/#es-array
- http://dev.w3.org/2006/webapi/WebIDL/#es-sequence

In particular, we no longer generate an empty Vector if the JavaScript value is not
an array but can still be converted to an IDL sequence. If the JavaScript value is
an object (Other than a Date or a RegExp), we now call [[Get]] on it with a property
name of "length" and call ToUint32() on the result. If this succeeds, we then
retrieve each item by calling [[Get]] with a property name corresponding to the
index.

This essentially means that IDL operations taking an array or a sequence in argument
will now be more permissive and convert non v8::Array by converting them to sequences.

BUG=263758
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154940

[haraken, 2013-07-24 15:59:29]

The code change looks good. We might want to have a thorough test that covers
what type can/cannot converted to what.

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
File LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js
(right):

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:37: var
arrayTypes = ["Array", "Int8Array", "Int16Array", "Int32Array", "Uint8Array",
"Uint16Array", "Uint32Array", "Float32Array", "Float64Array", "Object"];

Shall we also test Uint8ClampedArray?

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding.h
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:415: v8::Local<v8::Value>
v8Value(v8::Local<v8::Value>::New(value));

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.)

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:426: for (uint32_t i = 0; i < length; ++i) {

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

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:447: v8::Local<v8::Value>
v8Value(v8::Local<v8::Value>::New(value));

Please pass an Isolate to Local::New.

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:485: if (!value->IsObject() || value->IsDate() ||
value->IsRegExp()) {

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:12]

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
File LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js
(right):

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:15: array
= new Object();
array = {length: arrayValues.length};

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:18:
eval("array = new " + arrayType + "(" + arrayValues.length + ");");
No need for eval here

array = new window[arrayType](arrayValues.length);

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:37: var
arrayTypes = ["Array", "Int8Array", "Int16Array", "Int32Array", "Uint8Array",
"Uint16Array", "Uint32Array", "Float32Array", "Float64Array", "Object"];
Change this to pass the constructors instead of the strings?

var arrayTypes = [Array, Int8Array, Int16Array, ...]

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:40:
debug("* Test passing a " + arrayTypes[i] + " as input.");
then do arrayType[i].name

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding.h
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:419: } else {
else if

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:420: if (toV8Sequence(value, length,
isolate).IsEmpty())
Are we converting this twice. Once to see if it is empty and once more below?

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:426: for (uint32_t i = 0; i < length; ++i) {
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.

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:485: if (!value->IsObject() || value->IsDate() ||
value->IsRegExp()) {
Why are you special casing Date and RegExp? What about all the other object
type?

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

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
File LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js
(right):

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:15: array
= new Object();
On 2013/07/24 16:38:13, arv wrote:
> array = {length: arrayValues.length};

OK.

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:18:
eval("array = new " + arrayType + "(" + arrayValues.length + ");");
On 2013/07/24 16:38:13, arv wrote:
> No need for eval here
> 
> array = new window[arrayType](arrayValues.length);

Ok.

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:37: var
arrayTypes = ["Array", "Int8Array", "Int16Array", "Int32Array", "Uint8Array",
"Uint16Array", "Uint32Array", "Float32Array", "Float64Array", "Object"];
On 2013/07/24 15:59:30, haraken wrote:
> 
> Shall we also test Uint8ClampedArray?

Good idea.

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:37: var
arrayTypes = ["Array", "Int8Array", "Int16Array", "Int32Array", "Uint8Array",
"Uint16Array", "Uint32Array", "Float32Array", "Float64Array", "Object"];
On 2013/07/24 16:38:13, arv wrote:
> Change this to pass the constructors instead of the strings?
> 
> var arrayTypes = [Array, Int8Array, Int16Array, ...]

OK.

https://codereview.chromium.org/19969004/diff/1/LayoutTests/fast/canvas/scrip...
LayoutTests/fast/canvas/script-tests/canvas-lineDash-input-sequence.js:40:
debug("* Test passing a " + arrayTypes[i] + " as input.");
On 2013/07/24 16:38:13, arv wrote:
> then do arrayType[i].name

OK.

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding.h
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:415: v8::Local<v8::Value>
v8Value(v8::Local<v8::Value>::New(value));
On 2013/07/24 15:59:30, haraken wrote:
> 
> 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.)

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

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:426: for (uint32_t i = 0; i < length; ++i) {
On 2013/07/24 16:38:13, arv wrote:
> 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.

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.

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:426: for (uint32_t i = 0; i < length; ++i) {
On 2013/07/24 15:59:30, haraken wrote:
> 
> I'm just curious about why you choose uint32_t instead of size_t. We normally
> use size_t to iterate an array.

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().

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:447: v8::Local<v8::Value>
v8Value(v8::Local<v8::Value>::New(value));
On 2013/07/24 15:59:30, haraken wrote:
> 
> Please pass an Isolate to Local::New.

Ok.

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:485: if (!value->IsObject() || value->IsDate() ||
value->IsRegExp()) {
On 2013/07/24 15:59:30, haraken wrote:
> 
> 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.

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

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:485: if (!value->IsObject() || value->IsDate() ||
value->IsRegExp()) {
On 2013/07/24 16:38:13, arv wrote:
> Why are you special casing Date and RegExp? What about all the other object
> type?

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.

[do-not-use, 2013-07-25 08:46:13]

Could you please take another look?

[haraken, 2013-07-25 08:50:29]

LGTM

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

LGTM with nits

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding.h
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:485: if (!value->IsObject() || value->IsDate() ||
value->IsRegExp()) {
On 2013/07/25 08:01:26, Christophe Dumez wrote:
> On 2013/07/24 16:38:13, arv wrote:
> > Why are you special casing Date and RegExp? What about all the other object
> > type?
> 
> 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.

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

https://codereview.chromium.org/19969004/diff/9001/LayoutTests/fast/canvas/ca...
File LayoutTests/fast/canvas/canvas-lineDash-input-sequence.html (right):

https://codereview.chromium.org/19969004/diff/9001/LayoutTests/fast/canvas/ca...
LayoutTests/fast/canvas/canvas-lineDash-input-sequence.html:8: <script
src="script-tests/canvas-lineDash-input-sequence.js"></script>
Please do not add more html+js tests. Just put the js in the same file.

[do-not-use, 2013-07-25 18:56:55]

On 2013/07/25 18:24:41, arv wrote:
> LGTM with nits
> 
> https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding.h
> File Source/bindings/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/19969004/diff/1/Source/bindings/v8/V8Binding....
> Source/bindings/v8/V8Binding.h:485: if (!value->IsObject() || value->IsDate()
||
> value->IsRegExp()) {
> On 2013/07/25 08:01:26, Christophe Dumez wrote:
> > On 2013/07/24 16:38:13, arv wrote:
> > > Why are you special casing Date and RegExp? What about all the other
object
> > > type?
> > 
> > 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.
> 
> 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
> 
>
https://codereview.chromium.org/19969004/diff/9001/LayoutTests/fast/canvas/ca...
> File LayoutTests/fast/canvas/canvas-lineDash-input-sequence.html (right):
> 
>
https://codereview.chromium.org/19969004/diff/9001/LayoutTests/fast/canvas/ca...
> LayoutTests/fast/canvas/canvas-lineDash-input-sequence.html:8: <script
> src="script-tests/canvas-lineDash-input-sequence.js"></script>
> Please do not add more html+js tests. Just put the js in the same file.

Done.

[commit-bot: I haz the power, 2013-07-25 19:08:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/19969004/15001

[commit-bot: I haz the power, 2013-07-25 21:23:49]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-07-25 21:55:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/19969004/15001

[commit-bot: I haz the power, 2013-07-25 22:17:09]

Change committed as 154940