Bug 208702

Created attachment 392699 [details] Patch

Created attachment 392700 [details] Patch Adding Zan to the ChangeLogs

Created attachment 393019 [details] Buildfixes for Mac/iOS

Created attachment 393046 [details] Patch Xcode project changes

Created attachment 393218 [details] Patch Mac/iOS build fix. Missing file in pbxproj

Created attachment 393250 [details] Patch Fix for Windows build

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?

(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.

(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!

(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 :)

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.

Ž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.

Created attachment 393533 [details] Patch for landing Rebased against current ToT

(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...

Created attachment 393612 [details] Patch for landing v2 Build fixes for Mac/iOS

Created attachment 393613 [details] Patch for landing v2 Build fixes for Mac/iOS

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

Created attachment 393639 [details] Patch for landing v3 OpenXR not required for WPE. ENABLE_WEBXR off by default for XCode build system

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

<rdar://problem/60496156>

Comment on attachment 393639 [details] Patch for landing v3 Removing the r?. Already landed long time ago