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

Mon May 7 02:50:32 PDT 2018

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

--- Comment #18 from Pablo Saavedra <psaavedra at igalia.com> ---

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.

-- 
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/20180507/ddca4c64/attachment-0001.html>