[webkit-reviews] review granted: [Bug 85746] Add values for all features to Qt's features.pri : [Attachment 140434] Patch

[bugzilla-daemon at webkit.org]

Tor Arne Vestbø <vestbo at webkit.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 85746: Add values for all features to Qt's features.pri
https://bugs.webkit.org/show_bug.cgi?id=85746

Attachment 140434: Patch
https://bugs.webkit.org/attachment.cgi?id=140434&action=review

------- Additional Comments from Tor Arne Vestbø <vestbo at webkit.org>
The patch looks good! 

Having an exhaustive list in features.pri is fine, the way things work right
now is that we:

 1. Parse command line options (that as passed to qmake from build-webkit, eg
--enable-foo turns into DEFINES+=ENABLE_FOO=1
 2. Do dynamic feature checking based on optional dependencies (which might add
eg, ENABLE_VIDEO=1 to DEFINES, if the right libs are available). 
 3. Fill in any un-set features from the defaults in features.pri

This is a bit reverse, but that' just how it has to be done right now due to
qmake weirdness :)

So basically, if the dynamic check for eg. video fails, it will then end up
using the default in features.pri, which is ENABLE_VIDEO=0, since it's off by
default.

So for the Qt port (and Chromium) the list in Features.py really is a tri-state
(on/off/auto), with off/auto baked into one. I think that's fine as long as the
list in Features.py is not be used to say "oh, no port has this feature
enabled, we can remove it", since we might actually enable it dynamically. So
perhaps the script should have some sort of warning about making decisions
solely based on the output of the script?