The window.onvrdisplayactivate and window.onvrdisplaydeactivate events have been recently added to the WebVR spec.

The implementation of onvrdisplayactivate and onvrdisplaydeactivate events should also include VRDisplayEvent and handle the link traversal reason described in the spec: https://w3c.github.io/webvr/#interface-vrdisplayevent

I have created a separate bug to land the functionality for link traversal with onvrdisplayactivate: Bug 1254776

Comment on attachment 8803123 [details] Bug 1293333 - Part 1: Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events https://reviewboard.mozilla.org/r/87356/#review95908 LGTM, I just wanna know if it needs to do some checks for a nullptr at do_QueryInterface(). ::: dom/vr/VRDisplayEvent.cpp:75 (Diff revision 5) > +already_AddRefed<VRDisplayEvent> > +VRDisplayEvent::Constructor(const GlobalObject& aGlobal, const nsAString& aType, > + const VRDisplayEventInit& aEventInitDict, > + ErrorResult& aRv) > +{ > + nsCOMPtr<mozilla::dom::EventTarget> owner = do_QueryInterface(aGlobal.GetAsSupports()); Does it have chance the owner from do_QueryInterface() to be a nullptr here? Should we add some check?

Which parts of the first patch should I review?

Comment on attachment 8814509 [details] Bug 1293333 - Part 2: Add VRDisplayEvent to test_interfaces.html https://reviewboard.mozilla.org/r/95710/#review96014

Comment on attachment 8814519 [details] Bug 1293333 - Part 3: Add VRDisplayEvent to test_all_synthetic_events.html https://reviewboard.mozilla.org/r/95728/#review96016 ::: dom/events/test/test_all_synthetic_events.html:461 (Diff revision 1) > }, > }, > + VRDisplayEvent: { > + // Required argument expects a VRDisplay that can not > + // be created from Javascript. Need physical VR hardware > + // attached to get a VRDisplay. Shouldn't you be setting create to null here according to this comment? Also, per spec at <https://w3c.github.io/webvr/#interface-vrdisplayevent>, VRDisplayEvent does have a constructor and it can be created by JS code just fine, so this comment is completely wrong here. If your intention is indeed to make this event not creatable from JS, you need to first fix the spec and drop the [Constrcutor] attribute from the interface, remove the VRDisplayEventInit dictionary which would be pointless if we don't have a ctor for this interface, and fix the part 1 patch accordingly. r- until the spec, code and comments all agree on the desired behavior.

