Removing #ifdefs

Showing 1-14 of 14 messages
Removing #ifdefs Eric Seidel 4/5/13 1:25 AM
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.
Re: Removing #ifdefs Eric Seidel 4/5/13 1:39 AM
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
Re: Removing #ifdefs Eric Seidel 4/5/13 1:42 AM
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
Re: Removing #ifdefs Eric Seidel 4/5/13 2:22 AM
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. :)
Re: [blink-dev] Removing #ifdefs Jonathan Dixon 4/5/13 2:25 AM



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)



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.

Re: [blink-dev] Removing #ifdefs Eric Seidel 4/5/13 2:39 AM
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.
Re: [blink-dev] Re: Removing #ifdefs Ilya Tikhonovsky 4/5/13 6:11 AM
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.
Re: [blink-dev] Removing #ifdefs John Mellor 4/5/13 10:57 AM
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
Re: [blink-dev] Removing #ifdefs Adam Barth 4/5/13 11:09 AM
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

Re: [blink-dev] Removing #ifdefs Shawn Singh 4/5/13 11:27 AM
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.
Re: [blink-dev] Removing #ifdefs Ilya Tikhonovsky 4/5/13 11:36 AM

Regards,
Tim.
Re: [blink-dev] Removing #ifdefs Nico Weber 4/6/13 11:24 AM
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)

Re: [blink-dev] Removing #ifdefs Eric Seidel 7/11/13 11:42 AM
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)
Re: [blink-dev] Removing #ifdefs Eric Seidel 7/11/13 11:47 AM
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)