[webkit-reviews] review granted: [Bug 211888] [WebXR] Implement requestSession() : [Attachment 399656] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 19 00:02:07 PDT 2020
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.
More information about the webkit-reviews
mailing list