Bug 183162 - [WebVR] Convert VRPlatformDisplayInfo into a class
Summary: [WebVR] Convert VRPlatformDisplayInfo into a class
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:
 
Reported: 2018-02-27 05:37 PST by Sergio Villar Senin
Modified: 2018-02-28 00:46 PST (History)
6 users (show)

See Also:


Attachments
Patch (17.96 KB, patch)
2018-02-27 05:44 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (17.97 KB, patch)
2018-02-27 05:47 PST, Sergio Villar Senin
zan: review+
Details | Formatted Diff | Diff
Patch (18.01 KB, patch)
2018-02-27 08:51 PST, Sergio Villar Senin
no flags 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 2018-02-27 05:37:55 PST
[WebVR] Convert VRPlatformDisplayInfo into a class
Comment 1 Sergio Villar Senin 2018-02-27 05:44:31 PST
Created attachment 334686 [details]
Patch
Comment 2 Sergio Villar Senin 2018-02-27 05:47:10 PST
Created attachment 334687 [details]
Patch
Comment 3 Zan Dobersek 2018-02-27 07:01:21 PST
Comment on attachment 334687 [details]
Patch

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

> Source/WebCore/platform/vr/VRPlatformDisplay.h:49
> +    String displayName() const { return m_displayName; }

[1]: Should return a const reference.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:50
> +    void setDisplayName(String&& displayName) { m_displayName = WTFMove(displayName); }

For later reference, this works as intended.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:65
> +    FloatPoint3D eyeTranslation(Eye eye) const { return m_eyeTranslation[eye]; }

Ditto [1].

> Source/WebCore/platform/vr/VRPlatformDisplay.h:66
> +    void setEyeTranslation(Eye eye, FloatPoint3D&& translation) { m_eyeTranslation[eye] = translation; }

[2]: Rvalue references are only useful in these cases for classes that support move semantics, i.e. have a move constructor and/or move assignment operator. This works with String, but is not the case for FloatPoint3D, so I'd recommend using a simple const reference to the FloatPoint3D object as the parameter.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:74
> +    FieldOfView eyeFieldOfView(Eye eye) const { return m_eyeFieldOfView[eye]; }

Ditto [1].

> Source/WebCore/platform/vr/VRPlatformDisplay.h:75
> +    void setEyeFieldOfView(Eye eye, FieldOfView&& fieldOfView) { m_eyeFieldOfView[eye] = WTFMove(fieldOfView); }

Ditto [2] for FieldOfView. WTFMove() doesn't help.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:81
> +    RenderSize renderSize() const { return m_renderSize; }

Ditto [1].

> Source/WebCore/platform/vr/VRPlatformDisplay.h:82
> +    void setRenderSize(RenderSize&& renderSize) { m_renderSize = WTFMove(renderSize); }

Ditto [2] for RenderSize. WTFMove() doesn't help.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:84
> +    void setPlayAreaBounds(FloatSize&& playAreaBounds) { m_playAreaBounds = playAreaBounds; }

Ditto [2] for FloatSize.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:85
> +    std::optional<FloatSize> playAreaBounds() const { return m_playAreaBounds; }

Ditto [1].

> Source/WebCore/platform/vr/VRPlatformDisplay.h:87
> +    void setSittingToStandingTransform(TransformationMatrix&& transform) { m_sittingToStandingTransform = transform; }

Ditto [2] for TransformationMatrix.

> Source/WebCore/platform/vr/VRPlatformDisplay.h:88
> +    std::optional<TransformationMatrix> sittingToStandingTransform() const { return m_sittingToStandingTransform; }

Ditto [1].
Comment 4 Sergio Villar Senin 2018-02-27 07:48:28 PST
(In reply to Zan Dobersek from comment #3)
> Comment on attachment 334687 [details]
> > Source/WebCore/platform/vr/VRPlatformDisplay.h:66
> > +    void setEyeTranslation(Eye eye, FloatPoint3D&& translation) { m_eyeTranslation[eye] = translation; }
> 
> [2]: Rvalue references are only useful in these cases for classes that
> support move semantics, i.e. have a move constructor and/or move assignment
> operator. This works with String, but is not the case for FloatPoint3D, so
> I'd recommend using a simple const reference to the FloatPoint3D object as
> the parameter.

For some reason I don't know I thought they all had them (the move constructor). Yeah I'll replace them all with const references.
Comment 5 Sergio Villar Senin 2018-02-27 08:51:45 PST
Created attachment 334691 [details]
Patch

Patch for landing
Comment 6 Sergio Villar Senin 2018-02-28 00:45:35 PST
Committed r229089: <https://trac.webkit.org/changeset/229089>
Comment 7 Radar WebKit Bug Importer 2018-02-28 00:46:45 PST
<rdar://problem/37974126>