Bug 211506 – [WebXR] Implement WebGLRenderingContextBase::makeXRCompatible()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[WebXR] Implement WebGLRenderingContextBase::makeXRCompatible()

[Sergio Villar Senin]

Created attachment 398619 [details]
Patch

[Sergio Villar Senin]

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...

[youenn fablet]

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.

[Sergio Villar Senin]

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...

[Sergio Villar Senin]

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

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

Created attachment 398765 [details]
Patch

Use ScopeExit

[Darin Adler]

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?

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

Created attachment 399011 [details]
Patch

[youenn fablet]

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.

[Sergio Villar Senin]

Gentle ping for reviewers

[Sergio Villar Senin]

Created attachment 399249 [details]
Patch

Applied suggested changes. Also simplified the usage of NavigatorWebXR

[Sergio Villar Senin]

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

[Radar WebKit Bug Importer]

<rdar://problem/63218353>