Bug 185253 – [WPE] Fix build for ENABLE_TOUCH_EVENTS=OFF

• Component: WPE WebKit
• Status: RESOLVED WONTFIX

[Pablo Saavedra]

Build with no-touch-events fails:

    Tools/Scripts/build-webkit --release --wpe --no-touch-events

[Pablo Saavedra]

Created attachment 339408 [details]
patch

[Chris Dumez]

Comment on attachment 339408 [details]
patch

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

> Source/WebKit/ChangeLog:6
> +        This commit is a port of r190987 committed for GTK platform

This should come below the review by line.

> Source/WebKit/ChangeLog:7
> +        (http://trac.webkit.org/changeset/190987) to WPE

Missing period at the end.

> Source/WebKit/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

This should be above the commit message.

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:34
> +#if ENABLE(TOUCH_EVENTS)

Should be moved below other includes:
#include <wpe/wpe.h>

#if ENABLE(TOUCH_EVENTS)
#include "NativeWebTouchEvent.h"
#endif

> Tools/ChangeLog:6
> +        This commit is a port of r190987 committed for GTK platform

Same comments as for other changelog.

[Michael Catanzaro]

Comment on attachment 339408 [details]
patch

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

One more style nit:

> Source/WebKit/Shared/wpe/NativeWebTouchEventWPE.cpp:29
> +#if ENABLE(TOUCH_EVENTS)

Leave one blank line below here...

> Source/WebKit/Shared/wpe/NativeWebTouchEventWPE.cpp:48
> +#endif // ENABLE(TOUCH_EVENTS)

...and one above here.

[Pablo Saavedra]

Created attachment 339414 [details]
patch

[Pablo Saavedra]

Created attachment 339415 [details]
patch

[Michael Catanzaro]

Comment on attachment 339415 [details]
patch

Ooops, one more thing. I notice that ENABLE_TOUCH_EVENTS is actually forced on in OptionsWPE.cmake:

WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_TOUCH_EVENTS PRIVATE ON)

If we want to allow disabling touch support, that should be changed from PRIVATE to PUBLIC, and moved to the list of public options. And Zan should be the one to approve that.

I guess there's not really any strong reason to support disabling touch events currently, because it doesn't add any new dependency.

[Pablo Saavedra]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 339415 [details]
> patch
> 
> Ooops, one more thing. I notice that ENABLE_TOUCH_EVENTS is actually forced
> on in OptionsWPE.cmake:
> 
> WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_TOUCH_EVENTS PRIVATE ON)
> 
> If we want to allow disabling touch support, that should be changed from
> PRIVATE to PUBLIC, and moved to the list of public options. And Zan should
> be the one to approve that.
> 
> I guess there's not really any strong reason to support disabling touch
> events currently, because it doesn't add any new dependency.

I agree with your point.

... but curiously, this is not working as someone can expect: 

* Build latest Webkit with:

    Tools/Scripts/build-webkit --release --wpe  --no-touch-events 

  This generates a cmakeconfig which honors the --no-touch-events flag:

    #define ENABLE_TOUCH_EVENTS 0
    #define ENABLE_TOUCH_SLIDER 0

  (in the WebKitBuild/Release/cmakeconfig.h)


    //Toggle Touch Events support
    ENABLE_TOUCH_EVENTS:BOOL=OFF

    //Toggle Touch Slider support
    ENABLE_TOUCH_SLIDER:BOOL=OFF

  (in WebKitBuild/Release/CMakeCache.txt)

Not sure about which is the reason of this buggy behavior. Anyway, for correctness and consistency:

These are the default values:

grep -r -e "(ENABLE_TOUCH_EVENTS "  *
cmake/OptionsGTK.cmake:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_TOUCH_EVENTS PUBLIC ON)
cmake/WebKitFeatures.cmake:    WEBKIT_OPTION_DEFINE(ENABLE_TOUCH_EVENTS "Toggle Touch Events support" PRIVATE OFF)
cmake/OptionsMac.cmake:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_TOUCH_EVENTS PRIVATE OFF)
cmake/OptionsWPE.cmake:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_TOUCH_EVENTS PRIVATE ON)

Should I also change `cmake/OptionsWPE.cmake` as follows?: 

  cmake/OptionsWPE.cmake:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_TOUCH_EVENTS PUBLIC ON)

