JSBuiltinConstructor must always add builtin header

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

JSBuiltinConstructor must always add builtin header

[Xabier Rodríguez Calvar]

Created attachment 262345 [details]
Patch

If you compile the previously generated file with the initialization function ready in the .js file, compilation was failing because the header file couldn't be found. It is actually needed because the builtins include already the initialization function.

[youenn fablet]

LGTM

[youenn fablet]

Comment on attachment 262345 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262345&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5120
> +        return 1;

Just for style, returning 1 is not needed anymore.
It might be better to refactor the whole routine accordingly.

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> Comment on attachment 262345 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262345&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5120
> > +        return 1;
> 
> Just for style, returning 1 is not needed anymore.
> It might be better to refactor the whole routine accordingly.

I could rewrite to have a if-then and avoid that. I did it like this to pullute the patch less though it is not big deal, of course. What I can't avoid is the return inside the loop because it is supposed to bail out the first time it adds the header.

[Xabier Rodríguez Calvar]

(In reply to comment #4)
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5120
> > > +        return 1;
> > 
> > Just for style, returning 1 is not needed anymore.
> > It might be better to refactor the whole routine accordingly.
> 
> I could rewrite to have a if-then and avoid that. I did it like this to
> pollute the patch less though it is not big deal, of course. What I can't
> avoid is the return inside the loop because it is supposed to bail out the
> first time it adds the header.

Darin, wdyt about this? Should I change this when landing or do you like it as it is now?

[youenn fablet]

One possibility for the refactoring would be to transform AddIncludesForJSBuiltinMethods into something like NeedsJSBuiltinsInclude.

If you prefer to land this one directly, I can handle it in a follow-up patch.

[Xabier Rodríguez Calvar]

Comment on attachment 262345 [details]
Patch

(In reply to comment #6)
> One possibility for the refactoring would be to transform
> AddIncludesForJSBuiltinMethods into something like NeedsJSBuiltinsInclude.
> 
> If you prefer to land this one directly, I can handle it in a follow-up
> patch.

I can't do anything else than landing this since I'd need Darin's blessing for any other thing.

[WebKit Commit Bot]

Comment on attachment 262345 [details]
Patch

Clearing flags on attachment: 262345

Committed r190610: <http://trac.webkit.org/changeset/190610>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Filed https://bugs.webkit.org/show_bug.cgi?id=149837 as a follow-up