Bug 221225 - Implement WebXR getViewerPose and getPose
Summary: Implement WebXR getViewerPose and getPose
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebXR (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 220979
Blocks: 208988
  Show dependency treegraph
 
Reported: 2021-02-01 13:57 PST by Imanol Fernandez
Modified: 2021-02-19 01:23 PST (History)
8 users (show)

See Also:


Attachments
wip (109.63 KB, patch)
2021-02-01 14:43 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (92.04 KB, patch)
2021-02-12 08:16 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (93.44 KB, patch)
2021-02-15 04:34 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (93.54 KB, patch)
2021-02-15 11:04 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (93.83 KB, patch)
2021-02-16 04:42 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (93.21 KB, patch)
2021-02-16 12:26 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (93.66 KB, patch)
2021-02-17 10:07 PST, Imanol Fernandez
youennf: review+
Details | Formatted Diff | Diff
Patch for landing (94.00 KB, patch)
2021-02-18 02:50 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Imanol Fernandez 2021-02-01 13:57:41 PST
Implement WebXR getViewerPose and getPose
Comment 1 Imanol Fernandez 2021-02-01 14:43:54 PST
Created attachment 418928 [details]
wip
Comment 2 EWS Watchlist 2021-02-01 14:44:51 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 3 Radar WebKit Bug Importer 2021-02-08 13:58:21 PST
<rdar://problem/74112910>
Comment 4 Imanol Fernandez 2021-02-12 08:16:11 PST
Created attachment 420125 [details]
Patch
Comment 5 youenn fablet 2021-02-12 08:53:49 PST
Comment on attachment 420125 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:41


I would pass a WebXRSession& if possible.

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:82


auto

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:49
> +    , m_session(WTFMove(session))

Would be slightly more consistent to initialise m_session before m_IsAnimationFrame.
Also s/m_IsAnimationFrame/m_isAnimationFrame/

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:58
> +    auto isOutsideNativeBoundsOfBoundedReferenceSpace = [] (const WebXRSpace& space, const WebXRSpace&) {

Might be better as a free function.

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:74
> +    auto isLocalReferenceSpace = [] (const WebXRSpace& space) {

Ditto.

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:87
> +        // than 15m (suggested by specs) return true.

Should we return true here?

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:97
> +    bool emulatedPosition;

Might be good to initialise the value if possible

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:192
> +            const double near = m_session->renderState().depthNear();

s/const //

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:203
> +                aspect = (double) layer->framebufferWidth() / (double) layer->framebufferHeight();

static_cast

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:205
> +            const double far = m_session->renderState().depthFar();

s/const //

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:209
> +        xrView->setProjectionMatrix(projection);

WTFMove(projection)?

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:213
> +        ++index;

We could replace index by checking xrViews size

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:237
> +    RefPtr<WebXRPose> pose = WebXRPose::create(WebXRRigidTransform::create(populateValue->transform), populateValue->emulatedPosition);

directly return here?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:493
> +        auto frame = WebXRFrame::create(makeRef(*this), true /*isAnimationFrame*/);

Would read better by using an enum instead of a bool.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:550
> +    // 2. If the request does not originate from document, return false.

It does not seem like you implement the check here.

> Source/WebCore/Modules/webxr/WebXRSpace.cpp:41
> +WebXRSpace::WebXRSpace(Document& document, WeakPtr<WebXRSession>&& session, Ref<WebXRRigidTransform>&& offset)

I would pass a WebXRSession&

> Source/WebCore/Modules/webxr/WebXRView.cpp:50
> +void WebXRView::setProjectionMatrix(const std::array<float, 16>& projection)

Might be left to a follow-up but why not passing directly a Ref<Float32Array>&& here?

> Source/WebCore/Modules/webxr/WebXRView.h:50
> +    const Float32Array& projectionMatrix() const { return *m_projection; }

How can we guarantee that m_projection is not null?
Is there a way we could pass a Ref<Float32Array> in the constructor?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:606
> +    const double upTan = tan(fovUp);

why const? upTan is only used once so I am not sure it adds much.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:308
> +    XrPosef identityPose = {

s / = //

> LayoutTests/platform/wpe/TestExpectations:641
> +imported/w3c/web-platform-tests/webxr/xrFrame_getViewerPose_getPose.https.html [ Pass ]

Could they be enabled cross-platform?
Comment 6 Imanol Fernandez 2021-02-15 04:24:47 PST
Comment on attachment 420125 [details]
Patch

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

>> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:41
>> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document& document, WeakPtr<WebXRSession>&& session, XRReferenceSpaceType type)
> 
> I would pass a WebXRSession& if possible.

I don't think it's safe because JS can hold XRSpace instances after a XRSession is destroyed.

>> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:82
>> +    Ref<WebXRRigidTransform> offset = WebXRRigidTransform::create(m_originOffset->rawTransform() * offsetTransform.rawTransform());
> 
> auto

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:49
>> +    , m_session(WTFMove(session))
> 
> Would be slightly more consistent to initialise m_session before m_IsAnimationFrame.
> Also s/m_IsAnimationFrame/m_isAnimationFrame/

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:58
>> +    auto isOutsideNativeBoundsOfBoundedReferenceSpace = [] (const WebXRSpace& space, const WebXRSpace&) {
> 
> Might be better as a free function.

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:87
>> +        // than 15m (suggested by specs) return true.
> 
> Should we return true here?

Only if the distance is greater than 15m. I want to implement the limits logic there and in other paths in a separate follow-up patch to not make this one more complex.

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:97
>> +    bool emulatedPosition;
> 
> Might be good to initialise the value if possible

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:192
>> +            const double near = m_session->renderState().depthNear();
> 
> s/const //

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:203
>> +                aspect = (double) layer->framebufferWidth() / (double) layer->framebufferHeight();
> 
> static_cast

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:205
>> +            const double far = m_session->renderState().depthFar();
> 
> s/const //

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:209
>> +        xrView->setProjectionMatrix(projection);
> 
> WTFMove(projection)?

done, moved to the constructor

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:213
>> +        ++index;
> 
> We could replace index by checking xrViews size

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:237
>> +    RefPtr<WebXRPose> pose = WebXRPose::create(WebXRRigidTransform::create(populateValue->transform), populateValue->emulatedPosition);
> 
> directly return here?

done

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:493
>> +        auto frame = WebXRFrame::create(makeRef(*this), true /*isAnimationFrame*/);
> 
> Would read better by using an enum instead of a bool.

done

>> Source/WebCore/Modules/webxr/WebXRView.cpp:50
>> +void WebXRView::setProjectionMatrix(const std::array<float, 16>& projection)
> 
> Might be left to a follow-up but why not passing directly a Ref<Float32Array>&& here?

I moved it to the constructor

>> Source/WebCore/Modules/webxr/WebXRView.h:50
>> +    const Float32Array& projectionMatrix() const { return *m_projection; }
> 
> How can we guarantee that m_projection is not null?
> Is there a way we could pass a Ref<Float32Array> in the constructor?

done

>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:606
>> +    const double upTan = tan(fovUp);
> 
> why const? upTan is only used once so I am not sure it adds much.

Done. I'm used to default immutability in Rust so I'll tend to add const by default :)

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:308
>> +    XrPosef identityPose = {
> 
> s / = //

done

>> LayoutTests/platform/wpe/TestExpectations:641
>> +imported/w3c/web-platform-tests/webxr/xrFrame_getViewerPose_getPose.https.html [ Pass ]
> 
> Could they be enabled cross-platform?

I'll address that as a follow-up
Comment 7 youenn fablet 2021-02-15 04:32:13 PST
(In reply to Imanol Fernandez from comment #6)
> Comment on attachment 420125 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420125&action=review
> 
> >> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:41
> >> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document& document, WeakPtr<WebXRSession>&& session, XRReferenceSpaceType type)
> > 
> > I would pass a WebXRSession& if possible.
> 
> I don't think it's safe because JS can hold XRSpace instances after a
> XRSession is destroyed.

I was meaning to pass a WebXRSession& but store a WeakPtr<>.
This makes it clear for instance that session cannot be null.
Comment 8 Imanol Fernandez 2021-02-15 04:34:07 PST
Created attachment 420304 [details]
Patch

Address review feedback
Comment 9 Imanol Fernandez 2021-02-15 04:41:38 PST
(In reply to youenn fablet from comment #7)
> (In reply to Imanol Fernandez from comment #6)
> > Comment on attachment 420125 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=420125&action=review
> > 
> > >> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:41
> > >> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document& document, WeakPtr<WebXRSession>&& session, XRReferenceSpaceType type)
> > > 
> > > I would pass a WebXRSession& if possible.
> > 
> > I don't think it's safe because JS can hold XRSpace instances after a
> > XRSession is destroyed.
> 
> I was meaning to pass a WebXRSession& but store a WeakPtr<>.
> This makes it clear for instance that session cannot be null.

ah ok. I tried to use WebXRSession& initially but there is a problem with getOffsetReferencespace. In that case we'd need to convert the WeakPtr<WebXRSession> to WebXRSession to create the new XRReferenceSpace. The spec does not define any exception for that case: https://immersive-web.github.io/webxr/#dom-xrreferencespace-getoffsetreferencespace

The exceptions are handled in getPose or getViewerPose calls
Comment 10 youenn fablet 2021-02-15 06:52:10 PST
WebXRSpace was previously taking a Ref<WebXRSession> and this patch is changing it to a WeakPtr<WebXRSession>.
With this change, WebXRSpace needs to handle the case of a null WebXRSession, while previously, it was not possible. Can you describe why this is good to do so?

Are we getting closer to spec? It seems this particular change could potentially be observable (before GC, WebXRSpace does something, after GC it does something different).
Comment 11 Imanol Fernandez 2021-02-15 11:04:05 PST
Created attachment 420336 [details]
Patch

Fix build error in wincairo: near and far are non-standard extension keywords
Comment 12 Imanol Fernandez 2021-02-15 11:15:55 PST
(In reply to youenn fablet from comment #10)
> WebXRSpace was previously taking a Ref<WebXRSession> and this patch is
> changing it to a WeakPtr<WebXRSession>.
> With this change, WebXRSpace needs to handle the case of a null
> WebXRSession, while previously, it was not possible. Can you describe why
> this is good to do so?
> 
> Are we getting closer to spec? It seems this particular change could
> potentially be observable (before GC, WebXRSpace does something, after GC it
> does something different).

I made the change when I added https://immersive-web.github.io/webxr/#xrsession-viewer-reference-space in order to avoid a cycle between Session and the viewer reference space.
Comment 13 youenn fablet 2021-02-16 00:46:35 PST
(In reply to Imanol Fernandez from comment #12)
> (In reply to youenn fablet from comment #10)
> > WebXRSpace was previously taking a Ref<WebXRSession> and this patch is
> > changing it to a WeakPtr<WebXRSession>.
> > With this change, WebXRSpace needs to handle the case of a null
> > WebXRSession, while previously, it was not possible. Can you describe why
> > this is good to do so?
> > 
> > Are we getting closer to spec? It seems this particular change could
> > potentially be observable (before GC, WebXRSpace does something, after GC it
> > does something different).
> 
> I made the change when I added
> https://immersive-web.github.io/webxr/#xrsession-viewer-reference-space in
> order to avoid a cycle between Session and the viewer reference space.

From the spec I can see:
- Each XRSpace has a session which is set to the XRSession that created the XRSpace.
- Each XRSession has a viewer reference space, which is an XRReferenceSpace of type "viewer" with an identity transform origin offset.

It seems that as long as XRSpace is alive, its XRSession should be alive.
If XRSpace is never changing of XRSession, one possibility is to have:
- XRSession is RefCounted<XRSession>
- XRSpace is no longer a RefCounted<XRSpace>
- XRSpace stores a XRSession&
- XRSpace implements its ref/deref methods by calling ref/deref of its XRSession.
Comment 14 Imanol Fernandez 2021-02-16 04:39:10 PST
(In reply to youenn fablet from comment #13)
> (In reply to Imanol Fernandez from comment #12)
> > (In reply to youenn fablet from comment #10)
> > > WebXRSpace was previously taking a Ref<WebXRSession> and this patch is
> > > changing it to a WeakPtr<WebXRSession>.
> > > With this change, WebXRSpace needs to handle the case of a null
> > > WebXRSession, while previously, it was not possible. Can you describe why
> > > this is good to do so?
> > > 
> > > Are we getting closer to spec? It seems this particular change could
> > > potentially be observable (before GC, WebXRSpace does something, after GC it
> > > does something different).
> > 
> > I made the change when I added
> > https://immersive-web.github.io/webxr/#xrsession-viewer-reference-space in
> > order to avoid a cycle between Session and the viewer reference space.
> 
> From the spec I can see:
> - Each XRSpace has a session which is set to the XRSession that created the
> XRSpace.
> - Each XRSession has a viewer reference space, which is an XRReferenceSpace
> of type "viewer" with an identity transform origin offset.
> 
> It seems that as long as XRSpace is alive, its XRSession should be alive.
> If XRSpace is never changing of XRSession, one possibility is to have:
> - XRSession is RefCounted<XRSession>
> - XRSpace is no longer a RefCounted<XRSpace>
> - XRSpace stores a XRSession&
> - XRSpace implements its ref/deref methods by calling ref/deref of its
> XRSession.

That's a good option. I'm going to update the patch
Comment 15 Imanol Fernandez 2021-02-16 04:42:03 PST
Created attachment 420452 [details]
Patch

Use WebXRSession& in WebXRSpace
Comment 16 youenn fablet 2021-02-16 07:15:31 PST
If you want to keep the design of using only WebXRSession ref, you will need to keep all spaces in WebXRSession (see how XMLHtppRequestUpload uses the ref/deref mechanism I mentioned before).

Looking further the spec (I am not familiar with it), there is not a 1-1 relationship between session and space.
- Spaces always need a reference to their session. I think having a Ref<WebXRSession> is good.
- One space, the viewer space, is creating a circular reference as its session needs to ref it.

It is not totally clear to me whether the viewer space is exposed somehow to scripts.

One possibility would be to have the session keep a WeakPtr<> of the viewer space if it is easy to recreate it as needed.
Another possibility is for the viewer space to be a different class, not refcounted at all, but fully owned by the WebXRSession as a unique_ptr.
Comment 17 Imanol Fernandez 2021-02-16 07:59:18 PST
(In reply to youenn fablet from comment #16)
> If you want to keep the design of using only WebXRSession ref, you will need
> to keep all spaces in WebXRSession (see how XMLHtppRequestUpload uses the
> ref/deref mechanism I mentioned before).
> 
> Looking further the spec (I am not familiar with it), there is not a 1-1
> relationship between session and space.
> - Spaces always need a reference to their session. I think having a
> Ref<WebXRSession> is good.
> - One space, the viewer space, is creating a circular reference as its
> session needs to ref it.
> 
> It is not totally clear to me whether the viewer space is exposed somehow to
> scripts.
> 
> One possibility would be to have the session keep a WeakPtr<> of the viewer
> space if it is easy to recreate it as needed.
> Another possibility is for the viewer space to be a different class, not
> refcounted at all, but fully owned by the WebXRSession as a unique_ptr.

The viewer space owned by the WebXRSession is not exposed to script. It's only used in WebXRFrame::getViewerPose().

Another option would be to create a separate populatePose method in WebXRFrame for getViewerPose calls. That would avoid creating a new class at all or the ref/deref proxy solution but at the cost of some code reusability in WebXRFrame::populatePose();

Thinking more about the pros/cons of all the 4 solutions I like the one about creating a separate class for viewer space:
- It's simpler to understand than the ref/deref proxy solution.
- It doesn't require to recreate the viewer space if WeakPtr<> is not alive. XRFrame::getViewerPose is tipically called once per frame so I prefer to avoid allocations as much as possible
- It allows us to reuse the WebXRFrame::populatePose() logic.
Comment 18 Imanol Fernandez 2021-02-16 12:26:50 PST
Created attachment 420521 [details]
Patch

Create a different class to hold WebXRSession's viewer-space
Comment 19 youenn fablet 2021-02-17 08:34:17 PST
Comment on attachment 420521 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:78
> +    ASSERT(is<Document>(scriptExecutionContext()));

I guess you could do something like:
auto* document = downcast<Document>(scriptExecutionContext());
if (!document)
   ...

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.h:40
>      WTF_MAKE_ISO_ALLOCATED(WebXRBoundedReferenceSpace);

Can WebXRBoundedReferenceSpace be set as final?

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:39
> +

Unnecessary

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:142
> +    if (limit) {

We could probably compute limit closer to its actual use.

> Source/WebCore/Modules/webxr/WebXRFrame.h:68
> +    explicit WebXRFrame(Ref<WebXRSession>&&, IsAnimationFrame);

explici not needed.

> Source/WebCore/Modules/webxr/WebXRFrame.h:69
> +    WebXRFrame(WebXRSession&, bool isAnimationFrame);

To remove?

> Source/WebCore/Modules/webxr/WebXRFrame.idl:37
> +    [MayThrowException, CallWith=Document] WebXRPose? getPose(WebXRSpace space, WebXRSpace baseSpace);

Should we use the Document here, or the document that created the WebXRSession?

> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:102
>      ASSERT(is<Document>(scriptExecutionContext()));

ditto here, we can probably simplify a tiny bit.

> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:126
> +

not needed

> Source/WebCore/Modules/webxr/WebXRReferenceSpace.h:53
> +    WebXRReferenceSpace(Document&, Ref<WebXRSession>&&, Ref<WebXRRigidTransform>&&, XRReferenceSpaceType);

Should be either protected or private, not public though probably since we have create static method

> Source/WebCore/Modules/webxr/WebXRSpace.h:86
> +    WebXRSession& m_session;

A ref here is probably not safe since WebXRViewerSpace is refcounted, it could potentially outlive its WebXRSession.
If we cannot make WebXRViewerSpace a unique_ptr, let's have a WeakPtr there.
The alternative would be for WebXRSpace to be RefCounted.
Or to introduce a WebXRSpaceBase which is not refcounted from which would derive WebXRViewerSpace and WebXRSpace.
Comment 20 Imanol Fernandez 2021-02-17 09:56:54 PST
Comment on attachment 420521 [details]
Patch

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

>> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:78
>> +    ASSERT(is<Document>(scriptExecutionContext()));
> 
> I guess you could do something like:
> auto* document = downcast<Document>(scriptExecutionContext());
> if (!document)
>    ...

done

>> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.h:40
>>      WTF_MAKE_ISO_ALLOCATED(WebXRBoundedReferenceSpace);
> 
> Can WebXRBoundedReferenceSpace be set as final?

yes, done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:39
>> +
> 
> Unnecessary

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:142
>> +    if (limit) {
> 
> We could probably compute limit closer to its actual use.

done

>> Source/WebCore/Modules/webxr/WebXRFrame.h:68
>> +    explicit WebXRFrame(Ref<WebXRSession>&&, IsAnimationFrame);
> 
> explici not needed.

done

>> Source/WebCore/Modules/webxr/WebXRFrame.h:69
>> +    WebXRFrame(WebXRSession&, bool isAnimationFrame);
> 
> To remove?

yes, removed

>> Source/WebCore/Modules/webxr/WebXRFrame.idl:37
>> +    [MayThrowException, CallWith=Document] WebXRPose? getPose(WebXRSpace space, WebXRSpace baseSpace);
> 
> Should we use the Document here, or the document that created the WebXRSession?

We need both. I had to add it to implement a security check from the spec:

https://immersive-web.github.io/webxr/#poses-may-be-reported
"If session’s relevant global object is not the current global object, return false."

>> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:102
>>      ASSERT(is<Document>(scriptExecutionContext()));
> 
> ditto here, we can probably simplify a tiny bit.

done

>> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:126
>> +
> 
> not needed

done

>> Source/WebCore/Modules/webxr/WebXRReferenceSpace.h:53
>> +    WebXRReferenceSpace(Document&, Ref<WebXRSession>&&, Ref<WebXRRigidTransform>&&, XRReferenceSpaceType);
> 
> Should be either protected or private, not public though probably since we have create static method

done

>> Source/WebCore/Modules/webxr/WebXRSpace.h:86
>> +    WebXRSession& m_session;
> 
> A ref here is probably not safe since WebXRViewerSpace is refcounted, it could potentially outlive its WebXRSession.
> If we cannot make WebXRViewerSpace a unique_ptr, let's have a WeakPtr there.
> The alternative would be for WebXRSpace to be RefCounted.
> Or to introduce a WebXRSpaceBase which is not refcounted from which would derive WebXRViewerSpace and WebXRSpace.

Thanks for the review. I have changed WebXRSpace to be not RefCounted and make WebXRReferenceSpace RefCounted instead.

Now WebXRViewerSpace is not RefCounted and uses a unique_ptr in WebXRSession.
Comment 21 Imanol Fernandez 2021-02-17 10:07:37 PST
Created attachment 420660 [details]
Patch

Address review feedback
Comment 22 youenn fablet 2021-02-18 02:02:59 PST
Comment on attachment 420660 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:134
> +    const bool emulatedPosition = m_data.isPositionEmulated || !m_data.isPositionValid;

why const bool?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:60
> +    , m_viewerReferenceSpace(new WebXRViewerSpace(document, *this))

makeUnique<WebXRViewerSpace>(document, this)

> Source/WebCore/Modules/webxr/WebXRSpace.h:58
>      ScriptExecutionContext* scriptExecutionContext() const override { return ContextDestructionObserver::scriptExecutionContext(); }

final?

> Source/WebCore/Modules/webxr/WebXRSpace.h:60
> +    Ref<WebXRRigidTransform> m_originOffset;

Can we make it private and add a protected WebXRRigidTransform& getter?

> Source/WebCore/Modules/webxr/WebXRSpace.h:81
> +    void derefEventTarget() final { RELEASE_ASSERT_NOT_REACHED(); }

This is fine like this. Otherwise, you could do m_session.ref()/m_session.deref();
Comment 23 Imanol Fernandez 2021-02-18 02:49:31 PST
Comment on attachment 420660 [details]
Patch

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

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:134
>> +    const bool emulatedPosition = m_data.isPositionEmulated || !m_data.isPositionValid;
> 
> why const bool?

IMO it's a good practice to use const on local variables that don't change (e.g some discussion about this here: https://www.bfilipek.com/2016/12/please-declare-your-variables-as-const.html). I wish C++ had implicit const by default :)

I removed the const to follow the same style as existing code. This could be a https://webkit.org/code-style-guidelines/ follow-up discussion

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:60
>> +    , m_viewerReferenceSpace(new WebXRViewerSpace(document, *this))
> 
> makeUnique<WebXRViewerSpace>(document, this)

done

>> Source/WebCore/Modules/webxr/WebXRSpace.h:58
>>      ScriptExecutionContext* scriptExecutionContext() const override { return ContextDestructionObserver::scriptExecutionContext(); }
> 
> final?

done

>> Source/WebCore/Modules/webxr/WebXRSpace.h:60
>> +    Ref<WebXRRigidTransform> m_originOffset;
> 
> Can we make it private and add a protected WebXRRigidTransform& getter?

yes, done

>> Source/WebCore/Modules/webxr/WebXRSpace.h:81
>> +    void derefEventTarget() final { RELEASE_ASSERT_NOT_REACHED(); }
> 
> This is fine like this. Otherwise, you could do m_session.ref()/m_session.deref();

perfect, thanks
Comment 24 Imanol Fernandez 2021-02-18 02:50:17 PST
Created attachment 420818 [details]
Patch for landing

Patch for landing
Comment 25 youenn fablet 2021-02-18 03:12:52 PST
> IMO it's a good practice to use const on local variables that don't change
> (e.g some discussion about this here:
> https://www.bfilipek.com/2016/12/please-declare-your-variables-as-const.
> html). I wish C++ had implicit const by default :)
> 
> I removed the const to follow the same style as existing code. This could be
> a https://webkit.org/code-style-guidelines/ follow-up discussion

Agreed this adds some nice properties but this makes reading the code is a bit more tedious.
Starting a webkit thread might be indeed be a good idea.
Comment 26 EWS 2021-02-19 01:07:17 PST
Committed r273132: <https://commits.webkit.org/r273132>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420818 [details].