Bug 211506

Summary: [WebXR] Implement WebGLRenderingContextBase::makeXRCompatible()
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, darin, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, hi, kondapallykalyan, rniwa, svillar, webkit-bug-importer, youennf, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 208988    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch youennf: review+

Description Sergio Villar Senin 2020-05-06 05:51:05 PDT
[WebXR] Implement WebGLRenderingContextBase::makeXRCompatible()
Comment 1 Sergio Villar Senin 2020-05-06 08:29:36 PDT
Created attachment 398619 [details]
Patch
Comment 2 Sergio Villar Senin 2020-05-06 09:24:04 PDT
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 3 youenn fablet 2020-05-06 10:15:53 PDT
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 4 Sergio Villar Senin 2020-05-06 11:38:39 PDT
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...
Comment 5 Sergio Villar Senin 2020-05-07 06:37:44 PDT
It seems like it's a bug in the code generator. See https://bugs.webkit.org/show_bug.cgi?id=211528
Comment 6 Sergio Villar Senin 2020-05-07 06:46:54 PDT
(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.
Comment 7 Sergio Villar Senin 2020-05-07 11:10:04 PDT
Created attachment 398765 [details]
Patch

Use ScopeExit
Comment 8 Darin Adler 2020-05-09 17:09:47 PDT
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 9 Sergio Villar Senin 2020-05-10 09:29:05 PDT
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.
Comment 10 Sergio Villar Senin 2020-05-11 04:28:07 PDT
Created attachment 399011 [details]
Patch
Comment 11 youenn fablet 2020-05-12 04:37:33 PDT
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.
Comment 12 Sergio Villar Senin 2020-05-12 07:05:13 PDT
Gentle ping for reviewers
Comment 13 Sergio Villar Senin 2020-05-13 01:36:25 PDT
Created attachment 399249 [details]
Patch

Applied suggested changes. Also simplified the usage of NavigatorWebXR
Comment 14 Sergio Villar Senin 2020-05-14 00:45:24 PDT
Committed r261679: <https://trac.webkit.org/changeset/261679>
Comment 15 Radar WebKit Bug Importer 2020-05-14 00:46:15 PDT
<rdar://problem/63218353>