WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
223257
Implement WebXR Input Sources
https://bugs.webkit.org/show_bug.cgi?id=223257
Summary
Implement WebXR Input Sources
Imanol Fernandez
Reported
2021-03-16 08:53:35 PDT
-
https://immersive-web.github.io/webxr/#input
-
https://immersive-web.github.io/webxr-gamepads-module/#webxr-device-api-integration
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Imanol Fernandez
Comment 1
2021-03-16 09:53:41 PDT
Created
attachment 423344
[details]
Patch
Imanol Fernandez
Comment 2
2021-03-16 10:00:04 PDT
Created
attachment 423347
[details]
Patch Fix nit
Imanol Fernandez
Comment 3
2021-03-18 07:31:29 PDT
Created
attachment 423598
[details]
Patch Rebase onto main
Radar WebKit Bug Importer
Comment 4
2021-03-23 08:54:14 PDT
<
rdar://problem/75739455
>
Imanol Fernandez
Comment 5
2021-03-26 11:02:01 PDT
Created
attachment 424375
[details]
Patch Rebase onto main. Add CachedAttribute to pass sameObject related tests.
youenn fablet
Comment 6
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.
Imanol Fernandez
Comment 7
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.
Imanol Fernandez
Comment 8
2021-04-07 09:59:18 PDT
Created
attachment 425413
[details]
Patch Address review feedback and rebase onto main
Imanol Fernandez
Comment 9
2021-04-16 09:17:34 PDT
Created
attachment 426234
[details]
Patch Rebase onto main
youenn fablet
Comment 10
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.
Imanol Fernandez
Comment 11
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)
Imanol Fernandez
Comment 12
2021-04-22 04:52:04 PDT
Created
attachment 426801
[details]
Patch for landing
EWS
Comment 13
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug