This would make the binding code more readable. This might also allow in the future to disable this check for private functions, where this is redundant since usually already done by the caller.
Created attachment 290084 [details] WIP
Created attachment 290470 [details] Improving style
(In reply to comment #0) > This would make the binding code more readable. > This might also allow in the future to disable this check for private > functions, where this is redundant since usually already done by the caller. This might also allow simplifying the handling of promise-returning methods.
Comment on attachment 290470 [details] Improving style View in context: https://bugs.webkit.org/attachment.cgi?id=290470&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2851 > + } I tried here to stop mixing generator logic with generating code. I think it improves the readability to have such big chunks of outputted code without nested if/else. These chunks could later be moved to a template-dedicated file, like done for the built-in generator. This would allow to make the logic of the generator easier to understand. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2896 > + push(@implContent, "static inline JSValue ${getFunctionName}Getter(ExecState* state)\n"); > + push(@implContent, "{\n"); Ditto here.
EFL and GTK build issues are unrelated to that patch, the bots were broken at the upload time.
Comment on attachment 290470 [details] Improving style View in context: https://bugs.webkit.org/attachment.cgi?id=290470&action=review Change looks OK to me as long as it is performance neutral. Sadly this will conflict with my changes to CodeGeneratorJS.pm, so whoever goes second will need to merge. > Source/WebCore/ChangeLog:80 > + (WebCore::JSTestInterface::castForAttribute): > + (WebCore::jsTestInterfaceConstructorImplementsStaticReadOnlyAttr): > + (WebCore::jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter): > + (WebCore::jsTestInterfaceConstructorImplementsStaticAttr): > + (WebCore::jsTestInterfaceConstructorImplementsStaticAttrGetter): > + (WebCore::jsTestInterfaceImplementsStr1): > + (WebCore::jsTestInterfaceImplementsStr1Getter): > + (WebCore::jsTestInterfaceImplementsStr2): > + (WebCore::jsTestInterfaceImplementsStr2Getter): > + (WebCore::jsTestInterfaceImplementsStr3): > + (WebCore::jsTestInterfaceImplementsStr3Getter): > + (WebCore::jsTestInterfaceImplementsNode): > + (WebCore::jsTestInterfaceImplementsNodeGetter): > + (WebCore::jsTestInterfaceConstructorSupplementalStaticReadOnlyAttr): > + (WebCore::jsTestInterfaceConstructorSupplementalStaticReadOnlyAttrGetter): > + (WebCore::jsTestInterfaceConstructorSupplementalStaticAttr): > + (WebCore::jsTestInterfaceConstructorSupplementalStaticAttrGetter): > + (WebCore::jsTestInterfaceSupplementalStr1): > + (WebCore::jsTestInterfaceSupplementalStr1Getter): > + (WebCore::jsTestInterfaceSupplementalStr2): > + (WebCore::jsTestInterfaceSupplementalStr2Getter): > + (WebCore::jsTestInterfaceSupplementalStr3): > + (WebCore::jsTestInterfaceSupplementalStr3Getter): > + (WebCore::jsTestInterfaceSupplementalNode): > + (WebCore::jsTestInterfaceSupplementalNodeGetter): These lists of function names are not useful for the bindings expected results. Please delete them to make the change log readable.
Created attachment 291012 [details] Patch for landing
Comment on attachment 291012 [details] Patch for landing Clearing flags on attachment: 291012 Committed r206953: <http://trac.webkit.org/changeset/206953>
All reviewed patches have been landed. Closing bug.
Thanks for the review. > Change looks OK to me as long as it is performance neutral. Sadly this will > conflict with my changes to CodeGeneratorJS.pm, so whoever goes second will > need to merge. Hope the merge will not be too much trouble. > > Source/WebCore/ChangeLog:80 > > + (WebCore::JSTestInterface::castForAttribute): > > + (WebCore::jsTestInterfaceConstructorImplementsStaticReadOnlyAttr): > > + (WebCore::jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter): > > + (WebCore::jsTestInterfaceConstructorImplementsStaticAttr): > > + (WebCore::jsTestInterfaceConstructorImplementsStaticAttrGetter): > > + (WebCore::jsTestInterfaceImplementsStr1): > > + (WebCore::jsTestInterfaceImplementsStr1Getter): > > + (WebCore::jsTestInterfaceImplementsStr2): > > + (WebCore::jsTestInterfaceImplementsStr2Getter): > > + (WebCore::jsTestInterfaceImplementsStr3): > > + (WebCore::jsTestInterfaceImplementsStr3Getter): > > + (WebCore::jsTestInterfaceImplementsNode): > > + (WebCore::jsTestInterfaceImplementsNodeGetter): > > + (WebCore::jsTestInterfaceConstructorSupplementalStaticReadOnlyAttr): > > + (WebCore::jsTestInterfaceConstructorSupplementalStaticReadOnlyAttrGetter): > > + (WebCore::jsTestInterfaceConstructorSupplementalStaticAttr): > > + (WebCore::jsTestInterfaceConstructorSupplementalStaticAttrGetter): > > + (WebCore::jsTestInterfaceSupplementalStr1): > > + (WebCore::jsTestInterfaceSupplementalStr1Getter): > > + (WebCore::jsTestInterfaceSupplementalStr2): > > + (WebCore::jsTestInterfaceSupplementalStr2Getter): > > + (WebCore::jsTestInterfaceSupplementalStr3): > > + (WebCore::jsTestInterfaceSupplementalStr3Getter): > > + (WebCore::jsTestInterfaceSupplementalNode): > > + (WebCore::jsTestInterfaceSupplementalNodeGetter): > > These lists of function names are not useful for the bindings expected > results. Please delete them to make the change log readable. Done.