[webkit-reviews] review denied: [Bug 220979] Complete XRSession::requestAnimationFrame implementation : [Attachment 418924] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Imanol Fernandez
<ifernandez at igalia.com>'s request for review:
Bug 220979: Complete XRSession::requestAnimationFrame implementation
https://bugs.webkit.org/show_bug.cgi?id=220979

Attachment 418924: Patch

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




--- Comment #12 from Dean Jackson <dino at apple.com> ---
Comment on attachment 418924
  --> https://bugs.webkit.org/attachment.cgi?id=418924
Patch

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

Looking pretty good! Thanks Immanol and Sergio.

> Source/WebCore/ChangeLog:9
> +	   - Properly implement the render loop for immersive and inline XR
sessions
> +	   - Apply pending render state changes

It would be nice to have some more info in the ChangeLog. Maybe even per-file
notes, for areas where there was a lot of change.

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:44
>> +WebXRFrame::WebXRFrame(WebXRSession* session)
> 
> Can we tighten it by passing a WebXRSession&?

See question below. Why did this become a raw pointer rather than a Ref?

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:46
>> +	, m_session(session)
> 
> WebXRFrame is refcounted so could potentially be kept alive longer than its
session.
> Might be better to keep a ref or use a weakptr.

Since you can access the session from the XRFrame, I think this need to be a
ref. The session itself might get disconnected, but the object still needs to
exist.

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:55
>> +	return *m_session;
> 
> How can we guarantee this?

+1

If the session can never be null, then we shouldn't be storing a pointer.

> Source/WebCore/Modules/webxr/WebXRFrame.h:62
> +    bool m_active;

add { false }

> Source/WebCore/Modules/webxr/WebXRFrame.h:66
> -    Ref<WebXRSession> m_session;
> +    // Session owns the frame.
> +    WebXRSession* m_session;

Why is this now a pointer rather than a Ref? When can a Frame be created
without a Session?

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:208
>> +	scriptExecutionContext()->postTask([this, promise = WTFMove(promise),
type](auto& context) mutable {
> 
> There is no guarantee this will be valid within the callback.

weakThis = makeWeakPtr(this)
then check the value in the callback

> Source/WebCore/Modules/webxr/WebXRSession.cpp:248
> +    if (m_callbacks.size() == 1 && !m_animationFrame->isActive())

Why does the number of callbacks need to equal 1? Shouldn't this just be a
non-zero test?

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:463
>> +	m_device->requestFrame([this, protectedThis =
makeRef(*this)](PlatformXR::Device::FrameData frameData)
> 
> s/PlatformXR::Device::FrameDat/auto/

Oh cool. I didn't know you could do that.

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:512
>> +		    callback->handleEvent(now, m_animationFrame.get());
> 
> Is there a chance the event will cancel everything (in which case we should
stop everything), or will allow changing m_runningCallbacks synchronously?

The script here can do anything, but it looks like runningCallbacks is only
used here, so it might be safe. But that makes me ask why it is exposed as a
member variable rather than just a local variable in this block.

> Source/WebCore/platform/xr/PlatformXR.h:87
> +		       float x, y, z, w;

Separate line for each of these.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:435
> +    // Ensure that requestFrame() is called at least one more time to
properly end the session.
> +    callOnMainThread([this, weakThis = makeWeakPtr(this)]() {

Why is this necessary? Is this just causing the pending callbacks to be
flushed? Wouldn't it be better to delete them directly?