Bug 211506

Created attachment 398619 [details] Patch

Amazingly the generated JSWebGLRenderingContext.cpp does not have the ENABLE(WEBXR) for the makeXRCompatible() call. I'll research a bit but it looks like a code generation bug...

Comment on attachment 398619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398619&action=review > Source/WebCore/Modules/webxr/WebXRSystem.h:34 > +#include "WebGLContextAttributes.h" Can we forward declare instead of adding this include? > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3487 > + auto rejectPromiseWithInvalidStateError = [&] () { You can use ScopeExit instead to return without even calling the lambda. > Source/WebCore/html/canvas/WebGLRenderingContextBase.idl:606 > + [NewObject, Conditional=WEBXR] Promise<void> makeXRCompatible(); This should work, maybe put Conditional first in the list as this is what we usually do.

Comment on attachment 398619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398619&action=review >> Source/WebCore/Modules/webxr/WebXRSystem.h:34 >> +#include "WebGLContextAttributes.h" > > Can we forward declare instead of adding this include? Ah it's a rvalue, then I guess we can do it indeed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3487 >> + auto rejectPromiseWithInvalidStateError = [&] () { > > You can use ScopeExit instead to return without even calling the lambda. I'll change it. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.idl:606 >> + [NewObject, Conditional=WEBXR] Promise<void> makeXRCompatible(); > > This should work, maybe put Conditional first in the list as this is what we usually do. Yeah, I've already tried that. I've also tried with other interfaces that are also inherited just in case and everything works as expected. But it seems that nothing works in this particular file...

It seems like it's a bug in the code generator. See https://bugs.webkit.org/show_bug.cgi?id=211528

(In reply to Sergio Villar Senin from comment #4) > Comment on attachment 398619 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398619&action=review > > >> Source/WebCore/Modules/webxr/WebXRSystem.h:34 > >> +#include "WebGLContextAttributes.h" > > > > Can we forward declare instead of adding this include? > > Ah it's a rvalue, then I guess we can do it indeed. Now that I remember, I used include because that header defines an using for the struct, so unless we want to use the name defined in platform/ we should use the include instead of forward declaring it.

Created attachment 398765 [details] Patch Use ScopeExit

Comment on attachment 398765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398765&action=review > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3526 > + if (m_isXRCompatible) > + promise.resolve(); > + > + // If context was created on a compatible graphics adapter for the immersive XR device > + // Set contextâs XR compatible boolean to true and resolve promise. > + // Otherwise: Queue a task on the WebGL task source to perform the following steps: > + // TODO: add a way to verify that we're using a compatible graphics adapter. > + m_isXRCompatible = true; > + promise.resolve(); This looks strange. Is it correct to resolve the promise twice?

Comment on attachment 398765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398765&action=review >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3526 >> + promise.resolve(); > > This looks strange. Is it correct to resolve the promise twice? Oh, I somehow skipped an early return in the previous if block.

Created attachment 399011 [details] Patch

Comment on attachment 399011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399011&action=review > Source/WebCore/Modules/webxr/WebXRSystem.h:50 > + friend WebGLRenderingContextBase* HTMLCanvasElement::createContextWebGL(const String&, WebGLContextAttributes&&); Can we do without these two? It seems we need to add a getter and make public one method. > Source/WebCore/html/HTMLCanvasElement.cpp:430 > + Navigator& navigator = window->navigator(); auto or inline the call. > Source/WebCore/html/HTMLCanvasElement.cpp:431 > + NavigatorWebXR::from(navigator).xr(*canvasBaseScriptExecutionContext(), navigator).ensureImmersiveXRDeviceIsSelected(); We could also use scriptExecutionContext() which is shorter. But can we try to not passing the scriptExecutionContext at all and use the one of the navigator? > Source/WebCore/html/HTMLCanvasElement.cpp:443 > + // as this is handled automatically inside WebGLRenderingContextBase constructor. Should we add an ASSERT instead of this comment? > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:119 > +#if ENABLE(WEBXR) Not needed. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3517 > + if (!xrSystem.m_activeImmersiveDevice) A getter would be better.

Gentle ping for reviewers

Created attachment 399249 [details] Patch Applied suggested changes. Also simplified the usage of NavigatorWebXR

Committed r261679: <https://trac.webkit.org/changeset/261679>

<rdar://problem/63218353>