Bug 97250 – Unify the place where port features are listed

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Hugo Parente Lima]

Currently there are at least two places to list and toggle webkit features, some features are listed in one place but not in the other and vice versa, the known places are: Tools/Scripts/webkitperl/FeatureList.pm (used by build-webkit script) and cmake/WebKitFeatures.cmake, qt now use it own method to get the feature list, so I believe it doesn't use FeatureList.pm default values anymore (please correct me if I'm wrong).

Some examples features in one place but not in the other:

ENABLE_CSS_IMAGE_SET, ENABLE_LLINT, ENABLE_MEMORY_SAMPLER, ENABLE_GLIB_SUPPORT, ENABLE_LEGACY_VIEWPORT_ADAPTION is on cmake but not on FeatureList.pm

Also it's a bit confusing when you need to add a new feature flag, because you need to know all places to add this feature, it's redundant and error prone.

Other problem (with cmake based ports) is if you set the default value of a flag on cmake but this flag is listed on FeatureList.pm, the default value on FeatureList.pm will overwrite the cmake one, in other words when adding a feature you need to set the default value on both, cmake and FeatureList.pm, and keep them in sync.

My proposal is to have just two files, one listing all possible webkit features and other per-port listing the default (to true) features for this port, then FeatureList.pm and the cmake files create the feature list and their default values based on this two files. No sync, no forgotten feature, no mess.

[Hugo Parente Lima]

More feature flags not on FeatureList.pm: ENABLE_DRAG_SUPPORT, ENABLE_FAST_MOBILE_SCROLLING.

[Hugo Parente Lima]

Another one ENABLE_JIT, on cmake but not on FeatureList.pm

[Hugo Parente Lima]

Created attachment 165176 [details]
Patch

[Hugo Parente Lima]

The patch was tested just with the Efl port, because it's easy to get it's default features, so I need a list of default features from other ports before thinking about land this patch.

The Qt port isn't affected, because it doesn't use FeatureList.pm to get the default values.

[Eric Seidel (no email)]

This is very interesting, and very similar in goals to bug 85456.

[Eric Seidel (no email)]

As you'll note, the current FeatureDefines.pm was even generated by the script attached to bug 85456.

I'm supportive of either of these approaches.

[WebKit Review Bot]

Comment on attachment 165176 [details]
Patch

Attachment 165176 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13953742

[Patrick R. Gansterer]

