[JSC] Refactoring CodeGeneratorJS.pm to simplify adding support for overloaded constructors
Created attachment 179911 [details] Patch
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.
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.
Created attachment 179916 [details] Patch
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.
Comment on attachment 179916 [details] Patch LGTM
Comment on attachment 179916 [details] Patch Clearing flags on attachment: 179916 Committed r138008: <http://trac.webkit.org/changeset/138008>
All reviewed patches have been landed. Closing bug.