Bug 211888

Created attachment 399350 [details] Patch

Comment on attachment 399350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399350&action=review > Source/WebCore/Modules/webxr/WebXRRenderState.h:54 > + WebXRRenderState(Optional<double>&& fieldOfView); explicit > Source/WebCore/Modules/webxr/WebXRRenderState.h:55 > WebXRRenderState(const XRRenderStateInit&); explicit, but is this one used? > Source/WebCore/Modules/webxr/WebXRSession.cpp:38 > +Ref<WebXRSession> WebXRSession::create(ScriptExecutionContext& context, Ref<WebXRSystem>&& system, XRSessionMode mode, WeakPtr<PlatformXR::Device>&& device) Is there a plan to ship these in Workers? Should we use Document& directly? > Source/WebCore/Modules/webxr/WebXRSession.cpp:49 > + m_activeRenderState = WebXRRenderState::create(mode); Could be set above directly as m_activeRenderState(...) > Source/WebCore/Modules/webxr/WebXRSession.h:96 > + WebXRInputSourceArray m_inputSources; This should be a Ref<WebXRInputSourceArray> since this is ref counted. We should really make WebXRInputSourceArray constructor private. > Source/WebCore/Modules/webxr/WebXRSession.h:102 > + RefPtr<WebXRRenderState> m_activeRenderState; Should be a Ref instead of a RefPtr > Source/WebCore/Modules/webxr/WebXRSystem.cpp:113 > +PlatformXR::Device* WebXRSystem::obtainCurrentDevice(XRSessionMode mode, const JSFeaturesArray& requiredFeatures, const JSFeaturesArray& optionalFeatures) Is there a way to return a PlatformXR::Device&? > Source/WebCore/Modules/webxr/WebXRSystem.cpp:167 > +bool WebXRSystem::immersiveSessionRequestIsAllowedForGlobalObject(DOMWindow& globalObject, Document& document) const Do we have tests for all these conditions? > Source/WebCore/Modules/webxr/WebXRSystem.cpp:171 > + if (!globalObject.hasTransientActivation() /* TODO: || !launching a web app */) This TODO is not very clear to me, formatting is also not really right. I guess we should also check that are not launching a web app? > Source/WebCore/Modules/webxr/WebXRSystem.cpp:174 > + // 2. If the requesting document is not considered trustworthy, return false. I initially thought we were talking of SecureContext here. Maybe refer to https://immersive-web.github.io/webxr/#trustworthy here. > Source/WebCore/Modules/webxr/WebXRSystem.cpp:221 > +Optional<WebXRSystem::ResolvedRequestedFeatures> WebXRSystem::resolveRequestedFeatures(XRSessionMode mode, const XRSessionInit& init, WeakPtr<PlatformXR::Device>& device, JSC::JSGlobalObject* globalObject) const Probably want to pass a JSGlobalObject& As of WeakPtr<PlatformXR::Device>&, can we instead pass a Device*? > Source/WebCore/Modules/webxr/WebXRSystem.cpp:241 > + JSFeaturesArray requiredFeaturesWithDefaultFeatures = init.requiredFeatures; auto > Source/WebCore/Modules/webxr/WebXRSystem.cpp:339 > + ASSERT(globalObject); Is it guaranteed? > Source/WebCore/Modules/webxr/WebXRSystem.cpp:360 > + document.postTask([this, immersive, init, mode, promise = WTFMove(promise)] (ScriptExecutionContext& context) mutable { s/ (ScriptExecutionContext/(auto/ Are we sure this is valid in the lambda? > Source/WebCore/Modules/webxr/WebXRSystem.cpp:364 > + PlatformXR::Device* device = obtainCurrentDevice(mode, init.requiredFeatures, init.optionalFeatures); auto > Source/WebCore/Modules/webxr/WebXRSystem.cpp:367 > + queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [this, context = makeRef(context), device = makeWeakPtr(device), immersive, init, mode, promise = WTFMove(promise)] () mutable { We probably do not need to ref the context here.

