Bug 222270 – Implement WebXR getViewport

• Status: RESOLVED
• Resolution: FIXED
• Component: WebXR

[Imanol Fernandez]

Implement WebXRWebGLLayer::getViewport() and related viewport scaling support methods.

[Imanol Fernandez]

Created attachment 421197 [details]
Patch

[Imanol Fernandez]

Created attachment 421199 [details]
Patch

Fix changelog nit

[Sergio Villar Senin]

Comment on attachment 421197 [details]
Patch

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

Looking good, just a few changes here and there required.

> Source/WebCore/ChangeLog:9
> +        https://bugs.webkit.org/show_bug.cgi?id=222270

Duplicate entry

> Source/WebCore/ChangeLog:13
> +        * Modules/webxr/WebXRFrame.cpp: set WebXRView viewport modifiable value.

Nit, the comment is added in the line having the method name not in the line with the source file (unless it's a general comment for the file)

> Source/WebCore/Modules/webxr/WebXRSession.cpp:313
> +    // Only immersive sessions viewport scaling.

Maybe "support" is missing between "sessions" and "viewport"

> Source/WebCore/Modules/webxr/WebXRView.cpp:38
> +static constexpr double kMinViewportScale = 0.2;

Does it come from specs? If so please add a link for reference.

> Source/WebCore/Modules/webxr/WebXRViewport.h:50
> +    WebXRViewport(const IntRect&);

explicit

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:217
> +    // Compute the scaled viewport.

From the specs it looks like this computation should be only done if 6. is true, so I guess you have to move it all to the if block above.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:235
> +    else /// XREye::None

better write this as 

else {
 // XREye::None
 viewportRect.
}

[Imanol Fernandez]

Comment on attachment 421197 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        https://bugs.webkit.org/show_bug.cgi?id=222270
> 
> Duplicate entry

Fixed

>> Source/WebCore/ChangeLog:13
>> +        * Modules/webxr/WebXRFrame.cpp: set WebXRView viewport modifiable value.
> 
> Nit, the comment is added in the line having the method name not in the line with the source file (unless it's a general comment for the file)

Fixed

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:313
>> +    // Only immersive sessions viewport scaling.
> 
> Maybe "support" is missing between "sessions" and "viewport"

Yes, I wanted to add "support" work in that comment

>> Source/WebCore/Modules/webxr/WebXRView.cpp:38
>> +static constexpr double kMinViewportScale = 0.2;
> 
> Does it come from specs? If so please add a link for reference.

It's an arbitrary value. I added some comments. Chrome is using around 0.05 now and Gecko around 0.2 IIRC.

>> Source/WebCore/Modules/webxr/WebXRViewport.h:50
>> +    WebXRViewport(const IntRect&);
> 
> explicit

done

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:217
>> +    // Compute the scaled viewport.
> 
> From the specs it looks like this computation should be only done if 6. is true, so I guess you have to move it all to the if block above.

The spec only updates the viewports when dirty while the patch was computing them on each getViewport call. I updated the patch to user dirty checking. We also need to handle canvasResize to do it correctly

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:235
>> +    else /// XREye::None
> 
> better write this as 
> 
> else {
>  // XREye::None
>  viewportRect.
> }

done

[Imanol Fernandez]

Created attachment 421306 [details]
Patch

Address review feedback. Only compute the viewports when dirty

[Sergio Villar Senin]

Comment on attachment 421306 [details]
Patch

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

Awesome job, thanks!

> Source/WebCore/Modules/webxr/WebXRSession.cpp:313
> +    // Only immersive sessions support viewport scaling.

We can probably get rid of this comment, the code bellow is self-descriptive (let's do it in a follow up though)

[EWS]

Committed r273381: <https://commits.webkit.org/r273381>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421306 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74691035>