Bug 87185

Created attachment 143403 [details] Patch

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

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.

Created attachment 143415 [details] Patch 2

Comment on attachment 143415 [details] Patch 2 Looks OK!

Comment on attachment 143415 [details] Patch 2 Thank you!

Comment on attachment 143415 [details] Patch 2 Clearing flags on attachment: 143415 Committed r118113: <http://trac.webkit.org/changeset/118113>

All reviewed patches have been landed. Closing bug.