| Summary: | [WebXR] Implement XRSession::requestReferenceSpace() | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||
| Component: | New Bugs | Assignee: | Sergio Villar Senin <svillar> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | aperez, cgarcia, dino, jfernandez, svillar, webkit-bug-importer, youennf, zan | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 208988 | ||||||
| Attachments: |
|
||||||
|
Description
Sergio Villar Senin
2020-05-27 06:58:49 PDT
Created attachment 400331 [details]
Patch
Gentle ping 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? 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. Committed r262299: <https://trac.webkit.org/changeset/262299> |