[webkit-reviews] review granted: [Bug 213555] [WebXR] Implement WebXRSession::updateRenderState() : [Attachment 406505] 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 213555: [WebXR] Implement WebXRSession::updateRenderState()
https://bugs.webkit.org/show_bug.cgi?id=213555

Attachment 406505: Patch

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




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

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

> Source/WebCore/Modules/webxr/WebXRRenderState.cpp:63
> +WebXRRenderState::WebXRRenderState(const WebXRRenderState& other)
>  {
> -    return m_outputCanvas.get();
> +    m_baseLayer = other.baseLayer();
> +    m_depth = other.m_depth;
> +    m_inlineVerticalFieldOfView = other.m_inlineVerticalFieldOfView;
>  }

Would be nicer to use construction syntax rather than assignment.

> Source/WebCore/Modules/webxr/WebXRRenderState.h:31
> +#include "HTMLCanvasElement.h"
> +#include "WebXRWebGLLayer.h"

Seems like if you add includes of these headers you can remove some (many?) of
the others.

> Source/WebCore/Modules/webxr/WebXRRenderState.h:50
> +    Ref<WebXRRenderState> clone();

I think this should be const. Do you agree?

> Source/WebCore/Modules/webxr/WebXRRenderState.h:62
> +    void setBaseLayer(WebXRWebGLLayer* baseLayer) { m_baseLayer =
WTFMove(baseLayer); }

WTFMove has no effect on a raw pointer, so please omit it.

> Source/WebCore/Modules/webxr/WebXRRenderState.h:65
> +    HTMLCanvasElement* outputCanvas() const { return m_outputCanvas.get(); }
>  private:

We’d normally have a blank line here.

> Source/WebCore/Modules/webxr/WebXRRenderState.h:66
>      explicit WebXRRenderState(Optional<double>&& fieldOfView);

Seems unnecessary to use && for an optional of a scalar. Move is not more
efficient than copy.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:90
> +static inline bool isImmersive(XRSessionMode mode)

No real need to write "inline" here. Obviously we want this inlined, but I
don’t think the keyword has effect on whether it is.

> Source/WebCore/Modules/webxr/XRRenderStateInit.idl:34
>      WebXRWebGLLayer? baseLayer;
> +    sequence<WebXRLayer>? layers;

Everything in a dictionary is optional by default. So I am puzzled by the "?"
on these two lines.