Bug 105735 – Move ENABLE macros for WebCore out from Platform.h and add all the macros from the FeatureFlags wiki

• Component: Platform
• Status: RESOLVED FIXED

[Laszlo Gombos]

Move all ENABLE macros that are use in WebCore out from Platform.h and in the same file introduce all the other macros that are listed in the FeatureFlags wiki (http://trac.webkit.org/wiki/FeatureFlags).

First step for meta bug 105734 - see the description there for the larger motivation.

[Laszlo Gombos]

Created attachment 180699 [details]
1st take

Note that this is only a first step of some larger re-factoring.

I set the macros with no defaults in Platform.h - to "TBD". The value for TBD is not important at the moment as most build systems already define these macros, so these new rules in FeatureDefines.h will not execute. I hope to get rid of all these TBDs in a follow-up patch (I think most of these features can be just enabled by default and let the port specific build system disable them if they need to).

To get the list of all ENABLE macros one can just now run - grep -o "ENABLE_\(\w\+\)" wtf/FeatureDefines.h | sort | uniq.

To get the list of features enabled by default for a PLATFORM(XXX) assuming that the build system does not define them: gcc -E -dM -I. -DWTF_PLATFORM_XXX "wtf/Platform.h" | grep "ENABLE_\w\+ 1" | cut -d' ' -f2 | sort

[WebKit Review Bot]

Attachment 180699 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmak..." exit_code: 1
Source/WTF/wtf/FeatureDefines.h:283:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Laszlo Gombos]

Created attachment 180718 [details]
Fix style.

[Gyuyoung Kim]

Comment on attachment 180718 [details]
Fix style.

Looks fine on EFL port side. But, as we talk, other reviewers need to take a look this.

[Darin Adler]

Comment on attachment 180718 [details]
Fix style.

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

Seems like a step in the right direction. We do want to have the feature definition system separate from the platform detection system. What I had imagined was that each platform would have its own file listing every feature that it wanted to be different than the default, rather than having one central file with #if statements in it.

I don’t think that having a strict rule that every ENABLE has to be in FeatureDefines.h is quite right, though. It seems especially bad to move ENABLE_DEBUG_MATH_LAYOUT. Maybe we don’t want to use ENABLE() with that, but we should not pollute FeatureDefines.h with it.

> Source/WTF/wtf/FeatureDefines.h:56
> +/* ENABLE macro defaults for WebCore */

Something should say that these are in alphabetical order.

> Source/WTF/wtf/FeatureDefines.h:66
> +/* Add other platforms as they update their platfrom specific code to handle TextRun's with 8 bit data. */

Typo: platfrom

[Laszlo Gombos]

(In reply to comment #5)
> (From update of attachment 180718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180718&action=review

Thank a lot for the review Darin.
 
>  What I had imagined was that each platform would have its own file listing every feature that it wanted to be different than the default, rather than having one central file with #if statements in it.

I fully agree. I was planning on doing this work as a follow-up step. 

> I don’t think that having a strict rule that every ENABLE has to be in FeatureDefines.h is quite right, though. It seems especially bad to move ENABLE_DEBUG_MATH_LAYOUT. Maybe we don’t want to use ENABLE() with that, but we should not pollute FeatureDefines.h with it.

I had similar concerns writing the patch. I think it is best to not use ENABLE() for debug guards. I ended up including it just for completeness to match the list from the wiki page. I will remove this change from this patch before landing and will rename the macro instead as a separate patch.

> > Source/WTF/wtf/FeatureDefines.h:56
> > +/* ENABLE macro defaults for WebCore */
> 
> Something should say that these are in alphabetical order.

At the beginning of the FeatureDefines.h I put some languages that speak to this:

"Note that the file is not necessary sorted by the name of the macros to allow inter-dependencies between the defaults." 

I will add some language to indicate to keep it sorted as much as possible.

[Laszlo Gombos]

Created attachment 183298 [details]
patch for landing (review comments addressed)

[Kent Tamura]

Comment on attachment 183298 [details]
patch for landing (review comments addressed)

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

> Source/WTF/wtf/FeatureDefines.h:55
> +#define TBD 0

nit: The name 'TBD' is too short for the global namespace.  We had better rename it or #undef it at the bottom of the file.

[Laszlo Gombos]

Created attachment 184875 [details]
more cleanup

I decided to make some more changes instead of landing the previously r+ patch from Darin as there was no urgency to land. I removed TBD and come up with sensible defaults for the defines (for most of the defines this is 0).

I also structured the file so that each port has its own section that can be later moved out to a port specific file.

[Build Bot]

Comment on attachment 184875 [details]
more cleanup

Attachment 184875 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16119944

[Laszlo Gombos]

Created attachment 184899 [details]
disable progress element by default

PROGRESS_ELEMENT is disabled for PLATFORM(WIN) && !OS(WINCE)

[Benjamin Poulain]

Comment on attachment 184899 [details]
disable progress element by default

Any chance to autogenerate FeatureDefines.h?
It would be nice to split dependencies between features from platform configuration.

What about, having 2 config file:
1) Defining the dependencies between features.
2) Give a list of each feature for each port.

