[webkit-reviews] review granted: [Bug 213022] [WebXR] Add a preliminary implementation of XRWebGLLayer : [Attachment 401538] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 213022: [WebXR] Add a preliminary implementation of XRWebGLLayer
https://bugs.webkit.org/show_bug.cgi?id=213022

Attachment 401538: Patch

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




--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 401538
  --> https://bugs.webkit.org/attachment.cgi?id=401538
Patch

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

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:66
> +

Extra line here

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:78
> +		   // TODO: Initialize layer's resources or issue an
OperationError.

Use FIXME instead of TODO

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:94
> +    // TODO: implement this

Ditto.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:120
> +    m_isCompositionDisabled = m_session->mode() == XRSessionMode::Inline ?
true : false;

I guess you don't need the ? true : false

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:125
> +	   m_antialias = init.antialias;

This is duplicated, I would probably remove the previous one.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:129
> +	   computeRecommendedWebGLFramebufferResolution(recommendedSize);

computeRecommendedWebGLFramebufferResolution could return IntSize instead of
using a parameter.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:137
> +	   // TODO: create a proper opaque framebuffer.

FIXME

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:207
> +    IntSize nativeSize;
> +    computeNativeWebGLFramebufferResolution(nativeSize);

Better use return value instead of parameter.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:210
> +    IntSize recommendedSize;
> +    computeRecommendedWebGLFramebufferResolution(recommendedSize);

And here too. Unless those they really need to receive an existing IntSize to
be modified.