Bug 89070 – [JSC/V8] Improve DOMString[] support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kent Tamura]

If we use DOMString[] for
 - an argument of a non-overload function
 - an return value of a function,
our binding generators produce wrong code.

[Kent Tamura]

Created attachment 147510 [details]
Patch

[Kent Tamura]

Created attachment 147512 [details]
Patch 2

Remove useless comment

[Adam Barth]

Looks reasonable, but I'd like haraken to take a look.

[Kentaro Hara]

Comment on attachment 147512 [details]
Patch 2

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

Looks reasonable to me too.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:153
> +    if (!stringList)
> +        return jsUndefined();

Just a confirmation: You are intending to return (not [] but) undefined for a null stringList, right?

[Vineet Chaudhary (vineetc)]

Comment on attachment 147512 [details]
Patch 2

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-3075
> -        if (!$codeGenerator->SkipIncludeHeader($arrayType)) {

tkent: can we add "DOMString[]" to SkipIncludeHeader() ..? because I think this will fail for numerictypes like float[]. I mean will include float.h  to cpp files.

[Kent Tamura]

Comment on attachment 147512 [details]
Patch 2

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

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:153
>> +        return jsUndefined();
> 
> Just a confirmation: You are intending to return (not [] but) undefined for a null stringList, right?

Oh, this should be null to match V8 binding behavior.  I'll change it so.

Thanks.

[Kent Tamura]

Comment on attachment 147512 [details]
Patch 2

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-3075
>> -        if (!$codeGenerator->SkipIncludeHeader($arrayType)) {
> 
> tkent: can we add "DOMString[]" to SkipIncludeHeader() ..? because I think this will fail for numerictypes like float[]. I mean will include float.h  to cpp files.

ok, will do.

[Vineet Chaudhary (vineetc)]

(In reply to comment #7)
> (From update of attachment 147512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147512&action=review
> 
> >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-3075
> >> -        if (!$codeGenerator->SkipIncludeHeader($arrayType)) {
> > 
> > tkent: can we add "DOMString[]" to SkipIncludeHeader() ..? because I think this will fail for numerictypes like float[]. I mean will include float.h  to cpp files.
> 
> ok, will do.

Ahh sorry tkent previous comment you are handling that in elsif.

[Kent Tamura]

Comment on attachment 147512 [details]
Patch 2

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

>>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-3075
>>>> -        if (!$codeGenerator->SkipIncludeHeader($arrayType)) {
>>> 
>>> tkent: can we add "DOMString[]" to SkipIncludeHeader() ..? because I think this will fail for numerictypes like float[]. I mean will include float.h  to cpp files.
>> 
>> ok, will do.
> 
> Ahh sorry tkent previous comment you are handling that in elsif.

ok, no problem.

Anyway, we might not need to include JSDOMStringList.h and DOMStringList.h.

[Kent Tamura]

(In reply to comment #9)
> Anyway, we might not need to include JSDOMStringList.h and DOMStringList.h.

I'll investigate this later.

[Kent Tamura]

Created attachment 147517 [details]
Patch for landing

undefined -> null

[Kent Tamura]

Committed r120304: <http://trac.webkit.org/changeset/120304>

[Kent Tamura]

(In reply to comment #6)
> (From update of attachment 147512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147512&action=review
> 
> >> Source/WebCore/bindings/js/JSDOMBinding.cpp:153
> >> +        return jsUndefined();
> > 
> > Just a confirmation: You are intending to return (not [] but) undefined for a null stringList, right?
> 
> Oh, this should be null to match V8 binding behavior.  I'll change it so.

I was incorrect.  We doesn't have V8 binding code to convert a DOMStringList to v8 array.  I looked a wrong code.
Probably we should have different behavior for "DOMString[]" and "DOMString[]?".

[Kentaro Hara]

(In reply to comment #13)
> I was incorrect.  We doesn't have V8 binding code to convert a DOMStringList to v8 array.  I looked a wrong code.
> Probably we should have different behavior for "DOMString[]" and "DOMString[]?".

Yes. DOMString[] is not nullable, but DOMString[]? is nullable: http://www.w3.org/TR/WebIDL/#idl-nullable-type

[Kent Tamura]

Ok, code generators should produce v8ArrayOrNull() / jsArrayOrNull() for "DOMString[]?".  So, jsArray() / v8Array() should not return null/undefined.
I'll make a patch.