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

[bugzilla-daemon at webkit.org]

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

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

I am not sure about lifetime safety here, especially since we are using raw pointers.
It would be best to either use Ref or WeakPtr.
Or have a very simple ownership model.

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

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:44
> +WebXRFrame::WebXRFrame(WebXRSession* session)

Can we tighten it by passing a WebXRSession&?

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:45
> +    : m_active(false)

Might be better to set it in header file.

> 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.

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:55
> +    return *m_session;

How can we guarantee this?

> Source/WebCore/Modules/webxr/WebXRFrame.h:60
> +    WebXRFrame(WebXRSession*);

explicit

> Source/WebCore/Modules/webxr/WebXRSession.cpp:59
> +    , m_animationFrame(WebXRFrame::create(this))

Seems like we could use *this there.

> 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.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:221
> +        queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [this, type, promise = WTFMove(promise), protectedDocument = makeRef(downcast<Document>(context))]() mutable {

Why keeping a protected document here?
I guess context is equal to this->scriptExecutionContext().
Can we just use that?

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

s/PlatformXR::Device::FrameDat/auto/

> Source/WebCore/Modules/webxr/WebXRSession.cpp:464
> +    {

Should be at the end of the next line.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:479
> +        DOMHighResTimeStamp now = (MonotonicTime::now() - m_timeOrigin).milliseconds();

auto?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:483
> +        m_animationFrame->setTime(static_cast<DOMHighResTimeStamp>(frameData.predictedDisplayTime));

Is there a way to not have to do the static_cast.

> 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?

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:475
> +class InlineRequestAnimationFrameCallback: public RequestAnimationFrameCallback {

Should it be final?

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:482
> +    InlineRequestAnimationFrameCallback(ScriptExecutionContext& scriptExecutionContext, Function<void()>&& callback)

Please move this to private.
Otherwise people can create it outside of create.

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:487
> +    CallbackResult<void> handleEvent(double highResTimeMs) override

final?
Remove highResTimeMs

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:507
> +    auto document = downcast<Document>(m_scriptExecutionContext);

What is the guarantee m_scriptExecutionContext will be a Document?
Should constructor take a Document?
What is the guarantee for m_scriptExecutionContext to be valid here?

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

explicit

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:205
> +    HTMLCanvasElement* c = WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) {

return WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) -> HTMLCanvasElement* {
maybe?

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:212
> +            [](RefPtr<OffscreenCanvas>) {

const RefPtr<OffscreenCanvas>&?

> Source/WebCore/platform/xr/PlatformXR.h:96
> +    using RequestFrameCallback = WTF::Function<void(FrameData)>;

No need for WTF::
Probably also want to pass a FrameData&&

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438
> +        requestFrame([](FrameData) { });

s/FrameData/auto

> Source/WebCore/testing/WebFakeXRDevice.cpp:46
> +    m_supportsOrientationTracking = true;

Why not:
    , m_supportsOrientationTracking(true)
Or even better in the header declaration directly.

> Source/WebCore/testing/WebFakeXRDevice.cpp:76
> +    runningCallbacks.swap(m_callbacks);

Could be written as:
auto callbacks = WTFMove(m_callbacks)?

> Source/WebCore/testing/WebFakeXRDevice.cpp:85
> +        m_frameTimer.startOneShot(15_ms);

Might be good to use a constant

-- 
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/20210202/4c4d6d93/attachment-0001.htm>