Bug 80696 – Remove custom bindings for attribute type Array.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Arko Saha]

Whenever we need to do something with Array, we have to write custom bindings. Fix the code generator so that it can handle these Array cases. This issue is came up with bug https://bugs.webkit.org/show_bug.cgi?id=80269.

[Vineet Chaudhary (vineetc)]

Created attachment 131028 [details]
proposed patch

Uploading primary patch to remove the Custom bindings for Array.
To start with I have tried to remove the custom bindings from Console.idl only. There are few other .idl like ScriptProfileNode, Internals, MessageEvent, Clipboard..
I will update patch if this Approach looks oke.

Please let me know your initial review comments on this.

[WebKit Review Bot]

Comment on attachment 131028 [details]
proposed patch

Attachment 131028 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11894590

[Gustavo Noronha (kov)]

Comment on attachment 131028 [details]
proposed patch

Attachment 131028 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11898567

[Adam Barth]

Comment on attachment 131028 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131028&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1774
> +                                push(@implContent, "    const Vector<RefPtr<ScriptProfile> > vector = impl->$implGetterFunctionName();\n\n");

Why ScriptProfile?  Shouldn't it depend on the type?  This doesn't seem right.

Also, is the name "Array" used in the specs?  I thought WebIDL used syntax like DOMString[] that give the type of the array.

> Source/WebCore/page/Console.idl:49
> -        readonly attribute [CustomGetter] Array profiles;
> +        readonly attribute Array profiles;

This change affects JSC and V8 but you've only change the CodeGeneratorJS and not CodeGeneratorV8.

[Vineet Chaudhary (vineetc)]