Comment on attachment 399350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399350&action=review >> Source/WebCore/Modules/webxr/WebXRRenderState.h:54 >> + WebXRRenderState(Optional<double>&& fieldOfView); > > explicit ACK. >> Source/WebCore/Modules/webxr/WebXRRenderState.h:55 >> WebXRRenderState(const XRRenderStateInit&); > > explicit, but is this one used? Not now, but will be. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:38 >> +Ref<WebXRSession> WebXRSession::create(ScriptExecutionContext& context, Ref<WebXRSystem>&& system, XRSessionMode mode, WeakPtr<PlatformXR::Device>&& device) > > Is there a plan to ship these in Workers? Should we use Document& directly? Good point. I'll replace it. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:49 >> + m_activeRenderState = WebXRRenderState::create(mode); > > Could be set above directly as m_activeRenderState(...) Right. >> Source/WebCore/Modules/webxr/WebXRSession.h:96 >> + WebXRInputSourceArray m_inputSources; > > This should be a Ref<WebXRInputSourceArray> since this is ref counted. > We should really make WebXRInputSourceArray constructor private. This was an unintended change. I am not touching the input sources code ATM so I'll just revert this. >> Source/WebCore/Modules/webxr/WebXRSession.h:102 >> + RefPtr<WebXRRenderState> m_activeRenderState; > > Should be a Ref instead of a RefPtr Note that in follow up patches we'll assign the pendingRenderState to the activeRenderState so I think it's better to keep it as a RefPtr. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:113 >> +PlatformXR::Device* WebXRSystem::obtainCurrentDevice(XRSessionMode mode, const JSFeaturesArray& requiredFeatures, const JSFeaturesArray& optionalFeatures) > > Is there a way to return a PlatformXR::Device&? No, this must be a pointer because the output of ensureImmersiveXRDeviceIsSelected() might be a null device (as specified). >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:167 >> +bool WebXRSystem::immersiveSessionRequestIsAllowedForGlobalObject(DOMWindow& globalObject, Document& document) const > > Do we have tests for all these conditions? Yes, although I am not enabling some of them now because we need to implement more API to run them. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:171 >> + if (!globalObject.hasTransientActivation() /* TODO: || !launching a web app */) > > This TODO is not very clear to me, formatting is also not really right. > I guess we should also check that are not launching a web app? Yeah, that's the thing. I am not sure how to do that (not sure even if we can check that). I'll format it as any other comment. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:174 >> + // 2. If the requesting document is not considered trustworthy, return false. > > I initially thought we were talking of SecureContext here. > Maybe refer to https://immersive-web.github.io/webxr/#trustworthy here. OK will do it. BTW, this is the best check I could come up with, not sure if there is a better way to do that. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:221 >> +Optional<WebXRSystem::ResolvedRequestedFeatures> WebXRSystem::resolveRequestedFeatures(XRSessionMode mode, const XRSessionInit& init, WeakPtr<PlatformXR::Device>& device, JSC::JSGlobalObject* globalObject) const > > Probably want to pass a JSGlobalObject& > As of WeakPtr<PlatformXR::Device>&, can we instead pass a Device*? Makes sense. After all I was doing an ASSERT on the pointer. Regarding passing a pointer for the Device, yeah it seems sane to pass it, we don't need a WeakPtr for this call. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:241 >> + JSFeaturesArray requiredFeaturesWithDefaultFeatures = init.requiredFeatures; > > auto OK. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:339 >> + ASSERT(globalObject); > > Is it guaranteed? Hmm, right I guess we cannot guarantee that it was not detached. I'll return an InvalidAccessError in this case. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:360 >> + document.postTask([this, immersive, init, mode, promise = WTFMove(promise)] (ScriptExecutionContext& context) mutable { > > s/ (ScriptExecutionContext/(auto/ > > Are we sure this is valid in the lambda? OK regarding s/. Not sure what you mean by "valid in the lambda". In the case of a document.postTask() the ScripExecutionContext passed to the lambda is the document itself so it has to be valid. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:364 >> + PlatformXR::Device* device = obtainCurrentDevice(mode, init.requiredFeatures, init.optionalFeatures); > > auto OK. > Source/WebCore/Modules/webxr/WebXRSystem.cpp:389 > + return; BTW I replaced this early return with rejection and the above one by a ScopeExit. > Source/WebCore/testing/WebXRTest.cpp:60 > if (auto referenceSpaceType = parseEnumeration<XRReferenceSpaceType>(*context.execState(), feature)) Also added a check to verify that we have a valid globalObject here.

Created attachment 399644 [details] Patch

