Bug 213555 – [WebXR] Implement WebXRSession::updateRenderState()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[WebXR] Implement WebXRSession::updateRenderState()

[Sergio Villar Senin]

Created attachment 402648 [details]
Patch

[youenn fablet]

Comment on attachment 402648 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRRenderState.cpp:59
> +    m_depth = other.m_depth;

Would be better as m_depth(...), ...

> 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

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

WebXRWebGLLayer*

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

RefPtr<>&& and WTFMove

[Darin Adler]

Comment on attachment 402648 [details]
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.

[Sergio Villar Senin]

Comment on attachment 402648 [details]
Patch

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

Thanks Darin & Youenn!

>> 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.

Hmm indeed.

>>> 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.

ACK.

>> Source/WebCore/Modules/webxr/WebXRRenderState.h:57
>> +    void setInlineVerticalFieldOfView(double fieldOfView) { m_inlineVerticalFieldOfView = fieldOfView; }
> 
> No way to set it back to null?

I don't see why we'd like to do that. The getter returns an Optional because it might not be set, but once set it should not even be changed.

>>> 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?

Let's keep it as it then.

>>> 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.

I'll take that into account.

>> 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).

Yeah, makes total sense.

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.h:75
>> +    const WebXRSession& session() { return m_session.get(); }
> 
> The call to ".get()" here should be unnecessary.

Correct.

>> Source/WebCore/Modules/webxr/XRRenderStateInit.h:42
>> +    Optional<Vector<RefPtr<WebXRLayer>>> layers;
> 
> Should be Vector<Ref>, not Vector<RefPtr>.

Indeed.

>> Source/WebCore/Modules/webxr/XRRenderStateInit.idl:34
>> +    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.

Yeah, the "?" is the nullable type https://heycam.github.io/webidl/#dfn-nullable-type.

Regarding test coverage, there is no specific tests in the imported WPT for this code. I was planning to add a test to WPT and then import it. Would you be OK landing this and then importing the test in a follow-up or would it be better to wait until we have the test?

[Darin Adler]

Comment on attachment 402648 [details]
Patch

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

>>> Source/WebCore/Modules/webxr/XRRenderStateInit.idl:34
>>> +    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.
> 
> Yeah, the "?" is the nullable type https://heycam.github.io/webidl/#dfn-nullable-type.
> 
> Regarding test coverage, there is no specific tests in the imported WPT for this code. I was planning to add a test to WPT and then import it. Would you be OK landing this and then importing the test in a follow-up or would it be better to wait until we have the test?

Here’s my thinking:

If we aren’t landing the test at the same time as the code, or using a test that’s already checked in, I can’t check to see that we test both omitting layers and passing null for layers. It’s those kinds of questions that come to mind when the code is new. When the code is already there, it’s super-hard to spot what might be included or omitted in tests.

I’m really not sure what the tradeoffs are here, and the best way for us to work. Whatl I care about is where we *end up*, not the intermediate states, but with this strategy, how will we remember to be thorough about testing? In the context of fuzzer-founded crashes I’ve recently encountered web platform tests that were not all that comprehensive in edge case testing. And I’d really like to help with that if I can. It’s easier when reading code and then looking at tests to see what the code does that isn’t verified by tests. Similarly it’s easier to see when reading specifications and looking at tests to see what the specification states that isn’t verified by tests.

[Sergio Villar Senin]

(In reply to Darin Adler from comment #5)
> Comment on attachment 402648 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402648&action=review
> 
> >>> Source/WebCore/Modules/webxr/XRRenderStateInit.idl:34
> >>> +    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.
> > 
> > Yeah, the "?" is the nullable type https://heycam.github.io/webidl/#dfn-nullable-type.
> > 
> > Regarding test coverage, there is no specific tests in the imported WPT for this code. I was planning to add a test to WPT and then import it. Would you be OK landing this and then importing the test in a follow-up or would it be better to wait until we have the test?
> 
> Here’s my thinking:
> 
> If we aren’t landing the test at the same time as the code, or using a test
> that’s already checked in, I can’t check to see that we test both omitting
> layers and passing null for layers. It’s those kinds of questions that come
> to mind when the code is new. When the code is already there, it’s
> super-hard to spot what might be included or omitted in tests.
> 
> I’m really not sure what the tradeoffs are here, and the best way for us to
> work. Whatl I care about is where we *end up*, not the intermediate states,
> but with this strategy, how will we remember to be thorough about testing?
> In the context of fuzzer-founded crashes I’ve recently encountered web
> platform tests that were not all that comprehensive in edge case testing.
> And I’d really like to help with that if I can. It’s easier when reading
> code and then looking at tests to see what the code does that isn’t verified
> by tests. Similarly it’s easier to see when reading specifications and
> looking at tests to see what the specification states that isn’t verified by
> tests.

Fair enough, I was not comfortable either landing it without proper test coverage. I'll work on the test then. It's generally easier from the WebKit POV to land it first in WPT and then import it (exporting is still far from automatic), so I guess that's the approach I'll follow.

[Nikolas Zimmermann]

What's the status here?

[Sergio Villar Senin]

(In reply to Nikolas Zimmermann from comment #7)
> What's the status here?

We have to land & then import https://github.com/web-platform-tests/wpt/pull/24415 first. It's the test.

[Sergio Villar Senin]

Created attachment 406505 [details]
Patch

[Sergio Villar Senin]

Test was imported in bug 215224 so we do have now a test for this change. Darin, mind taking another look please?

[Darin Adler]

Comment on attachment 406505 [details]
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.

[Sergio Villar Senin]

Comment on attachment 406505 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/Modules/webxr/WebXRRenderState.cpp:63
>>  }
> 
> Would be nicer to use construction syntax rather than assignment.

OK

>> Source/WebCore/Modules/webxr/WebXRRenderState.h:31
>> +#include "WebXRWebGLLayer.h"
> 
> Seems like if you add includes of these headers you can remove some (many?) of the others.

Good point.

>> Source/WebCore/Modules/webxr/WebXRRenderState.h:50
>> +    Ref<WebXRRenderState> clone();
> 
> I think this should be const. Do you agree?

Sure!

>> 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.

Right, it was a && but then I replaced it by a raw pointer and forgot about the move

>> 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.

Right.

>> Source/WebCore/Modules/webxr/XRRenderStateInit.idl:34
>> +    sequence<WebXRLayer>? layers;
> 
> Everything in a dictionary is optional by default. So I am puzzled by the "?" on these two lines.

I think we talked about that before. The "?" specifies that the type is nullable. As you said it's optional by default but apart from that we're specifying that null is a valid value for the type. WebIDL distinguish between
- optional: a valid value would be "undefined"
- nullable: a valid value would be null

[Sergio Villar Senin]

Committed r265665: <https://trac.webkit.org/changeset/265665>

[Radar WebKit Bug Importer]

<rdar://problem/67054734>

[Darin Adler]

Comment on attachment 406505 [details]
Patch

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

>>> Source/WebCore/Modules/webxr/XRRenderStateInit.idl:34
>>> +    sequence<WebXRLayer>? layers;
>> 
>> Everything in a dictionary is optional by default. So I am puzzled by the "?" on these two lines.
> 
> I think we talked about that before. The "?" specifies that the type is nullable. As you said it's optional by default but apart from that we're specifying that null is a valid value for the type. WebIDL distinguish between
> - optional: a valid value would be "undefined"
> - nullable: a valid value would be null

We have tests that fail when we take out either one of those "?"