RESOLVED FIXED 223257
Implement WebXR Input Sources
https://bugs.webkit.org/show_bug.cgi?id=223257
Summary Implement WebXR Input Sources
Attachments
Patch (71.07 KB, patch)
2021-03-16 09:53 PDT, Imanol Fernandez
no flags
Patch (71.06 KB, patch)
2021-03-16 10:00 PDT, Imanol Fernandez
no flags
Patch (158.29 KB, patch)
2021-03-18 07:31 PDT, Imanol Fernandez
no flags
Patch (73.24 KB, patch)
2021-03-26 11:02 PDT, Imanol Fernandez
no flags
Patch (91.84 KB, patch)
2021-04-07 09:59 PDT, Imanol Fernandez
no flags
Patch (89.33 KB, patch)
2021-04-16 09:17 PDT, Imanol Fernandez
no flags
Patch for landing (88.33 KB, patch)
2021-04-22 04:52 PDT, Imanol Fernandez
no flags
Imanol Fernandez
Comment 1 2021-03-16 09:53:41 PDT
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
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.