[webkit-reviews] review granted: [Bug 223257] Implement WebXR Input Sources : [Attachment 426234] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Imanol Fernandez
<ifernandez at igalia.com>'s request for review:
Bug 223257: Implement WebXR Input Sources
https://bugs.webkit.org/show_bug.cgi?id=223257

Attachment 426234: Patch

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




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

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

> Source/WebCore/Modules/webxr/WebXRGamepad.cpp:40
> +{

What is -1?
A constant might make things clearer.

> Source/WebCore/Modules/webxr/WebXRGamepad.cpp:41
> +    m_lastUpdateTime =
MonotonicTime::fromRawSeconds(Seconds::fromMilliseconds(timestamp).value());

I would write it as m_connectTime(MonotonicTime::...)
ditto for m_lasUpdateTime, m_axes and m_buttons.

> Source/WebCore/Modules/webxr/WebXRInputSource.cpp:50
> +WebXRInputSource::WebXRInputSource(Ref<Document>&& document,
Ref<WebXRSession>&& session, double timestamp, const
PlatformXR::Device::FrameData::InputSource& source)

It seems like we can have a WebXRInputSource whose document is not the same as
the session document.
If so, this is fine. Otherwise, maybe we should just pass the session and when
needed get the session document.

> Source/WebCore/Modules/webxr/WebXRInputSource.cpp:153
> +    return XRInputSourceEvent::create(name, init);

how about a one liner here:
return XRInputSourceEvent::create(name, {
WebXRFrame::create(m_session.copyRef(), WebXRFrame::IsAnimationFrame::No),
makeRefPtr(*this) });

> Source/WebCore/Modules/webxr/WebXRInputSource.h:63
> +    RefPtr<WebXRSpace> gripSpace() const { return m_gripSpace; }

Can we return a WebXRSpace*?

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:48
> +    , m_session(WTFMove(session))

Ditto for document and session.

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:63
> +    return m_inputSources[index].ptr();

One liner?

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:90
> +	   });

If we have to do that, for now I would just create a
Vector<RefPtr<XRInputSourceEvent>> instead of having to recreate a vector.
And probably file a bug on binding generator since I am guessing this a
limitation of dictionaries.

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:93
> +	   m_session->queueTaskToDispatchEvent(m_session.get(),
TaskSource::WebXR, WTFMove(event));

Would be nice as follow-ups to be able to WTFMove the added and removed when
creating XRInputSourcesChangeEvent.

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:108
> +		   event->setFrameActive(false);

It seems we should do something like:
m_session->queueTaskKeepingObjectAlive(m_session.get(), TaskSource::WebXR,
[event = WTFMove(event)]() { 
  event->setFrameActive(true);
  dispatchEvent(event.copyRef());
  event->setFrameActive(true);
});

Can you add tests in that area?

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:124
> +	   if (inputSources.findMatching([&source](auto& item) { return
item.handle == source->handle(); }) == WTF::notFound) {

You could use anyOf here which might be a tad clearer.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:430
> +    //	 2. Fire an XRInputSourcesChangeEvent named inputsourceschange
on session with added set to sources.

It is not clear whether we are doing everything there.

> Source/WebCore/testing/WebFakeXRDevice.cpp:258
> +

Not needed.

> ChangeLog:10
> +	   * Source/cmake/OptionsWPE.cmake:

Do not find the change there.