Bug 80696 - Remove custom bindings for attribute type Array.
Summary: Remove custom bindings for attribute type Array.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vineet Chaudhary (vineetc)
URL:
Keywords:
Depends on: 81345 81705
Blocks: 80269 84929
  Show dependency treegraph
 
Reported: 2012-03-09 04:33 PST by Arko Saha
Modified: 2012-05-25 15:57 PDT (History)
19 users (show)

See Also:


Attachments
proposed patch (5.75 KB, patch)
2012-03-09 04:48 PST, Vineet Chaudhary (vineetc)
abarth: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (10.79 KB, patch)
2012-03-13 00:49 PDT, Vineet Chaudhary (vineetc)
gns: commit-queue-
Details | Formatted Diff | Diff
result of run-bindings-tests (17.55 KB, text/plain)
2012-03-15 04:31 PDT, Vineet Chaudhary (vineetc)
no flags Details
wip_patch_001 (21.62 KB, patch)
2012-03-15 05:16 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
patch_for_review (21.31 KB, patch)
2012-03-15 07:06 PDT, Vineet Chaudhary (vineetc)
abarth: review-
Details | Formatted Diff | Diff
patch (14.29 KB, patch)
2012-03-17 04:22 PDT, Vineet Chaudhary (vineetc)
haraken: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch (21.94 KB, patch)
2012-03-19 07:10 PDT, Vineet Chaudhary (vineetc)
haraken: review-
Details | Formatted Diff | Diff
updated_patch (21.10 KB, patch)
2012-03-19 11:29 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
updated_patch (22.89 KB, patch)
2012-03-20 00:07 PDT, Vineet Chaudhary (vineetc)
haraken: review-
Details | Formatted Diff | Diff
another_attempt (23.33 KB, patch)
2012-03-20 04:14 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
another_attempt_01 (23.53 KB, patch)
2012-03-20 06:46 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
patch_for_review (23.39 KB, patch)
2012-03-20 10:31 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arko Saha 2012-03-09 04:33:45 PST
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.
Comment 1 Vineet Chaudhary (vineetc) 2012-03-09 04:48:34 PST
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.
Comment 2 WebKit Review Bot 2012-03-09 05:28:33 PST
Comment on attachment 131028 [details]
proposed patch

Attachment 131028 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11894590
Comment 3 Gustavo Noronha (kov) 2012-03-09 06:47:45 PST
Comment on attachment 131028 [details]
proposed patch

