Bug 177298 - [WebVR] Add OpenVR to the tree and to the build
Summary: [WebVR] Add OpenVR to the tree and to the build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on: 182047 182284
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-21 04:41 PDT by Sergio Villar Senin
Modified: 2018-01-30 02:13 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.21 MB, patch)
2017-09-21 04:52 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (1.21 MB, patch)
2017-09-22 08:12 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (1.21 MB, patch)
2017-09-22 09:14 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (1.21 MB, patch)
2017-09-25 07:57 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (1.21 MB, patch)
2018-01-18 07:58 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (1.18 MB, patch)
2018-01-18 08:48 PST, Sergio Villar Senin
zan: review+
zan: commit-queue-
Details | Formatted Diff | Diff
Patch (1.18 MB, patch)
2018-01-24 06:59 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2017-09-21 04:41:02 PDT
[WebVR] Add OpenVR to the tree and to the build
Comment 1 Sergio Villar Senin 2017-09-21 04:52:35 PDT
Created attachment 321420 [details]
Patch
Comment 2 Sergio Villar Senin 2017-09-22 08:12:29 PDT
Created attachment 321548 [details]
Patch

Fixed Mac build issues
Comment 3 Sergio Villar Senin 2017-09-22 09:14:28 PDT
Created attachment 321554 [details]
Patch

Rebased
Comment 4 Sergio Villar Senin 2017-09-25 02:24:17 PDT
So the only missing thing is to avoid building OpenVR for iOS. I might need some help here...
Comment 5 Sergio Villar Senin 2017-09-25 07:57:58 PDT
Created attachment 321682 [details]
Patch

Temptative fix for iOS
Comment 6 Alex Christensen 2017-09-25 10:09:38 PDT
Is there an advantage to having OpenVR in the tree over having it be a dependency?
Comment 7 Michael Catanzaro 2017-09-25 12:35:17 PDT
Adding more bundled dependencies would be unacceptable for WebKitGTK+ and WPE.
Comment 8 Michael Catanzaro 2017-09-25 12:38:48 PDT
Comment on attachment 321682 [details]
Patch

If Valve isn't shipping a usable library, then you can fork this on GitHub or someplace else and we can depend on that.

