Bug 212407 – [WebXR] Implement XRSession::requestReferenceSpace()

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

[Sergio Villar Senin]

[WebXR] Implement XRSession::requestReferenceSpace()

[Sergio Villar Senin]

Created attachment 400331 [details]
Patch

[Sergio Villar Senin]

Gentle ping

[youenn fablet]

Comment on attachment 400331 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.h:42
> +    static Ref<WebXRBoundedReferenceSpace> create(ScriptExecutionContext*, WebXRSession&, XRReferenceSpaceType);

Can we use a Document& and a Ref<WebXRSession>&&?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:88
> +bool WebXRSession::referenceSpaceIsSupported(const XRReferenceSpaceType& type) const

s/const XRReferenceSpaceType& type/XRReferenceSpaceType type/

> Source/WebCore/Modules/webxr/WebXRSession.cpp:128
> +void WebXRSession::requestReferenceSpace(const XRReferenceSpaceType& type, RequestReferenceSpacePromise&& promise)

s/const XRReferenceSpaceType& type/XRReferenceSpaceType type/

> Source/WebCore/Modules/webxr/WebXRSession.cpp:132
> +    scriptExecutionContext()->postTask([this, promise = WTFMove(promise), type] (ScriptExecutionContext&) mutable {

scriptExecutionContext() might be null.
s/ScriptExecutionContext&/auto&/

> Source/WebCore/Modules/webxr/WebXRSession.cpp:149
> +        promise.resolve(*referenceSpace);

referenceSpace.releaseNonNull()

> Source/WebCore/Modules/webxr/WebXRSpace.h:32
> +#include <wtf/IsoMalloc.h>

Why do we need this one?

[Sergio Villar Senin]

Comment on attachment 400331 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.h:42
>> +    static Ref<WebXRBoundedReferenceSpace> create(ScriptExecutionContext*, WebXRSession&, XRReferenceSpaceType);
> 
> Can we use a Document& and a Ref<WebXRSession>&&?

We could indeed pass a Document&, I just used the ScriptExecutionContext* because that's what ContextDestructionObserver expects in the end but I can change it.

Regarding a ref to the session yeah we could do that as well as the session does not keep any reference to the reference spaces so there is no risk of ref cycles.

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:88
>> +bool WebXRSession::referenceSpaceIsSupported(const XRReferenceSpaceType& type) const
> 
> s/const XRReferenceSpaceType& type/XRReferenceSpaceType type/

right.

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:128
>> +void WebXRSession::requestReferenceSpace(const XRReferenceSpaceType& type, RequestReferenceSpacePromise&& promise)
> 
> s/const XRReferenceSpaceType& type/XRReferenceSpaceType type/

ok

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:132
>> +    scriptExecutionContext()->postTask([this, promise = WTFMove(promise), type] (ScriptExecutionContext&) mutable {
> 
> scriptExecutionContext() might be null.
> s/ScriptExecutionContext&/auto&/

OK

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:149
>> +        promise.resolve(*referenceSpace);
> 
> referenceSpace.releaseNonNull()

right

>> Source/WebCore/Modules/webxr/WebXRSpace.h:32
>> +#include <wtf/IsoMalloc.h>
> 
> Why do we need this one?

I don't see any reason.

[Sergio Villar Senin]

Committed r262299: <https://trac.webkit.org/changeset/262299>

[Radar WebKit Bug Importer]

<rdar://problem/63762051>