Bug 80696

Summary:

Remove custom bindings for attribute type Array.

Product:

WebKit

Reporter:

Arko Saha <arko>

Component:

New Bugs

Assignee:

Vineet Chaudhary (vineetc) <code.vineet>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, adamk, arv, code.vineet, dcheng, dglazkov, eric, gustavo, haraken, japhet, levin, mitz, ojan, ossy, rniwa, sam, webkit.review.bot, xan.lopez, zan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

81345, 81705

Bug Blocks:

80269, 84929

Attachments:

Description	Flags
proposed patch	abarth: review-, webkit.review.bot: commit-queue-
WIP Patch	gustavo: commit-queue-
result of run-bindings-tests	none
wip_patch_001	none
patch_for_review	abarth: review-
patch	haraken: review-, webkit.review.bot: commit-queue-
patch	haraken: review-
updated_patch	none
updated_patch	haraken: review-
another_attempt	none
another_attempt_01	none
patch_for_review	none

Arko Saha

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

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)	gustavo: 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
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Vineet Chaudhary (vineetc)

Comment 1 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.

WebKit Review Bot

Comment 2 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

Gustavo Noronha (kov)

Comment 3 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

Adam Barth

Comment 4 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.

Vineet Chaudhary (vineetc)

Comment 5 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.

Adam Barth

Comment 6 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;

Vineet Chaudhary (vineetc)

Comment 7 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?

Vineet Chaudhary (vineetc)

Comment 8 2012-03-13 00:49:41 PDT

Created attachment 131568 [details] WIP Patch Changes made to work with V8 too.

Gustavo Noronha (kov)

Comment 9 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

Kentaro Hara

Comment 10 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?

Erik Arvidsson

Comment 11 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.

Adam Barth

Comment 12 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.

Erik Arvidsson

Comment 13 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;

Adam Barth

Comment 14 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.

Erik Arvidsson

Comment 15 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>.

Vineet Chaudhary (vineetc)

Comment 16 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

Vineet Chaudhary (vineetc)

Comment 17 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.

Vineet Chaudhary (vineetc)

Comment 18 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.

Adam Barth

Comment 19 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> ?

Adam Barth

Comment 20 2012-03-15 13:36:03 PDT

Sequence<ScriptProfile>

Vineet Chaudhary (vineetc)

Comment 21 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.

Kentaro Hara

Comment 22 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.

Adam Barth

Comment 23 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

Vineet Chaudhary (vineetc)

Comment 24 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.

WebKit Review Bot

Comment 25 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

Kentaro Hara

Comment 26 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.

Vineet Chaudhary (vineetc)

Comment 27 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.

Vineet Chaudhary (vineetc)

Comment 28 2012-03-19 07:10:54 PDT

Created attachment 132575 [details] patch Patch as per above review comments.

WebKit Review Bot

Comment 29 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.

Vineet Chaudhary (vineetc)

Comment 30 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?

Kentaro Hara

Comment 31 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.

Kentaro Hara

Comment 32 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.

Kentaro Hara

Comment 33 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.

Vineet Chaudhary (vineetc)

Comment 34 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

Daniel Cheng

Comment 35 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.

Vineet Chaudhary (vineetc)

Comment 36 2012-03-19 11:29:29 PDT

Created attachment 132615 [details] updated_patch Modified patch as per review comments.

WebKit Review Bot

Comment 37 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.

Erik Arvidsson

Comment 38 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.

Kentaro Hara

Comment 39 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.

Vineet Chaudhary (vineetc)

Comment 40 2012-03-20 00:07:38 PDT

Created attachment 132773 [details] updated_patch

Kentaro Hara

Comment 41 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.

Vineet Chaudhary (vineetc)

Comment 42 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.

Vineet Chaudhary (vineetc)

Comment 43 2012-03-20 04:14:42 PDT

Created attachment 132792 [details] another_attempt

Kentaro Hara

Comment 44 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)'

Vineet Chaudhary (vineetc)

Comment 45 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.

Kentaro Hara

Comment 46 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.

Kentaro Hara

Comment 47 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()

Vineet Chaudhary (vineetc)

Comment 48 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()

Kentaro Hara

Comment 49 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.

Vineet Chaudhary (vineetc)

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

Kentaro Hara

Comment 51 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?

Vineet Chaudhary (vineetc)

Comment 52 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?

Kentaro Hara

Comment 53 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.

Vineet Chaudhary (vineetc)

Comment 54 2012-03-20 10:31:30 PDT

Created attachment 132850 [details] patch_for_review Done!!

Kentaro Hara

Comment 55 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!

Vineet Chaudhary (vineetc)

Comment 56 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.

WebKit Review Bot

Comment 57 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>

WebKit Review Bot

Comment 58 2012-03-20 11:27:17 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 59 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__

mitz

Comment 60 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.

Vineet Chaudhary (vineetc)

Comment 61 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.

Vineet Chaudhary (vineetc)

Comment 62 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>.

Erik Arvidsson

Comment 63 2012-04-24 14:26:34 PDT

Where are we trying to use sequence as an attribute?

Kentaro Hara

Comment 64 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.

Kentaro Hara

Comment 65 2012-04-24 14:33:38 PDT

Reopening the discussion

Erik Arvidsson

Comment 66 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.

Kentaro Hara

Comment 67 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?

Erik Arvidsson

Comment 68 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.

Kentaro Hara

Comment 69 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?

Erik Arvidsson

Comment 70 2012-04-24 15:18:16 PDT

(In reply to comment #69) > Is this expected? Sounds right to me.

Vineet Chaudhary (vineetc)

Comment 71 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?

Kentaro Hara

Comment 72 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;

David Levin

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

Vineet Chaudhary (vineetc)

Comment 74 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.

Kentaro Hara

Comment 75 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.

Zan Dobersek

Comment 76 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?

Adam Barth

Comment 77 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.

Note You need to log in before you can comment on or make changes to this bug.