Bug 74837

Summary: REGRESSION(r101445): [JSC] Generated code for custom getters and setters with the [Supplemental] IDL is wrong
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore Misc.Assignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, japhet, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 74599    
Attachments:
Description Flags
Patch none

Kentaro Hara
Reported 2011-12-18 22:11:20 PST
In bug 73162, we implemented the [Supplemental] IDL, but the generated code for custom getters and setters is wrong in JSC. In WebCore/bindings/scripts/test/JS/JSTestInterface.cpp, Wrong: JSValue jsTestInterfaceStr3(ExecState* exec, JSValue slotBase, const Identifier&) { JSTestInterface* castedThis = static_cast<JSTestInterface*>(asObject(slotBase)); return JSTestSupplemental::str3(castedThis, exec); } Correct: JSValue jsTestInterfaceStr3(ExecState* exec, JSValue slotBase, const Identifier&) { JSTestInterface* castedThis = static_cast<JSTestInterface*>(asObject(slotBase)); TestInterface* imp = static_cast<TestInterface*>(castedThis->impl()); return castedThis->str3(imp, exec); }
Attachments
Patch (10.49 KB, patch)
2011-12-18 22:22 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Kentaro Hara
Comment 1 2011-12-18 22:22:02 PST
Created attachment 119814 [details] Patch
Darin Adler
Comment 2 2011-12-18 22:25:00 PST
Comment on attachment 119814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119814&action=review > Source/WebCore/ChangeLog:25 > + TestInterface* imp = static_cast<TestInterface*>(castedThis->impl()); > + return castedThis->str3(imp, exec); Why both imp and impl? How about just using one abbreviation in both places.
Kentaro Hara
Comment 3 2011-12-18 22:33:19 PST
(In reply to comment #2) > (From update of attachment 119814 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119814&action=review > > > Source/WebCore/ChangeLog:25 > > + TestInterface* imp = static_cast<TestInterface*>(castedThis->impl()); > > + return castedThis->str3(imp, exec); > > Why both imp and impl? How about just using one abbreviation in both places. Darin: Yeah, we should unify their names. Since we cannot rename castedThis->impl() easily, we should rename 'imp' to 'impl'. But it appears that there are already so many 'imp' in CodeGeneratorJS.pm. Is it OK to rename it in another patch?
Darin Adler
Comment 4 2011-12-18 22:47:54 PST
Comment on attachment 119814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119814&action=review >>> Source/WebCore/ChangeLog:25 >>> + return castedThis->str3(imp, exec); >> >> Why both imp and impl? How about just using one abbreviation in both places. > > Darin: Yeah, we should unify their names. Since we cannot rename castedThis->impl() easily, we should rename 'imp' to 'impl'. But it appears that there are already so many 'imp' in CodeGeneratorJS.pm. Is it OK to rename it in another patch? To me it looked like the new imp was entirely new and easy to change to impl before it even landed. But if you’d like to do that in another patch along with changes to lots of existing cases of imp too it’s OK with me.
Kentaro Hara
Comment 5 2011-12-18 22:51:16 PST
(In reply to comment #4) > (From update of attachment 119814 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119814&action=review > To me it looked like the new imp was entirely new and easy to change to impl before it even landed. But if you’d like to do that in another patch along with changes to lots of existing cases of imp too it’s OK with me. Darin: Thanks, I'll rename them all at once in the next patch.
WebKit Review Bot
Comment 6 2011-12-18 23:16:17 PST
Comment on attachment 119814 [details] Patch Clearing flags on attachment: 119814 Committed r103221: <http://trac.webkit.org/changeset/103221>
WebKit Review Bot
Comment 7 2011-12-18 23:16:21 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.