WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Improving style
(219.66 KB, patch)
2016-10-02 10:49 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(204.23 KB, patch)
2016-10-08 05:37 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2016-09-28 08:29:02 PDT
Created
attachment 290084
[details]
WIP
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.
Top of Page
Format For Printing
XML
Clone This Bug