(In reply to comment #4)
Thanks Adam Barh for comments.

> Why ScriptProfile?  Shouldn't it depend on the type?  This doesn't seem right.

Can I add ArrayType to get the return types in codegenerator after that for Console.idl it would look like,
     readonly attribute [ArrayType=ScriptProfile] Array profiles;

> Also, is the name "Array" used in the specs?  I thought WebIDL used syntax like DOMString[] that give the type of the array.

According to spec reference http://www.w3.org/TR/WebIDL/#es-array recently syntax for type Array is changed from DOMString[] to Array, So can use name Array.
 
> > Source/WebCore/page/Console.idl:49
> > -        readonly attribute [CustomGetter] Array profiles;
> > +        readonly attribute Array profiles;
> 
> This change affects JSC and V8 but you've only change the CodeGeneratorJS and not CodeGeneratorV8.

Yes I need to modify for V8 also need to check for gobject too.
I will updated these once basic implementation for JS works.

[Adam Barth]

> According to spec reference http://www.w3.org/TR/WebIDL/#es-array recently syntax for type Array is changed from DOMString[] to Array, So can use name Array.

Where do you see that?  The link you provided show this as an example:

  readonly attribute unsigned short[] numbers;

[Vineet Chaudhary (vineetc)]

(In reply to comment #6)
> > According to spec reference http://www.w3.org/TR/WebIDL/#es-array recently syntax for type Array is changed from DOMString[] to Array, So can use name Array.
> 
> Where do you see that?  The link you provided show this as an example:
> 
>   readonly attribute unsigned short[] numbers;

Ahh sorry for misunderstanding.
I found the Array type in WebIDL http://www.w3.org/TR/WebIDL/#idl-array

I could not find any example in specs where it uses name Array but still ScriptProfileNode, Internals, MessageEvent, Clipboard all these idls uses name Array so I thought using name Array.

Can you please correct me if I am wrong?

[Vineet Chaudhary (vineetc)]

Created attachment 131568 [details]
WIP Patch

Changes made to work with V8 too.

[Gustavo Noronha (kov)]

Comment on attachment 131568 [details]
WIP Patch

Attachment 131568 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11945388

[Kentaro Hara]

Comment on attachment 131568 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131568&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. WIP

Would you please add run-bindings-tests (i.e. WebCore/bindings/scripts/tests/*.idl) so that we can confirm the change that this patch is making?

[Erik Arvidsson]

I find the WebIDL very confusing in this area. For JavaScript bindings we should follow the ECMAScript binding rules (so ignore all the #idl-foo parts). As far as I can tell that means we should use sequence<T> for Arrays. 

http://www.w3.org/TR/WebIDL/#es-sequence

"IDL sequence<T> values are represented by ECMAScript Array values."

The algorithm to convert an WebIDL sequence to a JS array matches what we want.

[Adam Barth]

Comment on attachment 131568 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131568&action=review

> Source/WebCore/page/Console.idl:49
> -        readonly attribute [CustomGetter] Array profiles;
> +        readonly attribute [ArrayType=ScriptProfile] Array profiles;

Why not just:

readonly attribute ScriptProfile[] profiles;

?  I think that should match WebIDL syntax better.

[Erik Arvidsson]

(In reply to comment #12)
> (From update of attachment 131568 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131568&action=review
> 
> > Source/WebCore/page/Console.idl:49
> > -        readonly attribute [CustomGetter] Array profiles;
> > +        readonly attribute [ArrayType=ScriptProfile] Array profiles;
> 
> Why not just:
> 
> readonly attribute ScriptProfile[] profiles;
> 
> ?  I think that should match WebIDL syntax better.

or

readonly attribute sequence<ScriptProfile> profiles;

[Adam Barth]

Looks like the two are slightly different:

http://www.w3.org/TR/WebIDL/#idl-sequence
http://www.w3.org/TR/WebIDL/#idl-array

We should be sure to use the syntax that matches the implementation semantics.

[Erik Arvidsson]

(In reply to comment #14)
> Looks like the two are slightly different:
> 
> http://www.w3.org/TR/WebIDL/#idl-sequence
> http://www.w3.org/TR/WebIDL/#idl-array
> 
> We should be sure to use the syntax that matches the implementation semantics.

Be careful. We want the #es-* for the JS bindings. Not the #idl-*. Based on those our semantics matches sequence<T>.

[Vineet Chaudhary (vineetc)]

Created attachment 132017 [details]
result of run-bindings-tests

(In reply to comment #10)

> Would you please add run-bindings-tests (i.e. WebCore/bindings/scripts/tests/*.idl) so that we can confirm the change that this patch is making?
Attached result of the run-bindings-tests after adding "readonly attribute [ArrayType=ScriptProfile] Array arrayTypeAttribute" to TestObj.idl

[Vineet Chaudhary (vineetc)]

Created attachment 132024 [details]
wip_patch_001

Modified the patch to include results of the run-bindings-tests.

[Vineet Chaudhary (vineetc)]

Created attachment 132039 [details]
patch_for_review

This patch which fixes the gtk build errors too.
Please let me know your review comments on this. Thanks.

[Adam Barth]

Comment on attachment 132039 [details]
patch_for_review

View in context: https://bugs.webkit.org/attachment.cgi?id=132039&action=review

> Source/WebCore/bindings/scripts/test/TestObj.idl:48
> +        readonly attribute [ArrayType=ScriptProfile] Array arrayTypeAttribute;

I thought we decided that the proper syntax was Sequence< ScriptProfile> ?

[Adam Barth]

Sequence<ScriptProfile>

[Vineet Chaudhary (vineetc)]

(In reply to comment #20)
> Sequence<ScriptProfile>

ohh sorry Adam Barth..

Is it Sequence<ScriptProfile> or sequence<ScriptProfile> ?

Also in either case we need to modify IDL parser to parse "<" and ">" because currently it looks IDL parser only parse "[" and "]" for attribute types.

[Kentaro Hara]

(In reply to comment #21)
> Also in either case we need to modify IDL parser to parse "<" and ">" because currently it looks IDL parser only parse "[" and "]" for attribute types.

You can just modify the regular expression in IDLStructure.pm, I think.

[Adam Barth]

> Is it Sequence<ScriptProfile> or sequence<ScriptProfile> ?

You're right: sequence<T>
http://www.w3.org/TR/WebIDL/#idl-sequence

[Vineet Chaudhary (vineetc)]

Created attachment 132458 [details]
patch

using sequence<ScriptProfile> instead of Array.
In this patch it uses same "jsArray" and "V8Array" as IDL sequence<T> values are represented by ECMAScript Array values.

[WebKit Review Bot]

Comment on attachment 132458 [details]
patch

Attachment 132458 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11967819

[Kentaro Hara]

Comment on attachment 132458 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132458&action=review

> Source/WebCore/bindings/js/JSDOMBinding.h:286
> +        if (vector.isEmpty())
> +            return JSC::jsNull();

Does this mean that if vector contains no items, then null is returned instead of []? Is it correct?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3008
> +    if ($type eq "sequence<ScriptProfile>") {
> +        AddToImplIncludes("<runtime/JSArray.h>", $conditional);
> +        AddToImplIncludes("JSScriptProfile.h", $conditional);
> +        AddToImplIncludes("ScriptProfile.h", $conditional);
> +        return "jsArray(exec, $thisValue->globalObject(), $value)";
> +    }

Shall we improve this code so that it can support a general array type? In other words, if $type is sequence<X>, then we want to add <runtime/JSArray.h>, JSX.h and X.h.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3776
> +    if ($type eq "sequence<ScriptProfile>") {
> +        AddToImplIncludes("V8ScriptProfile.h");
> +        AddToImplIncludes("ScriptProfile.h");
> +        return "v8Array($value)";
> +    }

Ditto.

Is V8Array() already defined?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:46
> +#include "V8ScriptProfile.h"

Also, #include "V8sequence<ScriptProfile>.h" (appeared below) should be removed from this file.

[Vineet Chaudhary (vineetc)]

(In reply to comment #26)
> > Source/WebCore/bindings/js/JSDOMBinding.h:286
> > +        if (vector.isEmpty())
> > +            return JSC::jsNull();
> 
> Does this mean that if vector contains no items, then null is returned instead of []? Is it correct?
Yes I think it should return jsNull() as custom bindings code also does so. 

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3008
> > +    if ($type eq "sequence<ScriptProfile>") {
> > +        AddToImplIncludes("<runtime/JSArray.h>", $conditional);
> > +        AddToImplIncludes("JSScriptProfile.h", $conditional);
> > +        AddToImplIncludes("ScriptProfile.h", $conditional);
> > +        return "jsArray(exec, $thisValue->globalObject(), $value)";
> > +    }
> 
> Shall we improve this code so that it can support a general array type? In other words, if $type is sequence<X>, then we want to add <runtime/JSArray.h>, JSX.h and X.h.

Done.
 
> Is V8Array() already defined?

I think that patch misses the V8Binding.h. Now its defined.
 
> > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:46
> > +#include "V8ScriptProfile.h"
> 
> Also, #include "V8sequence<ScriptProfile>.h" (appeared below) should be removed from this file.

Done.

[Vineet Chaudhary (vineetc)]

Created attachment 132575 [details]
patch

Patch as per above review comments.

[WebKit Review Bot]

Attachment 132575 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:234:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1063:  Extra space before )  [whitespace/parens] [2]
Source/WebCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vineet Chaudhary (vineetc)]

(In reply to comment #29)
> Attachment 132575 [details] did not pass style-queue:
> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:234:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1063:  Extra space before )  [whitespace/parens] [2]

Patch is failing because I has empty setter function currently. I am working on that planning to fix that in another bug. Is that ok?

[Kentaro Hara]

Comment on attachment 132575 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132575&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:170
> +    my @attributeType = split(/\W+/, $propType);
> +    if ($attributeType[0] eq "sequence") {
> +        return 1;
> +    }

I think you want to skip all array types here. Maybe you can add CodeGenerator::GetArrayType($type) to CodeGenerator.pm and use it in all code generators. 

sub GetArrayType {
    my $type = shift;
    return $1 if $type =~ /^sequence<([\w\d_]+)>$/;
    return "";   
}

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:248
> +    my @attributeType = split(/\W+/, $type);

Remove this

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:271
> +    } elsif ($attributeType[0] eq "sequence") {
>      } else {

You can just write

    } elsif (!$codeGenerator->GetArrayType($type)) {

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2953
> +    my @attributeType = split(/\W+/, $type);
> +    if ($attributeType[0] eq "sequence") {
> +        return "";
> +    }

Use GetArrayType($type).

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3015
> +    my @attributeType = split(/\W+/, $type);
> +    if ($attributeType[0] eq "sequence") {

my $attributeType = $codeGenerator->GetArrayType($type);
if ($attributeType) {

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:929
> +        my @attributeType = split(/\W+/, $returnType);
> +
> +        if ($attributeType[0] eq "sequence") {

Ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3572
> +    my @attributeType = split(/\W+/, $type);
> +    if ($attributeType[0] eq "sequence") {
> +        return "";
> +    }

Ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3792
> +    my @attributeType = split(/\W+/, $type);
> +    if ($attributeType[0] eq "sequence") {
> +        AddToImplIncludes("V8ScriptProfile.h");
> +        AddToImplIncludes("ScriptProfile.h");
> +        return "v8Array($value)";
> +    }

Ditto.

[Kentaro Hara]

(In reply to comment #30)
> (In reply to comment #29)
> > Attachment 132575 [details] [details] did not pass style-queue:
> > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:234:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
> > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1063:  Extra space before )  [whitespace/parens] [2]
> 
> Patch is failing because I has empty setter function currently. I am working on that planning to fix that in another bug. Is that ok?

OK, auto-generated code is full of style bugs. Maybe you can just ignore it unless it is obvious to fix.

[Kentaro Hara]

(In reply to comment #27)
> > > Source/WebCore/bindings/js/JSDOMBinding.h:286
> > > +        if (vector.isEmpty())
> > > +            return JSC::jsNull();
> > 
> > Does this mean that if vector contains no items, then null is returned instead of []? Is it correct?
> Yes I think it should return jsNull() as custom bindings code also does so. 

Which custom bindings are you referring to? It seems JSConsoleCustom.cpp has been returning constructArray(exec, globalObject(), list) even for an empty vector, where list is an empty MarkedArgumentBuffer.

[Vineet Chaudhary (vineetc)]

(In reply to comment #33)
> (In reply to comment #27)
> > > > Source/WebCore/bindings/js/JSDOMBinding.h:286
> > > > +        if (vector.isEmpty())
> > > > +            return JSC::jsNull();
> > > 
> > > Does this mean that if vector contains no items, then null is returned instead of []? Is it correct?
> > Yes I think it should return jsNull() as custom bindings code also does so. 
> 
> Which custom bindings are you referring to? It seems JSConsoleCustom.cpp has been returning constructArray(exec, globalObject(), list) even for an empty vector, where list is an empty MarkedArgumentBuffer.

I was refering to Clipboard custom bindings http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSClipboardCustom.cpp?rev=109182#L57

[Daniel Cheng]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #27)
> > > > > Source/WebCore/bindings/js/JSDOMBinding.h:286
> > > > > +        if (vector.isEmpty())
> > > > > +            return JSC::jsNull();
> > > > 
> > > > Does this mean that if vector contains no items, then null is returned instead of []? Is it correct?
> > > Yes I think it should return jsNull() as custom bindings code also does so. 
> > 
> > Which custom bindings are you referring to? It seems JSConsoleCustom.cpp has been returning constructArray(exec, globalObject(), list) even for an empty vector, where list is an empty MarkedArgumentBuffer.
> 
> I was refering to Clipboard custom bindings http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSClipboardCustom.cpp?rev=109182#L57

I don't think we want that behavior in Clipboard, since it makes it harder to check if an Array contains a certain value.

[Vineet Chaudhary (vineetc)]

Created attachment 132615 [details]
updated_patch

Modified patch as per review comments.

[WebKit Review Bot]

Attachment 132615 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:234:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Erik Arvidsson]

Comment on attachment 132615 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132615&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:930
> +            AddToImplIncludes("V8" . $attributeType . ".h");

Prefer placeholders over string concat

 AddToImplIncludes("V8${attributeType}.h");

>> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:234
>> +    sequence<ScriptProfile>* v = ;
> 
> Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]

This is a syntax error. Please fix.

[Kentaro Hara]

Comment on attachment 132615 [details]
updated_patch

The patch looks good to me except for arv@'s comments.

[Vineet Chaudhary (vineetc)]

Created attachment 132773 [details]
updated_patch

[Kentaro Hara]

Comment on attachment 132773 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132773&action=review

> Source/WebCore/bindings/js/JSDOMBinding.h:295
> +    Iterable jsValueToNative(JSC::ExecState* exec, JSC::JSValue value)

This should be named toJSArray() for naming consistency.

BTW, what's the difference between toJSArray() and existing toJSSequence()?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1112
> +        my @arrayType = split(/\W+/, $nativeType);

Please use GetArrayType()

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:969
> +    impl->setSequenceAttr(jsValueToNative(exec, value));

It seems that the argument type is inconsistent with that of V8 generated code. What type is setSequenceAttr() expecting for the first argument?

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:234
> +    ScriptProfile v = v8ValueToNative(value);

This is wrong. v8ValueToNative() returns Vector.

> Source/WebCore/bindings/v8/V8Binding.h:301
> +    Vector<T> v8ValueToNative(v8::Handle<v8::Value> value)

This should be named toV8Array(), for naming consistency with toInt32(), toFloat(), toWebCoreString(), etc.

[Vineet Chaudhary (vineetc)]

Comment on attachment 132773 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132773&action=review

>> Source/WebCore/bindings/js/JSDOMBinding.h:295
>> +    Iterable jsValueToNative(JSC::ExecState* exec, JSC::JSValue value)
> 
> This should be named toJSArray() for naming consistency.
> 
> BTW, what's the difference between toJSArray() and existing toJSSequence()?

As per the comments toJSSequence() which returns JDObject, Validates that the passed object is a sequence type per section 4.1.13 of the WebIDL spec.
Used in jsUnsignedLongArrayToVector() a simillar function.
Do we need same here?

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1112
>> +        my @arrayType = split(/\W+/, $nativeType);
> 
> Please use GetArrayType()

Actually I tried GetArrayType() but $nativeType is  sequence<ScriptProfile>*  so GetArrayType() returns "" all the time.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:969
>> +    impl->setSequenceAttr(jsValueToNative(exec, value));
> 
> It seems that the argument type is inconsistent with that of V8 generated code. What type is setSequenceAttr() expecting for the first argument?

I didn't got this. setSequenceAttr() first argument should be of type Vector isn't it? Can you please correct?

>> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:234
>> +    ScriptProfile v = v8ValueToNative(value);
> 
> This is wrong. v8ValueToNative() returns Vector.

Ohh should be Vector<RefPtr<ScriptProfile> > v = toV8Array(value);

>> Source/WebCore/bindings/v8/V8Binding.h:301
>> +    Vector<T> v8ValueToNative(v8::Handle<v8::Value> value)
> 
> This should be named toV8Array(), for naming consistency with toInt32(), toFloat(), toWebCoreString(), etc.

Will do.

[Vineet Chaudhary (vineetc)]

Created attachment 132792 [details]
another_attempt

[Kentaro Hara]

Comment on attachment 132792 [details]
another_attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=132792&action=review

Getting close!

> Source/WebCore/bindings/js/JSDOMBinding.h:296
> +    {

Here, would you add the check that checks if |value| is an array type (just like toV8Array() does)?

> Source/WebCore/bindings/js/JSDOMBinding.h:300
> +            String indexedValue = ustringToString(array->getIndex(i).toString(exec)->value(exec));

I am not sure if this code is correct. Where did you copy the code from?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3012
> +    my $attributeType = $codeGenerator->GetArrayType($type);

Nit: Let's use $arrayType instead of $attributeType, for consistency.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:927
> +        my $attributeType = $codeGenerator->GetArrayType($returnType);

$arrayType

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:928
> +

Nit: This empty line is not necessary.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3790
> +    my $attributeType = $codeGenerator->GetArrayType($type);

$arrayType

> Source/WebCore/bindings/v8/V8Binding.h:303
> +        v8::Local<v8::Value> v8Value(v8::Local<v8::Value>::New(value));

|v8Value| would not be necessary.

> Source/WebCore/bindings/v8/V8Binding.h:304
> +        if (!v8Value->IsArray())

You can just write 'if (!value->IsArray())'

> Source/WebCore/bindings/v8/V8Binding.h:308
> +        v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(v8Value);

You can just write 'Cast(value)'

[Vineet Chaudhary (vineetc)]

(In reply to comment #44)
> (From update of attachment 132792 [details])
> 
> Getting close!
Thanks you for quick review. :)
 
> > Source/WebCore/bindings/js/JSDOMBinding.h:296
> > +    {
> Here, would you add the check that checks if |value| is an array type (just like toV8Array() does)?

will do.

> > Source/WebCore/bindings/js/JSDOMBinding.h:300
> > +            String indexedValue = ustringToString(array->getIndex(i).toString(exec)->value(exec));
> 
> I am not sure if this code is correct. Where did you copy the code from?

I copied this code from  http://trac.webkit.org/browser/trunk/Source/WebCore/testing/js/JSInternalsCustom.cpp?rev=105698#L59 

Rest all Nits: will do.

[Kentaro Hara]

(In reply to comment #45)
> > > Source/WebCore/bindings/js/JSDOMBinding.h:300
> > > +            String indexedValue = ustringToString(array->getIndex(i).toString(exec)->value(exec));
> > 
> > I am not sure if this code is correct. Where did you copy the code from?
> 
> I copied this code from  http://trac.webkit.org/browser/trunk/Source/WebCore/testing/js/JSInternalsCustom.cpp?rev=105698#L59 

It looks OK. Thanks for the clarification.

[Kentaro Hara]

Comment on attachment 132792 [details]
another_attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=132792&action=review

> Source/WebCore/bindings/js/JSDOMBinding.h:295
> +    Iterable toJSArray(JSC::ExecState* exec, JSC::JSValue value)

Nit: Sorry for my previous comment, this is not a JS array. Shall we call it toNativeArray()?

> Source/WebCore/bindings/v8/V8Binding.h:301
> +    Vector<T> toV8Array(v8::Handle<v8::Value> value)

toNativeArray()

[Vineet Chaudhary (vineetc)]

(In reply to comment #47)
> (From update of attachment 132792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132792&action=review
> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:295
> > +    Iterable toJSArray(JSC::ExecState* exec, JSC::JSValue value)
> 
> Nit: Sorry for my previous comment, this is not a JS array. Shall we call it toNativeArray()?
> 
> > Source/WebCore/bindings/v8/V8Binding.h:301
> > +    Vector<T> toV8Array(v8::Handle<v8::Value> value)
> 
> toNativeArray()

Oke will do that but should it be toNativeArray() or jsToNativeArray() and v8ToNativeArray()

[Kentaro Hara]

(In reply to comment #48)
> (In reply to comment #47)
> Oke will do that but should it be toNativeArray() or jsToNativeArray() and v8ToNativeArray()

Both sound good, but toNativeArray() might be better, for consistency with toInt32(), toWebCoreString() etc.

[Vineet Chaudhary (vineetc)]

Created attachment 132817 [details]
another_attempt_01

1) Using toNativeArray()
2) Added isJSArray() check.

>|v8Value| would not be necessary.
> Source/WebCore/bindings/v8/V8Binding.h:304
> +        if (!v8Value->IsArray())
>You can just write 'if (!value->IsArray())'
> Source/WebCore/bindings/v8/V8Binding.h:308
> +        v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(v8Value);
>You can just write 'Cast(value)
I tried this but getting compile errors : like 

Source/WebCore/bindings/v8/V8Binding.h: In function ‘WTF::Vector<T> WebCore::toV8Array(v8::Handle<v8::Value>)’:
Source/WebCore/bindings/v8/V8Binding.h:307:70: error: no matching function for call to ‘v8::Local<v8::Array>::Cast(v8::Handle<v8::Value>&)’
Source/WebCore/bindings/v8/V8Binding.h:307:70: note: candidate is:
Source/WebKit/chromium/v8/include/v8.h:277:45: note: template<class S> static v8::Local v8::Local::Cast(v8::Local<S>) [with S = S, T = v8::Array]

So still using v8::Local<v8::Array>::Cast(v8Value).

[Kentaro Hara]

Comment on attachment 132817 [details]
another_attempt_01

View in context: https://bugs.webkit.org/attachment.cgi?id=132817&action=review

> Source/WebCore/bindings/js/JSDOMBinding.h:299
> +            throwError(exec, createSyntaxError(exec, "Expected Array"));

This exception won't be necessary. We should align the behavior between JSC and V8, and should not throw exception unless it is defined in the spec.

> Source/WebCore/bindings/js/JSDOMBinding.h:300
> +            return JSC::jsNull();

Shouldn't we return an empty Vector?

[Vineet Chaudhary (vineetc)]

(In reply to comment #51)
> (From update of attachment 132817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132817&action=review
> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:299
> > +            throwError(exec, createSyntaxError(exec, "Expected Array"));
> 
> This exception won't be necessary. We should align the behavior between JSC and V8, and should not throw exception unless it is defined in the spec.
> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:300
> > +            return JSC::jsNull();
> 
> Shouldn't we return an empty Vector?

Currently only Internals.idl has the getters and setters rest all are having readonly attribute. This behaviour matches with http://trac.webkit.org/browser/trunk/Source/WebCore/testing/js/JSInternalsCustom.cpp?rev=105698#L51 .

Also sorry for lack of understanding but I am not sure how to return empty vector for JS. 
Should it be return Iterable<> or something?

[Kentaro Hara]

(In reply to comment #52)
> > > Source/WebCore/bindings/js/JSDOMBinding.h:299
> > > +            throwError(exec, createSyntaxError(exec, "Expected Array"));
> > 
> > This exception won't be necessary. We should align the behavior between JSC and V8, and should not throw exception unless it is defined in the spec.

> Currently only Internals.idl has the getters and setters rest all are having readonly attribute. This behaviour matches with http://trac.webkit.org/browser/trunk/Source/WebCore/testing/js/JSInternalsCustom.cpp?rev=105698#L51 .

Aligning the behavior between JSC and V8 for the same attribute is more important. Let's remove throwError().

> > > Source/WebCore/bindings/js/JSDOMBinding.h:300
> > > +            return JSC::jsNull();
> > 
> > Shouldn't we return an empty Vector?
> 
> Also sorry for lack of understanding but I am not sure how to return empty vector for JS. 
> Should it be return Iterable<> or something?

By the way, why do you need to use Iterable? Can't we use Vector<T> (instead of Iterable) in the same way as V8Binding.h? imp->setSequenceAttr() will return Vector, and thus using Vector<T> in JSDOMBinding.h would make sense.

[Vineet Chaudhary (vineetc)]

Created attachment 132850 [details]
patch_for_review

Done!!

[Kentaro Hara]

Comment on attachment 132850 [details]
patch_for_review

Thank you very much for the patch. I hope that other custom arrays will be replaced with sequence<T> soon. Great work!

[Vineet Chaudhary (vineetc)]

(In reply to comment #55)
> (From update of attachment 132850 [details])
> Thank you very much for the patch. I hope that other custom arrays will be replaced with sequence<T> soon. Great work!

Thanks to you for your kind help.
I have already working fixes to follow soon for other bindings.

[WebKit Review Bot]

Comment on attachment 132850 [details]
patch_for_review

Clearing flags on attachment: 132850

Committed r111416: <http://trac.webkit.org/changeset/111416>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke a test on GTK and on Qt:
--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/dom/prototype-inheritance-2-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/dom/prototype-inheritance-2-actual.txt 
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS Array from inner.document.forms.testForm.0.ownerDocument.defaultView.console.profiles
+FAIL Array from inner.document.forms.testForm.0.ownerDocument.defaultView.console.profiles
 PASS Attr from inner.document.forms.testForm.0.attributes.0
 PASS AttrConstructor from inner.document.forms.testForm.0.attributes.0.constructor
 PASS AttrPrototype from inner.document.forms.testForm.0.attributes.0.__proto__

(In reply to comment #59)
> Reopen, because it broke a test on GTK and on Qt:

fast/dom/prototype-inheritance-2.html is also failing in Mac WebKit2. Filed bug 81705.

[Vineet Chaudhary (vineetc)]

(In reply to comment #59)
> Reopen, because it broke a test on GTK and on Qt:
> --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/dom/prototype-inheritance-2-expected.txt 
> +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/dom/prototype-inheritance-2-actual.txt 
> @@ -3,7 +3,7 @@
>  On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> 
> -PASS Array from inner.document.forms.testForm.0.ownerDocument.defaultView.console.profiles
> +FAIL Array from inner.document.forms.testForm.0.ownerDocument.defaultView.console.profiles
>  PASS Attr from inner.document.forms.testForm.0.attributes.0
>  PASS AttrConstructor from inner.document.forms.testForm.0.attributes.0.constructor
>  PASS AttrPrototype from inner.document.forms.testForm.0.attributes.0.__proto__

Can we close this issue as Bug 81705 is fixed.

[Vineet Chaudhary (vineetc)]

Hi All,

As per the comments https://bugs.webkit.org/show_bug.cgi?id=84540#c25

>As I commented, it seems wrong to support sequences in attributes:
>http://www.w3.org/TR/WebIDL/#idl-sequence

>"Sequences must not be used as the type of an attribute, constant or exception field."

So using the sequence<T> for the attributes seems wrong here. I think we have some confusion here between using sequence<T> and Array[].
Experts can you please clarify on this as we have already removed few custom bindings using sequence<T>.

[Erik Arvidsson]

Where are we trying to use sequence as an attribute?

[Kentaro Hara]

(In reply to comment #63)
> Where are we trying to use sequence as an attribute?

page/Console.idl:        readonly attribute sequence<ScriptProfile> profiles;
testing/Internals.idl:        attribute sequence<String> userPreferredLanguages;

As far as I understand,

- We wanted T[] or sequence<T> for attributes, methods, etc. In this bug, there was a discussion about which syntax we should use.
- We decided to use sequence<T>.
- But we found that sequence<T> is not allowed for attributes in the Web IDL spec.

[Kentaro Hara]

Reopening the discussion

[Erik Arvidsson]

I always found sequence<T> and T[] confusing...

The idea behind sequence<T> is that it returns a new Array every time which would cause the following to fail

console.profiles === console.profiles

However, for T[], it requires that the data structure is shared and that changes done in JS are somehow propagated into the DOM and vice versa. Not really what we want.

One way around this might be to use T[] but ensure that the object is frozen. That way there is no need to propagate changes back and forth because no changes are allowed.

[Kentaro Hara]

(In reply to comment #66)
> I always found sequence<T> and T[] confusing...
> 
> The idea behind sequence<T> is that it returns a new Array every time which would cause the following to fail
> 
> console.profiles === console.profiles
> 
> However, for T[], it requires that the data structure is shared and that changes done in JS are somehow propagated into the DOM and vice versa. Not really what we want.
> 
> One way around this might be to use T[] but ensure that the object is frozen. That way there is no need to propagate changes back and forth because no changes are allowed.

Thanks arv! I'd like to confirm my understanding:

- attribute sequence<T> t; should not be allowed in an IDL.
- attribute T[] t; should be allowed in an IDL. It returns a new Array every time.
- T[] method(); should be allowed in an IDL. It will return a shared Array.
- sequence<T> method(); should be allowed in an IDL. It will return a new Array every time.
- void method(T[] t); should be allowed in an IDL. The passed Array will be shared.
- void method(sequence<T> t); should be allowed in an IDL. The passed Array is not shared.

Correct?

[Erik Arvidsson]

I think that is correct except for:

(In reply to comment #67)
> - attribute T[] t; should be allowed in an IDL. It returns a new Array every time.

I think this should return the same shared Array every time.

[Kentaro Hara]

(In reply to comment #68)
> > - attribute T[] t; should be allowed in an IDL. It returns a new Array every time.
> 
> I think this should return the same shared Array every time.

Makes sense. Then what we need now would be to change

    page/Console.idl:        readonly attribute sequence<ScriptProfile> profiles;
    testing/Internals.idl:        attribute sequence<String> userPreferredLanguages;
    inspector/ScriptProfileNode.idl:        readonly attribute sequence<ScriptProfileNode> children;

to

    page/Console.idl:        readonly attribute ScriptProfile[] profiles;
    testing/Internals.idl:        attribute String[] userPreferredLanguages;
    inspector/ScriptProfileNode.idl:        readonly attribute ScriptProfileNode[] children;

BTW, the only place where we are using sequence<T> is

    dom/WebKitMutationObserver.idl:        sequence<MutationRecord> takeRecords();

Is this expected?

[Erik Arvidsson]

(In reply to comment #69)

> Is this expected?

Sounds right to me.

[Vineet Chaudhary (vineetc)]

(In reply to comment #69)
> (In reply to comment #68)
> > > - attribute T[] t; should be allowed in an IDL. It returns a new Array every time.
> > 
> > I think this should return the same shared Array every time.
> 
> Makes sense. Then what we need now would be to change
> 
>     page/Console.idl:        readonly attribute sequence<ScriptProfile> profiles;
>     testing/Internals.idl:        attribute sequence<String> userPreferredLanguages;
>     inspector/ScriptProfileNode.idl:        readonly attribute sequence<ScriptProfileNode> children;
> 
> to
> 
>     page/Console.idl:        readonly attribute ScriptProfile[] profiles;
>     testing/Internals.idl:        attribute String[] userPreferredLanguages;
>     inspector/ScriptProfileNode.idl:        readonly attribute ScriptProfileNode[] children;
> 
> BTW, the only place where we are using sequence<T> is
> 
>     dom/WebKitMutationObserver.idl:        sequence<MutationRecord> takeRecords();
> 
> Is this expected?

Thanks haraken/arv for clarifying on this. I hope we can use the current code with little modifications for T[].

Should we reopen previous bugs (Console, Internals, ScriptProfileNode) for new implementation or log new one?

[Kentaro Hara]

It seems that we've reached the consensus that we should use T[] for attributes. 

> Thanks haraken/arv for clarifying on this. I hope we can use the current code with little modifications for T[].

I hope so!

> Should we reopen previous bugs (Console, Internals, ScriptProfileNode) for new implementation or log new one?

Both are OK. (But I might prefer creating a new one with a title like "REGRESSION(rXXXXX): sequence<T> in YYYY.idl should be T[]". Personally I like one patch per one bug.)

Just a clarification: our objective would be to fix CodeGeneratorV8.pm and then make the followings work:

    page/Console.idl:        readonly attribute ScriptProfile[] profiles;
    testing/Internals.idl:        attribute String[] userPreferredLanguages;
    inspector/ScriptProfileNode.idl:        readonly attribute ScriptProfileNode[] children;

[David Levin]

(In reply to comment #72)
> It seems that we've reached the consensus that we should use T[] for attributes. 
> 
> > Thanks haraken/arv for clarifying on this. I hope we can use the current code with little modifications for T[].
> 
> I hope so!
> 
> > Should we reopen previous bugs (Console, Internals, ScriptProfileNode) for new implementation or log new one?
> 
> Both are OK. (But I might prefer creating a new one with a title like "REGRESSION(rXXXXX): sequence<T> in YYYY.idl should be T[]". Personally I like one patch per one bug.)

Agreed. New bugs are generally preferred (because otherwise it gets confusing about what comments apply to the patch in progress).

[Vineet Chaudhary (vineetc)]

(In reply to comment #72)

> Just a clarification: our objective would be to fix CodeGeneratorV8.pm and then make the followings work:

haraken I didn't got "to fix CodeGeneratorV8.pm"?

I have logged separate bug to address this regression Bug 84863.

[Kentaro Hara]

(In reply to comment #74)
> > Just a clarification: our objective would be to fix CodeGeneratorV8.pm and then make the followings work:
> 
> haraken I didn't got "to fix CodeGeneratorV8.pm"?

Sorry, you're right.

[Zan Dobersek]

Given the patch landed and the regression was fixed in a separate bug, should this bug be closed?

[Adam Barth]

Yes.  One patch per bug.  I suspect we haven't removed all the custom bindings for Arrays yet, but we can file more bugs about removing more custom bindings.