[webkit-reviews] review granted: [Bug 211888] [WebXR] Implement requestSession() : [Attachment 399656] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 211888: [WebXR] Implement requestSession()
https://bugs.webkit.org/show_bug.cgi?id=211888

Attachment 399656: Patch

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




--- Comment #8 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 399656
  --> https://bugs.webkit.org/attachment.cgi?id=399656
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399656&action=review

> Source/WebCore/Modules/webxr/WebXRSession.cpp:43
> +WebXRSession::WebXRSession(Document& document, Ref<WebXRSystem>&& system,
XRSessionMode mode, WeakPtr<PlatformXR::Device>&& device)

If the goal is for device to be valid initially, it might be better to pass a
PlatformXR::Device&

> Source/WebCore/Modules/webxr/WebXRSession.cpp:123
> +    queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [promise =
WTFMove(promise)] () mutable {

Do we need to keep the object alive?

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:233
> +	   resolvedFeatures.granted = device->enabledFeatures(mode);

It would make more sense to move that line after the early return in the next
two lines.

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:358
> +	   if (!inlineSessionRequestIsAllowedForGlobalObject(*globalObject,
document, init)) {

else if

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:365
> +    queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [this, document =
makeRef(document), immersive, init, mode, promise = WTFMove(promise)] ()
mutable {

Why taking a ref here but capture document by ref below? You could take a
weakref and move it as part of next lambda.
Another approach would be to get the global object from the promise itself,
which would simplify things.

> Source/WebCore/Modules/webxr/WebXRSystem.h:116
> +    Vector<Ref<WebXRSession>> m_listOfInlineSessions;

Should it be a vector or a hashset?

> Source/WebCore/platform/xr/PlatformXR.h:63
> +    using EnabledFeaturesPerModeMap = WTF::HashMap<SessionMode,
ListOfEnabledFeatures, WTF::IntHash<SessionMode>,
WTF::StrongEnumHashTraits<SessionMode>>;

s/WTF:://

> Source/WebCore/testing/WebXRTest.cpp:59
> +	       if (auto* globalObject = context.execState()) {

You could reserveInitialCapacity/uncheckedAppend.