Bug 69522 – Missing ifdef for some features in the V8 binding

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

[Rémi Duraffort]

Some ifdef about VIDEO, JAVASCRIPT_DEBUGGER and XSLT are missing in the V8 binding files.
These unprotected files can lead to build failures.

[Rémi Duraffort]

Created attachment 109950 [details]
Fix compilation

Fix the compilation if VIDEO, XSLT or JAVASCRIPT_DEBUGGER are not defined.

[Rémi Duraffort]

Created attachment 109955 [details]
Fix compilation

Missing url to bugzilla.

[Nayan Kumar K]

Earlier I had submitted similar patch to resolve such compilation issue when some of the macro's are disabled - https://bugs.webkit.org/show_bug.cgi?id=63920. Based on the review comments, I seems like we need to generate these files all the time and have the macro embedded within the generated cpp|h files :(. While submitting the re-factored V8 porting patch, I will try to follow this approach mentioned by Alexey Proskuryakov in the in bug #63920.

BTW, Aren't the changes in V8ScriptProfileNode[Custom].cpp files the part of https://bugs.webkit.org/attachment.cgi?id=102530?

[Rémi Duraffort]

(In reply to comment #3)
> Based on the review comments, I seems like we need to generate these files all the time and have the macro embedded within the generated cpp|h files :(. While submitting the re-factored V8 porting patch, I will try to follow this approach mentioned by Alexey Proskuryakov in the in bug #63920.
These ifdefs does not comment out generated files but features that might no be enabled.
The files are always present but not the features.
I think that's meaning of the review in #63920: generate every files even if they are not used afterward and ifdef the features inside the files. That's what my page is doing IMHO.


> BTW, Aren't the changes in V8ScriptProfileNode[Custom].cpp files the part of https://bugs.webkit.org/attachment.cgi?id=102530?
Arg, yes they are already part of your initial patch (sorry I haven't checked these ones).
They should be a separate patch for me as that's a generical bug (the feature should be ifdefed).
I will recreate the patch without your part.

[Rémi Duraffort]

Created attachment 109959 [details]
Fix compilation

Latest version: only fix the case of XSLT or VIDEO. (the last case belongs to another patch)

[Nayan Kumar K]

> The files are always present but not the features.
> I think that's meaning of the review in #63920: generate every files even if they are not used afterward and ifdef the features inside the files. That's what my page is doing IMHO.
 
Yes, you are right. Sorry for the noise.

[Adam Barth]

Comment on attachment 109959 [details]
Fix compilation

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

> Source/WebCore/ChangeLog:6
> +        Unreviewed build fix

I mean, it's being reviewed, so you can leave the "Reviewed by NOBODY (OOPS!) line and the tools will fill it in for you.

[WebKit Review Bot]

Comment on attachment 109959 [details]
Fix compilation

Clearing flags on attachment: 109959

Committed r96818: <http://trac.webkit.org/changeset/96818>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.