Bug 69522 - Missing ifdef for some features in the V8 binding
Summary: Missing ifdef for some features in the V8 binding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-06 05:35 PDT by Rémi Duraffort
Modified: 2011-10-06 09:36 PDT (History)
3 users (show)

See Also:


Attachments
Fix compilation (3.69 KB, patch)
2011-10-06 05:42 PDT, Rémi Duraffort
no flags Details | Formatted Diff | Diff
Fix compilation (3.75 KB, patch)
2011-10-06 06:35 PDT, Rémi Duraffort
no flags Details | Formatted Diff | Diff
Fix compilation (2.40 KB, patch)
2011-10-06 06:59 PDT, Rémi Duraffort
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rémi Duraffort 2011-10-06 05:35:38 PDT
Some ifdef about VIDEO, JAVASCRIPT_DEBUGGER and XSLT are missing in the V8 binding files.
These unprotected files can lead to build failures.
Comment 1 Rémi Duraffort 2011-10-06 05:42:04 PDT
Created attachment 109950 [details]
Fix compilation

Fix the compilation if VIDEO, XSLT or JAVASCRIPT_DEBUGGER are not defined.
Comment 2 Rémi Duraffort 2011-10-06 06:35:06 PDT
Created attachment 109955 [details]
Fix compilation

Missing url to bugzilla.
Comment 3 Nayan Kumar K 2011-10-06 06:37:57 PDT
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?
Comment 4 Rémi Duraffort 2011-10-06 06:47:23 PDT
(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.
Comment 5 Rémi Duraffort 2011-10-06 06:59:26 PDT
Created attachment 109959 [details]
Fix compilation

Latest version: only fix the case of XSLT or VIDEO. (the last case belongs to another patch)
Comment 6 Nayan Kumar K 2011-10-06 07:30:54 PDT
> 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.
Comment 7 Adam Barth 2011-10-06 07:37:47 PDT
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.
Comment 8 WebKit Review Bot 2011-10-06 09:36:41 PDT
Comment on attachment 109959 [details]
Fix compilation

Clearing flags on attachment: 109959

Committed r96818: <http://trac.webkit.org/changeset/96818>
Comment 9 WebKit Review Bot 2011-10-06 09:36:46 PDT
All reviewed patches have been landed.  Closing bug.