Bug 162677 – Refactor binding generated casted-this attribute checks

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

Created attachment 290084 [details]
WIP

[youenn fablet]

Created attachment 290470 [details]
Improving style

[youenn fablet]

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

EFL and GTK build issues are unrelated to that patch, the bots were broken at the upload time.

[Darin Adler]

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]

Created attachment 291012 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291012 [details]
Patch for landing

Clearing flags on attachment: 291012

Committed r206953: <http://trac.webkit.org/changeset/206953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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.