[webkit-reviews] review denied: [Bug 102686] [V8] toFastV8 for non-Nodes : [Attachment 174973] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied	review:
Bug 102686: [V8] toFastV8 for non-Nodes
https://bugs.webkit.org/show_bug.cgi?id=102686

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

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


The patch looks great. Marking r- due to the code duplication for AccessorInfo
and Arguments.

> Source/WebCore/ChangeLog:3
> +	   [V8] toFastV8 for non-Nodes

I'm a bit confused with the title. Is this patch intending to use toV8Fast()
for a case where IsDOMNodeType($type) is true?

Nit: toV8Fast

> Source/WebCore/ChangeLog:8
> +	   This patch makes most generated bindings use toFastV8

Ditto.

> Source/WebCore/ChangeLog:10
> +	   Additionally, a toFastV8 for v8::Arguments was added

Ditto.

> Source/WebCore/ChangeLog:11
> +	   to make this most of thia optimization.

Typo: thia. Sorry for nit-pickings.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:536
> +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const
v8::Arguments& info, Node*)

Nit: We're likely to use 'args' instead of 'info'.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:539
> +
> +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const
v8::Arguments& info, Node*)
> +{
> +    return toV8(impl, info.Holder(), info.GetIsolate());
> +}

We might not want to duplicate this function for AccessorInfo and Arguments.
Wouldn't it be possible to change the signature to

  v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, v8::Handle<v8::Object>
holder, Node*);

and thus eliminate the duplication?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:588
> +	   foreach my $argType ("AccessorInfo","Arguments") {

Nit: "AccessorInfo", "Arguments" (One space after ,)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:620
> +inline v8::Handle<v8::Value> toV8Fast(PassRefPtr< ${nativeType} > impl,
const v8::Arguments& info, Node* holder)

Nit: We are likely to use 'args' instead of 'info'.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4144
> +    if ($getFastArgs) {

Don't you need to check IsDOMNodeType($type) ?