Bug 87185

Summary: [V8] Refactor generation code for non-standard functions
Product: WebKit Reporter: Kent Tamura <tkent@chromium.org>
Component: WebCore Misc.Assignee: Kent Tamura <tkent@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, haraken@chromium.org, japhet@chromium.org, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 87086    
Attachments:
Description Flags
Patch
none
Patch 2 none

Description From 2012-05-22 17:04:13 PST
[V8] Refactor generation code for non-standard functions
------- Comment #1 From 2012-05-22 17:11:13 PST -------
Created an attachment (id=143403) [details]
Patch
------- Comment #2 From 2012-05-22 17:22:59 PST -------
(From update of attachment 143403 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143403&action=review

IsStandardFunction() looks good, but I am neutral to the refactoring of GenerateNonStandrdFunction(). It seems that GenerateNonStandrdFunction() "just" factors out a code, which would not be so beneficial.

> Source/WebCore/ChangeLog:11
> +        (IsStandardFunction): Introduce a new functio to check if a

Nit: function

> Source/WebCore/ChangeLog:15
> +        (GenerateImplementation): Use IsStandardFunction and GenerateNonStandrdFunction.

Nit: GenerateNonStandardFunction.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2127
> +sub GenerateNonStandrdFunction

GenerateStandardFunction

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206
> +        die "This shouldn't happen: " . IsStandardFunction($function) . "\n";

How does this message help? IsStandardFunction($function) would just output 0 or 1?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-2721
> -            $num_callbacks++;

This line seems to be lost in your patch.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2762
> +        next if IsStandardFunction($dataNode, $function);

Why is this check needed? It seems to be already checked above.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2763
> +        GenerateNonStandrdFunction($dataNode, $function);

GenerateNonStandardFunction
------- Comment #3 From 2012-05-22 17:44:46 PST -------
(From update of attachment 143403 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=143403&action=review

> IsStandardFunction() looks good, but I am neutral to the refactoring of GenerateNonStandrdFunction(). It seems that GenerateNonStandrdFunction() "just" factors out a code, which would not be so beneficial.

I need to add callsite to GenerateNonStandardFunction() in Bug 87086.

>> Source/WebCore/ChangeLog:11
>> +        (IsStandardFunction): Introduce a new functio to check if a
> 
> Nit: function

Done.

>> Source/WebCore/ChangeLog:15
>> +        (GenerateImplementation): Use IsStandardFunction and GenerateNonStandrdFunction.
> 
> Nit: GenerateNonStandardFunction.

Oops, correct all of instances.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206
>> +        die "This shouldn't happen: " . IsStandardFunction($function) . "\n";
> 
> How does this message help? IsStandardFunction($function) would just output 0 or 1?

I'll change it so that it prints $commentInfo.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-2721
>> -            $num_callbacks++;
> 
> This line seems to be lost in your patch.

In my new code, $num_callbacks++ is always called except for standard functions.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2762
>> +        next if IsStandardFunction($dataNode, $function);
> 
> Why is this check needed? It seems to be already checked above.

Because we need to skip standard functions.  The old code skipped standrad functions after some tasks for non-standard functions.

       if ($template eq "proto" && $conditional eq "" && $signature eq "defaultSignature" && $property_attributes eq "") {
            # Standard type of callback, already created in the batch, so skip it here.
            next;
       }
    # This is equivalent to IsStandardFunction().

Skipping the standard function in such deep place is not good for code readability.
The foreach loop for @{$dataNode->functions} above in this function is for standard functions, and
This foreach loop for @{$dataNode->functions} in this function is for non-standard functions.
The role of each foreach loops becomes clear.
------- Comment #4 From 2012-05-22 17:47:53 PST -------
Created an attachment (id=143415) [details]
Patch 2
------- Comment #5 From 2012-05-22 17:53:36 PST -------
(From update of attachment 143415 [details])
Looks OK!
------- Comment #6 From 2012-05-22 17:56:51 PST -------
(From update of attachment 143415 [details])
Thank you!
------- Comment #7 From 2012-05-22 20:55:59 PST -------
(From update of attachment 143415 [details])
Clearing flags on attachment: 143415

Committed r118113: <http://trac.webkit.org/changeset/118113>
------- Comment #8 From 2012-05-22 20:56:04 PST -------
All reviewed patches have been landed.  Closing bug.