Attachment 131028 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11898567
Comment 4 Adam Barth 2012-03-09 09:24:10 PST
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.
Comment 5 Vineet Chaudhary (vineetc) 2012-03-11 22:47:06 PDT
(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.
Comment 6 Adam Barth 2012-03-11 22:53:45 PDT
> 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;
Comment 7 Vineet Chaudhary (vineetc) 2012-03-12 00:17:23 PDT
(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?
Comment 8 Vineet Chaudhary (vineetc) 2012-03-13 00:49:41 PDT
Created attachment 131568 [details]
WIP Patch

Changes made to work with V8 too.
Comment 9 Gustavo Noronha (kov) 2012-03-13 02:58:22 PDT
Comment on attachment 131568 [details]
WIP Patch

Attachment 131568 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11945388
Comment 10 Kentaro Hara 2012-03-14 09:20:56 PDT
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?
Comment 11 Erik Arvidsson 2012-03-14 10:37:38 PDT
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.
Comment 12 Adam Barth 2012-03-14 14:01:20 PDT
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.
Comment 13 Erik Arvidsson 2012-03-14 14:03:00 PDT
(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;
Comment 14 Adam Barth 2012-03-14 14:07:06 PDT
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.
Comment 15 Erik Arvidsson 2012-03-14 14:14:53 PDT
(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>.
Comment 16 Vineet Chaudhary (vineetc) 2012-03-15 04:31:36 PDT
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
Comment 17 Vineet Chaudhary (vineetc) 2012-03-15 05:16:09 PDT
Created attachment 132024 [details]
wip_patch_001

Modified the patch to include results of the run-bindings-tests.
Comment 18 Vineet Chaudhary (vineetc) 2012-03-15 07:06:47 PDT
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.
Comment 19 Adam Barth 2012-03-15 13:35:46 PDT
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> ?
Comment 20 Adam Barth 2012-03-15 13:36:03 PDT
Sequence<ScriptProfile>
Comment 21 Vineet Chaudhary (vineetc) 2012-03-15 23:25:37 PDT
(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.
Comment 22 Kentaro Hara 2012-03-15 23:27:14 PDT
(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.
Comment 23 Adam Barth 2012-03-15 23:28:05 PDT
> Is it Sequence<ScriptProfile> or sequence<ScriptProfile> ?

You're right: sequence<T>
http://www.w3.org/TR/WebIDL/#idl-sequence
Comment 24 Vineet Chaudhary (vineetc) 2012-03-17 04:22:11 PDT
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.
Comment 25 WebKit Review Bot 2012-03-17 04:48:17 PDT
Comment on attachment 132458 [details]
patch

Attachment 132458 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11967819
Comment 26 Kentaro Hara 2012-03-17 06:38:53 PDT
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.
Comment 27 Vineet Chaudhary (vineetc) 2012-03-19 07:09:12 PDT
(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.
Comment 28 Vineet Chaudhary (vineetc) 2012-03-19 07:10:54 PDT
Created attachment 132575 [details]
patch

Patch as per above review comments.
Comment 29 WebKit Review Bot 2012-03-19 07:13:00 PDT
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.
Comment 30 Vineet Chaudhary (vineetc) 2012-03-19 07:35:04 PDT
(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?
Comment 31 Kentaro Hara 2012-03-19 07:37:06 PDT
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.
Comment 32 Kentaro Hara 2012-03-19 07:38:36 PDT
(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.
Comment 33 Kentaro Hara 2012-03-19 07:43:03 PDT
(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.
Comment 34 Vineet Chaudhary (vineetc) 2012-03-19 07:50:49 PDT
(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
Comment 35 Daniel Cheng 2012-03-19 08:21:02 PDT
(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.
Comment 36 Vineet Chaudhary (vineetc) 2012-03-19 11:29:29 PDT
Created attachment 132615 [details]
updated_patch

Modified patch as per review comments.
Comment 37 WebKit Review Bot 2012-03-19 11:32:42 PDT
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.
Comment 38 Erik Arvidsson 2012-03-19 11:45:35 PDT
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.
Comment 39 Kentaro Hara 2012-03-19 17:50:56 PDT
Comment on attachment 132615 [details]
updated_patch

The patch looks good to me except for arv@'s comments.
Comment 40 Vineet Chaudhary (vineetc) 2012-03-20 00:07:38 PDT
Created attachment 132773 [details]
updated_patch
Comment 41 Kentaro Hara 2012-03-20 01:16:25 PDT
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.
Comment 42 Vineet Chaudhary (vineetc) 2012-03-20 02:06:18 PDT
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.
Comment 43 Vineet Chaudhary (vineetc) 2012-03-20 04:14:42 PDT
Created attachment 132792 [details]
another_attempt
Comment 44 Kentaro Hara 2012-03-20 04:47:40 PDT
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)'
Comment 45 Vineet Chaudhary (vineetc) 2012-03-20 05:05:55 PDT
(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.
Comment 46 Kentaro Hara 2012-03-20 05:23:51 PDT
(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.
Comment 47 Kentaro Hara 2012-03-20 06:01:38 PDT
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()
Comment 48 Vineet Chaudhary (vineetc) 2012-03-20 06:16:35 PDT
(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()
Comment 49 Kentaro Hara 2012-03-20 06:19:15 PDT
(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.
Comment 50 Vineet Chaudhary (vineetc) 2012-03-20 06:46:59 PDT
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).
Comment 51 Kentaro Hara 2012-03-20 07:10:44 PDT
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?
Comment 52 Vineet Chaudhary (vineetc) 2012-03-20 08:39:36 PDT
(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?
Comment 53 Kentaro Hara 2012-03-20 08:45:41 PDT
(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.
Comment 54 Vineet Chaudhary (vineetc) 2012-03-20 10:31:30 PDT
Created attachment 132850 [details]
patch_for_review

Done!!
Comment 55 Kentaro Hara 2012-03-20 10:58:08 PDT
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!
Comment 56 Vineet Chaudhary (vineetc) 2012-03-20 11:24:58 PDT
(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.
Comment 57 WebKit Review Bot 2012-03-20 11:27:08 PDT
Comment on attachment 132850 [details]
patch_for_review

Clearing flags on attachment: 132850

Committed r111416: <http://trac.webkit.org/changeset/111416>
Comment 58 WebKit Review Bot 2012-03-20 11:27:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 59 Csaba Osztrogonác 2012-03-20 14:52:57 PDT
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__
Comment 60 mitz 2012-03-20 15:31:32 PDT
(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.
Comment 61 Vineet Chaudhary (vineetc) 2012-03-27 04:45:47 PDT
(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.
Comment 62 Vineet Chaudhary (vineetc) 2012-04-24 09:43:52 PDT
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>.
Comment 63 Erik Arvidsson 2012-04-24 14:26:34 PDT
Where are we trying to use sequence as an attribute?
Comment 64 Kentaro Hara 2012-04-24 14:32:43 PDT
(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.
Comment 65 Kentaro Hara 2012-04-24 14:33:38 PDT
Reopening the discussion
Comment 66 Erik Arvidsson 2012-04-24 14:37:52 PDT
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.
Comment 67 Kentaro Hara 2012-04-24 14:50:24 PDT
(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?
Comment 68 Erik Arvidsson 2012-04-24 14:55:45 PDT
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.
Comment 69 Kentaro Hara 2012-04-24 15:02:42 PDT
(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?
Comment 70 Erik Arvidsson 2012-04-24 15:18:16 PDT
(In reply to comment #69)

> Is this expected?

Sounds right to me.
Comment 71 Vineet Chaudhary (vineetc) 2012-04-24 22:43:02 PDT
(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?
Comment 72 Kentaro Hara 2012-04-24 22:48:53 PDT
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;
Comment 73 David Levin 2012-04-24 23:05:19 PDT
(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).
Comment 74 Vineet Chaudhary (vineetc) 2012-04-25 08:14:05 PDT
(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.
Comment 75 Kentaro Hara 2012-04-25 08:30:50 PDT
(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.
Comment 76 Zan Dobersek 2012-05-25 08:25:10 PDT
Given the patch landed and the regression was fixed in a separate bug, should this bug be closed?
Comment 77 Adam Barth 2012-05-25 15:57:00 PDT
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.