[Webkit-unassigned] [Bug 221225] Implement WebXR getViewerPose and getPose

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=221225

--- Comment #20 from Imanol Fernandez <ifernandez at igalia.com> ---
Comment on attachment 420521
  --> https://bugs.webkit.org/attachment.cgi?id=420521
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210217/71b90d7a/attachment.htm>