[Webkit-unassigned] [Bug 102686] [V8] toFastV8 for non-Nodes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=102686


Kentaro Hara <haraken at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #174973|                            |review-
               Flag|                            |




--- Comment #5 from Kentaro Hara <haraken at chromium.org>  2012-11-19 17:17:22 PST ---
(From update of attachment 174973)
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) ?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.