Bug 213022 - [WebXR] Add a preliminary implementation of XRWebGLLayer
Summary: [WebXR] Add a preliminary implementation of XRWebGLLayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks: 208988
  Show dependency treegraph
 
Reported: 2020-06-10 06:57 PDT by Sergio Villar Senin
Modified: 2020-06-22 05:29 PDT (History)
6 users (show)

See Also:


Attachments
Patch (17.89 KB, patch)
2020-06-10 07:12 PDT, Sergio Villar Senin
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2020-06-10 06:57:32 PDT
[WebXR] Add a preliminary implementation of XRWebGLLayer
Comment 1 Sergio Villar Senin 2020-06-10 07:12:30 PDT
Created attachment 401538 [details]
Patch
Comment 2 Sergio Villar Senin 2020-06-18 14:49:22 PDT
Ping reviewers
Comment 3 Carlos Garcia Campos 2020-06-19 00:38:19 PDT
Comment on attachment 401538 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:66
> +

Extra line here

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:78
> +                // TODO: Initialize layer's resources or issue an OperationError.

Use FIXME instead of TODO

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:94
> +    // TODO: implement this

Ditto.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:120
> +    m_isCompositionDisabled = m_session->mode() == XRSessionMode::Inline ? true : false;

I guess you don't need the ? true : false

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:125
> +        m_antialias = init.antialias;

This is duplicated, I would probably remove the previous one.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:129
> +        computeRecommendedWebGLFramebufferResolution(recommendedSize);

computeRecommendedWebGLFramebufferResolution could return IntSize instead of using a parameter.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:137
> +        // TODO: create a proper opaque framebuffer.

FIXME

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:207
> +    IntSize nativeSize;
> +    computeNativeWebGLFramebufferResolution(nativeSize);

Better use return value instead of parameter.

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:210
> +    IntSize recommendedSize;
> +    computeRecommendedWebGLFramebufferResolution(recommendedSize);

And here too. Unless those they really need to receive an existing IntSize to be modified.
Comment 4 Sergio Villar Senin 2020-06-19 07:46:42 PDT
Comment on attachment 401538 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:78
>> +                // TODO: Initialize layer's resources or issue an OperationError.
> 
> Use FIXME instead of TODO

OK

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:120
>> +    m_isCompositionDisabled = m_session->mode() == XRSessionMode::Inline ? true : false;
> 
> I guess you don't need the ? true : false

Stupid mistake indeed.

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:125
>> +        m_antialias = init.antialias;
> 
> This is duplicated, I would probably remove the previous one.

Right. I'll also fix the weird characters in the comment.

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:129
>> +        computeRecommendedWebGLFramebufferResolution(recommendedSize);
> 
> computeRecommendedWebGLFramebufferResolution could return IntSize instead of using a parameter.

Yes, compiler must be able to do the return value optimization so I agree it's better to return it.
Comment 5 Sergio Villar Senin 2020-06-22 05:28:17 PDT
Committed r263346: <https://trac.webkit.org/changeset/263346>
Comment 6 Radar WebKit Bug Importer 2020-06-22 05:29:16 PDT
<rdar://problem/64593229>