[webkit-reviews] review granted: [Bug 213555] [WebXR] Implement WebXRSession::updateRenderState() : [Attachment 402648] 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 402648: Patch

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




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

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

Looks OK, generally not sure what our strategy is to get all this right. Adding
all this code without test coverage worries me.

> Source/WebCore/Modules/webxr/WebXRRenderState.cpp:57
> +WebXRRenderState::WebXRRenderState(const WebXRRenderState& other)

Can we just write this instead?

    WebXRRenderState::WebXRRenderState(const WebXRRenderState&) = default;

I don’t see anything in the function that’s different from that.

>> Source/WebCore/Modules/webxr/WebXRRenderState.h:48
>> +	WebXRRenderState(const WebXRRenderState&);
> 
> That is probably not great to have this one as a public constructor given
this is a RefCounted object.
> Also, it might be best to pass the parameters, or have something like
Ref<WebXRRenderState> clone() for instance

Yes, this should be private.

> Source/WebCore/Modules/webxr/WebXRRenderState.h:57
> +    void setInlineVerticalFieldOfView(double fieldOfView) {
m_inlineVerticalFieldOfView = fieldOfView; }

No way to set it back to null?

>> Source/WebCore/Modules/webxr/WebXRRenderState.h:59
>> +	RefPtr<WebXRWebGLLayer> baseLayer() const { return m_baseLayer; }
> 
> WebXRWebGLLayer*

Not so sure about that. Isn’t our new path to always return RefPtr as part of
our long term security hardening?

>> Source/WebCore/Modules/webxr/WebXRRenderState.h:60
>> +	void setBaseLayer(RefPtr<WebXRWebGLLayer> baseLayer) { m_baseLayer =
baseLayer; }
> 
> RefPtr<>&& and WTFMove

Given how we’re using it, a place where it seems we do have a RefPtr we can
move, that seems a good choice. But, in general, setters like this can also
just take WebXRWebGLLayer*. Can’t think of any case where we’d just want to
take a RefPtr by value.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:127
> +	   m_pendingRenderState = makeRefPtr(new
WebXRRenderState(*m_activeRenderState));

This is not a good pattern. We should be calling a create function in
WebXRRenderState, not calling new directly here.

Note that where we do call new directly, it should be makeRef(*new), not
makeRefPtr(new).

> Source/WebCore/Modules/webxr/WebXRSession.idl:42
> +    [MayThrowException] void updateRenderState(optional XRRenderStateInit
stateInit);

I’m a little surprised that [MayThrowException] is still needed. We should
improve IDL code generation so it’s not.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.h:75
> +    const WebXRSession& session() { return m_session.get(); }

The call to ".get()" here should be unnecessary.

> Source/WebCore/Modules/webxr/XRRenderStateInit.h:42
> +    Optional<Vector<RefPtr<WebXRLayer>>> layers;

Should be Vector<Ref>, not Vector<RefPtr>.

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

I am surprised that these "?" are needed in a dictionary. In a dictionary,
optional is the default. What function does the "?" serve? Something about null
rather than omitting? Do we have test coverage? Adding all this code with no
tests seems risky. We could have a lot of things wrong and we wouldn’t know.