[Michael Catanzaro]

(In reply to Pablo Saavedra from comment #7)
> * Build latest Webkit with:
> 
>     Tools/Scripts/build-webkit --release --wpe  --no-touch-events 

Ah, those flags are not actually expected to work. The build-webkit level features list (FeatureList.pm) contains a subset of features that is not kept in sync with the real features list (in WebKitFeatures.cmake), and changing the values of the options it exposes is almost never supported. It'd be good to remove all these build-webkit flags to avoid confusion.
 
> Should I also change `cmake/OptionsWPE.cmake` as follows?: 

Yes:

(In reply to Michael Catanzaro from comment #6)
> If we want to allow disabling touch support, that should be changed from
> PRIVATE to PUBLIC, and moved to the list of public options. And Zan should
> be the one to approve that.

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 339415 [details]
> patch
> 
> Ooops, one more thing. I notice that ENABLE_TOUCH_EVENTS is actually forced
> on in OptionsWPE.cmake:
> 
> WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_TOUCH_EVENTS PRIVATE ON)
> 
> If we want to allow disabling touch support, that should be changed from
> PRIVATE to PUBLIC, and moved to the list of public options. And Zan should
> be the one to approve that.
> 
> I guess there's not really any strong reason to support disabling touch
> events currently, because it doesn't add any new dependency.

Sorry, I don't agree with you at all

This comes from the meta-wpe and meta-webkit recipe that provide this as configurable, as many other options that are not cmake public options.

One thing is fixing the build with a private option disabled, and another one is moving the build option from public to private.

Following your reasoning we should remove all the private cmake config options. I don't buy that, neither I think is a good idea.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #9)

> One thing is fixing the build with a private option disabled, and another
> one is moving the build option from public to private.
> 
......... ^ I meant: "and another one is moving the build option from private to public."

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #9)
> Following your reasoning we should remove all the private cmake config
> options. I don't buy that, neither I think is a good idea.

There's no point in having option visibility at all if we ignore the rules. It's simply not practical to expect that arbitrary ports build with arbitrary configurations of the 100+ private options defined in WebKitFeatures.cmake.

It's definitely wrong for meta-webkit to expose PRIVATE options. If the options are interesting to expose, they should almost surely be made PUBLIC. (Again, this should be approved by Zan.) Otherwise, they should be removed from meta-webkit.

If you want to support disabling ENABLE_TOUCH_EVENTS for WPE, you should make it PUBLIC. Otherwise, it's IMO wrong to add ENABLE_TOUCH_EVENT conditions in WPE-specific files. In the past, we've actively removed such conditions after discovering that the option is PRIVATE. Having such conditions creates the false impression that both codepaths are supported.

Exposing options should be done with care; we have way too many with GTK, and don't want to repeat that mistake for WPE. Previously, for WPE we've often taken the approach that options should be PRIVATE unless making them PUBLIC allows disabling a dependency (e.g. ENABLE_XSLT). In contrast, it's not clear why you would ever want to disable touch event support. Don't say "because the target device has no touchscreen," because disabling ENABLE_TOUCH_EVENTS doesn't avoid any dependency, and there's hardly any code supporting it, so the impact on binary size is probably unnoticeable, so there's probably no good motivation to disable it even if touch events will never be generated. There should be a good reason to expose such options. (That said, I don't have a strong opinion about this; if Zan wants ENABLE_TOUCH_EVENTS to be PUBLIC, then that's fine with me.)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #11)
> (That said, I don't have a strong opinion about this; if Zan wants
> ENABLE_TOUCH_EVENTS to be PUBLIC, then that's fine with me.)

Basically the tradeoffs I see to exposing it are:

* Benefits: none(?)

* Cost: one more configuration that can (and, obviously, will) often fail to build

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #12)
> (In reply to Michael Catanzaro from comment #11)
> > (That said, I don't have a strong opinion about this; if Zan wants
> > ENABLE_TOUCH_EVENTS to be PUBLIC, then that's fine with me.)
> 
> Basically the tradeoffs I see to exposing it are:
> 
> * Benefits: none(?)
> 
> * Cost: one more configuration that can (and, obviously, will) often fail to
> build

I have never argued to make this option public.

There is currently a build option (which is private), and building with it its broken. And here is a more than reasonable patch to fix that breakage.

I don't know why you are mixing one thing with the other.

If we aren't even going to accept patches for fixing the breakage for changing the value of private options, why having this private options at all?  What is the point? 

Why we don't convert all private options to unconditional definitions that can't be changed of value?

[Pablo Saavedra]

(In reply to Michael Catanzaro from comment #12)
> (In reply to Michael Catanzaro from comment #11)
> > (That said, I don't have a strong opinion about this; if Zan wants
> > ENABLE_TOUCH_EVENTS to be PUBLIC, then that's fine with me.)
> 
> Basically the tradeoffs I see to exposing it are:
> 
> * Benefits: none(?)
> 
> * Cost: one more configuration that can (and, obviously, will) often fail to
> build


What you usually refers like PUBLIC/PRIVATE is translated in cmake advance variables:


        option(${_name} "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}" ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
        if (NOT ${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
            mark_as_advanced(FORCE ${_name})
        endif ()

(L257 in cmake/WebKitFeatures.cmake)

This doesn't means you can't set this value in advanced mode, right?

mark_as_advanced([CLEAR|FORCE] VAR [VAR2 ...])

 Mark the named cached variables as advanced. An advanced variable will not be 
 displayed in any of the cmake GUIs unless the show advanced option is on. If 
 CLEAR is the first argument advanced variables are changed back to unadvanced. 
 If FORCE is the first argument, then the variable is made advanced. If neither 
 FORCE nor CLEAR is specified, new values will be marked as advanced, but if 
 the variable already has an advanced/non-advanced state, it will not be 
 changed.


I'd suggest keep the option as PRIVATE (as in the current version of the patch) and allowing the advance users activate/deactivate this flag. It makes more sense to me.

In opposite, if you don't prefer don't support touch events set to OFF, then I'd prefer inhibit set this as true with something like:

    SET_AND_EXPOSE_TO_BUILD(ENABLE_TOUCH_EVENTS TRUE)

Anyway, I see interested enough keep the choice to swap off/on the touch events feature.

ref: https://cmake.org/cmake/help/v3.0/command/mark_as_advanced.html

[Michael Catanzaro]

I am going to leave a rather long comment here, because I designed the option visibility system, and have some strong opinions as to how it ought to work. When I added option visibility in bug #143572 and bug #143831 and bug #143558, I was trying to communicate to users that the PRIVATE/ADVANCED options should not be expected to work, but I never considered that it might be better to just remove the options entirely. At the time, our build system was fairly ad-hoc, the options list was huge, and distributors were building WebKit with many different defines that were not even in the options list. We've come quite a long way since then.

I think there have been a lot of benefits to removing port-specific code backing PRIVATE options, so I will *very* strongly recommend that, if we land this, the patch should make ENABLE_TOUCH_EVENTS public in the same commit.

(In reply to Carlos Alberto Lopez Perez from comment #13)
> If we aren't even going to accept patches for fixing the breakage for
> changing the value of private options, why having this private options at
> all?  What is the point? 

Because it's an option that's supported by other WebKit ports (in particular, GTK) and we have a shared WebKitFeatures.cmake file.

> Why we don't convert all private options to unconditional definitions that
> can't be changed of value?

We actually could! The reason we have not is simple: nobody has suggested it to me before.

Currently, leaving the options PRIVATE allows for developers to more easily play around with them. We use mark_as_advanced() to ensure that they don't appear in CMake GUIs and to hint that they are unsupported. I always figured that was sufficient to prevent people from trying to use the private options, but every once in a while, somebody tries and winds up filing a bug when one doesn't work. There was a bug related to this just last week (though I can't find it anymore, it was the motivation behind reporting bug #184973).

So, back to your question:

> Why we don't convert all private options to unconditional definitions that
> can't be changed of value?

You're right: you have a very strong argument for doing so. We could, for example, change this:

         option(${_name} "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}"
 ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
         if (NOT ${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
             mark_as_advanced(FORCE ${_name})
         endif ()

To this:

         if (${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
             option(${_name} "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}"
 ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
         endif ()

And then that would resolve this little debate. The disadvantage being that you would now have to modify the CMake build system if experimenting with the PRIVATE options as a developer.

I have no strong preference as to whether we should change this or not. If you want to do it (and it doesn't break other ports' bots), I'll give r=me.

(In reply to Pablo Saavedra from comment #14) 
> What you usually refers like PUBLIC/PRIVATE is translated in cmake advance
> variables:
> 
> 
>         option(${_name} "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}"
> ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
>         if (NOT ${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
>             mark_as_advanced(FORCE ${_name})
>         endif ()
> 
> (L257 in cmake/WebKitFeatures.cmake)
> 
> This doesn't means you can't set this value in advanced mode, right?

Of course it can be set in advanced mode.

> I'd suggest keep the option as PRIVATE (as in the current version of the
> patch) and allowing the advance users activate/deactivate this flag. It
> makes more sense to me.

But look at the list of options. There is no way you can expect all 100 options there to plausibly work in WPE. Probably some will build if toggled, but most likely won't. You're submitting a patch to make it possible to build with only one of these options toggled, out of a list of over 100 other options, where almost all of those are probably still broken. We can conclude that changing hidden, advanced options for arbitrary ports cannot possibly be supported.

Why should ENABLE_TOUCH_EVENTS be special? At some point in the future, a developer who doesn't remember this bug will be looking over that file and notice that ENABLE_TOUCH_EVENTS is PRIVATE and ON for WPE, and remove all the conditionals that you've added here, then we'll be back to square one. Worse, this opens the floodgates for more similar "build fix" patches, for the other 100+ options. I would much rather simply not accept patches for the PRIVATE options, or implement Carlos Lopez's suggestion to take away the options entirely.

> In opposite, if you don't prefer don't support touch events set to OFF, then
> I'd prefer inhibit set this as true with something like:
> 
>     SET_AND_EXPOSE_TO_BUILD(ENABLE_TOUCH_EVENTS TRUE)

That would be broken, because then you would wind up with ENABLE_TOUCH_EVENTS enabled even if you build with -DENABLE_TOUCH_EVENTS=OFF.

---------------------------------------------------------------------------------

OK, now back to the question of whether ENABLE_TOUCH_EVENTS should be PUBLIC or PRIVATE. Since Zan has not weighed in here, I'd be willing to approve a patch that fixes the ENABLE_TOUCH_EVENTS build, if (a) it also makes the option PUBLIC, and if (b) you can state some benefit to disabling the option.

> Anyway, I see interested enough keep the choice to swap off/on the touch
> events feature.

My question is: why the interest? As I wrote above:

(In reply to Michael Catanzaro from comment #12)
> Basically the tradeoffs I see to exposing it are:
> 
> * Benefits: none(?)
> 
> * Cost: one more configuration that can (and, obviously, will) often fail to
> build

The cost of exposing the option is clearly larger than none. So there should be some articulable benefit to disabling the option. So here is my challenge to you both: can you state some reason as to why it would be beneficial to disable ENABLE_TOUCH_EVENTS in your build?

I'm skeptical that there is any such benefit. Is there a significant reduction in code size? (Highly doubtful.) Compilation time? (Highly doubtful.) Aren't touch events disabled at runtime if the device does not have a touch screen? I think they are for GTK, but if that's not working for WPE, then that might be a very good reason to expose the option as PUBLIC. But you can't find *any* good reason to disable the option, then surely instead of trying to make it work, you should just not use it.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #15)
> But you can't find *any* good reason to disable the option, then surely
> instead of trying to make it work, you should just not use it.

And, conversely, if you *do* have a good reason for using -DENABLE_TOUCH_EVENTS=OFF, then surely it should be made PUBLIC so that others can see it and benefit from it.

[Michael Catanzaro]

Oops, I meant to write: "But *if* you can't find any good reason to disable the option"

[Pablo Saavedra]

Thanks for this pretty good explanation of the reasons behind PRIVATE/PUBLIC configuration. 

They seem convincing to me and I have no objections at this point.


Of course, they are not reasonable reasons to make public this flag. I'm talking about in terms of disk, memory or performance optimizations. Even in terms of dependencies or so.

  I'd agree with to close this issue with a WONTFIX or equivalent. 

Let me share to you my congratulations with work done in this part. Maybe I'd suggest align the available flags in build-webkit Perl script respect the CMakelist options but this is of course another issue. Let me know if looks interesting enough for you. I could be happy to work on that. For example, implementing different subset of features in the @features list in the
webkitperl/FeatureList.pm file.


(In reply to Michael Catanzaro from comment #15)
> I am going to leave a rather long comment here, because I designed the
> option visibility system, and have some strong opinions as to how it ought
> to work. When I added option visibility in bug #143572 and bug #143831 and
> bug #143558, I was trying to communicate to users that the PRIVATE/ADVANCED
> options should not be expected to work, but I never considered that it might
> be better to just remove the options entirely. At the time, our build system
> was fairly ad-hoc, the options list was huge, and distributors were building
> WebKit with many different defines that were not even in the options list.
> We've come quite a long way since then.
> 
> I think there have been a lot of benefits to removing port-specific code
> backing PRIVATE options, so I will *very* strongly recommend that, if we
> land this, the patch should make ENABLE_TOUCH_EVENTS public in the same
> commit.
> 
> (In reply to Carlos Alberto Lopez Perez from comment #13)
> > If we aren't even going to accept patches for fixing the breakage for
> > changing the value of private options, why having this private options at
> > all?  What is the point? 
> 
> Because it's an option that's supported by other WebKit ports (in
> particular, GTK) and we have a shared WebKitFeatures.cmake file.
> 
> > Why we don't convert all private options to unconditional definitions that
> > can't be changed of value?
> 
> We actually could! The reason we have not is simple: nobody has suggested it
> to me before.
> 
> Currently, leaving the options PRIVATE allows for developers to more easily
> play around with them. We use mark_as_advanced() to ensure that they don't
> appear in CMake GUIs and to hint that they are unsupported. I always figured
> that was sufficient to prevent people from trying to use the private
> options, but every once in a while, somebody tries and winds up filing a bug
> when one doesn't work. There was a bug related to this just last week
> (though I can't find it anymore, it was the motivation behind reporting bug
> #184973).
> 
> So, back to your question:
> 
> > Why we don't convert all private options to unconditional definitions that
> > can't be changed of value?
> 
> You're right: you have a very strong argument for doing so. We could, for
> example, change this:
> 
>          option(${_name} "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}"
>  ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
>          if (NOT ${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
>              mark_as_advanced(FORCE ${_name})
>          endif ()
> 
> To this:
> 
>          if (${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
>              option(${_name}
> "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}"
>  ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
>          endif ()
> 
> And then that would resolve this little debate. The disadvantage being that
> you would now have to modify the CMake build system if experimenting with
> the PRIVATE options as a developer.
> 
> I have no strong preference as to whether we should change this or not. If
> you want to do it (and it doesn't break other ports' bots), I'll give r=me.
> 
> (In reply to Pablo Saavedra from comment #14) 
> > What you usually refers like PUBLIC/PRIVATE is translated in cmake advance
> > variables:
> > 
> > 
> >         option(${_name} "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}"
> > ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
> >         if (NOT ${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
> >             mark_as_advanced(FORCE ${_name})
> >         endif ()
> > 
> > (L257 in cmake/WebKitFeatures.cmake)
> > 
> > This doesn't means you can't set this value in advanced mode, right?
> 
> Of course it can be set in advanced mode.
> 
> > I'd suggest keep the option as PRIVATE (as in the current version of the
> > patch) and allowing the advance users activate/deactivate this flag. It
> > makes more sense to me.
> 
> But look at the list of options. There is no way you can expect all 100
> options there to plausibly work in WPE. Probably some will build if toggled,
> but most likely won't. You're submitting a patch to make it possible to
> build with only one of these options toggled, out of a list of over 100
> other options, where almost all of those are probably still broken. We can
> conclude that changing hidden, advanced options for arbitrary ports cannot
> possibly be supported.
> 
> Why should ENABLE_TOUCH_EVENTS be special? At some point in the future, a
> developer who doesn't remember this bug will be looking over that file and
> notice that ENABLE_TOUCH_EVENTS is PRIVATE and ON for WPE, and remove all
> the conditionals that you've added here, then we'll be back to square one.
> Worse, this opens the floodgates for more similar "build fix" patches, for
> the other 100+ options. I would much rather simply not accept patches for
> the PRIVATE options, or implement Carlos Lopez's suggestion to take away the
> options entirely.
> 
> > In opposite, if you don't prefer don't support touch events set to OFF, then
> > I'd prefer inhibit set this as true with something like:
> > 
> >     SET_AND_EXPOSE_TO_BUILD(ENABLE_TOUCH_EVENTS TRUE)
> 
> That would be broken, because then you would wind up with
> ENABLE_TOUCH_EVENTS enabled even if you build with -DENABLE_TOUCH_EVENTS=OFF.
> 
> -----------------------------------------------------------------------------
> ----
> 
> OK, now back to the question of whether ENABLE_TOUCH_EVENTS should be PUBLIC
> or PRIVATE. Since Zan has not weighed in here, I'd be willing to approve a
> patch that fixes the ENABLE_TOUCH_EVENTS build, if (a) it also makes the
> option PUBLIC, and if (b) you can state some benefit to disabling the option.
> 
> > Anyway, I see interested enough keep the choice to swap off/on the touch
> > events feature.
> 
> My question is: why the interest? As I wrote above:
> 
> (In reply to Michael Catanzaro from comment #12)
> > Basically the tradeoffs I see to exposing it are:
> > 
> > * Benefits: none(?)
> > 
> > * Cost: one more configuration that can (and, obviously, will) often fail to
> > build
> 
> The cost of exposing the option is clearly larger than none. So there should
> be some articulable benefit to disabling the option. So here is my challenge
> to you both: can you state some reason as to why it would be beneficial to
> disable ENABLE_TOUCH_EVENTS in your build?
> 
> I'm skeptical that there is any such benefit. Is there a significant
> reduction in code size? (Highly doubtful.) Compilation time? (Highly
> doubtful.) Aren't touch events disabled at runtime if the device does not
> have a touch screen? I think they are for GTK, but if that's not working for
> WPE, then that might be a very good reason to expose the option as PUBLIC.
> But you can't find *any* good reason to disable the option, then surely
> instead of trying to make it work, you should just not use it.

[Michael Catanzaro]

(In reply to Pablo Saavedra from comment #18)
>   I'd agree with to close this issue with a WONTFIX or equivalent. 

OK then.

> Let me share to you my congratulations with work done in this part. Maybe
> I'd suggest align the available flags in build-webkit Perl script respect
> the CMakelist options but this is of course another issue. Let me know if
> looks interesting enough for you. I could be happy to work on that. For
> example, implementing different subset of features in the @features list in
> the
> webkitperl/FeatureList.pm file.

Yeah, I agree this is a problem.

Unfortunately, the list of available options is different for different ports, so we can't just remove build options from build-webkit.

I think it'd be desirable to remove all the --disable and --enable flags entirely, and require users to use --cmakeargs instead. But I'm not sure if Apple would be OK with that.

It's not clear how we might be able to proceed here. Suggestions and assistance would be very welcome.

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #15)
> So, back to your question:
> 
> > Why we don't convert all private options to unconditional definitions that
> > can't be changed of value?
> 
> You're right: you have a very strong argument for doing so. We could, for
> example, change this:
> 
>          option(${_name} "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}"
>  ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
>          if (NOT ${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
>              mark_as_advanced(FORCE ${_name})
>          endif ()
> 
> To this:
> 
>          if (${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
>              option(${_name}
> "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}"
>  ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
>          endif ()
> 
> And then that would resolve this little debate. The disadvantage being that
> you would now have to modify the CMake build system if experimenting with
> the PRIVATE options as a developer.
> 
> I have no strong preference as to whether we should change this or not. If
> you want to do it (and it doesn't break other ports' bots), I'll give r=me.
> 

Definitively I think we should not allow to modify build options via a simple cmake switch argument (even if that is somehow hidden) and at the same time not accept patches to fix the breakage when that is modified.

So, I think we should make the current private options not options anymore, but unconditional values. Or at least only allow this values to be modified when DEVELOPER_MODE is enabled. 

At the same time that means we will have to make options that are current private public (like woff2).

And that will be better (IMHO) than the current status quo where there are lot of hidden options for which we don't accept patches to fix the breakage (which is kind of an unwritten rule), and where several downstream are toying with this config options.

[Michael Catanzaro]

That all sounds good to me, if you want to submit a patch in a different bug, or repurpose this one.

And good point about making ENABLE_WOFF2 public. I used it for buildroot-wpe in violation of my own rule.