:kip (Kearwood Gilbert) Assignee
	Description • 8 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.

	:kip (Kearwood Gilbert) Assignee
	Comment 2 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bf8af718c9c

	:kip (Kearwood Gilbert) Assignee
	Comment 3 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1ff06d8edd7

	:kip (Kearwood Gilbert) 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

	:kip (Kearwood Gilbert) 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

	:kip (Kearwood Gilbert) Assignee
	Comment 6 • 8 years ago

- Rebased changes
- Navigator.activeVRDisplays is no longer marked as cached in the webidl

	:kip (Kearwood Gilbert) 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.

	:kip (Kearwood Gilbert) Assignee
	Comment 8 • 8 years ago

The WebVR spec has been updated to use a FrozenArray.

	:kip (Kearwood Gilbert) Assignee
	Comment 10 • 8 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 11 • 8 years ago

I have rebased the patch on top of the current m-c and patchset from Bug 1250244.

	:kip (Kearwood Gilbert) 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?

	Boris Zbarsky [: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.

	:kip (Kearwood Gilbert) Assignee
	Comment 14 • 8 years ago

I have split the patch so the webidl change is separate

	Boris Zbarsky [:bzbarsky]
	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.

	:kip (Kearwood Gilbert) Assignee
	Comment 17 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64c7b731a874

	:kip (Kearwood Gilbert) 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 :-)

	George Wright (:gw280) (needinfo me!)
	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.

	:kip (Kearwood Gilbert) 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

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bc0196fd88e5
https://hg.mozilla.org/mozilla-central/rev/a2cfcd0a8f9b