1148005 - [WebVR] Update API to latest spec changes

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Update WebVR APIs

Update our API to the latest spec -- http://mozvr.github.io/webvr-spec/webvr.html

Changes described in https://mail.mozilla.org/pipermail/web-vr-discuss/2015-March/000566.html

Comment 1

[Andrea Marchesini [:baku]]

Comment on attachment 8583946 [details] [diff] [review]
Update WebVR APIs

Review of attachment 8583946 [details] [diff] [review]:
-----------------------------------------------------------------

Can you rebase your patch and submit a new one with this Constant/NewObject issue fixed? Thanks!

::: dom/vr/VRDevice.h
@@ +122,4 @@
>    nsRefPtr<DOMPoint> mAngularAcceleration;
>  };
>  
> +class VREyeParameters MOZ_FINAL : public nsWrapperCache

MOZ_FINAL is gone. Use 'final'. Update your tree.

@@ +124,5 @@
>  
> +class VREyeParameters MOZ_FINAL : public nsWrapperCache
> +{
> +public:
> +  explicit VREyeParameters(nsISupports* aParent,

explicit is not needed when we have more than 1 param.

@@ +137,5 @@
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(VREyeParameters)
> +
> +  VRFieldOfView* MinimumFieldOfView();
> +  VRFieldOfView* MaximumFieldOfView();
> +  VRFieldOfView* RecommendedFieldOfView();

These should not return new objects all the time.

@@ +144,5 @@
> +  VRFieldOfView* CurrentFieldOfView();
> +  virtual already_AddRefed<DOMRect> RenderRect();
> +
> +  nsISupports* GetParentObject() const { return mParent; }
> +  virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;

Yep, your tree has to be updated. This is changed and we don't have MOZ_OVERRIDE anymore.

::: dom/webidl/VRDevice.webidl
@@ +55,5 @@
> +[Pref="dom.vr.enabled",
> + HeaderFile="mozilla/dom/VRDevice.h"]
> +interface VREyeParameters {
> +  /* These values are expected to be static per-device/per-user */
> +  [Constant, NewObject] readonly attribute VRFieldOfView minimumFieldOfView;

This is wrong. An attribute cannot be a new Object and be constant.
Binding codegen should throw an error, actually.

Why do you want to have a new object all the time?
It means that if I do:

params.miniumFieldOfView === params.minimumFieldOfView, this is false.

@@ +117,4 @@
>     * will be null.
>     */
>    [NewObject]
> +  VRPositionState getState();

same line here as the other methods.

@@ +122,5 @@
> +  /*
> +   * Return the current instantaneous sensor state.
> +   */
> +  [NewObject]
> +  VRPositionState getImmediateState();

here too

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Update WebV APIs, v2

Whoops, rebased and updated.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8585041 [details] [diff] [review]
Update WebV APIs, v2

Review of attachment 8585041 [details] [diff] [review]:
-----------------------------------------------------------------

No mochitests for the new VRDevice methods/attributes?
Can you add some in a separate patch?

::: dom/vr/VRDevice.cpp
@@ +57,5 @@
>  {
> +  return VRFieldOfViewBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(VREyeParameters, mParent)

mParent, mMinFOV, mMaxFOV, mRecFOV, mCurFOV, mEyeTranslation, mRenderRect

@@ +80,5 @@
> +  mRenderRect = new DOMRect(aParent, aRenderRect.x, aRenderRect.y, aRenderRect.width, aRenderRect.height);
> +}
> +
> +already_AddRefed<VRFieldOfView>
> +VREyeParameters::MinimumFieldOfView()

VRFieldOfView*
VREyeParams::MinimumFieldOfView() const
{
  return mMinFOV;
}

same for the other getters.

@@ +253,4 @@
>    {
> +    gfx::IntSize sz(mHMD->SuggestedEyeResolution());
> +    gfx::VRHMDInfo::Eye eye = aEye == VREye::Left ? gfx::VRHMDInfo::Eye_Left : gfx::VRHMDInfo::Eye_Right;
> +    return new VREyeParameters(mParent,

nsRefPtrVREyeParameters> params = new ...
return params.forget();

::: dom/vr/VRDevice.h
@@ +65,4 @@
>  class VRFieldOfView final : public VRFieldOfViewReadOnly
>  {
>  public:
> +  explicit VRFieldOfView(nsISupports* aParent, const gfx::VRFieldOfView& aSrc)

explicit is not needed here.

@@ +70,5 @@
> +                            aSrc.upDegrees, aSrc.rightDegrees,
> +                            aSrc.downDegrees, aSrc.leftDegrees)
> +  {}
> +
> +  explicit VRFieldOfView(nsISupports* aParent,

same here.

@@ +150,5 @@
> +
> +  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(VREyeParameters)
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(VREyeParameters)
> +
> +  already_AddRefed<VRFieldOfView> MinimumFieldOfView();

VRFieldOfView* MinimumFieldOfView() const;

the same for the other getters.

@@ +231,4 @@
>  class HMDVRDevice : public VRDevice
>  {
>  public:
> +  virtual VREyeParameters* GetEyeParameters(VREye aEye) = 0;

virtual already_AddRefed<DOMPoint> GetEyeTranslation(VREye aEye) = 0;

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Update WebVR APIs, v3

Fixed up; grabbing another r+ just to make sure I've caught everything.  Still have very little experience with the new bindings.

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8585494 [details] [diff] [review]
Update WebVR APIs, v3

Review of attachment 8585494 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for these new comments but yep, can you provide a new version? Thanks!

::: dom/vr/VRDevice.cpp
@@ +57,5 @@
> +{
> +  return VRFieldOfViewBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(VREyeParameters, mParent, mMinFOV, mMaxFOV, mRecFOV, mCurFOV, mEyeTranslation, mRenderRect)

80chars max.

@@ +73,5 @@
> +{
> +  mMinFOV = new VRFieldOfView(aParent, aMinFOV);
> +  mMaxFOV = new VRFieldOfView(aParent, aMaxFOV);
> +  mRecFOV = new VRFieldOfView(aParent, aRecFOV);
> +  mCurFOV = new VRFieldOfView(aParent, aCurFOV);

What about if you create these objects only when needed? Something like:

	
VRFieldOfView*
VREyeParameters::MinimumFieldOfView()
{
  if (!mMinFOV) {
    mMinFOV = new VRFieldOfView(this, mMinFOVValue);
  }

  return mMinFOV;
}

@@ +75,5 @@
> +  mMaxFOV = new VRFieldOfView(aParent, aMaxFOV);
> +  mRecFOV = new VRFieldOfView(aParent, aRecFOV);
> +  mCurFOV = new VRFieldOfView(aParent, aCurFOV);
> +
> +  mEyeTranslation = new DOMPoint(aParent, aEyeTranslation.x, aEyeTranslation.y, aEyeTranslation.z, 0.0);

max 80 chars.

@@ +248,5 @@
> +    gfx::IntSize sz(mHMD->SuggestedEyeResolution());
> +    gfx::VRHMDInfo::Eye eye = aEye == VREye::Left ? gfx::VRHMDInfo::Eye_Left : gfx::VRHMDInfo::Eye_Right;
> +    nsRefPtr<VREyeParameters> params =
> +      new VREyeParameters(mParent,
> +                          gfx::VRFieldOfView(15, 15, 15, 15), // XXX min?

Do we have a follow up for this XXX ?
File it and write the bug number here.

::: dom/vr/VRDevice.h
@@ +74,5 @@
> +  VRFieldOfView(nsISupports* aParent,
> +                double aUpDegrees = 0.0, double aRightDegrees = 0.0,
> +                double aDownDegrees = 0.0, double aLeftDegrees = 0.0)
> +    : VRFieldOfViewReadOnly(aParent,
> +                            aUpDegrees, aRightDegrees, aDownDegrees, aLeftDegrees)

Do a better indentation here:

  : VRFieldOfViewReadOnly(aParent, aUpDegrees, aRightDegrees,
                          aDownDegrees, aLeftDegrees)

::: dom/webidl/VRDevice.webidl
@@ +55,5 @@
> +[Pref="dom.vr.enabled",
> + HeaderFile="mozilla/dom/VRDevice.h"]
> +interface VREyeParameters {
> +  /* These values are expected to be static per-device/per-user */
> +  [Constant, Cached] readonly attribute VRFieldOfView minimumFieldOfView;

just do [Constant] in anyone of those.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Update WebVR APIs, v4

Updated patch.

> What about if you create these objects only when needed? Something like:

GetEyeParameters will be called once or twice during content startup, and then will likely not be called again; most of the fields will get used during that call.  I didn't want to create them lazily since that would require adding even more members to the class... its size is not an issue, but I figured it wasn't a big deal

> XXX

No bug number for it; it's not important to fix, and is mainly a mild question to see if we even care about providing a minimum value.  I'll just implement it and push the minimum decision down to the gfx layer in a later patch.

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8585827 [details] [diff] [review]
Update WebVR APIs, v4

Review of attachment 8585827 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/345dda450c52