Bug 224353 – Initial implementation of WebChromeClient::enumerateImmersiveXRDevices() and XRDeviceProxy

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ada Chan]

Initial implementation of WebChromeClient::enumerateImmersiveXRDevices() and XRDeviceProxy

[Ada Chan]

Created attachment 425571 [details]
Patch

[Tim Horton]

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?

[Dean Jackson]

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 =

[Ada Chan]

(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.

[Ada Chan]

(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!

[Ada Chan]

Created attachment 425688 [details]
Patch

[EWS]

Patch 425688 does not build

[Ada Chan]

Created attachment 425768 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/76552456>