Initial implementation of WebChromeClient::enumerateImmersiveXRDevices() and XRDeviceProxy
Created attachment 425571 [details] Patch
Comment on attachment 425571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425571&action=review Does not seem outrageous but I didn't quite finish > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3410 > + bool hasArray = WTF::holds_alternative<std::array<float, 16>>(projection); Separately I wonder if we should give this std::array<float, 16> a typename > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3441 > + for (size_t i = 0; i < 16; ++i) { ...and if there's a way to extract this length from it, or otherwise not have to hard code this 16 here.. > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3539 > +#endif // ENABLE(WEBXR) Remind me why these are all here instead of down in WebCore with their types? I feel like you had a reason in one case, but the rest of these all seem pretty trivial?
Comment on attachment 425571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425571&action=review > Source/WebKit/Shared/Cocoa/XRDeviceProxy.h:31 > +#include <WebCore/PlatformXR.h> Maybe include WeakPtr.h too? You're getting it from somewhere but that might not always be the case with unified builds. Although maybe that's silly since the same applies for Vector, Ref and Optional. > Source/WebKit/Shared/Cocoa/XRDeviceProxy.mm:56 > + if (m_trackingAndRenderingClient) > + m_trackingAndRenderingClient->sessionDidEnd(); I'm surprised this isn't done via an accessor. > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3451 > + return { nullptr }; Is that different from a nullopt? > Source/WebKit/UIProcess/Cocoa/PlatformXRCoordinator.h:45 > + // Session creation/termination Nit: End comment with . > Source/WebKit/UIProcess/Cocoa/PlatformXRCoordinator.h:46 > + using OnSessionEndCallback = WTF::Function<void(XRDeviceIdentifier)>; No need for WTF:: > Source/WebKit/UIProcess/Cocoa/PlatformXRCoordinator.h:50 > + // Session display loop Ditto. > Source/WebKit/UIProcess/Cocoa/PlatformXRSystem.h:52 > + void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override; I think this can be final. > Source/WebKit/UIProcess/Cocoa/PlatformXRSystem.mm:92 > + if (PlatformXRCoordinator* xrCoordinator = PlatformXRSystem::xrCoordinator()) auto* > Source/WebKit/UIProcess/Cocoa/PlatformXRSystem.mm:98 > + if (PlatformXRCoordinator* xrCoordinator = PlatformXRSystem::xrCoordinator()) auto* > Source/WebKit/WebProcess/cocoa/PlatformXRSystemProxy.mm:61 > + RefPtr<XRDeviceProxy> device = deviceByIdentifier(deviceInfo.identifier); auto device = You could also do this in the if test statement if you like. > Source/WebKit/WebProcess/cocoa/PlatformXRSystemProxy.mm:89 > + RefPtr<XRDeviceProxy> device = deviceByIdentifier(deviceIdentifier); auto device = Could also be in the if statement test. > Source/WebKit/WebProcess/cocoa/PlatformXRSystemProxy.mm:97 > + XRDeviceProxy* deviceProxy = static_cast<XRDeviceProxy*>(device.ptr()); auto* deviceProxy =
(In reply to Tim Horton from comment #2) > Comment on attachment 425571 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425571&action=review > > Does not seem outrageous but I didn't quite finish > > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3410 > > + bool hasArray = WTF::holds_alternative<std::array<float, 16>>(projection); > > Separately I wonder if we should give this std::array<float, 16> a typename > > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3441 > > + for (size_t i = 0; i < 16; ++i) { > > ...and if there's a way to extract this length from it, or otherwise not > have to hard code this 16 here.. I give this a typename and define a constant for its length. > > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3539 > > +#endif // ENABLE(WEBXR) > > Remind me why these are all here instead of down in WebCore with their > types? I feel like you had a reason in one case, but the rest of these all > seem pretty trivial? Yep, I'll move these down to WebCore.
(In reply to Dean Jackson from comment #3) > Comment on attachment 425571 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425571&action=review > > > Source/WebKit/Shared/Cocoa/XRDeviceProxy.h:31 > > +#include <WebCore/PlatformXR.h> > > Maybe include WeakPtr.h too? You're getting it from somewhere but that might > not always be the case with unified builds. Although maybe that's silly > since the same applies for Vector, Ref and Optional. I'll include them all just in case. > > > Source/WebKit/Shared/Cocoa/XRDeviceProxy.mm:56 > > + if (m_trackingAndRenderingClient) > > + m_trackingAndRenderingClient->sessionDidEnd(); > > I'm surprised this isn't done via an accessor. Will add an accessor. > > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3451 > > + return { nullptr }; > > Is that different from a nullopt? PlatformXR::Device::FrameData::Projection is a Variant where std::nullptr_t is one of the possible types. I'm not sure if the original author considered using an Optional here. We can consider this in a future patch. > > > Source/WebKit/UIProcess/Cocoa/PlatformXRCoordinator.h:45 > > + // Session creation/termination > > Nit: End comment with . Updated. > > > Source/WebKit/UIProcess/Cocoa/PlatformXRCoordinator.h:46 > > + using OnSessionEndCallback = WTF::Function<void(XRDeviceIdentifier)>; > > No need for WTF:: Updated. > > > Source/WebKit/UIProcess/Cocoa/PlatformXRCoordinator.h:50 > > + // Session display loop > > Ditto. Updated. > > > Source/WebKit/UIProcess/Cocoa/PlatformXRSystem.h:52 > > + void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override; > > I think this can be final. Fixed. > > > Source/WebKit/UIProcess/Cocoa/PlatformXRSystem.mm:92 > > + if (PlatformXRCoordinator* xrCoordinator = PlatformXRSystem::xrCoordinator()) > > auto* > > > Source/WebKit/UIProcess/Cocoa/PlatformXRSystem.mm:98 > > + if (PlatformXRCoordinator* xrCoordinator = PlatformXRSystem::xrCoordinator()) > > auto* > > > Source/WebKit/WebProcess/cocoa/PlatformXRSystemProxy.mm:61 > > + RefPtr<XRDeviceProxy> device = deviceByIdentifier(deviceInfo.identifier); > > auto device = > > You could also do this in the if test statement if you like. > > > Source/WebKit/WebProcess/cocoa/PlatformXRSystemProxy.mm:89 > > + RefPtr<XRDeviceProxy> device = deviceByIdentifier(deviceIdentifier); > > auto device = > > Could also be in the if statement test. > > > Source/WebKit/WebProcess/cocoa/PlatformXRSystemProxy.mm:97 > > + XRDeviceProxy* deviceProxy = static_cast<XRDeviceProxy*>(device.ptr()); > > auto* deviceProxy = Updated the above with auto and if test statement. Thanks for all the feedback, Dean and Tim!
Created attachment 425688 [details] Patch
Patch 425688 does not build
Created attachment 425768 [details] Patch
Committed r275835 (236404@main): <https://commits.webkit.org/236404@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425768 [details].
<rdar://problem/76552456>