| Summary: | Unify the place where port features are listed | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hugo Parente Lima <hugo.lima> | ||||||||||
| Component: | Tools / Tests | Assignee: | Hugo Parente Lima <hugo.lima> | ||||||||||
| Status: | RESOLVED WONTFIX | ||||||||||||
| Severity: | Normal | CC: | dbates, dglazkov, eric, gtk-ews, gustavo, gyuyoung.kim, laszlo.gombos, luiz, mjs, paroga, peter+ews, rafael.lobo, rakuco, rwlbuis, tonikitoo, vestbo, webkit.review.bot, xan.lopez, yong.li.webkit | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Hugo Parente Lima
2012-09-20 12:36:22 PDT
More feature flags not on FeatureList.pm: ENABLE_DRAG_SUPPORT, ENABLE_FAST_MOBILE_SCROLLING. Another one ENABLE_JIT, on cmake but not on FeatureList.pm Created attachment 165176 [details]
Patch
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. This is very interesting, and very similar in goals to bug 85456. 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. Comment on attachment 165176 [details] Patch Attachment 165176 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13953742 (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. Comment on attachment 165176 [details] Patch Attachment 165176 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13975469 Comment on attachment 165176 [details] Patch Attachment 165176 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13935712 Comment on attachment 165176 [details] Patch Attachment 165176 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13951831 (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. (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. Created attachment 165210 [details]
Patch
Small fix on cmake regex
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. (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. Created attachment 165659 [details]
Patch
Added defaults for other ports based on what's on FeatureList.pm
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). (In reply to comment #15) > So, can I go forward with this? I would be interested in your replies to comment #5. :) 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? (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. (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. Created attachment 165865 [details]
Patch
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. 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 (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 (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. (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. So, what's the conclusion? Can I continue with this idea? I'm going to another approach: Try to solve the problem only for CMake based ports and upload a patch in another bug. Thanks. |