Closed
Bug 1284357
Opened 8 years ago
Closed 8 years ago
[webvr] Implement Navigator.activeVRDisplays
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: kip, Assigned: kip)
References
()
Details
Attachments
(4 files, 4 obsolete files)
970 bytes,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
17.01 KB,
patch
|
Details | Diff | Splinter Review | |
58 bytes,
text/x-review-board-request
|
Details | |
58 bytes,
text/x-review-board-request
|
gw280
:
review+
|
Details |
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•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years 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 | ||
Comment 5•8 years 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•8 years ago
|
||
- Rebased changes
- Navigator.activeVRDisplays is no longer marked as cached in the webidl
Attachment #8767787 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years 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•8 years ago
|
||
The WebVR spec has been updated to use a FrozenArray.
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8773519 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
FrozenArray is not yet implemented (coming in Bug 1236777), so using a workaround with the [Frozen] keyword.
Assignee | ||
Comment 11•8 years ago
|
||
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•8 years 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)
Comment 13•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Comment 16•8 years ago
|
||
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•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•8 years 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•8 years 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•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bc0196fd88e5
https://hg.mozilla.org/mozilla-central/rev/a2cfcd0a8f9b
Status: NEW → RESOLVED
Closed: 8 years 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.
Description
•