Bug 208702 - [WebXR] IDLs, stubs and build configuration for WPE
Summary: [WebXR] IDLs, stubs and build configuration for WPE
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:
Blocks: 208988
  Show dependency treegraph
 
Reported: 2020-03-06 02:31 PST by Sergio Villar Senin
Modified: 2020-03-16 08:50 PDT (History)
22 users (show)

See Also:


Attachments
Patch (220.09 KB, patch)
2020-03-06 03:04 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (220.33 KB, patch)
2020-03-06 03:15 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Buildfixes for Mac/iOS (223.01 KB, patch)
2020-03-09 02:45 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (259.90 KB, patch)
2020-03-09 09:59 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (260.21 KB, patch)
2020-03-11 04:07 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (262.51 KB, patch)
2020-03-11 09:21 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (321.21 KB, patch)
2020-03-13 13:59 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (320.04 KB, patch)
2020-03-13 15:00 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing v2 (329.81 KB, patch)
2020-03-15 04:18 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing v2 (328.54 KB, patch)
2020-03-15 04:22 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing v3 (327.78 KB, patch)
2020-03-16 02:51 PDT, Sergio Villar Senin
svillar: review?
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 2020-03-06 02:31:34 PST
[WebXR] IDLs, stubs and build configuration for WPE
Comment 1 Sergio Villar Senin 2020-03-06 03:04:44 PST
Created attachment 392699 [details]
Patch
Comment 2 Sergio Villar Senin 2020-03-06 03:15:43 PST
Created attachment 392700 [details]
Patch

Adding Zan to the ChangeLogs
Comment 3 Sergio Villar Senin 2020-03-09 02:45:46 PDT
Created attachment 393019 [details]
Buildfixes for Mac/iOS
Comment 4 Sergio Villar Senin 2020-03-09 09:59:16 PDT
Created attachment 393046 [details]
Patch

Xcode project changes
Comment 5 Sergio Villar Senin 2020-03-11 04:07:12 PDT
Created attachment 393218 [details]
Patch

Mac/iOS build fix. Missing file in pbxproj
Comment 6 Sergio Villar Senin 2020-03-11 09:21:53 PDT
Created attachment 393250 [details]
Patch

Fix for Windows build
Comment 7 Dean Jackson 2020-03-12 17:37:21 PDT
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?
Comment 8 Sergio Villar Senin 2020-03-13 01:35:50 PDT
(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.
Comment 9 Dean Jackson 2020-03-13 11:18:17 PDT
(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!
Comment 10 Sergio Villar Senin 2020-03-13 13:47:46 PDT
(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 :)
Comment 11 Sergio Villar Senin 2020-03-13 13:59:23 PDT
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.
Comment 12 Sergio Villar Senin 2020-03-13 14:01:14 PDT
Ž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.
Comment 13 Sergio Villar Senin 2020-03-13 15:00:06 PDT
Created attachment 393533 [details]
Patch for landing

Rebased against current ToT
Comment 14 Sergio Villar Senin 2020-03-13 15:04:54 PDT
(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...
Comment 15 Sergio Villar Senin 2020-03-15 04:18:26 PDT
Created attachment 393612 [details]
Patch for landing v2

Build fixes for Mac/iOS
Comment 16 Sergio Villar Senin 2020-03-15 04:22:31 PDT
Created attachment 393613 [details]
Patch for landing v2

Build fixes for Mac/iOS
Comment 17 Sergio Villar Senin 2020-03-15 04:26:40 PDT
Forgot to apply the changes to the WPE build system, patch to follow...
Comment 18 Sergio Villar Senin 2020-03-16 02:51:52 PDT
Created attachment 393639 [details]
Patch for landing v3

OpenXR not required for WPE. ENABLE_WEBXR off by default for XCode build system
Comment 19 Sergio Villar Senin 2020-03-16 08:49:56 PDT
Committed r258498: <https://trac.webkit.org/changeset/258498>
Comment 20 Radar WebKit Bug Importer 2020-03-16 08:50:14 PDT
<rdar://problem/60496156>