Bug 217752

Created attachment 411442 [details] Patch

Created attachment 411443 [details] Patch

Gentle ping for reviewers

Comment on attachment 411443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411443&action=review > Source/WebCore/platform/xr/PlatformXR.h:79 > - void enumerateImmersiveXRDevices(CompletionHandler<void(const Vector<std::unique_ptr<Device>>&)>&&); > + using DeviceList = Vector<std::unique_ptr<Device>>; > + void enumerateImmersiveXRDevices(CompletionHandler<void(DeviceList*)>&&); Is there a functionality-based reason behind this change? It's more assuring having to deal with an empty vector than a null pointer, plus you probably then need extra handling of a non-null pointer to an empty vector. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:198 > +#else > +{ > +} > +#endif // USE_OPENXR I would recommend separately guarding the m_workQueue initialization and the internals of the constructor implementation. Guarding the body of the constructor this way leans too much into the preprocessor IMO, in the sense that the preprocessor is used to considerably change the formation of the implementation file, instead of just changing behavior of specific elements of the implementation (in this case the constructor's initialization list and body).

Comment on attachment 411443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411443&action=review >> Source/WebCore/platform/xr/PlatformXR.h:79 >> + void enumerateImmersiveXRDevices(CompletionHandler<void(DeviceList*)>&&); > > Is there a functionality-based reason behind this change? It's more assuring having to deal with an empty vector than a null pointer, plus you probably then need extra handling of a non-null pointer to an empty vector. Just to avoid creating an empty vector in the error case. It does not require any extra handling but we can switch back to empty vectors. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:198 >> +#endif // USE_OPENXR > > I would recommend separately guarding the m_workQueue initialization and the internals of the constructor implementation. > > Guarding the body of the constructor this way leans too much into the preprocessor IMO, in the sense that the preprocessor is used to considerably change the formation of the implementation file, instead of just changing behavior of specific elements of the implementation (in this case the constructor's initialization list and body). Fair enough, I'll change it.

Comment on attachment 411443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411443&action=review >>> Source/WebCore/platform/xr/PlatformXR.h:79 >>> + void enumerateImmersiveXRDevices(CompletionHandler<void(DeviceList*)>&&); >> >> Is there a functionality-based reason behind this change? It's more assuring having to deal with an empty vector than a null pointer, plus you probably then need extra handling of a non-null pointer to an empty vector. > > Just to avoid creating an empty vector in the error case. It does not require any extra handling but we can switch back to empty vectors. An empty vector is cheap so it is reasonable to use a const ref than a const pointer.

Created attachment 411747 [details] Patch

Comment on attachment 411747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411747&action=review > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:66 > + WorkQueue& m_queue; Can this be strong reference (Ref<WorkQueue>) to the WorkQueue, or are you afraid of a cycle?