(In reply to comment #0)
> My proposal is to have just two files, one listing all possible webkit features and other per-port listing the default (to true) features for this port...
IMHO we need a possibility to set the features to false too in the port-specific files.

[kov's GTK+ EWS bot]

Comment on attachment 165176 [details]
Patch

Attachment 165176 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13975469

[Build Bot]

Comment on attachment 165176 [details]
Patch

Attachment 165176 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13935712

[Peter Beverloo (cr-android ews)]

Comment on attachment 165176 [details]
Patch

Attachment 165176 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13951831

[Hugo Parente Lima]

(In reply to comment #8)
> (In reply to comment #0)
> > My proposal is to have just two files, one listing all possible webkit features and other per-port listing the default (to true) features for this port...
> IMHO we need a possibility to set the features to false too in the port-specific files.

They are all default to false, then each port says just what should be turned ON by default, so to set a default to false just do nothing.

[Patrick R. Gansterer]

(In reply to comment #12)
> (In reply to comment #8)
> > (In reply to comment #0)
> > > My proposal is to have just two files, one listing all possible webkit features and other per-port listing the default (to true) features for this port...
> > IMHO we need a possibility to set the features to false too in the port-specific files.
> 
> They are all default to false, then each port says just what should be turned ON by default, so to set a default to false just do nothing.

Yes, but then 90% of the FeatureFiles.txt are the same for all ports, which I do not like to see. I'd suggest a DefaultFeatures.txt from which the individual ports can turn features on/off.

[Hugo Parente Lima]

Created attachment 165210 [details]
Patch

Small fix on cmake regex

[Hugo Parente Lima]

So, can I go forward with this?

I can use the Patrick R. Gansterer suggestion and transform the FeatureDefaultsPORT.txt in a key/value file like:

ENABLE_FOO_BAR = ON
ENABLE_BAR = OFF

And any missing flag will be considered OFF by default.

[Tor Arne Vestbø]

(In reply to comment #15)
> So, can I go forward with this?

Please hold on for a bit, I'd like to check how this fits with the Qt port.

Although we currently have our feature defaults in features.pri, that file were supposed to be auto-generated from FeaturesList.pm, once bug 85456 landed.

[Hugo Parente Lima]

Created attachment 165659 [details]
Patch

Added defaults for other ports based on what's on FeatureList.pm

[Patrick R. Gansterer]

I still don't see "general default values". I suggest an additional column in FeatureDescription.txt which contains the default value. E.g SVG is enabled across all ports. Then only the port specific differences against the "general defaults" need to be added to the other txt files (with =ON/OFF).

[Eric Seidel (no email)]

(In reply to comment #15)
> So, can I go forward with this?

I would be interested in your replies to comment #5. :)

[Tor Arne Vestbø]

Comment on attachment 165659 [details]
Patch

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

The way the Qt port works is similar to how this patch changes the CMake ports to work. We don't rely on build-webkit to pass in the features. Instead we go through the following steps:

  1. Read baseline features from features.pri
      These are features that are either a) always enabled or disabled, or b) disabled until we can detect platform support.
  2. Check for platform support in features.prf, and selectively enable features that were disabled by 1b.
  3. Read overrides from the command line (passed by build-webkit if you do eg --no-svg)
  4. Do some basic sanity checking, like disabling SVG fonts if SVG is disabled.

A few things to note:

  1. features.pri is a static list, but was prepared specifically to be auto-generated by the patch from bug 85456, based on Features.py
  2. features.pri needs to have a complete list of all feature defines, not just the ones that are enabled by default.

The reason for point 2 is twofold:

  a) Some of the binding generators will barf unless they are passed --defines "ENABLE_FOO=0" for every disabled define.
     This is something we should probably fix.
  b) Platform.h has some defaults of it's own, that will be enabled unless you pass -DENABLE_FOO=0, eg:

#if !defined(ENABLE_ICONDATABASE)
#define ENABLE_ICONDATABASE 1
#endif

Now, regarding the patch I think we can change the Qt buildsystem to do the same as CMake does here; that is, reading FeatureDescription.txt (for the complete set of features) plus FeatureDefaultsQt.txt, instead of features.pri, but it probably requires a small perl-script to translate from the txt format to something that qmake understands (eg the syntax already in features.pri).

What I'm missing here is a shared list of default features, like Patrick requests, so that common features like SVG won't be duplicated in each of the port files. I'm also wondering if we should remove any sort of default from Platform.h if the feature has been plumbed up to build-webkit. Finallly, I don't see how this patch adresses build system that can't read a list of features at build time (Xcode, eg). We'll still have redundancy between FeatureDefaultsAppleMacWebkit.txt and the Xcode file if I understand this correctly?

> Tools/Scripts/webkitperl/FeatureDescription.txt:22
> +3d-rendering               ENABLE_3D_RENDERING                  Toggle 3D Rendering support

The "Toggle" (and perhaps "support") is a bit redundant. Perhaps it can can be added by build-webkit when outputting --help ?

> Tools/Scripts/webkitperl/FeatureDescription.txt:108
> +system-malloc              USE_SYSTEM_MALLOC                    Toggle system allocator instead of TCmalloc

Shouldn't this be WTF_USE_SYSTEM_MALLOC?

[Hugo Parente Lima]

(In reply to comment #5)
> This is very interesting, and very similar in goals to bug 85456.

Hi

They have the same goal, the difference resides in the fact that you try to solve it using a script that need to be called when a new feature default is added/removed. It's good because if some port dislike the idea it can just ignore it and everything still work.

My original plan was to have just one location with the feature list per port in a "easy to read format" then build-webkit script and each build system can read it and create the same list of features at runtime, so there wont have difference in the enabled features if you compile the port using build-webkit script of call qmake/cmake directly.

The cons of my patch:
- I known nothing about Mac buildsystem, so I didn't think about it before writing the patch.
- Too many people involved.
- It's not 100% a opt in patch, theoretically it is, because if nobody does nothing the build-webkit script should return the same set of feature as before.

The cons of your patch:
- It doesn't really solve the problem but is a nice workaround, we still having n places to change, but at least we can use a script to generate some of these files.

[Hugo Parente Lima]

(In reply to comment #20)
> A few things to note:
> 
>   1. features.pri is a static list, but was prepared specifically to be auto-generated by the patch from bug 85456, based on Features.py
>   2. features.pri needs to have a complete list of all feature defines, not just the ones that are enabled by default.

This can be done, and it address the request from Patrick plus having a FeatureDefaultCommon.txt.
 
> The reason for point 2 is twofold:
> 
>   a) Some of the binding generators will barf unless they are passed --defines "ENABLE_FOO=0" for every disabled define.
>      This is something we should probably fix.
>   b) Platform.h has some defaults of it's own, that will be enabled unless you pass -DENABLE_FOO=0, eg:
> 
> #if !defined(ENABLE_ICONDATABASE)
> #define ENABLE_ICONDATABASE 1
> #endif
> 
> Now, regarding the patch I think we can change the Qt buildsystem to do the same as CMake does here; that is, reading FeatureDescription.txt (for the complete set of features) plus FeatureDefaultsQt.txt, instead of features.pri, but it probably requires a small perl-script to translate from the txt format to something that qmake understands (eg the syntax already in features.pri).
> 
> What I'm missing here is a shared list of default features, like Patrick requests, so that common features like SVG won't be duplicated in each of the port files. I'm also wondering if we should remove any sort of default from Platform.h if the feature has been plumbed up to build-webkit. Finallly, I don't see how this patch adresses build system that can't read a list of features at build time (Xcode, eg). We'll still have redundancy between FeatureDefaultsAppleMacWebkit.txt and the Xcode file if I understand this correctly?
> 
> > Tools/Scripts/webkitperl/FeatureDescription.txt:22
> > +3d-rendering               ENABLE_3D_RENDERING                  Toggle 3D Rendering support
> 
> The "Toggle" (and perhaps "support") is a bit redundant. Perhaps it can can be added by build-webkit when outputting --help ?

It's possible to guess all feature descriptions from their names, this would make things easy, because instead of a FeatureDescription.txt we would just have a FeaturesCommon.txt with a simple key-value list like:

ENABLE_3D_RENDERING = 0

And each port overwriting the features in port specific files.
 
> > Tools/Scripts/webkitperl/FeatureDescription.txt:108
> > +system-malloc              USE_SYSTEM_MALLOC                    Toggle system allocator instead of TCmalloc
> 
> Shouldn't this be WTF_USE_SYSTEM_MALLOC?

May be, I just got with I found on FeatureList.pm on WebKit trunk, just WinCE enable this feature and I don't know if WinCE is a dead port or not.

[Hugo Parente Lima]

Created attachment 165865 [details]
Patch

[Hugo Parente Lima]

New version: using FeaturesCommon.txt, with the default values.
description and command line switches are created based on the feature name, in each port specific file you can enable/disable a feature as Patrick suggested, the file is also much similar to the features.pri used by qmake.

[Patrick R. Gansterer]

Comment on attachment 165865 [details]
Patch

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

> Source/cmake/OptionsEfl.cmake:52
> +SET(ENABLE_API_TESTS ON)

you remove the "./configure" option this way, since you hardcode it now

> Tools/Scripts/webkitperl/FeatureDefaultsAppleWebKit.txt:24
> +ENABLE_SVG = 1

why do you need to set it here too? isn't it done in FeaturesCommon.txt already?

> Tools/Scripts/webkitperl/FeaturesCommon.txt:94
> +WTF_USE_SYSTEM_MALLOC                = 0

1) why not sort ENABLE_ before WTF_
2) you can't simply change it to WTF_ (WinCE port isn't dead!) see http://trac.webkit.org/browser/trunk/Source/WTF/wtf/FastMalloc.cpp?rev=129319#L97

[Hugo Parente Lima]

(In reply to comment #25)
> (From update of attachment 165865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165865&action=review
> 
> > Source/cmake/OptionsEfl.cmake:52
> > +SET(ENABLE_API_TESTS ON)

This doesn't fit on the feature list, besides is impossible do set this value using build-webkit script, so the only way to set it OFF is to call cmake directly, but you are right in the sense that it should be replaced by:

WEBKIT_OPTION_DEFINE(...)
 
> you remove the "./configure" option this way, since you hardcode it now
> 
> > Tools/Scripts/webkitperl/FeatureDefaultsAppleWebKit.txt:24
> > +ENABLE_SVG = 1
> 
> why do you need to set it here too? isn't it done in FeaturesCommon.txt already?

I don't need, the patch still as a proposal, so I didn't spend much time polishing FeatureDefaults* files of port I not able to compile.
 
> > Tools/Scripts/webkitperl/FeaturesCommon.txt:94
> > +WTF_USE_SYSTEM_MALLOC                = 0
> 
> 1) why not sort ENABLE_ before WTF_

It's just a matter of sort the file.

> 2) you can't simply change it to WTF_ (WinCE port isn't dead!) see http://trac.webkit.org/browser/trunk/Source/WTF/wtf/FastMalloc.cpp?rev=129319#L97

A typo, maybe after trying things after Tor Arne comments about this flag, thanks for pointing it out.
Ok

[Laszlo Gombos]

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

>   b) Platform.h has some defaults of it's own, that will be enabled unless you pass -DENABLE_FOO=0, eg:
> 
> #if !defined(ENABLE_ICONDATABASE)
> #define ENABLE_ICONDATABASE 1
> #endif

