Bug 216925

Created attachment 409560 [details] Patch

Comment on attachment 409560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409560&action=review > Source/WebCore/Modules/webxr/WebXRSession.cpp:62 > + m_device->initializeTrackingAndRendering(mode); This seems like this could be done in device constructor and shutdownTrackingAndRendering in device destructor. Are we sure there might not be two sessions using the same device at the same time?

(In reply to youenn fablet from comment #2) > Comment on attachment 409560 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409560&action=review > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:62 > > + m_device->initializeTrackingAndRendering(mode); > > This seems like this could be done in device constructor and > shutdownTrackingAndRendering in device destructor. The spec mentions the exact points when this should be done. Note that there could be multiple calls to initialize/shutdown if the user enters/leaves XR sessions in a row. > Are we sure there might not be two sessions using the same device at the > same time? There is no device concept in OpenXR API, so our XRDevice is basically an abstraction for a session and some other additional data.

(In reply to Sergio Villar Senin from comment #3) > (In reply to youenn fablet from comment #2) > > Comment on attachment 409560 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=409560&action=review > > > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:62 > > > + m_device->initializeTrackingAndRendering(mode); > > > > This seems like this could be done in device constructor and > > shutdownTrackingAndRendering in device destructor. > > The spec mentions the exact points when this should be done. Note that there > could be multiple calls to initialize/shutdown if the user enters/leaves XR > sessions in a row. I noticed the comment lacks some context. Device objects are created in a previous state even before the user requests a session. The author will firstly typically call XRSystem::isSessionSupported() to check whether a particular session (immersive VR, AR, inline) is supported. That call will end up creating the XR devices on demand. Then, after checking that the session is supported the author might request a XRSession on a particular device. That's the point were we would need to create the OpenXR session. There is no need to unconditionally create OpenXR sessions for any available device. Also, if no session is ever requested then no OpenXR session should be created even though the devices were there. Hope this clarifies a bit the implementation decision.

Comment on attachment 409560 [details] Patch Removing r?. I'm going first to make the current platform code fully asynchronous and then reupload this patch rebased on top of that work.

<rdar://problem/69832601>

Created attachment 412416 [details] Patch

Comment on attachment 412416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412416&action=review > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:416 > + }; Nit. Maybe we can write it like: return (mode == SessionMode::ImmersiveVr) ? XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO : XR_VIEW_CONFIGURATION_TYPE_PRIMARY_MONO;

Comment on attachment 412416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412416&action=review > Source/WebCore/Modules/webxr/WebXRSession.cpp:339 > + m_device->shutdownTrackingAndRendering(); Is there a guarantee shutdown will not be called twice. Should we ASSERT(!m_ended) at the start of shutdown()? or return early? > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:414 > + default: Usually we try to not add default and use all explicit values. Can we change it to: 'case SessionMode::ImmersiveAr'? > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:431 > + xrCreateSession(m_instance, &sessionCreateInfo, &m_session); Other call sites of xrCreateSession do check for the result. Should we add some logging and/or reset in case the session cannot be created? > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438 > + xrDestroySession(m_session); Is it ok to pass XR_NULL_HANDLE to xrDestroySession?

Comment on attachment 412416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412416&action=review Thanks for waiting so much for my replies, I had several higher prio tasks to tackle before this. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:339 >> + m_device->shutdownTrackingAndRendering(); > > Is there a guarantee shutdown will not be called twice. > Should we ASSERT(!m_ended) at the start of shutdown()? or return early? Not really, the caller might invoke session.end() from JS multiple times (although it does not make sense from the functional POV). So shutdown() might be indeed be called multiple times. The shutdown algorithm from specs seems to assume that the session is !ended so I'll add the condition to the calling place. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:414 >> + default: > > Usually we try to not add default and use all explicit values. > Can we change it to: 'case SessionMode::ImmersiveAr'? Sure. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:416 >> + }; > > Nit. Maybe we can write it like: > return (mode == SessionMode::ImmersiveVr) ? XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO : XR_VIEW_CONFIGURATION_TYPE_PRIMARY_MONO; Yeah, but I think it's a good idea to have the switch because this way static analysis tools could easily detect missing handling of some values. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:431 >> + xrCreateSession(m_instance, &sessionCreateInfo, &m_session); > > Other call sites of xrCreateSession do check for the result. > Should we add some logging and/or reset in case the session cannot be created? The session will be XR_NULL_HANDLE whenever we get a failure, but we can indeed add some logging here. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438 >> + xrDestroySession(m_session); > > Is it ok to pass XR_NULL_HANDLE to xrDestroySession? No, the OpenXR runtimes handle it properly but the standard says that it must be a valid session. I'll change it.

Comment on attachment 412416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412416&action=review Thanks for waiting so much for my replies, I had several higher prio tasks to tackle before this. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:339 >> + m_device->shutdownTrackingAndRendering(); > > Is there a guarantee shutdown will not be called twice. > Should we ASSERT(!m_ended) at the start of shutdown()? or return early? Not really, the caller might invoke session.end() from JS multiple times (although it does not make sense from the functional POV). So shutdown() might be indeed be called multiple times. The shutdown algorithm from specs seems to assume that the session is !ended so I'll add the condition to the calling place. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:414 >> + default: > > Usually we try to not add default and use all explicit values. > Can we change it to: 'case SessionMode::ImmersiveAr'? Sure. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:416 >> + }; > > Nit. Maybe we can write it like: > return (mode == SessionMode::ImmersiveVr) ? XR_VIEW_CONFIGURATION_TYPE_PRIMARY_STEREO : XR_VIEW_CONFIGURATION_TYPE_PRIMARY_MONO; Yeah, but I think it's a good idea to have the switch because this way static analysis tools could easily detect missing handling of some values. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:431 >> + xrCreateSession(m_instance, &sessionCreateInfo, &m_session); > > Other call sites of xrCreateSession do check for the result. > Should we add some logging and/or reset in case the session cannot be created? The session will be XR_NULL_HANDLE whenever we get a failure, but we can indeed add some logging here. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438 >> + xrDestroySession(m_session); > > Is it ok to pass XR_NULL_HANDLE to xrDestroySession? No, the OpenXR runtimes handle it properly but the standard says that it must be a valid session. I'll change it.

Created attachment 415435 [details] Patch

Comment on attachment 415435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415435&action=review > Source/WebCore/Modules/webxr/WebXRSession.cpp:61 > + // If no other features of the user agent have done so already, perform the necessary platform-specific steps to > // initialize the device's tracking and rendering capabilities, including showing any necessary instructions to the user. This seems like a comment that should be in the initializeTrackingAndRendering function, not here at the call site. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:37 > -template<typename T, XrStructureType StructureType> > +template <typename T, XrStructureType StructureType> Not sure we should make this change. I, for one, prefer the other form. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:65 > WTF_MAKE_FAST_ALLOCATED; > + > public: Should use WTF_MAKE_STRUCT_FAST_ALLOCATED so we don’t have to explicitly use public here. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:243 > + , m_session(XR_NULL_HANDLE) Can we do this in the class definition instead of here in the constructor? > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:397 > + return XR_VIEW_CONFIGURATION_TYPE_MAX_ENUM; Why this value? I think it would be better to return a valid value in this unreachable case. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:59 > + void shutdownTrackingAndRendering() final; The verb "shut down" is two words, so normally we’d want the "d" in "shutDown" here to be capitalized. The word "shutdown" is a noun or adjective. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:74 > + XrViewConfigurationType m_currentViewConfiguration; Not sure a "type" should be named "configuration".

Committed r271288: <https://trac.webkit.org/changeset/271288>

Comment on attachment 415435 [details] Patch What happens if the device disconnects or shuts down the session itself? Does that go through ::end()? Also, I'm not sure we should fire the "end" event until we've really shutdown the device, which might not be immediate.