[webvr] Implement Navigator.activeVRDisplays

RESOLVED FIXED in Firefox 51

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: kip, Assigned: kip)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
WebVR 1.0 includes a new property added to Navigator, activeVRDisplays

Bug 1250244 includes all of WebVR 1.0 except for activeVRDisplays.

Please apply the patchset in Bug 1250244 before this bug.
(Assignee)

Comment 1

2 years ago
Created attachment 8767787 [details] [diff] [review]
bug1284357.patch
(Assignee)

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bf8af718c9c
(Assignee)

Comment 3

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1ff06d8edd7
(Assignee)

Comment 4

a year ago
Our webidl parser does not appear to support non-cached arrays of objects returned by a property.  Once this is in place, the attached patch will enable the activeVRDisplays property
(Assignee)

Updated

a year ago
Depends on: 1288556
(Assignee)

Comment 5

a year ago
Bug 1288556 must land first to avoid a WebIDL parser error:

WebIDL.WebIDLError: error: A non-cached attribute cannot be of a sequence type, /Users/kearwood/build/firefox/obj-x86_64-apple-darwin15.6.0-debug/dom/bindings/Navigator.webidl line 293:11
(Assignee)

Comment 6

a year ago
Created attachment 8773519 [details] [diff] [review]
Bug 1284357 - [webvr] Implement Navigator.activeVRDisplays

- Rebased changes
- Navigator.activeVRDisplays is no longer marked as cached in the webidl
Attachment #8767787 - Attachment is obsolete: true
(Assignee)

Comment 7

a year ago
The WebVR spec has changed activeVRDisplays to return a FrozenArray rather than a Sequence.  I'll update the patch to match.
(Assignee)

Comment 8

a year ago
The WebVR spec has been updated to use a FrozenArray.
(Assignee)

Comment 9

a year ago
Created attachment 8774930 [details] [diff] [review]
Bug 1284357 - [webvr] Implement Navigator.activeVRDisplays
Attachment #8773519 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
See Also: → bug 1236777
(Assignee)

Comment 10

a year ago
FrozenArray is not yet implemented (coming in Bug 1236777), so using a workaround with the [Frozen] keyword.
(Assignee)

Comment 11

a year ago
Created attachment 8779880 [details] [diff] [review]
Bug 1284357 - [webvr] Implement Navigator.activeVRDisplays

I have rebased the patch on top of the current m-c and patchset from Bug 1250244.
Attachment #8774930 - Attachment is obsolete: true
(Assignee)

Comment 12

a year ago
This patch is now working as expected.

Once Bug 1250244 lands, I will post the attached patch to MozReview and assign out to a reviewer.

bz: This is touching similar code to that you reviewed in Bug 1250244.  Would you have some cycles to take a look at it?  If you're busy, perhaps you could review just the webidl change?
Flags: needinfo?(bzbarsky)
I can review the IDL changes, sure.  For the rest of it, it might be better to ask someone more familiar with the existing VR code to review the rest of it.

Either way, I'm on vacation all of next week, so whatever I review will have to be before then, or after I get back.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 14

a year ago
Created attachment 8780267 [details] [diff] [review]
Bug 1284357 - Part 1: Add navigator.activeVRDisplays attribute to webidl

I have split the patch so the webidl change is separate
Attachment #8779880 - Attachment is obsolete: true
Attachment #8780267 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

a year ago
Created attachment 8780268 [details] [diff] [review]
Bug 1284357 - Part 2: Implement Navigator.activeVRDisplays
Comment on attachment 8780267 [details] [diff] [review]
Bug 1284357 - Part 1: Add navigator.activeVRDisplays attribute to webidl

In the !mWindow || !mWindow->GetDocShell() case, do we want to return an empty array or throw?  Right now you do the former...

>+  aDisplays.Clear();

This line is not needed; the bindings will pass in an empty array for you to fill.

>+  nsTArray<RefPtr<mozilla::dom::VRDisplay>> displays;

All this code is inside the mozilla::dom namespace, so no need to have that namespace on there, right?

>+  void NotifyActiveVRDisplaysChanged();

Please put out internal Notify* methods not in the middle of the Web IDL method list in the header...

>+nsGlobalWindow::NotifyActiveVRDisplaysChanged()

Will anyone ever call this on an outer window?  If not, why the forwarding bit?  In general, we are trying to remove all the forwarding cruft and shouldn't be adding new things that forward.

If this is only called on inner windows, assert accordingly and document it in the header.

I didn't look at the gfx/ bits.  r=me on the IDL and DOM bits with the above issues fixed.
Attachment #8780267 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64c7b731a874
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

a year ago
I have applied the changes recommended by bz in Comment 16 and updated the patches.

Since Bug 1250244 has landed, I can use MozReview again :-)

Comment 21

a year ago
mozreview-review
Comment on attachment 8782620 [details]
Bug 1284357 - Part 2: Implement Navigator.activeVRDisplays

https://reviewboard.mozilla.org/r/72724/#review71286

gfx stuff looks fine. haven't reviewed the DOM stuff.
Attachment #8782620 - Flags: review?(gwright) → review+
(Assignee)

Comment 22

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0196fd88e544ad61579688f1420a7d6e8b6545
Bug 1284357 - Part 1: Add navigator.activeVRDisplays attribute to webidl,r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2cfcd0a8f9bb765cd3a8c2047d1cdbc5abfaa83
Bug 1284357 - Part 2: Implement Navigator.activeVRDisplays,r=gw280

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc0196fd88e5
https://hg.mozilla.org/mozilla-central/rev/a2cfcd0a8f9b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.