Bug 213555

Created attachment 402648 [details] Patch

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

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.

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?

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.

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

What's the status here?

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

Created attachment 406505 [details] Patch

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

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.

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

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

<rdar://problem/67054734>

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