[webkit-reviews] review denied: [Bug 87965] Need customized toDOMStringList for IndexedDB JSC binding. : [Attachment 147030] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Charles Wei
<charles.wei at torchmobile.com.cn>'s request for review:
Bug 87965: Need customized toDOMStringList for IndexedDB JSC binding.
https://bugs.webkit.org/show_bug.cgi?id=87965

Attachment 147030: Patch
https://bugs.webkit.org/attachment.cgi?id=147030&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=147030&action=review


> Source/WebCore/ChangeLog:18
> +	   * bindings/js/JSDOMBinding.h:
> +	   (WebCore):

This file is missing.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:291
> +    if (!isJSArray(value))
> +	   return 0;

> V8NativeObject::HasInstance(V8Value)	seems V8-specific logic. I don't see
corresponding JSC code in any of generated JSC binding code or custom JSC
binding code. Please point 
> to me if there's any that I should follow in JSC.

V8DOMStringList::HasInstance(value) corresponds to
value.inherits(&JSDOMStringList::s_info)

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:204
> +    return 1 if $type eq "CSSStyleDeclaration" or $type eq "MediaList" or
$type eq "DOMStringList" or $type eq "DOMStringList[]"	or $type eq
"DOMTokenList" or $type eq "DOMSettableTokenList";

Shouldn't this be $type eq "DOMString[]"?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1024
> +	   } elsif ($interfaceName eq "DOMStringList") {
> +	       push(@headerContent, "PassRefPtr<DOMStringList>
toDOMStringList(JSC::ExecState*, JSC::JSValue);\n");

This would be no longer needed. The declaration should be in JSDOMBinding.h.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2830
> -    "DOMString[]" => "DOMStringList*",
> +    "DOMString[]" => "RefPtr<DOMStringList>",

Why do you need to change DOMStringList* to RefPtr<DOMStringList>?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2995
>      if ($type eq "DOMString[]") {
>	   AddToImplIncludes("JSDOMStringList.h", $conditional);
> -	   return "toDOMStringList($value)";
> +	   return "toDOMStringList(exec, $value)";
> +    }
> +
> +    if ($type eq "DOMStringList") {
> +	   AddToImplIncludes("JSDOMStringList.h", $conditional);
> +	   return "toDOMStringList(exec, $value)";
>      }

You can wrap up two if statements into one.