Bug 87185 – [V8] Refactor generation code for non-standard functions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kent Tamura]

[V8] Refactor generation code for non-standard functions

[Kent Tamura]

Created attachment 143403 [details]
Patch

[Kentaro Hara]

Comment on attachment 143403 [details]
Patch

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

[Kent Tamura]

Comment on attachment 143403 [details]
Patch

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.

[Kent Tamura]

Created attachment 143415 [details]
Patch 2

[Kentaro Hara]

Comment on attachment 143415 [details]
Patch 2

Looks OK!

[Kent Tamura]

Comment on attachment 143415 [details]
Patch 2

Thank you!

[WebKit Review Bot]

Comment on attachment 143415 [details]
Patch 2

Clearing flags on attachment: 143415

Committed r118113: <http://trac.webkit.org/changeset/118113>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.