I don't want to allow any new dependencies in ThirdParty. ANGLE is bad enough, now we have brotli (unacceptable) and woff2 (also unacceptable) and libwebrtc (if a fork is really necessary, that could still be done in an external repo)... where does it stop?
Comment 9 Sergio Villar Senin 2017-09-26 02:06:44 PDT
(In reply to Alex Christensen from comment #6)
> Is there an advantage to having OpenVR in the tree over having it be a
> dependency?

Nobody is shipping the library AFAIK, it's just a repo on github.
Comment 10 Sergio Villar Senin 2017-09-26 02:07:10 PDT
(In reply to Michael Catanzaro from comment #7)
> Adding more bundled dependencies would be unacceptable for WebKitGTK+ and
> WPE.

Those are strong words that require further explanations.
Comment 11 Carlos Garcia Campos 2017-09-26 02:19:41 PDT
I agree that adding more things to third party is never desirable, but every case is different and sometimes there's no other choice. If valve doesn't make releases of OpenVR, and all we have is a github repo, how are our users ever going to use the feature that depend on OpenVR? We could add the github repo to our jhbuild for testing purposes, but that wouldn't work for other ports. And again, if distros are not shipping the library, users will never get the feature.
Comment 12 Michael Catanzaro 2017-09-26 08:04:46 PDT
I don't agree, there is always an alternative to bundling inside ThirdParty. If upstream does not produce usable releases *and* cannot be convinced to do so, then we can fork the library and maintain it elsewhere, outside the WebKit repo. That's not a desirable result, but it at least ensures there will be one system package for Linux distros, so that WebKit doesn't have its own separate copy of the library.

In this case, there are many OpenVR releases in the github repo, so releases aren't the problem. I don't know if Valve releases a usable Linux library or not, though. If it's not usable, *and* we fail to convince upstream to fix that, then we will indeed have to fork it, but that doesn't need to be done inside WebKit.

"And again, if distros are not shipping the library, users will never get the feature." I don't agree here either. We can get whatever library we want into Debian and Fedora, as long as it's free software, and other distros will follow. But I bet we won't even need to; probably someone else will package it for us as soon as WebKit needs it.

I think the only reason to bundle inside ThirdParty is if we need to make large WebKit-specific changes that would not possibly be useful to any other library.

(In reply to Sergio Villar Senin from comment #10)
> Those are strong words that require further explanations.

Come on, bundling dependencies is how we wind up with a Google-quality product. This is WebKit, we don't sink to that level. This is not how quality software development works on Linux.

With the exception of ANGLE, we've avoided bundling for years and years. We've regressed only recently by allowing third-party deps for fonts. We should never have done so, as that was totally nonessential. Even with xdg-mime, we should probably have pushed our changes to the upstream repo rather than copying it into ThirdParty; now we've got a weird result where glib and WebKit have separate versions of xdg-mime with different bugs, and the upstream version is just broken. That's bad for everyone.
Comment 13 Carlos Garcia Campos 2017-09-26 08:20:29 PDT
Are you volunteering to fork the project and maintain it?
Comment 14 Michael Catanzaro 2017-09-26 08:28:38 PDT
Nope, but that should be a requirement if the upstream releases are not usable for us. A big "if" as I see there are binary .so files inside the repo (quality engineering, no doubt). I suspect all we need to do is write a FindOpenVR.cmake.
Comment 15 Michael Catanzaro 2017-09-26 08:31:55 PDT
The README says it just has to be built with -DBUILD_SHARED=1. I bet it would be no problem to use it as an external dependency.
Comment 16 Sergio Villar Senin 2017-09-26 08:58:44 PDT
(In reply to Michael Catanzaro from comment #12)

> In this case, there are many OpenVR releases in the github repo, so releases
> aren't the problem. I don't know if Valve releases a usable Linux library or
> not, though. If it's not usable, *and* we fail to convince upstream to fix
> that, then we will indeed have to fork it, but that doesn't need to be done
> inside WebKit.
> 
> "And again, if distros are not shipping the library, users will never get
> the feature." I don't agree here either. We can get whatever library we want
> into Debian and Fedora, as long as it's free software, and other distros
> will follow. But I bet we won't even need to; probably someone else will
> package it for us as soon as WebKit needs it.

In the hypothetical case that you get the library inside Debian and Fedora (I haven't inspected in detail the licensing terms or any other requirement to get it inside those projects) you still need to get it shipped in MacOS. How are you going to deal with that?
Comment 17 Michael Catanzaro 2017-09-26 09:48:30 PDT
(In reply to Sergio Villar Senin from comment #16)
> In the hypothetical case that you get the library inside Debian and Fedora
> (I haven't inspected in detail the licensing terms or any other requirement
> to get it inside those projects) 

If there would be any such licensing problem, then we surely would not want that problem to apply to WebKit by importing that code.

> you still need to get it shipped in MacOS.
> How are you going to deal with that?

That's up to Apple to decide. If they want to depend on it, they'll figure it out. They might even import it under Source/ThirdParty for use on Macs only. It's not any problem for us.
Comment 18 Sergio Villar Senin 2017-09-26 10:03:40 PDT
(In reply to Michael Catanzaro from comment #17)
> (In reply to Sergio Villar Senin from comment #16)
> > In the hypothetical case that you get the library inside Debian and Fedora
> > (I haven't inspected in detail the licensing terms or any other requirement
> > to get it inside those projects) 
> 
> If there would be any such licensing problem, then we surely would not want
> that problem to apply to WebKit by importing that code.
> 
> > you still need to get it shipped in MacOS.
> > How are you going to deal with that?
> 
> That's up to Apple to decide. If they want to depend on it, they'll figure
> it out. They might even import it under Source/ThirdParty for use on Macs
> only. It's not any problem for us.

Michael we should think about the project as a whole, not just in terms of ports. Apple is fine with us using OpenVR to implement at least a first version of the spec, so it is not "their" problem but ours as well. This is not just a possibility, it's how things are at the moment. So if you have a better proposal then just tell us, we're all ears. So far the ThirdParty possibility is the only one over the table. I agree is far from ideal, but it's the only one that allows us to go on implementing the spec while we don't have packages for all the architectures we care of. Also we can remove it later at any moment as we don't plan to make changes on the library.
Comment 19 Michael Catanzaro 2017-09-26 10:28:18 PDT
I have many better proposals. You don't need to wait for distro packages to start developing OpenVR support. Distro packages will appear after WebKit depends on it. Surely you can install it manually in the meantime. That's what I would do until you have layout tests that you want running on the bots. Alternatively, you can make a Debian package for yourself: it's not that hard. Contributing it  upstream is even better. Using our jhbuild environment to build the package, as we do for openwebrtc, is another option. That's what you should do if you want the feature enabled in developer mode and running on the bots. Or, if we absolutely have to fork the library (and I don't think we do), we can do without adding it to the WebKit source tree.

So there are many options for developing this feature that don't require bundling OpenVR. It's common that we add a new external dependency to WebKit. This is not a big deal. But bundling dependencies is a big deal: that's just not acceptable for WebKit, not unless there are truly exceptional circumstances and we need to make big WebKit-specific changes to the dependency. I'm sorry that by accepting the woff and brotli and xdg-mime dependencies we perhaps created the impression that this might be acceptable for future dependencies; it was a mistake to accept those in the first place.
Comment 20 Michael Catanzaro 2017-09-26 10:32:13 PDT
(In reply to Michael Catanzaro from comment #19)
> Using our jhbuild environment to build the package, as we do for openwebrtc, is another option. That's what you should do if you want the feature enabled in
> developer mode and running on the bots.

That's my favorite option for now. You'll need to add it to jhbuild eventually if you want the feature to be enabled by default in developer mode anyway, since we won't be able to rely on the package existing in distros for a while. Might as well get it working now.
Comment 21 Sergio Villar Senin 2017-09-26 11:16:48 PDT
(In reply to Michael Catanzaro from comment #20)
> (In reply to Michael Catanzaro from comment #19)
> > Using our jhbuild environment to build the package, as we do for openwebrtc, is another option. That's what you should do if you want the feature enabled in
> > developer mode and running on the bots.
> 
> That's my favorite option for now. You'll need to add it to jhbuild
> eventually if you want the feature to be enabled by default in developer
> mode anyway, since we won't be able to rely on the package existing in
> distros for a while. Might as well get it working now.

Not sure if I'm not explaining myself properly or if you don't want to address the actual issue. I know I can do all that stuff to *develop* the feature *on Linux*. However there are 2 problems. The first one is that nobody is shipping it ATM. I know it isn't impossible to add the packages to Linux distros, but the truth is that so far nobody has done that. Anyway seems something that can be eventually fixed. (I am not doing it as I don't have neither the time nor the interest to create and maintain the packages).

Now the second issue. Mac does also need to use the library, jhbuild or $distro packaging won't fix anything for the Mac port. Again, so far I haven't seen any realistic alternative to support that use case.
Comment 22 Michael Catanzaro 2017-09-26 11:40:34 PDT
OK.

The first issue is indeed not a problem. Just add OpenVR to the JHBuild environment. Packaging will sort itself out later on. It's way easier to create a new package than to unbundle something from an existing package.

Regarding the second issue: I don't know how dependencies are handled on Mac, so I don't know what the correct approach is. Are you planning to get the feature working on Mac? If so, perhaps adding OpenVR to Source/ThirdParty is a valid option for Mac. But GTK/WPE should not use it at all. They should use a FindOpenVR.cmake and there should be no problems building with OpenVR support if the first step in the build process is rm -rf Source/ThirdParty/OpenVR. I sincerely believe it's important for WebKit that we strictly enforce this.
Comment 23 Sergio Villar Senin 2017-09-27 01:41:22 PDT
(In reply to Michael Catanzaro from comment #22)
> OK.
> 
> The first issue is indeed not a problem. Just add OpenVR to the JHBuild
> environment. Packaging will sort itself out later on. It's way easier to
> create a new package than to unbundle something from an existing package.
> 
> Regarding the second issue: I don't know how dependencies are handled on
> Mac, so I don't know what the correct approach is. Are you planning to get
> the feature working on Mac? If so, perhaps adding OpenVR to
> Source/ThirdParty is a valid option for Mac. But GTK/WPE should not use it
> at all. They should use a FindOpenVR.cmake and there should be no problems
> building with OpenVR support if the first step in the build process is rm
> -rf Source/ThirdParty/OpenVR. I sincerely believe it's important for WebKit
> that we strictly enforce this.

Yes, as I said many times, the implementation is meant to be valid for Linux(GTK/WPE) and Mac platforms. All of them are going to use the same API (OpenVR) which will handle the platform differences for us.

Honestly it does not make much sense to me to have OpenVR in the tree for Mac and then build it as external dependency for GTK/WPE. It'll just add maintenance burden. Note that once there is a solution for Mac we can switch to jhbuild quite easily.

Alex what do you think about this? Would it be possible to get a package of OpenVR for Mac or some other way to use it as an external dependency instead of in-tree ?
Comment 24 Zan Dobersek 2017-09-27 02:11:02 PDT
This feature is in the earliest stages of development. It's possible changes will have to be made to OpenVR itself.

I'd rather see the library more tightly coupled into the project for this initial phase, applying necessary fixes as required.

Once the feature matures and we actually get to ship it in whichever port, we can see what the most appropriate solution would be.
Comment 25 Sergio Villar Senin 2018-01-18 07:58:34 PST
Created attachment 331621 [details]
Patch

Hopefully fixing all the builds.
Comment 26 Sergio Villar Senin 2018-01-18 08:24:06 PST
OK I'm giving up on fixing the iOS build. I'll enable it only for WPE/Gtk and open a new bug to fix the build for Mac only. This struggle with build systems is effectively blocking further developments.
Comment 27 Sergio Villar Senin 2018-01-18 08:48:58 PST
Created attachment 331627 [details]
Patch
Comment 28 Sergio Villar Senin 2018-01-23 02:09:17 PST
Ping reviewers
Comment 29 Zan Dobersek 2018-01-23 03:02:32 PST
Comment on attachment 331627 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1302
> +#if PLATFORM(GTK) || PLATFORM(WPE)
> +#define USE_OPENVR 1
> +#endif

You don't need the define here, it should be provided through SET_AND_EXPOSE_TO_BUILD() commands in CMake files.

> ChangeLog:21
> +2018-01-18  Sergio Villar Senin  <svillar@igalia.com>
> +
> +        [WebVR] Add OpenVR to the tree and to the build
> +        https://bugs.webkit.org/show_bug.cgi?id=177298
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Source/CMakeLists.txt:
> +        * Source/cmake/OptionsGTK.cmake: Enable USE_OPENVR.
> +        * Source/cmake/OptionsWPE.cmake: Ditto.
> +
> +2017-09-22  Sergio Villar Senin  <svillar@igalia.com>
> +
> +        [WebVR] Add OpenVR to the tree and to the build
> +        https://bugs.webkit.org/show_bug.cgi?id=177298
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Source/CMakeLists.txt:
> +        * WebKit.xcworkspace/contents.xcworkspacedata:
> +

Double entries.
Comment 30 Sergio Villar Senin 2018-01-24 06:59:42 PST
Created attachment 332154 [details]
Patch

Patch for landing
Comment 31 Sergio Villar Senin 2018-01-24 07:05:34 PST
Committed r227518: <https://trac.webkit.org/changeset/227518>
Comment 32 Radar WebKit Bug Importer 2018-01-24 07:07:41 PST
<rdar://problem/36823398>
Comment 33 Michael Catanzaro 2018-01-24 07:46:36 PST
Comment on attachment 331627 [details]
Patch

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

I have a couple comments on the CMake changes and on the new GTK/WPE API. Sorry I didn't review it before it landed.

First, this is feature is now enabled by default in releases, but it's surely intended to be experimental and disabled in releases. We also had an agreement that OpenVR source code should not be included in the tarball at all, at least for the time being, to encourage proper distro packaging of OpenVR. So Source/ThirdParty/openvr ought to be added as an excluded directory in Tools/gtk/manifest.txt.in. (IMO it's up to you and Zan whether or not to exclude it from Tools/wpe/manifest.txt.in.) Then the build system needs a couple changes to cope with this... I'll comment on those inline.

Also, the patch that landed added new public GTK/WPE WebKitSettings API, though I don't see it here. They are missing Since: 2.20 tags, which should be added if we keep the new APIs. But I actually don't want to add a public runtime setting until the build flag is ready to be enabled by default. For years, we've had useless WebKitSettings APIs to enable MSE and WebRTC that actually do nothing in releases because those are disabled at build time. The MSE API will work for the first time in 2.20, but the WebRTC API still does nothing to this day. I'd much rather we avoid making this mistake again, so I'd request that you completely remove the new public API. We don't need it; instead, let's just make sure WebVR is enabled by default in developer builds. Look into WebPreferences.yaml and find the WebVREnabled setting there, and change the defaultValue from false to DEFAULT_EXPERIMENTAL_FEATURES_ENABLED. Then move the setting to the bottom of the file and add "category: experimental", and finally add "condition: PLATFORM(GTK) || PLATFORM(WPE)" so that it doesn't suddenly appear in Safari's experimental features list. Then we'll have WebVR enabled for developers without the need to add new API that won't work in releases.

> Source/cmake/OptionsGTK.cmake:343
> +SET_AND_EXPOSE_TO_BUILD(USE_OPENVR TRUE)

This isn't OK because it's enabled unconditionally and will break tarball builds once you add the excludes to the tarball manifests. We need to add a new build option. If it was going to be an ENABLE-style option, I would say to add it in WebKitFeatures.cmake, but we don't add USE- options there (except for USE_SYSTEM_MALLOC) so instead it will have to be added as GTK- and WPE-specific options instead. For GTK, that should be done in OptionsGTK.cmake under the "Private options specific to the GTK+ port" comment header. We want it enabled by default for developer builds (including our buildbots), but not for release builds. That should look something like this:

WEBKIT_OPTION_DEFINE(USE_REDIRECTED_XCOMPOSITE_WINDOW "Whether to use a Redirected XComposite Window for accelerated compositing in X11." PRIVATE ON)
WEBKIT_OPTION_DEFINE(USE_OPENVR "Whether to enable WebVR." PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES})

Then the SET_AND_EXPOSE_TO_BUILD line should be removed.

> Source/cmake/OptionsWPE.cmake:108
> +SET_AND_EXPOSE_TO_BUILD(USE_OPENVR TRUE)

Then for WPE, if you decide to include OpenVR in the tarballs, I would defer to you and Zan on whether the option should be PUBLIC or PRIVATE, since while we don't want to expose experimental features in GTK, we already do have one exposed in WPE. But if it's not included in the tarballs, it should definitely be PRIVATE. Regardless, the default value should depend on ${ENABLE_EXPERIMENTAL_FEATURES}, the option should be declared in a new group with a new comment "Options specific to the WPE port" to indicate that it's not overriding an option that's already declared in WebKitFeatures.cmake.
Comment 34 Michael Catanzaro 2018-01-24 08:03:03 PST
(In reply to Michael Catanzaro from comment #33)
> and add "category: experimental", and finally add "condition:
> PLATFORM(GTK) || PLATFORM(WPE)" so that it doesn't suddenly appear in
> Safari's experimental features list.

(Unless someone from Apple is working on this, in which case we should probably not add any condition: to the setting.)
Comment 35 Carlos Garcia Campos 2018-01-25 06:26:33 PST
I agree with Michael, FWIW. Let's revert the new API additions, please.
Comment 36 Sergio Villar Senin 2018-01-25 06:53:28 PST
(In reply to Michael Catanzaro from comment #33)
> Comment on attachment 331627 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331627&action=review
> 
> I have a couple comments on the CMake changes and on the new GTK/WPE API.
> Sorry I didn't review it before it landed.

A bit unfortunate to get this review once I manage to land it after 4 months... Anyway some of your comments are worth the burden.


> First, this is feature is now enabled by default in releases, but it's
> surely intended to be experimental and disabled in releases. We also had an
> agreement that OpenVR source code should not be included in the tarball at
> all, at least for the time being, to encourage proper distro packaging of
> OpenVR. So Source/ThirdParty/openvr ought to be added as an excluded
> directory in Tools/gtk/manifest.txt.in. (IMO it's up to you and Zan whether
> or not to exclude it from Tools/wpe/manifest.txt.in.) Then the build system
> needs a couple changes to cope with this... I'll comment on those inline.

Right, I forgot to remove it from the tarball. I guess you don't mind if I do it in a separate patch.
 
> Also, the patch that landed added new public GTK/WPE WebKitSettings API,
> though I don't see it here. They are missing Since: 2.20 tags, which should
> be added if we keep the new APIs. But I actually don't want to add a public
> runtime setting until the build flag is ready to be enabled by default. For
> years, we've had useless WebKitSettings APIs to enable MSE and WebRTC that
> actually do nothing in releases because those are disabled at build time.
> The MSE API will work for the first time in 2.20, but the WebRTC API still
> does nothing to this day. I'd much rather we avoid making this mistake
> again, so I'd request that you completely remove the new public API. We
> don't need it; instead, let's just make sure WebVR is enabled by default in
> developer builds. Look into WebPreferences.yaml and find the WebVREnabled
> setting there, and change the defaultValue from false to
> DEFAULT_EXPERIMENTAL_FEATURES_ENABLED. Then move the setting to the bottom
> of the file and add "category: experimental", and finally add "condition:
> PLATFORM(GTK) || PLATFORM(WPE)" so that it doesn't suddenly appear in
> Safari's experimental features list. Then we'll have WebVR enabled for
> developers without the need to add new API that won't work in releases.

So the new API was not meant to be included in this patch, it was just some local changes I had for future patches. I'll revert that addition.

> > Source/cmake/OptionsGTK.cmake:343
> > +SET_AND_EXPOSE_TO_BUILD(USE_OPENVR TRUE)
> 
> This isn't OK because it's enabled unconditionally and will break tarball
> builds once you add the excludes to the tarball manifests. We need to add a
> new build option. If it was going to be an ENABLE-style option, I would say
> to add it in WebKitFeatures.cmake, but we don't add USE- options there
> (except for USE_SYSTEM_MALLOC) so instead it will have to be added as GTK-
> and WPE-specific options instead. For GTK, that should be done in
> OptionsGTK.cmake under the "Private options specific to the GTK+ port"
> comment header. We want it enabled by default for developer builds
> (including our buildbots), but not for release builds. That should look
> something like this:
> 
> WEBKIT_OPTION_DEFINE(USE_REDIRECTED_XCOMPOSITE_WINDOW "Whether to use a
> Redirected XComposite Window for accelerated compositing in X11." PRIVATE ON)
> WEBKIT_OPTION_DEFINE(USE_OPENVR "Whether to enable WebVR." PRIVATE
> ${ENABLE_EXPERIMENTAL_FEATURES})
> 
> Then the SET_AND_EXPOSE_TO_BUILD line should be removed.
> 
> > Source/cmake/OptionsWPE.cmake:108
> > +SET_AND_EXPOSE_TO_BUILD(USE_OPENVR TRUE)
> 
> Then for WPE, if you decide to include OpenVR in the tarballs, I would defer
> to you and Zan on whether the option should be PUBLIC or PRIVATE, since
> while we don't want to expose experimental features in GTK, we already do
> have one exposed in WPE. But if it's not included in the tarballs, it should
> definitely be PRIVATE. Regardless, the default value should depend on
> ${ENABLE_EXPERIMENTAL_FEATURES}, the option should be declared in a new
> group with a new comment "Options specific to the WPE port" to indicate that
> it's not overriding an option that's already declared in
> WebKitFeatures.cmake.

I don't agree with having private build options for WPE and GTK. I want it to be available for all the architectures. Even though I am not including the mac build system changes I want Mac developers to quickly catch up.
Comment 37 Michael Catanzaro 2018-01-25 09:52:55 PST
(In reply to Sergio Villar Senin from comment #36)
> I don't agree with having private build options for WPE and GTK. I want it
> to be available for all the architectures. Even though I am not including
> the mac build system changes I want Mac developers to quickly catch up.

OK, then can you name the option ENABLE_WEBVR instead? ENABLE_ options may go into WebKitFeatures.cmake to be shared across ports. So then you can add it there. You'll still want to make the same changes to OptionsGTK.cmake and OptionsWPE.cmake to override the default values, but the options would just be sorted differently (with the other private options that are shared with other WebKit ports).

(If you want to keep USE_OPENVR for use in the code, then you can have ENABLE_WEBVR as the cross-platform option defined in WebKitFeatures.h, and then turn on USE_OPENVR dependent on the value of ENABLE_WEBVR, either in Options*.cmake using SET_AND_EXPOSE_TO_BUILD or in Platform.h.)
Comment 38 Michael Catanzaro 2018-01-25 09:54:11 PST
(In reply to Michael Catanzaro from comment #37)
> OK, then can you name the option ENABLE_WEBVR instead? ENABLE_ options may
> go into WebKitFeatures.cmake to be shared across ports. So then you can add
> it there. You'll still want to make the same changes to OptionsGTK.cmake and
> OptionsWPE.cmake to override the default values, but the options would just
> be sorted differently (with the other private options that are shared with
> other WebKit ports).
> 
> (If you want to keep USE_OPENVR for use in the code, then you can have
> ENABLE_WEBVR as the cross-platform option defined in WebKitFeatures.h, and
> then turn on USE_OPENVR dependent on the value of ENABLE_WEBVR, either in
> Options*.cmake using SET_AND_EXPOSE_TO_BUILD or in Platform.h.)

BTW it would be best to file a new bug report for this and get the patch reviewed, since the requested change is a bit complicated.
Comment 39 Sergio Villar Senin 2018-01-26 00:40:08 PST
(In reply to Michael Catanzaro from comment #37)
> (In reply to Sergio Villar Senin from comment #36)
> > I don't agree with having private build options for WPE and GTK. I want it
> > to be available for all the architectures. Even though I am not including
> > the mac build system changes I want Mac developers to quickly catch up.
> 
> OK, then can you name the option ENABLE_WEBVR instead? ENABLE_ options may
> go into WebKitFeatures.cmake to be shared across ports. So then you can add
> it there. You'll still want to make the same changes to OptionsGTK.cmake and
> OptionsWPE.cmake to override the default values, but the options would just
> be sorted differently (with the other private options that are shared with
> other WebKit ports).

I didn't want to use ENABLE_ on purpose. WebVR will be implemented using the OpenVR backend at first, but does not depend at all on OpenVR. I am not very familiar with the latest changes in build machinery, but if it is not really possible to define a system-wide USE_OPENVR then I'd prefer to do it case by case as you suggested in the beginning.

> (If you want to keep USE_OPENVR for use in the code, then you can have
> ENABLE_WEBVR as the cross-platform option defined in WebKitFeatures.h, and
> then turn on USE_OPENVR dependent on the value of ENABLE_WEBVR, either in
> Options*.cmake using SET_AND_EXPOSE_TO_BUILD or in Platform.h.)
Comment 40 Michael Catanzaro 2018-01-26 05:25:02 PST
(In reply to Sergio Villar Senin from comment #39)
> I didn't want to use ENABLE_ on purpose. WebVR will be implemented using the
> OpenVR backend at first, but does not depend at all on OpenVR. I am not very
> familiar with the latest changes in build machinery, but if it is not really
> possible to define a system-wide USE_OPENVR then I'd prefer to do it case by
> case as you suggested in the beginning.

It's possible, but it's not the style we use for options. Options shared between ports use ENABLE_ because ports can choose to implement them using different technologies (e.g. ENABLE_WEB_RTC vs. USE_OPENWEBRTC). If you look in WebKitFeatures.cmake you'll see the only USE_ option there is USE_SYSTEM_MALLOC. So best to not change that now.

I don't think it's a big deal to expose ENABLE_WEBRTC as the public feature, and use that to set USE_OPENVR, but it's up to you what you prefer.