Bug 223257 - Implement WebXR Input Sources
Summary: Implement WebXR Input Sources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebXR (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Imanol Fernandez
URL:
Keywords: InRadar
Depends on:
Blocks: 208988
  Show dependency treegraph
 
Reported: 2021-03-16 08:53 PDT by Imanol Fernandez
Modified: 2021-04-22 07:06 PDT (History)
12 users (show)

See Also:


Attachments
Patch (71.07 KB, patch)
2021-03-16 09:53 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (71.06 KB, patch)
2021-03-16 10:00 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (158.29 KB, patch)
2021-03-18 07:31 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (73.24 KB, patch)
2021-03-26 11:02 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (91.84 KB, patch)
2021-04-07 09:59 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (89.33 KB, patch)
2021-04-16 09:17 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch for landing (88.33 KB, patch)
2021-04-22 04:52 PDT, Imanol Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Imanol Fernandez 2021-03-16 09:53:41 PDT
Created attachment 423344 [details]
Patch
Comment 2 Imanol Fernandez 2021-03-16 10:00:04 PDT
Created attachment 423347 [details]
Patch

Fix nit
Comment 3 Imanol Fernandez 2021-03-18 07:31:29 PDT
Created attachment 423598 [details]
Patch

Rebase onto main
Comment 4 Radar WebKit Bug Importer 2021-03-23 08:54:14 PDT
<rdar://problem/75739455>
Comment 5 Imanol Fernandez 2021-03-26 11:02:01 PDT
Created attachment 424375 [details]
Patch

Rebase onto main. Add CachedAttribute to pass sameObject related tests.
Comment 6 youenn fablet 2021-04-06 08:40:03 PDT
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.
Comment 7 Imanol Fernandez 2021-04-07 09:58:19 PDT
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.
Comment 8 Imanol Fernandez 2021-04-07 09:59:18 PDT
Created attachment 425413 [details]
Patch

Address review feedback and rebase onto main
Comment 9 Imanol Fernandez 2021-04-16 09:17:34 PDT
Created attachment 426234 [details]
Patch

Rebase onto main
Comment 10 youenn fablet 2021-04-21 09:07:49 PDT
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.
Comment 11 Imanol Fernandez 2021-04-22 04:49:42 PDT
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)
Comment 12 Imanol Fernandez 2021-04-22 04:52:04 PDT
Created attachment 426801 [details]
Patch for landing
Comment 13 EWS 2021-04-22 07:06:15 PDT
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].