[webkit-reviews] review denied: [Bug 225025] [WebXR] Use weak pointers to reference WebXRSessions : [Attachment 427945] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Dean Jackson <dino at apple.com>'s request
for review:
Bug 225025: [WebXR] Use weak pointers to reference WebXRSessions
https://bugs.webkit.org/show_bug.cgi?id=225025

Attachment 427945: Patch

https://bugs.webkit.org/attachment.cgi?id=427945&action=review




--- Comment #26 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 427945
  --> https://bugs.webkit.org/attachment.cgi?id=427945
Patch

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

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:47
> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document&
document, WebXRSession* session, XRReferenceSpaceType type)

This should take a WebXRSession& not a WebXRSession*. The only caller knows
session is non-null on construction.

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:52
> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document&
document, WebXRSession* session, Ref<WebXRRigidTransform>&& offset,
XRReferenceSpaceType type)

Also should probably be a reference. Is this called? Maybe it can be removed.

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:57
> +WebXRBoundedReferenceSpace::WebXRBoundedReferenceSpace(Document& document,
WebXRSession* session, Ref<WebXRRigidTransform>&& offset, XRReferenceSpaceType
type)

Should be a reference.

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

Is this actually needed? Does the WebXRSession own a
WebXRBoundedReferenceSpace?

> Source/WebCore/Modules/webxr/WebXRFrame.h:82
> -    Ref<WebXRSession> m_session;
> +    WeakPtr<WebXRSession> m_session;

Same question. Is this needed? Does the session own the frame?

> Source/WebCore/Modules/webxr/WebXRSession.h:132
> -    Ref<WebXRInputSourceArray> m_inputSources;
> +    RefPtr<WebXRInputSourceArray> m_inputSources;

What is this change for?