[webkit-reviews] review granted: [Bug 216925] [WebXR] Initial implemention of device initialization/shutdown with OpenXR : [Attachment 415435] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 216925: [WebXR] Initial implemention of device initialization/shutdown with
OpenXR
https://bugs.webkit.org/show_bug.cgi?id=216925

Attachment 415435: Patch

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




--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 415435
  --> https://bugs.webkit.org/attachment.cgi?id=415435
Patch

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

> Source/WebCore/Modules/webxr/WebXRSession.cpp:61
> +    // If no other features of the user agent have done so already, perform
the necessary platform-specific steps to
>      // initialize the device's tracking and rendering capabilities,
including showing any necessary instructions to the user.

This seems like a comment that should be in the initializeTrackingAndRendering
function, not here at the call site.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:37
> -template<typename T, XrStructureType StructureType>
> +template <typename T, XrStructureType StructureType>

Not sure we should make this change. I, for one, prefer the other form.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:65
>      WTF_MAKE_FAST_ALLOCATED;
> +
>  public:

Should use WTF_MAKE_STRUCT_FAST_ALLOCATED so we don’t have to explicitly use
public here.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:243
> +    , m_session(XR_NULL_HANDLE)

Can we do this in the class definition instead of here in the constructor?

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:397
> +    return XR_VIEW_CONFIGURATION_TYPE_MAX_ENUM;

Why this value? I think it would be better to return a valid value in this
unreachable case.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:59
> +    void shutdownTrackingAndRendering() final;

The verb "shut down" is two words, so normally we’d want the "d" in "shutDown"
here to be capitalized.

The word "shutdown" is a noun or adjective.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.h:74
> +    XrViewConfigurationType m_currentViewConfiguration;

Not sure a "type" should be named "configuration".