[webkit-reviews] review granted: [Bug 220835] Send the end XRSessionEvent after the platform-specific steps for session shutdown have completed : [Attachment 418489] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Ada Chan <adachan at apple.com>'s
request for review:
Bug 220835: Send the end XRSessionEvent after the platform-specific steps for
session shutdown have completed
https://bugs.webkit.org/show_bug.cgi?id=220835

Attachment 418489: Patch

https://bugs.webkit.org/attachment.cgi?id=418489&action=review




--- Comment #19 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 418489
  --> https://bugs.webkit.org/attachment.cgi?id=418489
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.