RESOLVED FIXED 162677
Refactor binding generated casted-this attribute checks
https://bugs.webkit.org/show_bug.cgi?id=162677
Summary Refactor binding generated casted-this attribute checks
youenn fablet
Reported 2016-09-28 08:20:49 PDT
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.
Attachments
WIP (226.24 KB, patch)
2016-09-28 08:29 PDT, youenn fablet
no flags
Improving style (219.66 KB, patch)
2016-10-02 10:49 PDT, youenn fablet
no flags
Patch for landing (204.23 KB, patch)
2016-10-08 05:37 PDT, youenn fablet
no flags
youenn fablet
Comment 1 2016-09-28 08:29:02 PDT
youenn fablet
Comment 2 2016-10-02 10:49:20 PDT
Created attachment 290470 [details] Improving style
youenn fablet
Comment 3 2016-10-02 11:56:45 PDT
(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.
youenn fablet
Comment 4 2016-10-06 22:57:01 PDT
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.
youenn fablet
Comment 5 2016-10-06 23:11:53 PDT
EFL and GTK build issues are unrelated to that patch, the bots were broken at the upload time.
Darin Adler
Comment 6 2016-10-07 09:15:38 PDT
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.
youenn fablet
Comment 7 2016-10-08 05:37:50 PDT
Created attachment 291012 [details] Patch for landing
WebKit Commit Bot
Comment 8 2016-10-08 06:12:03 PDT
Comment on attachment 291012 [details] Patch for landing Clearing flags on attachment: 291012 Committed r206953: <http://trac.webkit.org/changeset/206953>
WebKit Commit Bot
Comment 9 2016-10-08 06:12:08 PDT
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 10 2016-10-08 07:08:35 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.