Bug 182999 – [WebVR][OpenVR] Retrieve displayId and the z-depth of eye view frustum

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[WebVR][OpenVR] Retrieve displayId and the z-depth of eye view frustum

[Sergio Villar Senin]

Created attachment 334365 [details]
Patch

[Zan Dobersek]

Comment on attachment 334365 [details]
Patch

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

> Source/WebCore/Modules/webvr/VRDisplay.cpp:49
> +    , m_depthNear(0.01) // Default value from spec.
> +    , m_depthFar(10000) // Default value from spec.

Members that are being initialized with such constants can be initialized at the point of declaration.

> Source/WebCore/Modules/webvr/VRDisplay.h:110
>      String m_displayName;
> +    unsigned m_displayId;

Nit: these should follow the order of the relevant getters.

> Source/WebCore/Modules/webvr/VRDisplay.h:113
> +    double m_depthNear;
> +    double m_depthFar;

For instance:

    double m_depthNear { 0.01 };
    double m_depthFar { 10000 };

> Source/WebCore/platform/vr/VRManager.cpp:64
> +unsigned long VRManager::s_displayIdentifier = 0;
> +
> +unsigned long VRManager::generateUniqueDisplayIdentifier()
> +{
> +    return ++s_displayIdentifier;
> +}

I think this should be an implementation detail of the OpenVR backend. I.e. some other implementation may have a better way of determining the unique display identifier than to use a static counter.

> Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:36
> +    m_displayInfo.displayIdentifier = VRManager::generateUniqueDisplayIdentifier();

unsigned long is being narrowed down to unsigned.

[Sergio Villar Senin]

Created attachment 334600 [details]
Patch

[Zan Dobersek]

Comment on attachment 334600 [details]
Patch

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

> Source/WebCore/Modules/webvr/VRDisplay.h:63
> +    unsigned displayId() const { return m_displayId; }

displayId attribute on the VRDisplay interface has the 'unsigned long' type, which in IDLTypes.h (through IDUnsignedLong) maps to uint32_t. IMO we should explicitly use uint32_t here too, as well as in the m_displayId declaration in this class.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:50
> +    unsigned long displayIdentifier;

Since this directly maps to the displayId attribute in VRDisplay, this too should be uint32_t.

> Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:38
> +    m_displayInfo.displayIdentifier = ++s_displayIdentifier;

Problem is that in this case you're assigning from a 64-bit value to a 32-bit one. In reality this probably won't be a problem, but it's a small issue.

[Sergio Villar Senin]

Created attachment 334608 [details]
Patch

Patch for landing

[Sergio Villar Senin]

Committed r229014: <https://trac.webkit.org/changeset/229014>

[Radar WebKit Bug Importer]

<rdar://problem/37898645>