(In reply to Sam Weinig from comment #8) > Comment on attachment 411747 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411747&action=review > > > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:66 > > + WorkQueue& m_queue; > > Can this be strong reference (Ref<WorkQueue>) to the WorkQueue, or are you > afraid of a cycle? There shouldn't be any cycle because devices do not hold references to the PlatformXR::Instance. However I'm using a reference because the ownership of the queue is not transferred. According to [1] (I know it's a bit old) " * If ownership and lifetime are guaranteed, a data member can be a raw reference or pointer. * If the class needs to hold ownership or guarantee lifetime, the data member should be a Ref or RefPtr." That's why I preferred to use a reference, but I don't have a strong opinion here, so I can switch to a Ref if you think it's more appropriate. [1] https://webkit.org/blog/5381/refptr-basics/

Comment on attachment 411747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411747&action=review > Source/WebCore/Modules/webxr/WebXRSystem.cpp:94 > + }); Why not callOnMainThread(WTFMove(callback))? > Source/WebCore/platform/xr/PlatformXR.h:78 > + using DeviceList = Vector<std::unique_ptr<Device>>; Can we use a Vector<UniqueRef<>>? > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:232 > + callback(emptyList); We can probably use callback({ }) > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:254 > + m_immersiveXRDevices.append(makeUnique<OpenXRDevice>(systemId, m_impl->xrInstance(), m_impl->queue())); How are we sure this is valid here? Should we instead use a weak pointer? Something like: m_impl->queue().dispatch([weakThis = makeWeakPtr(this)...] ... callOnMainThread([weakThis = WTFMove(weakThis)...] { if (!weakThis) { callback({ }); return; } ... }); > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:257 > + callback(m_immersiveXRDevices); Ditto here. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:262 > + callbackOnExit.release(); Just my taste but it reads better if callbackOnExit.release(); is called just before moving callback in callOnMainThread lambda.

Comment on attachment 411747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411747&action=review Thanks for the review! Uploading a new patch soon. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:94 >> + }); > > Why not callOnMainThread(WTFMove(callback))? Oh indeed. I had some debug code inside that block, that's why I didn't realize about it. >> Source/WebCore/platform/xr/PlatformXR.h:78 >> + using DeviceList = Vector<std::unique_ptr<Device>>; > > Can we use a Vector<UniqueRef<>>? Makes total sense to me. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:232 >> + callback(emptyList); > > We can probably use callback({ }) Right, now that we're returning a const reference it should be indeed possible. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:254 >> + m_immersiveXRDevices.append(makeUnique<OpenXRDevice>(systemId, m_impl->xrInstance(), m_impl->queue())); > > How are we sure this is valid here? Should we instead use a weak pointer? > Something like: > m_impl->queue().dispatch([weakThis = makeWeakPtr(this)...] > ... > callOnMainThread([weakThis = WTFMove(weakThis)...] { > if (!weakThis) { > callback({ }); > return; > } > ... > }); Because this is a singleton created with a LazyNeverDestroyed. >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:262 >> + callbackOnExit.release(); > > Just my taste but it reads better if callbackOnExit.release(); is called just before moving callback in callOnMainThread lambda. Sure, not strong feelings about that.

Created attachment 411853 [details] Patch

Comment on attachment 411853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411853&action=review > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:235 > + m_immersiveXRDevices.clear(); We are modifying m_immersiveXRDevices in the work queue but later append to it in the main thread. This is probably unsafe. We could probably simply remove this clear here and clear it when appending. Instead of appending, we could do something like m_immersiveXRDevices = Vector::from(make...). > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:254 > + // device initialization is completed. This seems a bit weird to do that handling here. Could we do something like: - create the device - have the device an initialise method with a completion handler. - call callback when initialise completion handler is called?

(In reply to youenn fablet from comment #13) > Comment on attachment 411853 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411853&action=review > > > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:235 > > + m_immersiveXRDevices.clear(); > > We are modifying m_immersiveXRDevices in the work queue but later append to > it in the main thread. > This is probably unsafe. > We could probably simply remove this clear here and clear it when appending. > Instead of appending, we could do something like m_immersiveXRDevices = > Vector::from(make...). Makes sense. > > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:254 > > + // device initialization is completed. > > This seems a bit weird to do that handling here. > Could we do something like: > - create the device > - have the device an initialise method with a completion handler. > - call callback when initialise completion handler is called? Seems cleaner indeed.

Created attachment 411967 [details] Patch Finally decided to add the callback to the constructor. Creating an init() method was kind of artificial, forced us to expose it just for this case and also required adding type specialization traits to downcast the Devices in the DeviceList to OpenXR devices

Comment on attachment 411967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411967&action=review > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:250 > + m_immersiveXRDevices.clear(); clear not needed. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:255 > + })); It might read better like this: prepareDevice(systemId, m_impl->xrInstance(), m_impl->queue(), [...](auto&& device) { m_immersiveXRDevices = DeviceList::from(WTFMove(device)); callback(m_immersiveXRDevices); }); It seems to me though that if m_immersiveXRDevices already has a device which is matching systemId and instance, we might just be able to return m_immersiveXRDevices directly?

<rdar://problem/70572576>

Comment on attachment 411967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411967&action=review >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:255 >> + })); > > It might read better like this: > prepareDevice(systemId, m_impl->xrInstance(), m_impl->queue(), [...](auto&& device) { > m_immersiveXRDevices = DeviceList::from(WTFMove(device)); > callback(m_immersiveXRDevices); > }); > It seems to me though that if m_immersiveXRDevices already has a device which is matching systemId and instance, we might just be able to return m_immersiveXRDevices directly? Not sure what prepareDevice() would look like. Note that we pass a callback to the constructor to ensure that the device has properly initialized everything in the work queue. I don't know how you could achieve that unless you move all the code in the constructor to that prepareDevice() method. But you cannot do that because the code in the OpenXRDevice constructor initializes several internal attributes of OpenXRDevice. I had previously thought about that but I concluded that it would be better to do everything inside the OpenXRDevice class mainly for encapsulation (although I agree that having a completion callback for a constructor is not very usual).

