Bug 222270 - Implement WebXR getViewport
Summary: Implement WebXR getViewport
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebXR (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 208988
  Show dependency treegraph
 
Reported: 2021-02-22 08:01 PST by Imanol Fernandez
Modified: 2021-02-24 05:10 PST (History)
6 users (show)

See Also:


Attachments
Patch (23.09 KB, patch)
2021-02-22 08:42 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (22.96 KB, patch)
2021-02-22 08:55 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (26.46 KB, patch)
2021-02-23 05:54 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Imanol Fernandez 2021-02-22 08:01:51 PST
Implement WebXRWebGLLayer::getViewport() and related viewport scaling support methods.
Comment 1 Imanol Fernandez 2021-02-22 08:42:02 PST
Created attachment 421197 [details]
Patch
Comment 2 Imanol Fernandez 2021-02-22 08:55:33 PST
Created attachment 421199 [details]
Patch

Fix changelog nit
Comment 3 Sergio Villar Senin 2021-02-22 09:09:01 PST
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.
}
Comment 4 Imanol Fernandez 2021-02-23 05:52:33 PST
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
Comment 5 Imanol Fernandez 2021-02-23 05:54:56 PST
Created attachment 421306 [details]
Patch

Address review feedback. Only compute the viewports when dirty
Comment 6 Sergio Villar Senin 2021-02-24 05:05:21 PST
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)
Comment 7 EWS 2021-02-24 05:09:55 PST
Committed r273381: <https://commits.webkit.org/r273381>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421306 [details].
Comment 8 Radar WebKit Bug Importer 2021-02-24 05:10:18 PST
<rdar://problem/74691035>