Comment on attachment 399350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399350&action=review >>> Source/WebCore/Modules/webxr/WebXRSystem.cpp:360 >>> + document.postTask([this, immersive, init, mode, promise = WTFMove(promise)] (ScriptExecutionContext& context) mutable { >> >> s/ (ScriptExecutionContext/(auto/ >> >> Are we sure this is valid in the lambda? > > OK regarding s/. > > Not sure what you mean by "valid in the lambda". In the case of a document.postTask() the ScripExecutionContext passed to the lambda is the document itself so it has to be valid. From a quick look, there is no guarantee the pointer 'this' will stay valid when the lambda executes.

Comment on attachment 399350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399350&action=review >>>> Source/WebCore/Modules/webxr/WebXRSystem.cpp:360 >>>> + document.postTask([this, immersive, init, mode, promise = WTFMove(promise)] (ScriptExecutionContext& context) mutable { >>> >>> s/ (ScriptExecutionContext/(auto/ >>> >>> Are we sure this is valid in the lambda? >> >> OK regarding s/. >> >> Not sure what you mean by "valid in the lambda". In the case of a document.postTask() the ScripExecutionContext passed to the lambda is the document itself so it has to be valid. > > From a quick look, there is no guarantee the pointer 'this' will stay valid when the lambda executes. Ah :) now I understand, yeah we should protect it.

Created attachment 399656 [details] Patch

Comment on attachment 399656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399656&action=review > Source/WebCore/Modules/webxr/WebXRSession.cpp:43 > +WebXRSession::WebXRSession(Document& document, Ref<WebXRSystem>&& system, XRSessionMode mode, WeakPtr<PlatformXR::Device>&& device) If the goal is for device to be valid initially, it might be better to pass a PlatformXR::Device& > Source/WebCore/Modules/webxr/WebXRSession.cpp:123 > + queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [promise = WTFMove(promise)] () mutable { Do we need to keep the object alive? > Source/WebCore/Modules/webxr/WebXRSystem.cpp:233 > + resolvedFeatures.granted = device->enabledFeatures(mode); It would make more sense to move that line after the early return in the next two lines. > Source/WebCore/Modules/webxr/WebXRSystem.cpp:358 > + if (!inlineSessionRequestIsAllowedForGlobalObject(*globalObject, document, init)) { else if > Source/WebCore/Modules/webxr/WebXRSystem.cpp:365 > + queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [this, document = makeRef(document), immersive, init, mode, promise = WTFMove(promise)] () mutable { Why taking a ref here but capture document by ref below? You could take a weakref and move it as part of next lambda. Another approach would be to get the global object from the promise itself, which would simplify things. > Source/WebCore/Modules/webxr/WebXRSystem.h:116 > + Vector<Ref<WebXRSession>> m_listOfInlineSessions; Should it be a vector or a hashset? > Source/WebCore/platform/xr/PlatformXR.h:63 > + using EnabledFeaturesPerModeMap = WTF::HashMap<SessionMode, ListOfEnabledFeatures, WTF::IntHash<SessionMode>, WTF::StrongEnumHashTraits<SessionMode>>; s/WTF::// > Source/WebCore/testing/WebXRTest.cpp:59 > + if (auto* globalObject = context.execState()) { You could reserveInitialCapacity/uncheckedAppend.

Comment on attachment 399656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399656&action=review >> Source/WebCore/Modules/webxr/WebXRSession.cpp:43 >> +WebXRSession::WebXRSession(Document& document, Ref<WebXRSystem>&& system, XRSessionMode mode, WeakPtr<PlatformXR::Device>&& device) > > If the goal is for device to be valid initially, it might be better to pass a PlatformXR::Device& The thing is that the device might dissapear (i.e. user disconnecting it) so that's the point of having a weak pointer for it. We can still pass a reference and make the weak pointer inside the session. And BTW I'm changing the Ref<WebXRSystem>&& by just a WebXRSystem&, otherwhise we'd be adding a circular dependency. The XRSystem owns the XRSession's so it should be enough to have either a raw pointer or a reference. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:123 >> + queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [promise = WTFMove(promise)] () mutable { > > Do we need to keep the object alive? We still need to implement the 3.1 step, i.e., properly shutdown the platform session, we'll need the WebXRSession alive to properly do that. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:233 >> + resolvedFeatures.granted = device->enabledFeatures(mode); > > It would make more sense to move that line after the early return in the next two lines. Agree, I just wanted to follow literally what the spec says, but from the implementation POV it should be as you suggest. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:365 >> + queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [this, document = makeRef(document), immersive, init, mode, promise = WTFMove(promise)] () mutable { > > Why taking a ref here but capture document by ref below? You could take a weakref and move it as part of next lambda. > Another approach would be to get the global object from the promise itself, which would simplify things. OK I'll do it as suggested. Taking the global object from the promise is indeed possible but we need the document anyway to create the session. >> Source/WebCore/Modules/webxr/WebXRSystem.h:116 >> + Vector<Ref<WebXRSession>> m_listOfInlineSessions; > > Should it be a vector or a hashset? Semantically it's indeed a set because there won't be any duplicates. We don't really need the fast lookup from hashmaps but I agree to make the change. Also I could replace the Vector<WeakPtr<>> bellow by a WeakHashSet.

Committed r261863: <https://trac.webkit.org/changeset/261863>

<rdar://problem/63397466>