[Webkit-unassigned] [Bug 185253] [WPE] Fix build for ENABLE_TOUCH_EVENTS=OFF

Fri May 4 09:20:12 PDT 2018

https://bugs.webkit.org/show_bug.cgi?id=185253

--- Comment #15 from Michael Catanzaro <mcatanzaro at igalia.com> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180504/f7de785f/attachment.html>