Bug 223257 – Implement WebXR Input Sources

• Component: WebXR
• Status: RESOLVED FIXED

[Imanol Fernandez]

- https://immersive-web.github.io/webxr/#input
- https://immersive-web.github.io/webxr-gamepads-module/#webxr-device-api-integration

[Imanol Fernandez]

Created attachment 423344 [details]
Patch

[Imanol Fernandez]

Created attachment 423347 [details]
Patch

Fix nit

[Imanol Fernandez]

Created attachment 423598 [details]
Patch

Rebase onto main

[Radar WebKit Bug Importer]

<rdar://problem/75739455>

[Imanol Fernandez]

Created attachment 424375 [details]
Patch

Rebase onto main. Add CachedAttribute to pass sameObject related tests.

[youenn fablet]

Comment on attachment 424375 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRInputSource.h:52
> +class WebXRInputSpace : public RefCounted<WebXRInputSpace>, public WebXRSpace {

It seems like it could have its own file.

> Source/WebCore/Modules/webxr/WebXRInputSource.h:77
> +class WebXRGamepad: public PlatformGamepad {

Can we move it to its own file as well?

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:62
> +    return m_inputSources.at(index).ptr();

Is there a guarantee index is smaller than size()?

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:122
> +    for (int i = m_inputSources.size() - 1; i >= 0; --i) {

size() is probably a size_t.
Can we use removeAllMatching instead?

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.h:61
> +    Document& m_document;

This is probably not safe to do that, given WebXRInputSourceArray is refcounted, it could outlive its document.

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.h:62
> +    WebXRSession& m_session;

Ditto here.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:435
> +        m_inputPromiseResolved = true;

I do not see any input promise resolved around. Are we sure this is the right name.

[Imanol Fernandez]

Comment on attachment 424375 [details]
Patch

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

>> Source/WebCore/Modules/webxr/WebXRInputSource.h:52
>> +class WebXRInputSpace : public RefCounted<WebXRInputSpace>, public WebXRSpace {
> 
> It seems like it could have its own file.

Done

>> Source/WebCore/Modules/webxr/WebXRInputSource.h:77
>> +class WebXRGamepad: public PlatformGamepad {
> 
> Can we move it to its own file as well?

Done

>> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:62
>> +    return m_inputSources.at(index).ptr();
> 
> Is there a guarantee index is smaller than size()?

Good catch, I added a bounds check

>> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:122
>> +    for (int i = m_inputSources.size() - 1; i >= 0; --i) {
> 
> size() is probably a size_t.
> Can we use removeAllMatching instead?

yes, done

>> Source/WebCore/Modules/webxr/WebXRInputSourceArray.h:61
>> +    Document& m_document;
> 
> This is probably not safe to do that, given WebXRInputSourceArray is refcounted, it could outlive its document.

Done. I also chaged WebXRInputSource::m_document to Ref. The InputSourceArrays is cleared in WebXRSession::shutdown() so there shouldn't be any ref cycles.

>> Source/WebCore/Modules/webxr/WebXRInputSourceArray.h:62
>> +    WebXRSession& m_session;
> 
> Ditto here.

done

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:435
>> +        m_inputPromiseResolved = true;
> 
> I do not see any input promise resolved around. Are we sure this is the right name.

It's how the spec names it. I changed the name to m_inputInitialized to make it easier to understand.

[Imanol Fernandez]

Created attachment 425413 [details]
Patch

Address review feedback and rebase onto main

[Imanol Fernandez]

Created attachment 426234 [details]
Patch

Rebase onto main

[youenn fablet]

Comment on attachment 426234 [details]
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.

[Imanol Fernandez]

Comment on attachment 426234 [details]
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.

It's the default gamepad id defined in https://immersive-web.github.io/webxr-gamepads-module/#gamepad-differences.

I moved it to a constant with a link to the spec.

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

I couldn't do that because they are members from the base class

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

InputSources and the array are always constructed from session, so they are going to be the same. I changed the code as you suggested.

>> 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) });

The problem with the one liner is we get a missing initializer for members because XRInputSourceEvent has three extra values from the base EventInit class

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

Yes, done

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

Done

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

Done

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

Done. I'll file the binding generator bug.

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

Ok, I'll address that as a follow-up

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

Done, this is already tested with WPT tests

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

Done

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

I improved the comments

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

Done

>> ChangeLog:10
>> +        * Source/cmake/OptionsWPE.cmake:
> 
> Do not find the change there.

The change is adding this line: SET_AND_EXPOSE_TO_BUILD(ENABLE_GAMEPAD ON)

[Imanol Fernandez]

Created attachment 426801 [details]
Patch for landing

[EWS]

Committed r276433 (236896@main): <https://commits.webkit.org/236896@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426801 [details].