[webkit-reviews] review granted: [Bug 182692] [WebVR][OpenVR] Implement getVRDisplays() : [Attachment 334028] Patch

[bugzilla-daemon at webkit.org]

Zan Dobersek <zan at falconsigh.net> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 182692: [WebVR][OpenVR] Implement getVRDisplays()
https://bugs.webkit.org/show_bug.cgi?id=182692

Attachment 334028: Patch

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




--- Comment #7 from Zan Dobersek <zan at falconsigh.net> ---
Comment on attachment 334028
  --> https://bugs.webkit.org/attachment.cgi?id=334028
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334028&action=review

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:52
> +	   for (auto platformDisplay : platformDisplays.value())

auto&, otherwise you're copying the WeakPtr object here. Nothing wrong with
that in itself, but it's not efficient.

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:53
> +	       displays.append(VRDisplay::create(context, platformDisplay));

The WeakPtr object should be moved into the VRDisplay::create() call here.

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:55
> +	   });

This indentation is off.

> Source/WebCore/Modules/webvr/VRDisplay.cpp:39
> +    auto display = adoptRef(*new VRDisplay(context, platformDisplay));

platformDisplay should be moved ...

> Source/WebCore/Modules/webvr/VRDisplay.cpp:44
> +VRDisplay::VRDisplay(ScriptExecutionContext& context,
WeakPtr<VRPlatformDisplay> platformDisplay)

... into the WeakPtr<VRPlatformDisplay>&& parameter.

> Source/WebCore/Modules/webvr/VRDisplay.cpp:57
> -    return false;
> +    return m_display->getDisplayInfo().isConnected;

This should check that m_display is still valid.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:34
> +enum VRDisplayCapabilityFlags {
> +    None = 0,
> +    Position = 1 << 1,
> +    Orientation = 1 << 2,
> +    ExternalDisplay = 1 << 3,
> +    Present = 1 << 4
> +};

IMO the enum should be named `VRDisplayCapabilityFlag`, and a
`VRDisplayCapabilityFlags` should be aliased to the `unsigned` type, but up to
you.

You can take this up in a separate patch, perhaps moving the whole enum and
type definition into a separate header so that the whole VRPlatformDisplay.h
header isn't included in VRDisplayCapabilities.h.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:57
> +    WeakPtr<VRPlatformDisplay> createWeakPtr() { return
m_weakPtrFactory.createWeakPtr(*this); }
> +private:
> +
> +    WeakPtrFactory<VRPlatformDisplay> m_weakPtrFactory;

Space oddity.

> Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.h:40
> +    vr::IVRSystem *m_system;
> +    vr::IVRChaperone *m_chaperone;
> +    vr::IVRCompositor *m_compositor;

Should be using WebKit pointer variable style.