Created attachment 412120 [details] Patch for landing

(In reply to Sergio Villar Senin from comment #18) > Comment on attachment 411967 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411967&action=review > > >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:255 > >> + })); > > > > It might read better like this: > > prepareDevice(systemId, m_impl->xrInstance(), m_impl->queue(), [...](auto&& device) { > > m_immersiveXRDevices = DeviceList::from(WTFMove(device)); > > callback(m_immersiveXRDevices); > > }); > > It seems to me though that if m_immersiveXRDevices already has a device which is matching systemId and instance, we might just be able to return m_immersiveXRDevices directly? > > Not sure what prepareDevice() would look like. Note that we pass a callback > to the constructor to ensure that the device has properly initialized > everything in the work queue. I don't know how you could achieve that unless > you move all the code in the constructor to that prepareDevice() method. But > you cannot do that because the code in the OpenXRDevice constructor > initializes several internal attributes of OpenXRDevice. I had previously > thought about that but I concluded that it would be better to do everything > inside the OpenXRDevice class mainly for encapsulation (although I agree > that having a completion callback for a constructor is not very usual). I'd like to hear your opinion anyway about what I wrote above, but note that in the last patch I have just uploaded, the code looks better because I noticed that the OpenXRDevice should call the callback in the main thread because the CompletionHandler was created in the main thread. So the final code looks like: m_immersiveXRDevices = DeviceList::from(...., [..., callback = WTFMove(callback)]() { ASSERT(isMainThread()); callback(m_immersiveXRDevices); }); which still requires the completion handler in the constructor but is much more readable.

(In reply to Sergio Villar Senin from comment #20) > (In reply to Sergio Villar Senin from comment #18) > > Comment on attachment 411967 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=411967&action=review > > > > >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:255 > > >> + })); > > > > > > It might read better like this: > > > prepareDevice(systemId, m_impl->xrInstance(), m_impl->queue(), [...](auto&& device) { > > > m_immersiveXRDevices = DeviceList::from(WTFMove(device)); > > > callback(m_immersiveXRDevices); > > > }); > > > It seems to me though that if m_immersiveXRDevices already has a device which is matching systemId and instance, we might just be able to return m_immersiveXRDevices directly? > > > > Not sure what prepareDevice() would look like. Note that we pass a callback > > to the constructor to ensure that the device has properly initialized > > everything in the work queue. I don't know how you could achieve that unless > > you move all the code in the constructor to that prepareDevice() method. But > > you cannot do that because the code in the OpenXRDevice constructor > > initializes several internal attributes of OpenXRDevice. I had previously > > thought about that but I concluded that it would be better to do everything > > inside the OpenXRDevice class mainly for encapsulation (although I agree > > that having a completion callback for a constructor is not very usual). > > I'd like to hear your opinion anyway about what I wrote above, but note that > in the last patch I have just uploaded, the code looks better because I > noticed that the OpenXRDevice should call the callback in the main thread > because the CompletionHandler was created in the main thread. So the final > code looks like: > > m_immersiveXRDevices = DeviceList::from(...., [..., callback = > WTFMove(callback)]() { > ASSERT(isMainThread()); > callback(m_immersiveXRDevices); > }); > > which still requires the completion handler in the constructor but is much > more readable. Youenn ping ^^^

Comment on attachment 412120 [details] Patch for landing LGTM. The callback on constructor feels a bit weird but I guess this is fine. Ideally, we would not need to hop to background thread, then hop to main thread to create the device, then hop to background thread to initialise it, then hop to main thread to continue processing. We would do something like: hop to background thread, create/initialize device, hop to main thread with the device (or a cross thread safe device structure), create the vector and continue processing. Might be worth trying to achieve this in a follow-up patch.

Committed r269032: <https://trac.webkit.org/changeset/269032>