(In reply to :Ehsan Akhgari from comment #11) > Which parts of the first patch should I review? In the first part, could you please review the webidl changes? Very appreciated, thanks!

(In reply to :Ehsan Akhgari from comment #13) > Comment on attachment 8814519 [details] > Bug 1293333 - Part 3: Add VRDisplayEvent to test_all_synthetic_events.html > > https://reviewboard.mozilla.org/r/95728/#review96016 > > ::: dom/events/test/test_all_synthetic_events.html:461 > (Diff revision 1) > > }, > > }, > > + VRDisplayEvent: { > > + // Required argument expects a VRDisplay that can not > > + // be created from Javascript. Need physical VR hardware > > + // attached to get a VRDisplay. > > Shouldn't you be setting create to null here according to this comment? Thanks, I will correct this in an updated patch. > > Also, per spec at <https://w3c.github.io/webvr/#interface-vrdisplayevent>, > VRDisplayEvent does have a constructor and it can be created by JS code just > fine, so this comment is completely wrong here. While there is a constructor for VRDisplayEvent, there is no constructor for VRDisplay which is a required member in VRDisplayEventInit. Only when real VR hardware is physically attached can a VRDisplay be returned by Navigator.getVRDisplays(). > > If your intention is indeed to make this event not creatable from JS, you > need to first fix the spec and drop the [Constrcutor] attribute from the > interface, remove the VRDisplayEventInit dictionary which would be pointless > if we don't have a ctor for this interface, and fix the part 1 patch > accordingly. > > r- until the spec, code and comments all agree on the desired behavior. The event is intended to be creatable from JS, but this is only possible to test with VR hardware connected. It should be possible to test this once Bug 1229480 is complete, enabling simulation of VR hardware on the test machines. Would it be acceptable to make the change above (set create to null) and update the comment to reference Bug 1229480?

Comment on attachment 8803123 [details] Bug 1293333 - Part 1: Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events https://reviewboard.mozilla.org/r/87356/#review95908 > Does it have chance the owner from do_QueryInterface() to be a nullptr here? Should we add some check? If owner is null, this will be handled in Event::SetOwner (Event.cpp). I am following the same pattern as other events that implement Constructor().

Comment on attachment 8803123 [details] Bug 1293333 - Part 1: Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events https://reviewboard.mozilla.org/r/87356/#review96692 Looks good! Thanks.

(In reply to :kip (Kearwood Gilbert) from comment #15) > (In reply to :Ehsan Akhgari from comment #13) > > Comment on attachment 8814519 [details] > > Bug 1293333 - Part 3: Add VRDisplayEvent to test_all_synthetic_events.html > > > > https://reviewboard.mozilla.org/r/95728/#review96016 > > > > ::: dom/events/test/test_all_synthetic_events.html:461 > > (Diff revision 1) > > > }, > > > }, > > > + VRDisplayEvent: { > > > + // Required argument expects a VRDisplay that can not > > > + // be created from Javascript. Need physical VR hardware > > > + // attached to get a VRDisplay. > > > > Shouldn't you be setting create to null here according to this comment? > Thanks, I will correct this in an updated patch. > > > > Also, per spec at <https://w3c.github.io/webvr/#interface-vrdisplayevent>, > > VRDisplayEvent does have a constructor and it can be created by JS code just > > fine, so this comment is completely wrong here. > While there is a constructor for VRDisplayEvent, there is no constructor for > VRDisplay which is a required member in VRDisplayEventInit. Only when real > VR hardware is physically attached can a VRDisplay be returned by > Navigator.getVRDisplays(). I see now, I was misreading the comment I think. All's good here. > > If your intention is indeed to make this event not creatable from JS, you > > need to first fix the spec and drop the [Constrcutor] attribute from the > > interface, remove the VRDisplayEventInit dictionary which would be pointless > > if we don't have a ctor for this interface, and fix the part 1 patch > > accordingly. > > > > r- until the spec, code and comments all agree on the desired behavior. > The event is intended to be creatable from JS, but this is only possible to > test with VR hardware connected. It should be possible to test this once > Bug 1229480 is complete, enabling simulation of VR hardware on the test > machines. > > Would it be acceptable to make the change above (set create to null) and > update the comment to reference Bug 1229480? Yes, of course!

Comment on attachment 8814519 [details] Bug 1293333 - Part 3: Add VRDisplayEvent to test_all_synthetic_events.html https://reviewboard.mozilla.org/r/95728/#review96834

Comment on attachment 8803123 [details] Bug 1293333 - Part 1: Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events https://reviewboard.mozilla.org/r/87356/#review96836 Minusing since the IDL doesn't match what's in the spec. ::: dom/webidl/VRDisplayEvent.webidl:8 (Diff revision 7) > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + enum VRDisplayEventReason { > + "navigation", > + "mounted", Nit: I would fix the ordering of these first two fields to make them match the spec. ::: dom/webidl/VRDisplayEvent.webidl:15 (Diff revision 7) > + "unmounted", > +}; > + > +dictionary VRDisplayEventInit : EventInit { > + required VRDisplay display; > + required VRDisplayEventReason reason; This field isn't marked as required in the spec. ::: dom/webidl/VRDisplayEvent.webidl:19 (Diff revision 7) > + required VRDisplay display; > + required VRDisplayEventReason reason; > +}; > + > +[Pref="dom.vr.enabled", > + HeaderFile="mozilla/dom/VRDisplayEvent.h", The [HeaderFile] annotation seems needless. Do you get a compiler error when you remove it? If yes, please explain why. I doubt this is the right way to fix it. If not, please remove this line. ::: dom/webidl/VRDisplayEvent.webidl:23 (Diff revision 7) > +[Pref="dom.vr.enabled", > + HeaderFile="mozilla/dom/VRDisplayEvent.h", > + Constructor(DOMString type, VRDisplayEventInit eventInitDict)] > +interface VRDisplayEvent : Event { > + readonly attribute VRDisplay display; > + readonly attribute VRDisplayEventReason reason; This should be nullable per spec.

I've rebased the patches and corrected the issues in Comment 27. I'll do a try run for sanity sake as it has been some time since these patches were last checked. Did local testing on an HTC Vive with success.

Comment on attachment 8803123 [details] Bug 1293333 - Part 1: Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events https://reviewboard.mozilla.org/r/87356/#review110026 r=me on the WebIDL bits. ::: dom/webidl/VRDisplayEvent.webidl:6 (Diff revision 8) > +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + enum VRDisplayEventReason { Nit: unneeded initial whitespace.

Comment on attachment 8832659 [details] Bug 1293333 - Part 4: Update web-platform-tests Expectation Data The try push revealed that our recently added web-platform-tests include WebVR API conformance checks. As these patches update to a more complete implementation of WebVR, it is necessary to update the web-platform-tests expectation data to match.

Comment on attachment 8832659 [details] Bug 1293333 - Part 4: Update web-platform-tests Expectation Data https://reviewboard.mozilla.org/r/108824/#review110336

Pushed by kgilbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c57f055716dd Part 1: Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events r=daoshengmu,Ehsan https://hg.mozilla.org/integration/autoland/rev/c4b622322122 Part 2: Add VRDisplayEvent to test_interfaces.html r=Ehsan https://hg.mozilla.org/integration/autoland/rev/5b7d202e8797 Part 3: Add VRDisplayEvent to test_all_synthetic_events.html r=Ehsan https://hg.mozilla.org/integration/autoland/rev/c3e3c6983068 Part 4: Update web-platform-tests Expectation Data r=Ehsan

https://hg.mozilla.org/mozilla-central/rev/c57f055716dd https://hg.mozilla.org/mozilla-central/rev/c4b622322122 https://hg.mozilla.org/mozilla-central/rev/5b7d202e8797 https://hg.mozilla.org/mozilla-central/rev/c3e3c6983068

Too late for 51. Mark 51 won't fix.

52 is about to go to release, wontfix.

I've made sure the compat information is updated on the relevant ref pages: https://developer.mozilla.org/en-US/docs/Web/API/Window/onvrdisplaydeactivate https://developer.mozilla.org/en-US/docs/Web/API/Window/onvrdisplayactivate I've put them as supported in Fx55 onwards, as that's when we turned the API on by default, right? I've not updated the Fx55 rel notes, as we already list WebVR 1.1 as a new API in there (https://developer.mozilla.org/en-US/Firefox/Releases/55#New_APIs). Let me know if that sits OK with you. Thanks!