A little script reads the two, and output the FeatureDefines.h for the current port.

The (only?) advantage would be readability.

[Laszlo Gombos]

(In reply to comment #12)
> (From update of attachment 184899 [details])

Thanks for the review Benjamin !

> Any chance to autogenerate FeatureDefines.h?

I think this is an idea worth exploring. I would like to do this work as a separate patch as this patch is already large in size and I think we can reasonably expect that auto generation would spark a longer discussions (which also ties into the "align build systems" discussion).
 
> It would be nice to split dependencies between features from platform configuration.
> 
> What about, having 2 config file:
> 1) Defining the dependencies between features.
> 2) Give a list of each feature for each port.

As follow-up work I would like to separate the port specific rules into port specific files - but this would need buy in from each individual ports as they would have to maintain these files later on.

> The (only?) advantage would be readability.

My hope was that we do not hold up this work until we have consensus on how to improve the readability of the code further. I think the patch is an improvement over the current state in its current form.

[Laszlo Gombos]

The patch introduces wtf/FeatureDefines.h which is conceptually the same as default/Port.h as described on webkit-dev in a mail from Maciej - https://lists.webkit.org/pipermail/webkit-dev/2013-January/023542.html .

[Benjamin Poulain]

Comment on attachment 184899 [details]
disable progress element by default

I have nothing against this, but seriously this is putting lipstick on a pig.

I think it would be easy and convenient to auto-generate the header from simple config files.
I hope you are not gonna land this and go directly for a clean approach. If not, this patch is better than nothing.

[Adam Barth]

Comment on attachment 184899 [details]
disable progress element by default

Please don't land this patch.  Comments to follow.

[Adam Barth]

Comment on attachment 184899 [details]
disable progress element by default

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

> Source/WTF/wtf/FeatureDefines.h:340
> +/* --------- Chromium port (Windows, Max, Unix) --------- */
> +#if PLATFORM(CHROMIUM)
> +
> +/* On OS(DARWIN) for PLATFORM(CHROMIUM) we can't override the global operator new and delete
> + * because some object are allocated by WebKit and deallocated by the embedder. */
> +#if OS(DARWIN)
> +#if !defined(ENABLE_GLOBAL_FASTMALLOC_NEW)
> +#define ENABLE_GLOBAL_FASTMALLOC_NEW 0
> +#endif
> +#endif
> +
> +#if !defined(ENABLE_JAVASCRIPT_I18N_API)
> +#define ENABLE_JAVASCRIPT_I18N_API 1
> +#endif
> +
> +#if !defined(ENABLE_SUBPIXEL_LAYOUT)
> +#define ENABLE_SUBPIXEL_LAYOUT 1
> +#endif
> +
> +#endif /* PLATFORM(CHROMIUM) */

Deleting these ENABLE macros from Platform.h is great!  However, rather than defining those macros in this header, the correct thing to do for the Chromium port is to move them to http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/features.gypi with all of their friends.

(Sorry for the brief message earlier.  I was on a conference calls and I wanted to look at this patch in more detail before it landed.)

[Laszlo Gombos]

Created attachment 188783 [details]
for cq with Adam's feedback + rebase

[WebKit Review Bot]

Comment on attachment 188783 [details]
for cq with Adam's feedback + rebase

Clearing flags on attachment: 188783

Committed r143211: <http://trac.webkit.org/changeset/143211>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Brianceau]

This patch breaks our sh4 build bot: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/20363

It appears that it also breaks the Qt Linux Release build too: http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/72399

[Ádám Kallai]

It broke QtWebKit build on Mountain Lion:

http://build.webkit.sed.hu/builders/x86-64%20Mountain%20Lion%20Qt%20Release%20%28WebGL%20Tester%29/builds/2732/steps/compile-webkit/logs/stdio

[Laszlo Gombos]

Created attachment 188920 [details]
build fix when NETSCAPE_PLUGIN_API is disabled

[Chris Dumez]

Committed r143248: <http://trac.webkit.org/changeset/143248>