Bug 208702 – [WebXR] IDLs, stubs and build configuration for WPE

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[WebXR] IDLs, stubs and build configuration for WPE

[Sergio Villar Senin]

Created attachment 392699 [details]
Patch

[Sergio Villar Senin]

Created attachment 392700 [details]
Patch

Adding Zan to the ChangeLogs

[Sergio Villar Senin]

Created attachment 393019 [details]
Buildfixes for Mac/iOS

[Sergio Villar Senin]

Created attachment 393046 [details]
Patch

Xcode project changes

[Sergio Villar Senin]

Created attachment 393218 [details]
Patch

Mac/iOS build fix. Missing file in pbxproj

[Sergio Villar Senin]

Created attachment 393250 [details]
Patch

Fix for Windows build

[Dean Jackson]

Comment on attachment 393250 [details]
Patch

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

My overall comment is that we should think about how we'll name the implementation of this API. By claiming the XR prefix, it means the implementation in platform will have to use something else. But we probably want to use XR there. So we could have the .idl files use a WebXR prefix, but use InterfaceName to expose them with just an XR prefix.

Then for the simple enum types, like XRHandedness, which will likely be used directly in the implementation, ignore the WebXR prefix but only have the XRHandedness.idl file in Modules/webxr and the implementation files in platform.

Also, I don't like that the actual code is wrapped in ENABLE_WEBGL. I think there needs to be a new WEBXR conditional.

> Source/WebCore/Modules/webxr/NavigatorWebXR.idl:28
> +    Conditional=WEBGL

It might be worth adding a compile time flag for WEBXR too. ENABLE_WEBXR?

> Source/WebCore/Sources.txt:435
> +Modules/webxr/NavigatorWebXR.cpp @no-unify
> +Modules/webxr/XRBoundedReferenceSpace.cpp @no-unify
> +Modules/webxr/XRFrame.cpp @no-unify
> +Modules/webxr/XRInputSource.cpp @no-unify
> +Modules/webxr/XRInputSourceArray.cpp @no-unify
> +Modules/webxr/XRInputSourceEvent.cpp @no-unify
> +Modules/webxr/XRInputSourcesChangeEvent.cpp @no-unify
> +Modules/webxr/XRPose.cpp @no-unify
> +Modules/webxr/XRReferenceSpace.cpp @no-unify
> +Modules/webxr/XRReferenceSpaceEvent.cpp @no-unify
> +Modules/webxr/XRRenderState.cpp @no-unify
> +Modules/webxr/XRRigidTransform.cpp @no-unify
> +Modules/webxr/XRSession.cpp @no-unify
> +Modules/webxr/XRSessionEvent.cpp @no-unify
> +Modules/webxr/XRSpace.cpp @no-unify
> +Modules/webxr/XRSystem.cpp @no-unify
> +Modules/webxr/XRView.cpp @no-unify
> +Modules/webxr/XRViewerPose.cpp @no-unify
> +Modules/webxr/XRViewport.cpp @no-unify
> +Modules/webxr/XRWebGLLayer.cpp @no-unify

Why not unify?

[Sergio Villar Senin]

