[Webkit-unassigned] [Bug 222607] Implement WebXR Opaque Framebuffer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=222607

--- Comment #8 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 422371
  --> https://bugs.webkit.org/attachment.cgi?id=422371
Patch

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

Not confident doing a formal review here so just a few drive-by comments.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:60
> +static constexpr double MinFramebufferScalingFactor = 0.2;

static is not needed for constants like these.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:66
> +        return Exception { OperationError };

Please pass an exception message as second parameter. This is not Web-developer friendly.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:84
> +        return Exception { OperationError };

ditto about exception message.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:95
> +        return Exception { OperationError };

ditto about exception message.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:347
> +    auto opaque = std::unique_ptr<WebXROpaqueFramebuffer>(new WebXROpaqueFramebuffer(handle, WTFMove(framebuffer), context, WTFMove(attributes), width, height));

Why isn't this using makeUnique<>()?

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:366
> +    if (gl) {

if (auto gl = m_context.graphicsContextGL()) {

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.h:129
> +

Nit: extra line

> Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:33
> +{

Can we ASSERT(isMainThread()) since you are incrementing a global thread-unsafe integer?

> Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:58
> +    return std::unique_ptr<OpenXRLayerProjection>(new OpenXRLayerProjection(makeUniqueRefFromNonNullUniquePtr(WTFMove(swapchain))));

makeUnique<>()?

> Source/WebCore/platform/xr/openxr/OpenXRLayer.cpp:76
> +Optional<XrCompositionLayerBaseHeader*> OpenXRLayerProjection::endFrame(const Device::Layer& layer, XrSpace space, const Vector<XrView>& frameViews)

Seems weird to have an optional of a pointer. Do you really need to distinguish WTF::nullopt and nullptr? Looking at the implementation, it does not seem like it.

> Source/WebCore/platform/xr/openxr/OpenXRSwapchain.h:32
> +class OpenXRSwapchain final {

Why final? There is no base class.

> Source/WebCore/platform/xr/openxr/OpenXRSwapchain.h:45
> +    OpenXRSwapchain(XrInstance, XrSession, XrSwapchain, const XrSwapchainCreateInfo&, Vector<XrSwapchainImageOpenGLKHR>&&);

Seems this should be private given we have a create() function?

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:246
> +    m_queue.dispatch([this, layers = WTFMove(layers)]() mutable {

What guarantees |this| is still alive when the lambda runs on the work queue. This looks unsafe.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210305/43b4b703/attachment-0001.htm>