Bug 220835

Created attachment 418099 [details] Patch

Comment on attachment 418099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418099&action=review > Source/WebCore/Modules/webxr/WebXRSession.h:106 > + void shutdown(InitiatedBySystem); Should the parameter have a default value?

(In reply to Dean Jackson from comment #3) > Comment on attachment 418099 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418099&action=review > > > Source/WebCore/Modules/webxr/WebXRSession.h:106 > > + void shutdown(InitiatedBySystem); > > Should the parameter have a default value? I don't think it's needed as it's a private method in WebXRSession and it's only called twice (once with each value). By leaving out the default value, it forces the caller to think about which value to use when calling it. Do you think there's a good reason for a default value here?

Created attachment 418209 [details] Patch

Some minor updates to the last version: - Added [MayThrowException] to the end() method in WebXRSession.idl. - A couple of comment changes

Comment on attachment 418209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418209&action=review > Source/WebCore/ChangeLog:16 > + Any thoughts on how we could test this?

Comment on attachment 418209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418209&action=review >> Source/WebCore/ChangeLog:16 >> + > > Any thoughts on how we could test this? Maybe using the WebFakeXRDevice we have in WebCore/testing? I guess we should at least test the following situations * supports shutdown notification with InitiatedBySystem::Yes and InitiatedBySystem::No * *does not* support shutdown notification with InitiatedBySystem::Yes and InitiatedBySystem::No > Source/WebCore/Modules/webxr/WebXRSession.cpp:391 > + return Exception { InvalidStateError }; I don't remember something like this in the spec. I guess you should report an issue in the specs github along with a test for the WPT test suite. > Source/WebCore/Modules/webxr/WebXRSession.cpp:415 > +// MARK: - TrackingAndRenderingClient What's this? > Source/WebCore/platform/xr/PlatformXR.h:53 > + // TODO: handle visibility changes WebKit uses FIXME instead of TODO > Source/WebCore/platform/xr/PlatformXR.h:77 > + virtual bool willNotifySessionShutdown() const { return false; } This is a bit of bikeshedding but willDoXXX() methods in Webkit normally are called before a DoXXX() method, i.e., are used to let clients set up themselves for a later call. In this case I think it's better to use something like supportsSessionShutdownNotification().

(In reply to Sergio Villar Senin from comment #8) > Comment on attachment 418209 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418209&action=review > > >> Source/WebCore/ChangeLog:16 > >> + > > > > Any thoughts on how we could test this? > > Maybe using the WebFakeXRDevice we have in WebCore/testing? > > I guess we should at least test the following situations > * supports shutdown notification with InitiatedBySystem::Yes and > InitiatedBySystem::No > * *does not* support shutdown notification with InitiatedBySystem::Yes and > InitiatedBySystem::No I'll take a look at the existing webxr layout tests to see how I can use WebFakeXRDevice to test these situations. > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:391 > > + return Exception { InvalidStateError }; > > I don't remember something like this in the spec. I guess you should report > an issue in the specs github along with a test for the WPT test suite. This is more of my attempt to simplify the handling of this situation. A more complicated alternative would be to save off the end promise (so we'll end up storing a list of these end promises) and resolve them when the session termination has completed. > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:415 > > +// MARK: - TrackingAndRenderingClient > > What's this? It's a protocol that WebXRSession implements. PlatformXR::Device communicates session updates (such as when the session termination has completed) through this protocol. > > > Source/WebCore/platform/xr/PlatformXR.h:53 > > + // TODO: handle visibility changes > > WebKit uses FIXME instead of TODO OK, will fix. > > > Source/WebCore/platform/xr/PlatformXR.h:77 > > + virtual bool willNotifySessionShutdown() const { return false; } > > This is a bit of bikeshedding but willDoXXX() methods in Webkit normally are > called before a DoXXX() method, i.e., are used to let clients set up > themselves for a later call. In this case I think it's better to use > something like supportsSessionShutdownNotification(). That's a good idea. I'll switch to the method name you suggested.

Created attachment 418451 [details] Patch

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Created attachment 418478 [details] Patch

Added new tests under http/wpt/webxr.

(In reply to Ada Chan from comment #13) > Added new tests under http/wpt/webxr. Are these tests coming from upstream web-platform-tests (wpt) or did you write them? If they are imported from upstream wpt, they need to go under LayoutTests/imported/w3c/web-platform-tests/. If you wrote them, you put them in the right place already.

(In reply to Chris Dumez from comment #14) > (In reply to Ada Chan from comment #13) > > Added new tests under http/wpt/webxr. > > Are these tests coming from upstream web-platform-tests (wpt) or did you > write them? If they are imported from upstream wpt, they need to go under > LayoutTests/imported/w3c/web-platform-tests/. If you wrote them, you put > them in the right place already. Yes, I wrote these tests, not from upstream wpt.

Comment on attachment 418478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418478&action=review > Source/WebCore/Modules/webxr/WebXRSession.cpp:61 > + m_device->setTrackingAndRenderingClient(makeWeakPtr((PlatformXR::TrackingAndRenderingClient*)this)); Please don't use C-casts (static_cast<> is preferred). Also, why is this cast needed? > Source/WebCore/Modules/webxr/WebXRSession.cpp:323 > + Ref<WebXRSession> protectedThis(*this); I think we like this pattern more nowadays: auto protectedThis = makeRef(*this); > Source/WebCore/Modules/webxr/WebXRSession.cpp:390 > + if (m_ended) I am looking at the spec [1] but I don't see this behavior specified anyway. Why should we throw in this case? [1] https://immersive-web.github.io/webxr/#dom-xrsession-end > Source/WebCore/Modules/webxr/WebXRSession.cpp:391 > + return Exception { InvalidStateError }; Please provide a proper error message as second parameter. > Source/WebCore/Modules/webxr/WebXRSession.cpp:400 > + m_endPromise = WTF::makeUnique<EndPromise>(WTFMove(promise)); WTF:: should not me needed. Also why the makeUnique? Cannot we just do m_endPromise = WTFMove(promise) and have the data member use Optional<EndPromise>? > Source/WebCore/Modules/webxr/WebXRSession.cpp:415 > +// MARK: - TrackingAndRenderingClient Not sure what that is. We don't do this in WebKit usually :) > Source/WebCore/Modules/webxr/WebXRSession.h:84 > You will likely need something like this to avoid your earlier cast: using EventTargetWithInlineData::weakPtrFactory; using WeakValueType = EventTargetWithInlineData::WeakValueType; > Source/WebCore/Modules/webxr/WebXRSession.h:105 > + enum class InitiatedBySystem { No, Yes }; enum class InitiatedBySystem : bool { No, Yes }; > Source/WebCore/platform/xr/PlatformXR.h:73 > + void setTrackingAndRenderingClient(WeakPtr<TrackingAndRenderingClient>&& client) { m_trackingAndRenderingClient = client; } = WTFMove(client)

(In reply to Chris Dumez from comment #16) > Comment on attachment 418478 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418478&action=review > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:61 > > + m_device->setTrackingAndRenderingClient(makeWeakPtr((PlatformXR::TrackingAndRenderingClient*)this)); > > Please don't use C-casts (static_cast<> is preferred). Also, why is this > cast needed? Thanks for your later suggestion, I don't need this cast anymore. > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:323 > > + Ref<WebXRSession> protectedThis(*this); > > I think we like this pattern more nowadays: > auto protectedThis = makeRef(*this); Got it! > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:390 > > + if (m_ended) > > I am looking at the spec [1] but I don't see this behavior specified anyway. > Why should we throw in this case? > > [1] https://immersive-web.github.io/webxr/#dom-xrsession-end It simplifies the handling of this scenario, and turns out this matches Chromium's behavior too. > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:391 > > + return Exception { InvalidStateError }; > > Please provide a proper error message as second parameter. Done. > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:400 > > + m_endPromise = WTF::makeUnique<EndPromise>(WTFMove(promise)); > > WTF:: should not me needed. Removed. > > Also why the makeUnique? Cannot we just do m_endPromise = WTFMove(promise) > and have the data member use Optional<EndPromise>? Yep, I've followed your suggestion. > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:415 > > +// MARK: - TrackingAndRenderingClient > > Not sure what that is. We don't do this in WebKit usually :) Oh I see. It was the strangeness of the comment! I'll remove it. > > > Source/WebCore/Modules/webxr/WebXRSession.h:84 > > > > You will likely need something like this to avoid your earlier cast: > using EventTargetWithInlineData::weakPtrFactory; > using WeakValueType = EventTargetWithInlineData::WeakValueType; > > > Source/WebCore/Modules/webxr/WebXRSession.h:105 > > + enum class InitiatedBySystem { No, Yes }; > > enum class InitiatedBySystem : bool { No, Yes }; Fixed. > > > Source/WebCore/platform/xr/PlatformXR.h:73 > > + void setTrackingAndRenderingClient(WeakPtr<TrackingAndRenderingClient>&& client) { m_trackingAndRenderingClient = client; } > > = WTFMove(client) Fixed. Thanks for the feedback!

Created attachment 418489 [details] Patch

Comment on attachment 418489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418489&action=review r=me with changes > Source/WebCore/Modules/webxr/WebXRSession.cpp:391 > + return Exception { InvalidStateError, "Cannot end a session more than once" }; return Exception { InvalidStateError, "Cannot end a session more than once"_s }; is slightly more efficient since the second parameter is a String. > Source/WebCore/Modules/webxr/WebXRSession.cpp:400 > + m_endPromise = WTFMove(promise); I would suggest adding the following assertion right before the assignment, just to make sure we never overwrite an existing promise (which would then never get resolved): ASSERT(!m_endPromise); > Source/WebCore/Modules/webxr/WebXRSession.h:89 > + void sessionDidEnd() final; Does this really need to be public? > LayoutTests/platform/mac/TestExpectations:2261 > +# --- Start WebXR tests --- # I would just use: # WebXR tests > LayoutTests/platform/mac/TestExpectations:2263 > +http/wpt/webxr/xrSession_end_device_reports_shutdown.https.html [ Pass ] You likely want to [ Skip ] http/wpt/webxr in LayoutTests/TestExpectations, like we do for imported/w3c/web-platform-tests/webxr already. If the tests are not skipped globally, then it makes no sense to enabled them for specific platforms. > LayoutTests/platform/mac/TestExpectations:2266 > +# --- End WebXR tests --- # I don't think we need this end comment.

Comment on attachment 418489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418489&action=review > LayoutTests/platform/mac/TestExpectations:2264 > +http/wpt/webxr/xrSession_ended_by_system.https.html [ Pass ] Do you foresee that we'll add tests to http/wpt/webxr that don't pass on macOS? If not, I'd suggest marking the whole folder as [ Pass ]: http/wpt/webxr [ Pass ]

(In reply to Chris Dumez from comment #20) > Comment on attachment 418489 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418489&action=review > > > LayoutTests/platform/mac/TestExpectations:2264 > > +http/wpt/webxr/xrSession_ended_by_system.https.html [ Pass ] > > Do you foresee that we'll add tests to http/wpt/webxr that don't pass on > macOS? If not, I'd suggest marking the whole folder as [ Pass ]: > http/wpt/webxr [ Pass ] That's a good point. I'll change it.

(In reply to Chris Dumez from comment #19) > Comment on attachment 418489 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418489&action=review > > r=me with changes > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:391 > > + return Exception { InvalidStateError, "Cannot end a session more than once" }; > > return Exception { InvalidStateError, "Cannot end a session more than > once"_s }; > > is slightly more efficient since the second parameter is a String. Fixed. > > > Source/WebCore/Modules/webxr/WebXRSession.cpp:400 > > + m_endPromise = WTFMove(promise); > > I would suggest adding the following assertion right before the assignment, > just to make sure we never overwrite an existing promise (which would then > never get resolved): > ASSERT(!m_endPromise); Good idea. Fixed. > > > Source/WebCore/Modules/webxr/WebXRSession.h:89 > > + void sessionDidEnd() final; > > Does this really need to be public? Nope, moved it to private. > > > LayoutTests/platform/mac/TestExpectations:2261 > > +# --- Start WebXR tests --- # > > I would just use: > # WebXR tests Fixed. > > > LayoutTests/platform/mac/TestExpectations:2263 > > +http/wpt/webxr/xrSession_end_device_reports_shutdown.https.html [ Pass ] > > You likely want to [ Skip ] http/wpt/webxr in LayoutTests/TestExpectations, > like we do for imported/w3c/web-platform-tests/webxr already. If the tests > are not skipped globally, then it makes no sense to enabled them for > specific platforms. I see. I added the skip in LayoutTests/TestExpectations. > > > LayoutTests/platform/mac/TestExpectations:2266 > > +# --- End WebXR tests --- # > > I don't think we need this end comment. OK, fixed.

Created attachment 418493 [details] Patch

A couple of comments about the tests. You're adding a lot of resource files from WPT that are already in LayoutTests/imported/w3c/web-platform-tests/webxr/resources. I don't see the point of importing them in a different directory. Secondly, some of them might indeed be non exportable to WPT as you're testing a WebKit implementation detail but http/ does not seem the best place to me to put those on. Last but not least, I still believe that if we want to throw exceptions in the end() method then we must send a PR to the specs and the corresponding tests to the WPT test suite. The fact that Chrome does it reinforces that idea. BTW would you mind adding those tests also to the WPE TestExpectations file?

(In reply to Sergio Villar Senin from comment #24) > A couple of comments about the tests. You're adding a lot of resource files > from WPT that are already in > LayoutTests/imported/w3c/web-platform-tests/webxr/resources. I don't see the > point of importing them in a different directory. It is true that tests in http/wpt/ have access to resources in LayoutTests/imported/w3c/web-platform-tests/webxr/ so there is no need for duplication. IIRC, when the WPT server runs, files under http/wpt/ are exposed under imported/w3c/web-platform-tests/webkit/ > > Secondly, some of them might indeed be non exportable to WPT as you're > testing a WebKit implementation detail but http/ does not seem the best > place to me to put those on. I personally don't see an issue with putting browser-specific tests under http/tests/wpt as long as they use the WPT infrastructure. > > Last but not least, I still believe that if we want to throw exceptions in > the end() method then we must send a PR to the specs and the corresponding > tests to the WPT test suite. The fact that Chrome does it reinforces that > idea. Maybe filing an issue against the spec on GitHub at least? > > BTW would you mind adding those tests also to the WPE TestExpectations file?

(In reply to Chris Dumez from comment #25) > (In reply to Sergio Villar Senin from comment #24) > > A couple of comments about the tests. You're adding a lot of resource files > > from WPT that are already in > > LayoutTests/imported/w3c/web-platform-tests/webxr/resources. I don't see the > > point of importing them in a different directory. > > It is true that tests in http/wpt/ have access to resources in > LayoutTests/imported/w3c/web-platform-tests/webxr/ so there is no need for > duplication. IIRC, when the WPT server runs, files under http/wpt/ are > exposed under imported/w3c/web-platform-tests/webkit/ I have removed the files in resources and updated the paths to those files in the tests. > > > > > Secondly, some of them might indeed be non exportable to WPT as you're > > testing a WebKit implementation detail but http/ does not seem the best > > place to me to put those on. > > I personally don't see an issue with putting browser-specific tests under > http/tests/wpt as long as they use the WPT infrastructure. > > > > > Last but not least, I still believe that if we want to throw exceptions in > > the end() method then we must send a PR to the specs and the corresponding > > tests to the WPT test suite. The fact that Chrome does it reinforces that > > idea. > > Maybe filing an issue against the spec on GitHub at least? I'll look into it. > > > > > BTW would you mind adding those tests also to the WPE TestExpectations file? I'll add them to wpe/TestExpectations.

Created attachment 418567 [details] Patch

I've filed an issue against the spec regarding XRSession.end(): https://github.com/immersive-web/webxr/issues/1166

Committed r271988: <https://trac.webkit.org/changeset/271988> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418567 [details].

It looks like the changes in https://trac.webkit.org/changeset/271988/webkit introduced 3 constantly failing tests: http/wpt/webxr/xrSession_end_device_reports_shutdown.https.html http/wpt/webxr/xrSession_reject_multiple_end.https.html http/wpt/webxr/xrSession_ended_by_system.https.html History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&test=http%2Fwpt%2Fwebxr%2FxrSession_end_device_reports_shutdown.https.html&test=http%2Fwpt%2Fwebxr%2FxrSession_ended_by_system.https.html&test=http%2Fwpt%2Fwebxr%2FxrSession_reject_multiple_end.https.html This did not fail on EWS because these tests only run in Big Sur

Thanks Truitt! I've filed: https://bugs.webkit.org/show_bug.cgi?id=221092