(In reply to Dean Jackson from comment #7)
> Comment on attachment 393250 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393250&action=review
> 
> My overall comment is that we should think about how we'll name the
> implementation of this API. By claiming the XR prefix, it means the
> implementation in platform will have to use something else. But we probably
> want to use XR there. So we could have the .idl files use a WebXR prefix,
> but use InterfaceName to expose them with just an XR prefix.
> 
> Then for the simple enum types, like XRHandedness, which will likely be used
> directly in the implementation, ignore the WebXR prefix but only have the
> XRHandedness.idl file in Modules/webxr and the implementation files in
> platform.

OK, so:
* XRSession.idl -> WebXRSession.idl
  [] interface XRSession : EventTarget
* XRHandedness.idl ==

> Also, I don't like that the actual code is wrapped in ENABLE_WEBGL. I think
> there needs to be a new WEBXR conditional.

I reviewed https://webkit.org/feature-policy/ many times and it was indeed border line to me. Anyway adding the conditional build is a good idea.
 
> > Source/WebCore/Modules/webxr/NavigatorWebXR.idl:28
> > +    Conditional=WEBGL
> 
> It might be worth adding a compile time flag for WEBXR too. ENABLE_WEBXR?
> 
> > Source/WebCore/Sources.txt:435
> > +Modules/webxr/NavigatorWebXR.cpp @no-unify
> > +Modules/webxr/XRBoundedReferenceSpace.cpp @no-unify
> > +Modules/webxr/XRFrame.cpp @no-unify
> > +Modules/webxr/XRInputSource.cpp @no-unify
> > +Modules/webxr/XRInputSourceArray.cpp @no-unify
> > +Modules/webxr/XRInputSourceEvent.cpp @no-unify
> > +Modules/webxr/XRInputSourcesChangeEvent.cpp @no-unify
> > +Modules/webxr/XRPose.cpp @no-unify
> > +Modules/webxr/XRReferenceSpace.cpp @no-unify
> > +Modules/webxr/XRReferenceSpaceEvent.cpp @no-unify
> > +Modules/webxr/XRRenderState.cpp @no-unify
> > +Modules/webxr/XRRigidTransform.cpp @no-unify
> > +Modules/webxr/XRSession.cpp @no-unify
> > +Modules/webxr/XRSessionEvent.cpp @no-unify
> > +Modules/webxr/XRSpace.cpp @no-unify
> > +Modules/webxr/XRSystem.cpp @no-unify
> > +Modules/webxr/XRView.cpp @no-unify
> > +Modules/webxr/XRViewerPose.cpp @no-unify
> > +Modules/webxr/XRViewport.cpp @no-unify
> > +Modules/webxr/XRWebGLLayer.cpp @no-unify
> 
> Why not unify?

Just because we thought that in this early stages of development were most of the changes will affect just 2-3 files at a time compilations would be faster. Also we could invoke single compilation targets to quickly check while adding stuff to specific files.

[Dean Jackson]

(In reply to Sergio Villar Senin from comment #8)
> (In reply to Dean Jackson from comment #7)
> > Comment on attachment 393250 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=393250&action=review
> > 
> > My overall comment is that we should think about how we'll name the
> > implementation of this API. By claiming the XR prefix, it means the
> > implementation in platform will have to use something else. But we probably
> > want to use XR there. So we could have the .idl files use a WebXR prefix,
> > but use InterfaceName to expose them with just an XR prefix.
> > 
> > Then for the simple enum types, like XRHandedness, which will likely be used
> > directly in the implementation, ignore the WebXR prefix but only have the
> > XRHandedness.idl file in Modules/webxr and the implementation files in
> > platform.
> 
> OK, so:
> * XRSession.idl -> WebXRSession.idl
>   [] interface XRSession : EventTarget

WebXRSession.idl has:
"""
[
  Conditional=WEBXR,
  InterfaceName=XRSession,
  ...
] WebXRSession : EventTarget ...
"""
and WebXRSession.{h,cpp}. The "InterfaceName" means it will be exposed to the Web with that name. These would all live in Modules/webxr to serve as our frontend.


> * XRHandedness.idl ==

Since this one is just an enum, and it is likely we'd use the values in our backend implementation as well as the frontend, we can put the IDL in Modules/webxr without a prefix (or InterfaceName) and the .h/cpp can go in platform/graphics/xr or something.

see Modules/webgpu/GPULoadOp.idl as an example.


> 
> > Also, I don't like that the actual code is wrapped in ENABLE_WEBGL. I think
> > there needs to be a new WEBXR conditional.
> 
> I reviewed https://webkit.org/feature-policy/ many times and it was indeed
> border line to me. Anyway adding the conditional build is a good idea.

Excellent!

[Sergio Villar Senin]

(In reply to Dean Jackson from comment #9)
> (In reply to Sergio Villar Senin from comment #8)
> > (In reply to Dean Jackson from comment #7)
> > > Comment on attachment 393250 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=393250&action=review
> > > 
> > > My overall comment is that we should think about how we'll name the
> > > implementation of this API. By claiming the XR prefix, it means the
> > > implementation in platform will have to use something else. But we probably
> > > want to use XR there. So we could have the .idl files use a WebXR prefix,
> > > but use InterfaceName to expose them with just an XR prefix.
> > > 
> > > Then for the simple enum types, like XRHandedness, which will likely be used
> > > directly in the implementation, ignore the WebXR prefix but only have the
> > > XRHandedness.idl file in Modules/webxr and the implementation files in
> > > platform.
> > 
> > OK, so:
> > * XRSession.idl -> WebXRSession.idl
> >   [] interface XRSession : EventTarget
> 
> WebXRSession.idl has:
> """
> [
>   Conditional=WEBXR,
>   InterfaceName=XRSession,
>   ...
> ] WebXRSession : EventTarget ...
> """
> and WebXRSession.{h,cpp}. The "InterfaceName" means it will be exposed to
> the Web with that name. These would all live in Modules/webxr to serve as
> our frontend.

Yeah I know, I was just writting down the suggested changes for reference :)

[Sergio Villar Senin]

Created attachment 393527 [details]
Patch for landing

Applied the suggested changes. Renamed interfaces to WebXRxxx. Added new build flag ENABLE_WEBXR with a dependency to ENABLE_WEBGL. Restored a Constructor commented by mistake in WebXRWebGLLayer.idl.

[Sergio Villar Senin]

Žan, mind taking a look also to the WPE build options? I think they're fine (enabled for developer builds, disabled by default for all the ports) but always better to double check.

[Sergio Villar Senin]

Created attachment 393533 [details]
Patch for landing

Rebased against current ToT

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #12)
> Žan, mind taking a look also to the WPE build options? I think they're fine
> (enabled for developer builds, disabled by default for all the ports) but
> always better to double check.

This would force us to install OpenXR in the WPE bots, and also will also force all WPE developers to do so. Maybe perhaps we should just add it in disabled state by default. It's very likely that only me will build WebXR in WPE for a while...

[Sergio Villar Senin]

Created attachment 393612 [details]
Patch for landing v2

Build fixes for Mac/iOS

[Sergio Villar Senin]

Created attachment 393613 [details]
Patch for landing v2

Build fixes for Mac/iOS

[Sergio Villar Senin]

Forgot to apply the changes to the WPE build system, patch to follow...

[Sergio Villar Senin]

Created attachment 393639 [details]
Patch for landing v3

OpenXR not required for WPE. ENABLE_WEBXR off by default for XCode build system

[Sergio Villar Senin]

Committed r258498: <https://trac.webkit.org/changeset/258498>

[Radar WebKit Bug Importer]

<rdar://problem/60496156>

[Sergio Villar Senin]

Comment on attachment 393639 [details]
Patch for landing v3

Removing the r?. Already landed long time ago