[webkit-reviews] review granted: [Bug 231482] [WebXR] Replace the session reference in WebXRSpace subclasses with weak pointers : [Attachment 440718] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Dean Jackson <dino at apple.com>'s request
for review:
Bug 231482: [WebXR] Replace the session reference in WebXRSpace subclasses with
weak pointers
https://bugs.webkit.org/show_bug.cgi?id=231482

Attachment 440718: Patch

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




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

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

> Source/WebCore/ChangeLog:12
> +	   WebXRSpace had a pure virtual session() accessor that returned
> +	   a reference to a WebXRSession. This made subclasses hold strong
> +	   references to the WebXRSession, and would become problematic
> +	   for a WebXRSpace subclass that was (indirectly) owned by the
> +	   WebXRSession (found in the Hand Input module).

Pretty sure this was already causing leaks elsewhere for other XRSpaces, like
the targetRaySpace and gripSpace in WebXRInputSource.

> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:72
> +    if (!m_session)
> +	   return identity;

This feels a bit weird to me. It at least needs a comment explaining itself,
but I would assume this would propagate an Exception to the caller.

> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:120
> +    if (!m_session)
> +	   return { };

Same as above.

> Source/WebCore/Modules/webxr/WebXRSpace.cpp:61
> +    WebXRSession* xrSession = session();
> +    if (!xrSession)
> +	   return false;

Seems like this should be an exception rather than just returning false.