V8 generated code is missing include of V8BindingMacros.h for STRING_TO_V8PARAMETER_EXCEPTION_BLOCK

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kristian Monsen]

Created attachment 66711 [details]
Proposed patch

The header does not get included through other headers on Android.

Introduced in this CL:
http://trac.webkit.org/changeset/66664/trunk/WebCore/bindings/scripts/CodeGeneratorV8.pm

[Steve Block]

The required include of V8BindingMacros.h was lost in http://trac.webkit.org/changeset/66521

[Steve Block]

Comment on attachment 66711 [details]
Proposed patch

> +        Compile fix for Android. Explicitly add needed header for
> +        STRING_TO_V8PARAMETER_EXCEPTION_BLOCK as they do not get included through
> +        other headers on Android.
Need a bug URL

Remember to set r? and cq? if you want the patch to be reviewed and added to the commit-queue.

[Kristian Monsen]

Created attachment 66715 [details]
Updated patch with Bug#

[Kristian Monsen]

(In reply to comment #2)
> (From update of attachment 66711 [details])
> > +        Compile fix for Android. Explicitly add needed header for
> > +        STRING_TO_V8PARAMETER_EXCEPTION_BLOCK as they do not get included through
> > +        other headers on Android.
> Need a bug URL
> 
> Remember to set r? and cq? if you want the patch to be reviewed and added to the commit-queue.

I was not yet ready for review, as I was looking for where it got introduced. Thanks for noticing the missing bug url.

[Steve Block]

Comment on attachment 66715 [details]
Updated patch with Bug#

Is the include of V8BindingMacros.h from GenerateNormalAttrSetter() still required?

[Kristian Monsen]

(In reply to comment #5)
> (From update of attachment 66715 [details])
> Is the include of V8BindingMacros.h from GenerateNormalAttrSetter() still required?

This is the CL that actually caused the problem:
http://trac.webkit.org/changeset/66521/trunk/WebCore/bindings/scripts/CodeGeneratorV8.pm

I grepped through the generated files, and it seems like the header is never included twice at least.

[Steve Block]

> I grepped through the generated files, and it seems like the header is never
> included twice at least.
Sure, but this doesn't mean that we don't add the header to the implIncludes map more than once.

[anton muhin]

(In reply to comment #7)
> > I grepped through the generated files, and it seems like the header is never
> > included twice at least.
> Sure, but this doesn't mean that we don't add the header to the implIncludes map more than once.

Sorry guys for introducing this.  Could we setup BBs to watch for Android breakage?

[anton muhin]

Just in case: I've added more cases to use the macro.  I hope you already have necessary includes on those paths.

[Steve Block]

> Sorry guys for introducing this.  Could we setup BBs to watch for Android
> breakage?
No problem. We'd love to have an Android BB, but we have a lot of upstreaming to do first!

> Just in case: I've added more cases to use the macro.  I hope you already have
> necessary includes on those paths.
It looks like the macro is only used from ConvertToV8Parameter(), to which this patch adds the include, so I think we're OK.

Anton, do you know if the include from GenerateNormalAttrSetter() can be removed, or are you happy with this patch as it is?

[anton muhin]

(In reply to comment #10)
> > Just in case: I've added more cases to use the macro.  I hope you already have
> > necessary includes on those paths.
> It looks like the macro is only used from ConvertToV8Parameter(), to which this patch adds the include, so I think we're OK.
> 
> Anton, do you know if the include from GenerateNormalAttrSetter() can be removed, or are you happy with this patch as it is?

Yes, you're right.  I think it's the best place for inclusion and we don't want to specialize unless build time is prohibitive (which should not be the case, hopefully)

[WebKit Commit Bot]

Comment on attachment 66715 [details]
Updated patch with Bug#

Clearing flags on attachment: 66715

Committed r66888: <http://trac.webkit.org/changeset/66888>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.