Bug 105271

Summary: [JSC] Refactoring CodeGeneratorJS.pm to simplify adding support for overloaded constructors
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: Tools / TestsAssignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ggaren, haraken, japhet, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 103226    
Attachments:
Description Flags
Patch
none
Patch none

Tommy Widenflycht
Reported 2012-12-18 02:38:26 PST
[JSC] Refactoring CodeGeneratorJS.pm to simplify adding support for overloaded constructors
Attachments
Patch (43.22 KB, patch)
2012-12-18 02:44 PST, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
Patch (43.42 KB, patch)
2012-12-18 03:45 PST, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Tommy Widenflycht
Comment 1 2012-12-18 02:44:34 PST
Created attachment 179911 [details] Patch
Tommy Widenflycht
Comment 2 2012-12-18 02:46:19 PST
This patch splits the very large function that generates constructor code into a few smaller ones. No changes in actual generated code but some functions in the generated bindings code moves.
Kentaro Hara
Comment 3 2012-12-18 02:57:38 PST
Comment on attachment 179911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179911&action=review Looks OK. Added minor comments. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3728 > + if (IsConstructable($interface) && !IsCustomConstructable($interface)) { Why is this check needed? It looks like we don't have this check now. If this check is really needed, it would make more sense to check it inside GenerateConstructorDefinition(), because GenerateConstructorDefinition() already has checks about it. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3899 > +sub GenerateConstructorSupportDefinitions Maybe GenerateConstructorHelperMethods() is a better name? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3993 > +sub IsCustomConstructable Let's rename to HasCustomConstructor() to keep the name consistent between JSC and V8.
Tommy Widenflycht
Comment 4 2012-12-18 03:45:39 PST
Created attachment 179916 [details] Patch
Tommy Widenflycht
Comment 5 2012-12-18 03:47:00 PST
Comment on attachment 179911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179911&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3728 >> + if (IsConstructable($interface) && !IsCustomConstructable($interface)) { > > Why is this check needed? It looks like we don't have this check now. If this check is really needed, it would make more sense to check it inside GenerateConstructorDefinition(), because GenerateConstructorDefinition() already has checks about it. Removed check. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3899 >> +sub GenerateConstructorSupportDefinitions > > Maybe GenerateConstructorHelperMethods() is a better name? Done. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3993 >> +sub IsCustomConstructable > > Let's rename to HasCustomConstructor() to keep the name consistent between JSC and V8. Done.
Kentaro Hara
Comment 6 2012-12-18 03:48:22 PST
Comment on attachment 179916 [details] Patch LGTM
WebKit Review Bot
Comment 7 2012-12-18 04:08:50 PST
Comment on attachment 179916 [details] Patch Clearing flags on attachment: 179916 Committed r138008: <http://trac.webkit.org/changeset/138008>
WebKit Review Bot
Comment 8 2012-12-18 04:08:54 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.