Bug 74837 – REGRESSION(r101445): [JSC] Generated code for custom getters and setters with the [Supplemental] IDL is wrong

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kentaro Hara]

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);
    }

[Kentaro Hara]

Created attachment 119814 [details]
Patch

[Darin Adler]

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]

(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 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]

(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 on attachment 119814 [details]
Patch

Clearing flags on attachment: 119814

Committed r103221: <http://trac.webkit.org/changeset/103221>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.