Closed Bug 1293333 Opened 8 years ago Closed 7 years ago

[webvr] Emit `vrdisplayactivate` and `vrdisplaydeactivate` events on `window`

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox54 --- fixed

People

(Reporter: kip, Assigned: kip)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [webvr])

Attachments

(4 files)

Bug 1293333 - Part 1: Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events
58 bytes, text/x-review-board-request
daoshengmu
: review+
ehsan.akhgari
: review+
Details
Bug 1293333 - Part 2: Add VRDisplayEvent to test_interfaces.html
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1293333 - Part 3: Add VRDisplayEvent to test_all_synthetic_events.html
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1293333 - Part 4: Update web-platform-tests Expectation Data
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details 
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
Keywords: feature
Blocks: webvr_1.1
Blocks: 1254776
Blocks: 1313239
I have created a separate bug to land the functionality for link traversal with onvrdisplayactivate:

Bug 1254776
Summary: [webvr] Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events → [webvr] Emit `vrdisplayactivate` and `vrdisplaydeactivate` events on `window`
Whiteboard: [webvr]
Attachment #8814509 - Flags: review?(ehsan)
Attachment #8814519 - Flags: review?(ehsan)
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?
Flags: needinfo?(kgilbert)
Comment on attachment 8814509 [details]
Bug 1293333 - Part 2: Add VRDisplayEvent to test_interfaces.html

https://reviewboard.mozilla.org/r/95710/#review96014
Attachment #8814509 - Flags: review?(ehsan) → review+
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.
Attachment #8814519 - Flags: review?(ehsan) → review-
(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!
Flags: needinfo?(kgilbert)
(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?
Flags: needinfo?(ehsan)
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.
Attachment #8803123 - Flags: review?(dmu) → review+
(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!
Flags: needinfo?(ehsan)
Comment on attachment 8814519 [details]
Bug 1293333 - Part 3: Add VRDisplayEvent to test_all_synthetic_events.html

https://reviewboard.mozilla.org/r/95728/#review96834
Attachment #8814519 - Flags: review?(ehsan) → review+
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.
Attachment #8803123 - Flags: review?(ehsan) → review-
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.
Blocks: 1335606
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.
Attachment #8803123 - Flags: review?(ehsan) → review+
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.
Attachment #8832659 - Flags: review?(ehsan)
Comment on attachment 8832659 [details]
Bug 1293333 - Part 4: Update web-platform-tests Expectation Data

https://reviewboard.mozilla.org/r/108824/#review110336
Attachment #8832659 - Flags: review?(ehsan) → review+
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
Too late for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
status-firefox52: --- → affected
52 is about to go to release, wontfix.
status-firefox52: affected → 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!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: