[webkit-reviews] review granted: [Bug 224353] Initial implementation of WebChromeClient::enumerateImmersiveXRDevices() and XRDeviceProxy : [Attachment 425571] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Ada Chan <adachan at apple.com>'s
request for review:
Bug 224353: Initial implementation of
WebChromeClient::enumerateImmersiveXRDevices() and XRDeviceProxy
https://bugs.webkit.org/show_bug.cgi?id=224353

Attachment 425571: Patch

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




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