Removing #ifdefs

1,145 views
Skip to first unread message

Eric Seidel

unread,
Apr 5, 2013, 4:25:14 AM4/5/13
to blink-dev
I have created a spreadsheet for organizing our efforts to remove
#ifdefs from Blink:
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AidRaO7Awc-DdElvSU1ZRGlSdkh3eGtIYkhvVE5UbWc#gid=0

The list was generated by a small line of shell run from the WebCore directory:
grep -r "^#if ENABLE" * -h | tr '()' ' ' | awk '{print $3}' | sort |
uniq -c | sort -r

We need to document the default state of each ifdef before we can remove it.

Ideally when we're done we will have 0 ifdefs in Blink. I don't
expect to reach that goal, but I expect to come close.

Please feel encouraged to take ownership one and post a patch. I
would recommend starting with the less frequent ifdefs first.

I believe there must exist scripts to help us do this. I'm about to try:
http://dotat.at/prog/unifdef/
but if others have favorites, I'd love to hear about them.

Thanks.

Eric Seidel

unread,
Apr 5, 2013, 4:39:10 AM4/5/13
to blink-dev
unifdef looks like too big a hammer.

Here is a smaller script I wrote last time we had to do this (8 years
ago). Please forgive my perl:
https://bugs.webkit.org/show_bug.cgi?id=5877

Eric Seidel

unread,
Apr 5, 2013, 4:42:30 AM4/5/13
to blink-dev
This is the crux of that old perl:
http://www.phost.de/~stefan/Files/partpp.pl

And here is a list of existing chromium enable defaults:
https://chromium.googlesource.com/chromium/blink/+/master/Source/WebKit/chromium/features.gypi

Eric Seidel

unread,
Apr 5, 2013, 5:22:02 AM4/5/13
to blink-dev, Dan Bates, Ryosuke Niwa
Here is a fully working (hackish) script:
https://gist.github.com/eseidel/5317911

I've successfully used it to remove ENABLE(CSS_SHADERS) from the css directory.

(This is largely just resurrecting work from:
https://bugs.webkit.org/show_bug.cgi?id=5877)

It's quirky, and requires some hacking, but should get the job done.

Maybe someone with better perl skills than I could turn it into
something easier to use. :)

Jonathan Dixon

unread,
Apr 5, 2013, 5:25:48 AM4/5/13
to Eric Seidel, blink-dev
On 5 April 2013 01:25, Eric Seidel <ese...@google.com> wrote:
I have created a spreadsheet for organizing our efforts to remove
#ifdefs from Blink:
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AidRaO7Awc-DdElvSU1ZRGlSdkh3eGtIYkhvVE5UbWc#gid=0

The list was generated by a small line of shell run from the WebCore directory:
grep -r "^#if ENABLE" * -h | tr '()' ' ' | awk '{print $3}' | sort |
uniq -c | sort -r

We need to document the default state of each ifdef before we can remove it.

Ideally when we're done we will have 0 ifdefs in Blink.  I don't
expect to reach that goal, but I expect to come close.


Any guidelines on what could remain ifdef? e.g.
- everything that's already conditional in chromium (features.gypi lines 160 - 260)
- things we don't currently enable at all, but conceivably might at some point? MATHML comes to mind, but there are likely more
- other?

Some of those might turn into runtime-only switches? But that would presumably be a more detailed effort (take more than a mere perl script)

Eric Seidel

unread,
Apr 5, 2013, 5:39:18 AM4/5/13
to jo...@chromium.org, blink-dev
The goal is to remove #ifdefs and replace them with either:
- Always on code (INSPECTOR, CHROMIUM, SVG, etc.)
- Runtime flags (new features in development)
- Nothing (MAC, DASHBOARD, JSC, MATHML)

Features which we currently have off and are not under active
development should just be removed. We can always resurrect them from
svn history at a later date.

Features which are currently under active development should move to
runtime flags if they have not already. The #ifdefs are then
redundant and can be removed.

A large majority of WebKit's #ifdefs are for features which we've had
enabled for years, but never got around to removing the ifdef. :)

I expect we will have some #ifdefs after this, but I'd like to move
from our current 150 different feature flags, to something more like 5
feature #ifdefs and however many CPU/OS config flags are needed.

