[webkit-reviews] review granted: [Bug 183162] [WebVR] Convert VRPlatformDisplayInfo into a class : [Attachment 334687] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 27 07:01:21 PST 2018


Zan Dobersek <zan at falconsigh.net> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 183162: [WebVR] Convert VRPlatformDisplayInfo into a class
https://bugs.webkit.org/show_bug.cgi?id=183162

Attachment 334687: Patch

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




--- Comment #3 from Zan Dobersek <zan at falconsigh.net> ---
Comment on attachment 334687
  --> https://bugs.webkit.org/attachment.cgi?id=334687
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].


More information about the webkit-reviews mailing list