1219124 - Enable HMDVRDevice API for Android in test_interface.html

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daosheng Mu[:daoshengmu]]

According to the comments of Bug 1215147, we would like to enable VR API on Firefox for Android. We have to modify HMDVRDevice attribute in test_interface.html to help the patch of Bug 1215147 to be mochitest successfully.

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: Enable HMDVRDevice API for Android

Enable VR API for Firefox Android in test_interface

Comment 2

[Daosheng Mu[:daoshengmu]]

Kip. I know you are working on Bug 1218482. In my Bug 1215147, it would happen mochitest fail after I enable VR API at Android/config, I have to modify it in test_interface.html. Do you need me help you enable other platform as well?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8679816 [details] [diff] [review]
Enable HMDVRDevice API for Android

A few things:

1)  Obviously, this needs to land in the same changeset (not just same push) as the existing patch in bug 1215147.

2)  Is there a reason you didn't do:

  {name: "HMDVRDevice", android: true}

if you're really just enabling on Android, instead of trying to list all the places you're _not_ enabling?

3)  Bug 1215147 talks about enabling on nightly/devedition, right?  This patch is claiming it's enabled in release.  Which is it?

Comment 4

[:kip (Kearwood Gilbert)]

(In reply to Daosheng Mu[:daoshengmu] from comment #2)
> Kip. I know you are working on Bug 1218482. In my Bug 1215147, it would
> happen mochitest fail after I enable VR API at Android/config, I have to
> modify it in test_interface.html. Do you need me help you enable other
> platform as well?
Thanks, Daosheng.  For Bug 1218482, we only have want to land in nightly, with a plan to let it ride the trains to 45.  To keep things simple, I will be delaying the commit that enables it until after the 44 merge date.  It appears that the line changed in your patch will need to include all platforms before landing my bug, 1218482, with a Dom peer approval.  I can add this to my patch, and include bz as a reviewer, as he is responding to this as well.

Comment 5

[:kip (Kearwood Gilbert)]

If we could enable the VR API for all platforms here instead of just Android, then it could save an extra Review by bz.  These objects won't be able to be constructed without the Dom.vr.enabled flag set separately.

I would leave this decision to BZ if he'd like to combine his two reviews here.  If he prefers, I can simply change this to all platforms in my patch for Bug 1218482 separately.
(In reply to :kip (Kearwood Gilbert) from comment #4)
> (In reply to Daosheng Mu[:daoshengmu] from comment #2)
> > Kip. I know you are working on Bug 1218482. In my Bug 1215147, it would
> > happen mochitest fail after I enable VR API at Android/config, I have to
> > modify it in test_interface.html. Do you need me help you enable other
> > platform as well?
> Thanks, Daosheng.  For Bug 1218482, we only have want to land in nightly,
> with a plan to let it ride the trains to 45.  To keep things simple, I will
> be delaying the commit that enables it until after the 44 merge date.  It
> appears that the line changed in your patch will need to include all
> platforms before landing my bug, 1218482, with a Dom peer approval.  I can
> add this to my patch, and include bz as a reviewer, as he is responding to
> this as well.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The point of this test is express the policy we have.  Then the test checks whether our actual shipping code matches the policy.

That means that the test needs to change whenever the policy changes.

So if our policy is that VR should be enabled on all platforms, then the test should test that.  If our policy is that it should be enabled on Android only, it should test _that_.  If the policy is that it's enabled on all platforms but only in nightly, the test should test _that_.  Does that help?

Comment 7

[:kip (Kearwood Gilbert)]

Thanks Boris,

I have added the change to test_interface.html to enable the API for all platforms to Bug 1218482 so that it takes effect only when we intend to enable for all platforms.

(In reply to Boris Zbarsky [:bz] from comment #6)
> The point of this test is express the policy we have.  Then the test checks
> whether our actual shipping code matches the policy.
> 
> That means that the test needs to change whenever the policy changes.
> 
> So if our policy is that VR should be enabled on all platforms, then the
> test should test that.  If our policy is that it should be enabled on
> Android only, it should test _that_.  If the policy is that it's enabled on
> all platforms but only in nightly, the test should test _that_.  Does that
> help?

Comment 8

[Daosheng Mu[:daoshengmu]]

Thanks Kip and Boris,
It should support all-platform not just Firefox Android. Obviously, Bug 1218482 would solve this bug. Boris, please just review Bug 1218482.

Comment 9

[Daosheng Mu[:daoshengmu]]

It has solved by Bug 1215147.