[webkit-reviews] review granted: [Bug 208702] [WebXR] IDLs, stubs and build configuration for WPE : [Attachment 393250] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 208702: [WebXR] IDLs, stubs and build configuration for WPE
https://bugs.webkit.org/show_bug.cgi?id=208702

Attachment 393250: Patch

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




--- Comment #7 from Dean Jackson <dino at apple.com> ---
Comment on attachment 393250
  --> https://bugs.webkit.org/attachment.cgi?id=393250
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?