If any of these feel controversial, I would recommend leaving them for
now and we can come back in a later pass. We're still very much at
the low-hanging-fruit stage.

I clearly should have included PLATFORM and other non-ENABLE #ifdefs
in that spreadsheet, as those are likely much easier to remove.

Ilya Tikhonovsky

unread,
Apr 5, 2013, 9:11:45 AM4/5/13
to Eric Seidel, blink-dev, Dan Bates, Ryosuke Niwa
The script works almost fine.

I just added the case for ENABLE(*) into eval_unary which works the same way as the case for defined(*)

Also I fixed eval_and and now for complex statements like 
#if ENABLE(INSPECTOR) && ENABLE(WORKERS) 

it generates 

#if ENABLE(WORKERS) 

instead of 

#if 1 && ENABLE(WORKERS)



Regards,
Tim.
partpp.pl

John Mellor

unread,
Apr 5, 2013, 1:57:15 PM4/5/13
to Eric Seidel, jo...@chromium.org, blink-dev, Adam Barth
What about code which is stable but only used by some chromium platforms?

For example, Android uses ENABLE(TEXT_AUTOSIZING) to reformat desktop webpages so they are legible on phones and tablets. Currently (since trac.webkit.org/changeset/121907) this is compiled in on all Chromium platforms, but disabled at runtime on non-Android platforms. This has been very helpful in ensuring good test coverage (for example the Chromium Linux bots run the Text Autosizing Layout Tests). But it's conceivable that platforms like Chromium Mac which never run on phones or tablets might prefer to skip compiling it; or Opera, who don't use it even on mobile.

Text Autosizing is pretty lightweight, so in this case there would be negligible binary size saving from avoiding compiling it, and I suspect the benefits of having less compile flags outweigh the benefits of skipping compiling it; but perhaps for a larger component this would be less clear? What's our policy?

For context, see the patch proposing to remove this compile flag: https://codereview.chromium.org/13723004/

Cheers,
John

Adam Barth

unread,
Apr 5, 2013, 2:09:23 PM4/5/13
to John Mellor, Eric Seidel, jo...@chromium.org, blink-dev
The general approach we've been discussing is to use runtime flags rather than compile time flags for the majority of features.  The are a number of benefits to this approach, including testability, as you mention.  In some cases where there is a big impact to binary size or when a runtime flag has a measurable performance cost, we might want to use a compile-time flag.  In the case of text autosizing, the impact to binary size is very small and seems outweighed by being able to test the feature on every platform.

Adam

Shawn Singh

unread,
Apr 5, 2013, 2:27:14 PM4/5/13
to Adam Barth, John Mellor, Eric Seidel, jo...@chromium.org, blink-dev
I think there's a lot of #if USE(....) guards too, which we should consider removing, right?   For example, the one that has my is USE(ACCELERATED_COMPOSITING) in the core rendering/ code.

Ilya Tikhonovsky

unread,
Apr 5, 2013, 2:36:21 PM4/5/13
to Shawn Singh, Adam Barth, John Mellor, Eric Seidel, jo...@chromium.org, blink-dev

Regards,
Tim.

Nico Weber

unread,
Apr 6, 2013, 2:24:52 PM4/6/13
to Ilya Tikhonovsky, Shawn Singh, Adam Barth, John Mellor, Eric Seidel, Jonathan Dixon, blink-dev
If you're using this script, be aware that it produces the wrong output for #elifs, it deletes both #if and #elif body:

$ cat test.c
#if USE(CG)
typedef CGImageSourceRef NativeImageDecoderPtr;
#elif !PLATFORM(CHROMIUM)
class ImageDecoder;
typedef ImageDecoder* NativeImageDecoderPtr;
#endif

$  perl -I Tools/Scripts partpp.pl -UCG test.c > test-rewritten.c
#elif is not yet implemented, ignoring.

$ cat test-rewritten.c 
#elif !PLATFORM(CHROMIUM)

Eric Seidel

unread,
Jul 11, 2013, 2:42:46 PM7/11/13
to Nico Weber, Ilya Tikhonovsky, Shawn Singh, Adam Barth, John Mellor, Jonathan Dixon, blink-dev
Circling back on this.

