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: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | 81345, 81705 | ||||||||||||||||||||||||||||
Bug Blocks: | 80269, 84929 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Arko Saha
2012-03-09 04:33:45 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 on attachment 131028 [details] proposed patch Attachment 131028 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11894590 Comment on attachment 131028 [details] proposed patch Attachment 131028 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11898567 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. (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. > 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;
(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? Created attachment 131568 [details]
WIP Patch
Changes made to work with V8 too.
Comment on attachment 131568 [details] WIP Patch Attachment 131568 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11945388 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? 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 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. (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; 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. (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>. 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 Created attachment 132024 [details]
wip_patch_001
Modified the patch to include results of the run-bindings-tests.
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 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> ? Sequence<ScriptProfile> (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. (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. > Is it Sequence<ScriptProfile> or sequence<ScriptProfile> ? You're right: sequence<T> http://www.w3.org/TR/WebIDL/#idl-sequence 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 on attachment 132458 [details] patch Attachment 132458 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11967819 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. (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. Created attachment 132575 [details]
patch
Patch as per above review comments.
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.
(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 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. (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. (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. (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 (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. Created attachment 132615 [details]
updated_patch
Modified patch as per review comments.
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 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 on attachment 132615 [details]
updated_patch
The patch looks good to me except for arv@'s comments.
Created attachment 132773 [details]
updated_patch
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 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. Created attachment 132792 [details]
another_attempt
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)' (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. (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 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() (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() (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. 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 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? (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? (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. Created attachment 132850 [details]
patch_for_review
Done!!
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!
(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 on attachment 132850 [details] patch_for_review Clearing flags on attachment: 132850 Committed r111416: <http://trac.webkit.org/changeset/111416> All reviewed patches have been landed. Closing bug. 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. (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. 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>. Where are we trying to use sequence as an attribute? (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. Reopening the discussion 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. (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? 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. (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? (In reply to comment #69) > Is this expected? Sounds right to me. (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? 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; (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). (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. (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. Given the patch landed and the regression was fixed in a separate bug, should this bug be closed? 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. |