Build rules in Platform.h have a great advantage - they are cross-platform and cross-port - shared by all build systems the project supports. I wonder if we should also consider a direction to solve the "Unify the place where port features are listed" that would put the defines and its defaults into a header file. 

> More feature flags not on FeatureList.pm: ENABLE_DRAG_SUPPORT, ENABLE_FAST_MOBILE_SCROLLING.

Is it a goal to offer a way to set/unset all feature flags individually for all ports ? I am not sure if build-webkit should really offer to build configurations that are not necessary valid/supported.

[Hugo Parente Lima]

(In reply to comment #27)
> (In reply to comment #20)
> > (From update of attachment 165659 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=165659&action=review
> 
> >   b) Platform.h has some defaults of it's own, that will be enabled unless you pass -DENABLE_FOO=0, eg:
> > 
> > #if !defined(ENABLE_ICONDATABASE)
> > #define ENABLE_ICONDATABASE 1
> > #endif
> 
> Build rules in Platform.h have a great advantage - they are cross-platform and cross-port - shared by all build systems the project supports. I wonder if we should also consider a direction to solve the "Unify the place where port features are listed" that would put the defines and its defaults into a header file. 

At least for cmake based ports all end in a header file included by config.h (cmakeconfig.h), but this header should be generated by the build system anyway to enable/disable the features chosen at configure time.

[Hugo Parente Lima]

So, what's the conclusion? Can I continue with this idea?

[Hugo Parente Lima]

I'm going to another approach: Try to solve the problem only for CMake based ports and upload a patch in another bug.

Thanks.