[webkit-reviews] review denied: [Bug 103162] [cmake] Unify the way default features are enabled/disable on CMake based ports. : [Attachment 181932] Patch

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.gombos at webkit.org> has denied Hugo Parente Lima
<hugo.lima at openbossa.org>'s request for review:
Bug 103162: [cmake] Unify the way default features are enabled/disable on CMake
based ports.
https://bugs.webkit.org/show_bug.cgi?id=103162

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

------- Additional Comments from Laszlo Gombos <laszlo.gombos at webkit.org>
It seems to me that this patch will introduce some regressions, r- for that.

> > Try running "${CMAKE_CXX_COMPILER} -E -dM -IWTF;. -DWTF_PLATFORM_XXX
"Features.h" and see what is the best way to process the output to populate
featureIndex.
> 
> But what about WinCE port? They use cmake too and probably not g++.

CMAKE_CXX_COMPILER is set to the MS Compiler by default on Windows which also
has a way to invoke the preprocessor -
http://msdn.microsoft.com/en-us/library/ms924239.aspx .

> "Why should I be able to set or see in the help message a Blackberry specific
option when compiling the Efl port?"

This is an ambitious goal. 

If this is the intention than I expect that you would need to detect all the
dependencies that are responsible for optional build options (e.g you should
only list ACCESSIBILITY is a webkit-build option for EFL if ATK is available in
the system) every time build-webkit --help runs (and repeat the detection when
the actual build is executed).

It might make sense to implement this functionality as a separate patch as I do
not think this is essential for what you're trying to do for this bug. It also
seems to me that your patch introduces a few regressions:

1./ build-webkit --efl defaults to ACCESSIBILITY OFF even though I have ATK in
the system. This seems to be a regression and it should default to ON when ATK
is available.

2./ build-webkit --wince no longer list SVG as a build option. I suspect (I
have not tried) that this patch will also make SVG turned OFF by default for
WinCE. This seems to be a regression as previously SVG was turned on by default
on all CMake based ports in WebKitFeatures.cmake.


The description of this bug is the following:

> After this patch you need to modify just one file to change the default value
of a WebKit feature.

It seems to me that after this patch if a feature is meant to be supported or
disabled for all 3 CMake based ports, it would need to be added to 3 files
(instead of 1 file as it was promised or instead of 2 files which is the
current practice). Can you perhaps restate the goal of this bug or maybe go
back to the original idea of sharing the defaults between CMake based ports ? 

> Source/cmake/OptionsBlackBerry.cmake:24
> -    add_definitions(-DWTF_USE_EXPORT_MACROS=1)
> +    SET_HARDCODED_DEFINE(WTF_USE_EXPORT_MACROS 1)

It seems to me that this will turn definitions that are passed to the compiler
on the command line into CMake variables that are listed in CMakeCache.txt and
will now be configuration option in tools like cmake-gui. Why ? Also why do you
need to add this information to cmakeconfig.h.in ?