[Webkit-unassigned] [Bug 220979] Complete XRSession::requestAnimationFrame implementation

[bugzilla-daemon at webkit.org]

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

--- Comment #18 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 419584
  --> https://bugs.webkit.org/attachment.cgi?id=419584
Patch

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

> Source/WebCore/Modules/webxr/WebXRSession.cpp:80
> +    m_animationFrame = WebXRFrame::create(makeRef(*this));

This creates a ref cycle. Is there a way to not create a ref cycle?
Do we need to store the animation frame? As a ref?
If not, how are we making sure to break it? It seems shutdown will break the ref cycle but I am not sure we will always go there.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:231
> +            auto document = makeRef(downcast<Document>(*scriptExecutionContext()));

There is no guarantee scriptExecutionContext() will not be null, please exit early if null.
Also why refing the document?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:522
> +            m_runningCallbacks.swap(m_callbacks);

Do we need m_runningCallbacks? Can we just do 
auto runningCallbacks = WTFMove(m_callbacks); or std::exchange

> Source/WebCore/Modules/webxr/WebXRSession.cpp:536
> +                callback->handleEvent(now, *m_animationFrame);

This is still a bit weak to have handleEvent here and m_runningCallbacks be accessed elsewhere.
I wonder how we could make this more robust.

> Source/WebCore/Modules/webxr/WebXRSession.h:94
> +    explicit WebXRSession(Document&, WebXRSystem&, XRSessionMode, PlatformXR::Device&);

No need for explicit.

> Source/WebCore/Modules/webxr/WebXRSystem.h:109
> +        DummyInlineDevice(ScriptExecutionContext&);

explicit

> Source/WebCore/Modules/webxr/WebXRSystem.h:118
> +        ScriptExecutionContext* m_scriptExecutionContext;

nullptr here just in case.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:214
> +    return c;

Can you try to do just: return switchOn(...)

-- 
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/20210208/1249c95a/attachment-0001.htm>