We're very close to fulfilling this dream.

A quick grep of the source this morning shows only a few ENABLE_ macros left:
grep -r ENABLE\( * -h | grep -v "//" | sort | uniq -c | sort -r -n
(small amount of editing)

3 #if ENABLE(DEFAULT_RENDER_THEME)
3 #if !ENABLE(RUBBER_BANDING)
3 #if ENABLE(THREADING_GENERIC)
4 #if ENABLE(COMPARE_AND_SWAP)
4 #if !ENABLE(INPUT_MULTIPLE_FIELDS_UI)
4 #if ENABLE(PARTITION_ALLOC)
4 #if ENABLE(STREAM)
4 #if ENABLE(VERBOSE_STACK_STATS)
6 #if ENABLE(TCMALLOC_HARDENING)
6 #if ENABLE(TOUCH_ICON_LOADING)
8 #if ENABLE(ACCELERATED_OVERFLOW_SCROLLING)
8 #if ENABLE(NAVIGATOR_CONTENT_UTILS)
9 #if ENABLE(8BIT_TEXTRUN)
11 #if ENABLE(ORIENTATION_EVENTS)
12 #if ENABLE(ENCRYPTED_MEDIA_V2)
12 #if ENABLE(USERSELECT_ALL)
13 #if ENABLE(MEDIA_CAPTURE)
16 #if ENABLE(NOTIFICATIONS)
18 #if ENABLE(OPENTYPE_VERTICAL)
18 #if ENABLE(PAN_SCROLLING)
21 #if ENABLE(CALENDAR_PICKER)
21 #if ENABLE(LEGACY_NOTIFICATIONS)
21 #if ENABLE(WTF_MALLOC_VALIDATION)
24 #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
25 #if ENABLE(RUBBER_BANDING)
28 #if ENABLE(SVG)
42 #if ENABLE(WEBVTT_REGIONS)
52 #if ENABLE(CSS3_TEXT)
71 #if ENABLE(INPUT_SPEECH)
79 #if ENABLE(INPUT_MULTIPLE_FIELDS_UI)
100 #if ENABLE(SVG_FONTS)
120 #if ENABLE(WEB_AUDIO)

Eric Seidel

unread,
Jul 11, 2013, 2:47:00 PM7/11/13
to Nico Weber, Ilya Tikhonovsky, Shawn Singh, Adam Barth, John Mellor, Jonathan Dixon, blink-dev
Sorry, I had junk in my checkout. Those results are misleading.

Here is a better sample:

3 #if ENABLE(CUSTOM_SCHEME_HANDLER)
3 #if ENABLE(DEFAULT_RENDER_THEME)
3 #if ENABLE(THREADING_GENERIC)
4 #if ENABLE(COMPARE_AND_SWAP)
4 #if ENABLE(PARTITION_ALLOC)
4 #if ENABLE(STREAM)
4 #if ENABLE(VERBOSE_STACK_STATS)
5 #if ENABLE(MEDIA_CAPTURE)
6 #if ENABLE(TCMALLOC_HARDENING)
6 #if ENABLE(TOUCH_ICON_LOADING)
8 #if ENABLE(NAVIGATOR_CONTENT_UTILS)
11 #if ENABLE(ORIENTATION_EVENTS)
12 #if ENABLE(ENCRYPTED_MEDIA_V2)
12 #if ENABLE(USERSELECT_ALL)
16 #if ENABLE(NOTIFICATIONS)
18 #if ENABLE(OPENTYPE_VERTICAL)
18 #if ENABLE(PAN_SCROLLING)
21 #if ENABLE(CALENDAR_PICKER)
21 #if ENABLE(LEGACY_NOTIFICATIONS)
21 #if ENABLE(WTF_MALLOC_VALIDATION)
24 #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
25 #if ENABLE(RUBBER_BANDING)
42 #if ENABLE(WEBVTT_REGIONS)
44 #if ENABLE(CSS3_TEXT)
47 #if ENABLE(INPUT_SPEECH)
59 #if ENABLE(INPUT_MULTIPLE_FIELDS_UI)
96 #if ENABLE(SVG_FONTS)
120 #if ENABLE(WEB_AUDIO)
Reply all
Reply to author
Forward
0 new messages