[meta] [webvr] WebVR 1.0 API

RESOLVED FIXED in Firefox 51

Status

()

Core
Graphics
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: kip, Assigned: kip)

Tracking

(Depends on: 2 bugs, Blocks: 6 bugs, {dev-doc-needed})

unspecified
mozilla51
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [webvr])

MozReview Requests

()

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

Attachments

(9 attachments, 4 obsolete attachments)

58 bytes, text/x-review-board-request
bas
: review+
Details | Review
58 bytes, text/x-review-board-request
gw280
: review+
Details | Review
58 bytes, text/x-review-board-request
gw280
: review+
Details | Review
58 bytes, text/x-review-board-request
gw280
: review+
Details | Review
58 bytes, text/x-review-board-request
gw280
: review+
Details | Review
58 bytes, text/x-review-board-request
masayuki
: review+
Details | Review
58 bytes, text/x-review-board-request
gw280
: review+
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
(Assignee)

Description

a year ago
In collaboration with other browser developers, we have proposed a "WebVR 1.0" API.  A URL pointing to the specification and WebIDL will be added once it is posted publicly.

The existing WebVR API should be considered deprecated -- WebVR 1.0 will not be backwards compatible with the existing experimental API.
(Assignee)

Updated

a year ago
Summary: [webvr] Implement WebVR 1.0 API → [meta] [webvr] WebVR 1.0 API
(Assignee)

Comment 1

a year ago
Created attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review commit: https://reviewboard.mozilla.org/r/36163/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36163/
(Assignee)

Comment 2

a year ago
I have uploaded a WIP patch.  I will update this as I iteratively replace the old WebVR API with 1.0.  Please don't commit until I remove "WIP" from the commit message.
(Assignee)

Comment 3

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/1-2/
(Assignee)

Comment 4

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/2-3/
Keywords: dev-doc-needed
(Assignee)

Updated

a year ago
Depends on: 1196672
(Assignee)

Updated

a year ago
Depends on: 1254776
(Assignee)

Updated

a year ago
Depends on: 1256444
(Assignee)

Comment 5

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/3-4/
(Assignee)

Comment 6

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/4-5/
(Assignee)

Updated

a year ago
Blocks: 1258504
(Assignee)

Comment 7

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/5-6/
(Assignee)

Updated

a year ago
Blocks: 1186578

Updated

a year ago
Whiteboard: [webvr]
(Assignee)

Comment 8

a year ago
Created attachment 8736520 [details]
MozReview Request: Initial OpenVR/SteamVR support

Review commit: https://reviewboard.mozilla.org/r/43339/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43339/
(Assignee)

Comment 9

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/6-7/
(Assignee)

Comment 10

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/7-8/
(Assignee)

Updated

a year ago
Attachment #8736520 - Attachment is obsolete: true
(Assignee)

Comment 11

a year ago
Created attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review commit: https://reviewboard.mozilla.org/r/46855/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46855/
Attachment #8722648 - Attachment description: MozReview Request: Bug 1250244 - (WIP, Don't commit) Implement WebVR 1.0 API → MozReview Request: Bug 1250244 - Part 4: (WIP, Don't commit) Implement WebVR 1.0 API
(Assignee)

Comment 12

a year ago
Created attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review commit: https://reviewboard.mozilla.org/r/46857/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46857/
(Assignee)

Comment 13

a year ago
Created attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review commit: https://reviewboard.mozilla.org/r/46859/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46859/
(Assignee)

Comment 14

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/8-9/
(Assignee)

Comment 15

a year ago
Created attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review commit: https://reviewboard.mozilla.org/r/46869/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46869/
Attachment #8722648 - Attachment description: MozReview Request: Bug 1250244 - Part 4: (WIP, Don't commit) Implement WebVR 1.0 API → MozReview Request: Bug 1250244 - Part 6: (WIP, Don't commit) Implement WebVR 1.0 API
(Assignee)

Comment 16

a year ago
Created attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review commit: https://reviewboard.mozilla.org/r/46871/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46871/
(Assignee)

Comment 17

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/9-10/
(Assignee)

Comment 18

a year ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/1-2/
(Assignee)

Comment 19

a year ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/1-2/
(Assignee)

Comment 20

a year ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/1-2/
(Assignee)

Comment 21

a year ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/1-2/
(Assignee)

Comment 22

a year ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/1-2/
(Assignee)

Comment 23

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/10-11/
(Assignee)

Comment 24

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/11-12/
(Assignee)

Comment 25

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/12-13/
Duplicate of this bug: 1269057
(Assignee)

Comment 27

a year ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/2-3/
Attachment #8741942 - Attachment description: MozReview Request: Bug 1250244 - Part 3: (WIP, Don't commit) Add Union with “components” member to math classes to enable array access to members. → MozReview Request: Bug 1250244 - Part 3: (WIP, Don't commit) Add Union with "components" member to math classes to enable array access to members.
(Assignee)

Comment 28

a year ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/2-3/
(Assignee)

Comment 29

a year ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/2-3/
(Assignee)

Comment 30

a year ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/2-3/
(Assignee)

Comment 31

a year ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/2-3/
(Assignee)

Comment 32

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/13-14/
(Assignee)

Comment 33

a year ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/3-4/
(Assignee)

Comment 34

a year ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/3-4/
(Assignee)

Comment 35

a year ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/3-4/
(Assignee)

Comment 36

a year ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/3-4/
(Assignee)

Comment 37

a year ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/3-4/
(Assignee)

Comment 38

a year ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/14-15/
(Assignee)

Updated

11 months ago
Depends on: 1276066
(Assignee)

Comment 39

11 months ago
Created attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

- Oculus 0.5 runtime was the last to support OSX, but is no longer supported
  on the latest (El Capitan) OSX version and does not support current
  shipping Oculus hardware.
- Oculus 1.x runtime will continue to be supported for Oculus on Windows.

Review commit: https://reviewboard.mozilla.org/r/55870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55870/
Attachment #8741952 - Attachment description: MozReview Request: Bug 1250244 - Part 4: (WIP, Don't commit) Remove FullScreenOptions parameter from Element.RequestFullScreen → MozReview Request: Bug 1250244 - Part 1: (WIP, Don't commit) Remove FullScreenOptions parameter from Element.RequestFullScreen
Attachment #8741942 - Attachment description: MozReview Request: Bug 1250244 - Part 3: (WIP, Don't commit) Add Union with "components" member to math classes to enable array access to members. → MozReview Request: Bug 1250244 - Part 2: (WIP, Don't commit) Remove old VR rendering paths
Attachment #8741940 - Attachment description: MozReview Request: Bug 1250244 - Part 1: (WIP, Don't commit) Rename VRDevice to VRDisplay → MozReview Request: Bug 1250244 - Part 5: (WIP, Don't commit) Rename VRDevice to VRDisplay
Attachment #8741941 - Attachment description: MozReview Request: Bug 1250244 - Part 2: (WIP, Don't commit) Replace VRStateValidFlags with VRDisplayCapabilityFlags → MozReview Request: Bug 1250244 - Part 6: (WIP, Don't commit) Replace VRStateValidFlags with VRDisplayCapabilityFlags
Attachment #8741953 - Attachment description: MozReview Request: Bug 1250244 - Part 5: (WIP, Don't commit) Add GamePad.displayID → MozReview Request: Bug 1250244 - Part 7: (WIP, Don't commit) Add GamePad.displayID
Attachment #8722648 - Attachment description: MozReview Request: Bug 1250244 - Part 6: (WIP, Don't commit) Implement WebVR 1.0 API → MozReview Request: Bug 1250244 - Part 9: (WIP, Don't commit) Implement WebVR 1.0 API
(Assignee)

Comment 40

11 months ago
Created attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

- The Cardboard VR support has hardcoded values and uses low-performance
  orientation APIs and rendering paths.
- There is little benefit to this Cardboard VR implementation over using
  polyfills.
- A future implementation would be based on Google VR support in Android N
  and/or Samsung Gear VR Oculus Mobile APIs.

Review commit: https://reviewboard.mozilla.org/r/55872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55872/
(Assignee)

Comment 41

11 months ago
Created attachment 8757454 [details]
MozReview Request: Bug 1250244 - Part 8: Implement TextureForwarder

- TextureForwarder is a base class that fits between
  CompositableForwarder and ISurfaceAllocator, enabling
  the texture forwarding functionality to be shared
  by subclasses that do not wish to bring in the
  layerization and compositor dependencies in
  CompositableForwarder.

Review commit: https://reviewboard.mozilla.org/r/55874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55874/
(Assignee)

Comment 42

11 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/4-5/
(Assignee)

Comment 43

11 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/4-5/
(Assignee)

Comment 44

11 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/4-5/
(Assignee)

Comment 45

11 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/4-5/
(Assignee)

Comment 46

11 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/4-5/
(Assignee)

Comment 47

11 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/15-16/
(Assignee)

Updated

11 months ago
Depends on: 1276359
(Assignee)

Updated

11 months ago
Blocks: 1276711
(Assignee)

Updated

11 months ago
Blocks: 1269460
(Assignee)

Comment 48

11 months ago
I am re-basing the patches on top of Bug 1198518, which is currently in m-i.
Depends on: 1198518
(Assignee)

Updated

11 months ago
Depends on: 1276811
(Assignee)

Comment 49

11 months ago
I am splitting the patchset up into smaller pieces that are easier to review.  Some parts are not VR specific and have been broken out into separate bugs.

For the remainder of the patchset, which may not be as straightforward, I'll set up a Vidyo room to get some high level feedback before assigning out reviews on the particular pieces.

If you are trying these patches, please ensure that all of the patches from the dependencies are applied before this patchset.
(Assignee)

Updated

11 months ago
Blocks: 1277040
(Assignee)

Comment 50

11 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/5-6/
Attachment #8722648 - Attachment description: MozReview Request: Bug 1250244 - Part 9: (WIP, Don't commit) Implement WebVR 1.0 API → MozReview Request: Bug 1250244 - Part 8: (WIP, Don't commit) Implement WebVR 1.0 API
(Assignee)

Comment 51

11 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/5-6/
(Assignee)

Comment 52

11 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/1-2/
(Assignee)

Comment 53

11 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/1-2/
(Assignee)

Comment 54

11 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/5-6/
(Assignee)

Comment 55

11 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/5-6/
(Assignee)

Comment 56

11 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/5-6/
(Assignee)

Comment 57

11 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/16-17/
(Assignee)

Updated

11 months ago
Attachment #8757454 - Attachment is obsolete: true
(Assignee)

Comment 58

11 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/6-7/
(Assignee)

Comment 59

11 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/2-3/
(Assignee)

Comment 60

11 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/2-3/
(Assignee)

Comment 61

11 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/6-7/
(Assignee)

Comment 62

11 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/6-7/
(Assignee)

Comment 63

11 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/6-7/
(Assignee)

Comment 64

11 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/17-18/
(Assignee)

Comment 65

11 months ago
Bug 1276811 is dependent on Bug 1176011, which will be held back until after the Jun 6th uplift.  I suggest we plan on landing this WebVR 1.0 patchset shortly after as well to allow them to go through a full period in Nightly before being uplifted.
(Assignee)

Comment 66

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e8c6096eb55
(Assignee)

Comment 67

11 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/6-7/
Attachment #8741952 - Attachment description: MozReview Request: Bug 1250244 - Part 1: (WIP, Don't commit) Remove FullScreenOptions parameter from Element.RequestFullScreen → Bug 1250244 - Part 1: (WIP, Don't commit) Remove FullScreenOptions parameter from Element.RequestFullScreen
Attachment #8741942 - Attachment description: MozReview Request: Bug 1250244 - Part 2: (WIP, Don't commit) Remove old VR rendering paths → Bug 1250244 - Part 2: (WIP, Don't commit) Remove old VR rendering paths
Attachment #8757452 - Attachment description: MozReview Request: Bug 1250244 - Part 3: (WIP, Don't commit) Remove Oculus 0.5 Runtime support → Bug 1250244 - Part 3: (WIP, Don't commit) Remove Oculus 0.5 Runtime support
Attachment #8757453 - Attachment description: MozReview Request: Bug 1250244 - Part 4: (WIP, Don't commit) Remove Cardboard VR Support → Bug 1250244 - Part 4: (WIP, Don't commit) Remove Cardboard VR Support
Attachment #8741940 - Attachment description: MozReview Request: Bug 1250244 - Part 5: (WIP, Don't commit) Rename VRDevice to VRDisplay → Bug 1250244 - Part 5: (WIP, Don't commit) Rename VRDevice to VRDisplay
Attachment #8741941 - Attachment description: MozReview Request: Bug 1250244 - Part 6: (WIP, Don't commit) Replace VRStateValidFlags with VRDisplayCapabilityFlags → Bug 1250244 - Part 6: (WIP, Don't commit) Replace VRStateValidFlags with VRDisplayCapabilityFlags
Attachment #8741953 - Attachment description: MozReview Request: Bug 1250244 - Part 7: (WIP, Don't commit) Add GamePad.displayID → Bug 1250244 - Part 7: (WIP, Don't commit) Add GamePad.displayID
Attachment #8722648 - Attachment description: MozReview Request: Bug 1250244 - Part 8: (WIP, Don't commit) Implement WebVR 1.0 API → Bug 1250244 - Part 8: (WIP, Don't commit) Implement WebVR 1.0 API
(Assignee)

Comment 68

11 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/7-8/
(Assignee)

Comment 69

11 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/3-4/
(Assignee)

Comment 70

11 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/3-4/
(Assignee)

Comment 71

11 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/7-8/
(Assignee)

Comment 72

11 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/7-8/
(Assignee)

Comment 73

11 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/7-8/
(Assignee)

Comment 74

11 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/18-19/
(Assignee)

Comment 75

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef5a7d00934b
(Assignee)

Comment 76

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=34e09b70d186
(Assignee)

Comment 77

11 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/7-8/
Attachment #8741952 - Attachment description: Bug 1250244 - Part 1: (WIP, Don't commit) Remove FullScreenOptions parameter from Element.RequestFullScreen → Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen
Attachment #8741942 - Attachment description: Bug 1250244 - Part 2: (WIP, Don't commit) Remove old VR rendering paths → Bug 1250244 - Part 2: Remove old VR rendering paths
Attachment #8757452 - Attachment description: Bug 1250244 - Part 3: (WIP, Don't commit) Remove Oculus 0.5 Runtime support → Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support
Attachment #8757453 - Attachment description: Bug 1250244 - Part 4: (WIP, Don't commit) Remove Cardboard VR Support → Bug 1250244 - Part 4: Remove Cardboard VR Support
Attachment #8741940 - Attachment description: Bug 1250244 - Part 5: (WIP, Don't commit) Rename VRDevice to VRDisplay → Bug 1250244 - Part 5: Rename VRDevice to VRDisplay
Attachment #8741941 - Attachment description: Bug 1250244 - Part 6: (WIP, Don't commit) Replace VRStateValidFlags with VRDisplayCapabilityFlags → Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags
Attachment #8741953 - Attachment description: Bug 1250244 - Part 7: (WIP, Don't commit) Add GamePad.displayID → Bug 1250244 - Part 7: Add GamePad.displayID
(Assignee)

Comment 78

11 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/8-9/
(Assignee)

Comment 79

11 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/4-5/
(Assignee)

Comment 80

11 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/4-5/
(Assignee)

Comment 81

11 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/8-9/
(Assignee)

Comment 82

11 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/8-9/
(Assignee)

Comment 83

11 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/8-9/
(Assignee)

Comment 84

11 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/19-20/
(Assignee)

Updated

11 months ago
Attachment #8741952 - Flags: review?(gwright)
Attachment #8741942 - Flags: review?(gwright)
Attachment #8757452 - Flags: review?(gwright)
Attachment #8757453 - Flags: review?(gwright)
Attachment #8741940 - Flags: review?(gwright)
Attachment #8741941 - Flags: review?(gwright)
Attachment #8741953 - Flags: review?(gwright)
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

https://reviewboard.mozilla.org/r/46869/#review56036

This looks fine to me, but I'm not a DOM peer so my r+ counts for nothing :)
Attachment #8741952 - Flags: review?(gwright) → review+
(Assignee)

Comment 86

11 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/20-21/
(Assignee)

Comment 87

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d9fcbf0da4d
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

https://reviewboard.mozilla.org/r/46859/#review56186

gfx stuff lgtm

::: dom/base/nsGlobalWindow.cpp:6388
(Diff revision 9)
>          PrepareForFullscreenTransition(getter_AddRefs(transitionData));
>      }
>    }
> -  nsCOMPtr<nsIScreen> screen = aHMD ? aHMD->GetScreen() : nullptr;
>    if (!performTransition) {
> -    return aWindow->SetWidgetFullscreen(aReason, aFullscreen, widget, screen);
> +    return aWindow->SetWidgetFullscreen(aReason, aFullscreen, widget, nullptr);

Might be worth adding a comment here explaining what passing nullptr as the screen argument means here
Attachment #8741942 - Flags: review?(gwright) → review+
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

https://reviewboard.mozilla.org/r/55870/#review56188

This looks fine to me, but Oculus 0.5.x was also the last SDK that supported Linux. Is this something we've consciously decided not to support any more?
Attachment #8757452 - Flags: review?(gwright) → review+
Attachment #8757453 - Flags: review?(gwright) → review+
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

https://reviewboard.mozilla.org/r/55872/#review56190

Are we working on Cardboard VR support? This seems like a regression to me (killing off cardboard VR for now, but with the intent of reimplementing it in the future?). I don't know how bad the perf is on the current version and if it is as bad as the commit message suggests, this seems fine to me. Technically, the patch looks fine.
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

https://reviewboard.mozilla.org/r/46855/#review56192
Attachment #8741940 - Flags: review?(gwright) → review+
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

https://reviewboard.mozilla.org/r/46857/#review56194

Looks fine. I'd prefer Capability_ to Cap_ but either is fine with me.

::: gfx/vr/gfxVR.h:63
(Diff revision 9)
> +   */
> +  Cap_External = 1 << 4,
> +  /**
> +   * Cap_All used for validity checking during IPC serialization
> +   */
> +  Cap_All = (1 << 5) - 1

Yay inline docs! \o/
Attachment #8741941 - Flags: review?(gwright) → review+
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review56198

lgtm, but I'm not a DOM peer.
Attachment #8741953 - Flags: review?(gwright) → review+
(Assignee)

Updated

10 months ago
Depends on: 1284292
(Assignee)

Updated

10 months ago
Depends on: 1284324
(Assignee)

Comment 94

10 months ago
Created attachment 8767761 [details]
Bug 1250244 - Part 9: Implement WebVR DOM Events

Review commit: https://reviewboard.mozilla.org/r/62164/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62164/
(Assignee)

Comment 95

10 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/8-9/
(Assignee)

Comment 96

10 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/9-10/
(Assignee)

Comment 97

10 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/5-6/
(Assignee)

Comment 98

10 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/5-6/
(Assignee)

Comment 99

10 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/9-10/
(Assignee)

Comment 100

10 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/9-10/
(Assignee)

Comment 101

10 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/9-10/
(Assignee)

Comment 102

10 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/21-22/
(Assignee)

Updated

10 months ago
Attachment #8767761 - Flags: review?(masayuki)
(Assignee)

Comment 103

10 months ago
I have assigned Masayuki to review the DOM Events portion, which I've split out into a separate "Part 9" patch.  Please ping me on IRC (:kip) or here if I can be of any assistance.
(Assignee)

Updated

10 months ago
Depends on: 1284357
(Assignee)

Comment 104

10 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/22-23/
(Assignee)

Comment 105

10 months ago
Comment on attachment 8767761 [details]
Bug 1250244 - Part 9: Implement WebVR DOM Events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62164/diff/1-2/
(Assignee)

Comment 106

10 months ago
Implementation of Navigator.activeVRDisplays has been split off into Bug 1284357.
Attachment #8767761 - Flags: review?(masayuki) → review-
Comment on attachment 8767761 [details]
Bug 1250244 - Part 9: Implement WebVR DOM Events

https://reviewboard.mozilla.org/r/62164/#review58952

::: dom/base/Navigator.cpp:2055
(Diff revision 2)
> +  nsGlobalWindow* win = nsGlobalWindow::Cast(mWindow);
> +  win->SetHasVREventListener(true);
> +

I guess that this is a good point, although, I'm not sure (you must know this better than me!).

::: dom/base/nsGlobalWindow.h:500
(Diff revision 2)
>                             nsIWidget* aWidget, nsIScreen* aScreen);
>    bool FullScreen() const;
>  
>    // Inner windows only.
>    virtual void SetHasGamepadEventListener(bool aHasGamepad = true) override;
> +  virtual void SetHasVREventListener(bool aHasVREvents = true) override;

I don't think that you need to manage if nsGlobalWindow has event listeners of VR events. Do you want not to waste the memory space with VREventObserver? Although, I think that it's unnecessary because the instance is really small and the instance count of nsGlobalWindow is not so big. But if you really want to optimize that, how about EventListenerManager to notify DOM window of adding VR event listener and nsGlobalWindow isn't store its state with mHasVREvents?

I meant that at least mHasVREvents isn't necessary since checking mVREventObserver is always same result.

::: dom/base/nsGlobalWindow.h:1775
(Diff revision 2)
> +
> +  // Inner windows only.
> +  // Indicates whether this window wants VR events
> +  bool                   mHasVREvents : 1;

As I said above, I think that this is not necessary.

::: dom/base/nsGlobalWindow.cpp:1720
(Diff revision 2)
>  
>  #if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)
>    mOrientationChangeObserver = nullptr;
>  #endif
>  
> +  mVREventObserver = nullptr;

You clear mVREventObserver here but you call DisableVRUpdate() for clearing mVREventObserver later, why?

::: dom/base/nsGlobalWindow.cpp:1760
(Diff revision 2)
>  #ifdef MOZ_GAMEPAD
>    DisableGamepadUpdates();
>    mHasGamepad = false;
>    mGamepads.Clear();
>  #endif
> +  DisableVRUpdates();

Here.

::: dom/base/nsGlobalWindow.cpp:9961
(Diff revision 2)
> +  if (mHasVREvents && !mVREventObserver) {
> +    mVREventObserver = new mozilla::dom::VREventObserver(this);
> +  }

So, only when |!mVREventObserver|, you should create the instance.

::: dom/base/nsGlobalWindow.cpp:13106
(Diff revision 2)
>      if (ac) {
>        for (uint32_t i = 0; i < mEnabledSensors.Length(); i++)
>          ac->AddWindowListener(mEnabledSensors[i], this);
>      }
>      EnableGamepadUpdates();
> +    EnableVRUpdates();

Um, only when you need to resume something with this method call, you need mHasVREvents? But I don't find the place where you want to disable VR updates but not setting mHasVREvents to false.

If it's your bug and you really need mHasVREvents for this purpose, keeping mHasVREvents is fine but I still don't like the method name because it's never called with false.

::: dom/base/nsPIDOMWindow.h:474
(Diff revision 2)
> +  * Tell this window that there is an observer for VR events
> +  *
> +  * Inner windows only.
> +  */
> +  virtual void SetHasVREventListener(bool aHasVREvents = true) = 0;

You won't set false with this method. So, I think NotifyOfAddingVREventListener() is better.

::: dom/events/EventListenerManager.cpp:422
(Diff revision 2)
> +  } else if (aTypeAtom == nsGkAtoms::onvrdisplayconnected ||
> +             aTypeAtom == nsGkAtoms::onvrdisplaydisconnected ||
> +             aTypeAtom == nsGkAtoms::onvrdisplaypresentchange) {
> +    if (nsPIDOMWindowInner* window = GetInnerWindowForTarget()) {
> +      window->SetHasVREventListener();
> +    }

I still don't understand why nsGlobalWindow needs to cache if it has VR event listeners. According to the event names, the events are not fired so many times, e.g., not like mousemove, mouseenter, mouseleave, therefore, I have no idea why you need the optimization.

::: dom/events/EventNameList.h:590
(Diff revision 2)
>                    eBasicEventClass)
>  WINDOW_ONLY_EVENT(devicelight,
>                    eDeviceLight,
>                    EventNameType_None,
>                    eBasicEventClass)
>  

nit: unnecessary empty line (IIRC, this file doesn't separate events with empty lines).

::: dom/vr/VREventObserver.h:13
(Diff revision 2)
> +namespace dom {
> +class VREventObserver final

nit: I want you to add an empty line between these lines.

::: dom/vr/VREventObserver.h:25
(Diff revision 2)
> +  void NotifyVRDisplayConnected();
> +  void NotifyVRDisplayDisconnected();
> +  void NotifyVRDisplayPresentChange();
> +
> +private:
> +  

nit: Empty line but having whitespaces. I think that you don't need this line.

::: dom/vr/VREventObserver.cpp:12
(Diff revision 2)
> +#include "VREventObserver.h"
> +
> +#include "nsGlobalWindow.h"
> +#include "VRManagerChild.h"
> +
> +using namespace mozilla::dom;

VREventObserver is in mozilla::dom. So, please do not use "using" here. It's unclear whether VREventObserver is in :: or mozilla:: or mozilla::dom::.

::: dom/vr/VREventObserver.cpp:42
(Diff revision 2)
> +
> +void
> +VREventObserver::NotifyVRDisplayConnected()
> +{
> +  if (mWindow->AsInner()->IsCurrentInnerWindow()) {
> +    mWindow->GetOuterWindow()->DispatchCustomEvent(NS_LITERAL_STRING("vrdisplayconnected"));

over 80 characters.

::: dom/vr/VREventObserver.cpp:50
(Diff revision 2)
> +
> +void
> +VREventObserver::NotifyVRDisplayDisconnected()
> +{
> +  if (mWindow->AsInner()->IsCurrentInnerWindow()) {
> +    mWindow->GetOuterWindow()->DispatchCustomEvent(NS_LITERAL_STRING("vrdisplaydisconnected"));

ditto.

::: dom/vr/VREventObserver.cpp:59
(Diff revision 2)
> +  if (mWindow->AsInner()->IsCurrentInnerWindow() && !mWindow->GetOuterWindow()->IsBackground()) {
> +    mWindow->GetOuterWindow()->DispatchCustomEvent(NS_LITERAL_STRING("vrdisplaypresentchange"));
> +  }

ditto.

::: gfx/vr/ipc/VRManagerChild.cpp:464
(Diff revision 2)
>  void
>  VRManagerChild::FireDOMVRDisplayConnectedEvent()
>  {
> +  for (auto& listener : mListeners) {
> +    listener->NotifyVRDisplayConnected();
> +  }
>  }
>  
>  void
>  VRManagerChild::FireDOMVRDisplayDisconnectedEvent()
>  {
> +  for (auto& listener : mListeners) {
> +    listener->NotifyVRDisplayDisconnected();
> +  }
>  }
>  
>  void
>  VRManagerChild::FireDOMVRDisplayPresentChangeEvent()
>  {
> +  for (auto& listener : mListeners) {
> +    listener->NotifyVRDisplayPresentChange();
> +  }
> +}

Hmm, I'm not sure these methods are always called at safe to dispatch DOM events. That's the most important point you to dispatch DOM events from C++ code.

You can check it with nsContentUtils::IsSafeToRunScript(). If it's false, your event may cause some security issues. If these methods are not guaranteed to be called only when it's safe, please use nsContentUtils::AddScriptRunner() for dispatching the events.

The second hunk of this changeset muse be a good reference for you:
https://hg.mozilla.org/mozilla-central/diff/e4d4400c93b1/editor/libeditor/base/nsEditor.cpp

::: gfx/vr/ipc/VRManagerChild.cpp:499
(Diff revision 2)
> +    return; // already exists
> +  }
> +
> +  mListeners.AppendElement(aObserver);
> +  if (mListeners.Length() == 1) {
> +    Unused << SendSetHaveEventListener(true);

I'm not sure what this does...

::: gfx/vr/ipc/VRManagerChild.cpp:515
(Diff revision 2)
> -}
> +  }
> +
> +  mListeners.RemoveElement(aObserver);
> +
> +  if (mListeners.Length() == 0) {
> +    Unused << SendSetHaveEventListener(false);

ditto.
(Assignee)

Updated

10 months ago
Depends on: 1284984
(Assignee)

Comment 108

10 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/9-10/
Attachment #8722648 - Attachment description: Bug 1250244 - Part 8: (WIP, Don't commit) Implement WebVR 1.0 API → Bug 1250244 - Part 7: (WIP, Don't commit) Implement WebVR 1.0 API
Attachment #8741953 - Attachment description: Bug 1250244 - Part 7: Add GamePad.displayID → Bug 1250244 - Part 8: Implement WebVR DOM Events
(Assignee)

Comment 109

10 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/10-11/
(Assignee)

Comment 110

10 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/6-7/
(Assignee)

Comment 111

10 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/6-7/
(Assignee)

Comment 112

10 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/10-11/
(Assignee)

Comment 113

10 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/10-11/
(Assignee)

Comment 114

10 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/23-24/
(Assignee)

Comment 115

10 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/10-11/
(Assignee)

Updated

10 months ago
Attachment #8767761 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8757453 - Flags: review+
(Assignee)

Comment 116

10 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/10-11/
Attachment #8741952 - Attachment description: Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen → Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,
Attachment #8741942 - Attachment description: Bug 1250244 - Part 2: Remove old VR rendering paths → Bug 1250244 - Part 2: Remove old VR rendering paths,
Attachment #8757452 - Attachment description: Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support → Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,
Attachment #8757453 - Attachment description: Bug 1250244 - Part 4: Remove Cardboard VR Support → Bug 1250244 - Part 4: Remove Cardboard VR Support,
Attachment #8741940 - Attachment description: Bug 1250244 - Part 5: Rename VRDevice to VRDisplay → Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,
Attachment #8741941 - Attachment description: Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags → Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,
Attachment #8741953 - Flags: review+
(Assignee)

Comment 117

10 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/11-12/
(Assignee)

Comment 118

10 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/7-8/
(Assignee)

Comment 119

10 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/7-8/
(Assignee)

Comment 120

10 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/11-12/
(Assignee)

Comment 121

10 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/11-12/
(Assignee)

Comment 122

10 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/24-25/
(Assignee)

Comment 123

10 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/11-12/
(Assignee)

Comment 124

10 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

r+'ed by gw280 earlier.
Attachment #8757453 - Flags: review+
(Assignee)

Comment 125

10 months ago
The earlier "part 7" patch has been split out into a separate bug:

Bug 1284984 (Add Gamepad.displayID)

Please note that when I pushed the updated patch set to MozReview for this bug, this resulted in prior reviews being associated to the incorrect patches.  Part 8 (Dom Events) has not yet been r+'ed by :masayuki, even though it says so.
(Assignee)

Comment 126

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfcdbd653deb
(Assignee)

Comment 127

10 months ago
Created attachment 8768982 [details]
Bug 1250244 - Part 9: Update test_interfaces.html

- DOM Mochitests ensure that a dom peer has reviewed
  new interfaces listed in the test_interfaces.html file.
- This patch includes the changes to this file that
  will require DOM Peer review.

Review commit: https://reviewboard.mozilla.org/r/62956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62956/
Attachment #8757453 - Flags: review+
(Assignee)

Comment 128

10 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/11-12/
(Assignee)

Comment 129

10 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/12-13/
(Assignee)

Comment 130

10 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/8-9/
(Assignee)

Comment 131

10 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/8-9/
(Assignee)

Comment 132

10 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/12-13/
(Assignee)

Comment 133

10 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/12-13/
(Assignee)

Comment 134

10 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/25-26/
(Assignee)

Comment 135

10 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/12-13/
(Assignee)

Comment 136

10 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/12-13/
(Assignee)

Comment 137

10 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/13-14/
(Assignee)

Comment 138

10 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/9-10/
(Assignee)

Comment 139

10 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/9-10/
(Assignee)

Comment 140

10 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/13-14/
(Assignee)

Comment 141

10 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/13-14/
(Assignee)

Comment 142

10 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/26-27/
(Assignee)

Comment 143

10 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/13-14/
(Assignee)

Comment 144

10 months ago
Comment on attachment 8768982 [details]
Bug 1250244 - Part 9: Update test_interfaces.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62956/diff/1-2/
(Assignee)

Comment 145

10 months ago
Comment on attachment 8768982 [details]
Bug 1250244 - Part 9: Update test_interfaces.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62956/diff/2-3/
(Assignee)

Comment 146

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24a709ea60a
(Assignee)

Comment 147

10 months ago
Thanks for taking at look at my patch and giving helpful feedback, Masayuki.

Sorry, my later push to reviewboard has moved your comments to another patch.  I'll add my replies below:

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #107)
> Comment on attachment 8767761 [details]
> Bug 1250244 - Part 9: Implement WebVR DOM Events
> 
> https://reviewboard.mozilla.org/r/62164/#review58952
> 
> ::: dom/base/Navigator.cpp:2055
> (Diff revision 2)
> > +  nsGlobalWindow* win = nsGlobalWindow::Cast(mWindow);
> > +  win->SetHasVREventListener(true);
> > +
> 
> I guess that this is a good point, although, I'm not sure (you must know
> this better than me!).
> 
> ::: dom/base/nsGlobalWindow.h:500
> (Diff revision 2)
> >                             nsIWidget* aWidget, nsIScreen* aScreen);
> >    bool FullScreen() const;
> >  
> >    // Inner windows only.
> >    virtual void SetHasGamepadEventListener(bool aHasGamepad = true) override;
> > +  virtual void SetHasVREventListener(bool aHasVREvents = true) override;
> 
> I don't think that you need to manage if nsGlobalWindow has event listeners
> of VR events. Do you want not to waste the memory space with
> VREventObserver? Although, I think that it's unnecessary because the
> instance is really small and the instance count of nsGlobalWindow is not so
> big. But if you really want to optimize that, how about EventListenerManager
> to notify DOM window of adding VR event listener and nsGlobalWindow isn't
> store its state with mHasVREvents?
> 
> I meant that at least mHasVREvents isn't necessary since checking
> mVREventObserver is always same result.

In this case, I am following the pattern used by gamepad events.
I use mHasVREvents to re-create the VREventListener when
nsGlobalWindow::ResumeTimeouts is called.

VREventObserver uses very little memory; however, it also triggers enumeration
of VR hardware.  We don't want to enumerate VR hardware until the users visits
a site that is waiting for the VR events, as the enumeration will unfortunately
turn on physical hardware even if we don't use it after the enumeration.

> 
> ::: dom/base/nsGlobalWindow.h:1775
> (Diff revision 2)
> > +
> > +  // Inner windows only.
> > +  // Indicates whether this window wants VR events
> > +  bool                   mHasVREvents : 1;
> 
> As I said above, I think that this is not necessary.

Please see my earlier comment about nsGlobalWindow::ResumeTimeouts

> 
> ::: dom/base/nsGlobalWindow.cpp:1720
> (Diff revision 2)
> >  
> >  #if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)
> >    mOrientationChangeObserver = nullptr;
> >  #endif
> >  
> > +  mVREventObserver = nullptr;
> 
> You clear mVREventObserver here but you call DisableVRUpdate() for clearing
> mVREventObserver later, why?

Yes, this is redundant.  I have removed it.

> 
> ::: dom/base/nsGlobalWindow.cpp:1760
> (Diff revision 2)
> >  #ifdef MOZ_GAMEPAD
> >    DisableGamepadUpdates();
> >    mHasGamepad = false;
> >    mGamepads.Clear();
> >  #endif
> > +  DisableVRUpdates();
> 
> Here.
> 
> ::: dom/base/nsGlobalWindow.cpp:9961
> (Diff revision 2)
> > +  if (mHasVREvents && !mVREventObserver) {
> > +    mVREventObserver = new mozilla::dom::VREventObserver(this);
> > +  }
> 
> So, only when |!mVREventObserver|, you should create the instance.

Please see my prior comment about nsGlobalWindow::ResumeTimeouts.  In this
case, we don't want to re-create mVREventObserver unless it is needed.

> 
> ::: dom/base/nsGlobalWindow.cpp:13106
> (Diff revision 2)
> >      if (ac) {
> >        for (uint32_t i = 0; i < mEnabledSensors.Length(); i++)
> >          ac->AddWindowListener(mEnabledSensors[i], this);
> >      }
> >      EnableGamepadUpdates();
> > +    EnableVRUpdates();
> 
> Um, only when you need to resume something with this method call, you need
> mHasVREvents? But I don't find the place where you want to disable VR
> updates but not setting mHasVREvents to false.
> 
> If it's your bug and you really need mHasVREvents for this purpose, keeping
> mHasVREvents is fine but I still don't like the method name because it's
> never called with false.

Thanks for finding this, it is my bug :-)

I was trying to follow the pattern used by gamepad, and didn't add the
necessary call to DisableVRUpdates in nsGlobalWindow::SuspendTimeouts.

I have now added DisableVRUpdates() right below DisableGamepadUpdates()
in nsGlobalWindow::SuspendTimeouts.

> 
> ::: dom/base/nsPIDOMWindow.h:474
> (Diff revision 2)
> > +  * Tell this window that there is an observer for VR events
> > +  *
> > +  * Inner windows only.
> > +  */
> > +  virtual void SetHasVREventListener(bool aHasVREvents = true) = 0;
> 
> You won't set false with this method. So, I think
> NotifyOfAddingVREventListener() is better.

Good point.  I have renamed it and removed the boolean parameter.

> 
> ::: dom/events/EventListenerManager.cpp:422
> (Diff revision 2)
> > +  } else if (aTypeAtom == nsGkAtoms::onvrdisplayconnected ||
> > +             aTypeAtom == nsGkAtoms::onvrdisplaydisconnected ||
> > +             aTypeAtom == nsGkAtoms::onvrdisplaypresentchange) {
> > +    if (nsPIDOMWindowInner* window = GetInnerWindowForTarget()) {
> > +      window->SetHasVREventListener();
> > +    }
> 
> I still don't understand why nsGlobalWindow needs to cache if it has VR
> event listeners. According to the event names, the events are not fired so
> many times, e.g., not like mousemove, mouseenter, mouseleave, therefore, I
> have no idea why you need the optimization.
> 

See comment above.  We don't want to force VR hardware to be turned on until
the user visits a WebVR site that is looking for the VR hardware.

> ::: dom/events/EventNameList.h:590
> (Diff revision 2)
> >                    eBasicEventClass)
> >  WINDOW_ONLY_EVENT(devicelight,
> >                    eDeviceLight,
> >                    EventNameType_None,
> >                    eBasicEventClass)
> >  
> 
> nit: unnecessary empty line (IIRC, this file doesn't separate events with
> empty lines).

Fixed, thanks!

> 
> ::: dom/vr/VREventObserver.h:13
> (Diff revision 2)
> > +namespace dom {
> > +class VREventObserver final
> 
> nit: I want you to add an empty line between these lines.

Fixed.

> 
> ::: dom/vr/VREventObserver.h:25
> (Diff revision 2)
> > +  void NotifyVRDisplayConnected();
> > +  void NotifyVRDisplayDisconnected();
> > +  void NotifyVRDisplayPresentChange();
> > +
> > +private:
> > +  
> 
> nit: Empty line but having whitespaces. I think that you don't need this
> line.

Fixed.

> 
> ::: dom/vr/VREventObserver.cpp:12
> (Diff revision 2)
> > +#include "VREventObserver.h"
> > +
> > +#include "nsGlobalWindow.h"
> > +#include "VRManagerChild.h"
> > +
> > +using namespace mozilla::dom;
> 
> VREventObserver is in mozilla::dom. So, please do not use "using" here. It's
> unclear whether VREventObserver is in :: or mozilla:: or mozilla::dom::.

VREventObserver belongs in mozilla::dom::

I have removed the "using namespace" and replaced with:

namespace mozilla {
namespace dom {

... and the end of the file:

} // namespace dom
} // namespace mozilla

> 
> ::: dom/vr/VREventObserver.cpp:42
> (Diff revision 2)
> > +
> > +void
> > +VREventObserver::NotifyVRDisplayConnected()
> > +{
> > +  if (mWindow->AsInner()->IsCurrentInnerWindow()) {
> > +    mWindow->GetOuterWindow()->DispatchCustomEvent(NS_LITERAL_STRING("vrdisplayconnected"));
> 
> over 80 characters.
Fixed by splitting to multiple lines.
> 
> ::: dom/vr/VREventObserver.cpp:50
> (Diff revision 2)
> > +
> > +void
> > +VREventObserver::NotifyVRDisplayDisconnected()
> > +{
> > +  if (mWindow->AsInner()->IsCurrentInnerWindow()) {
> > +    mWindow->GetOuterWindow()->DispatchCustomEvent(NS_LITERAL_STRING("vrdisplaydisconnected"));
> 
> ditto.
Fixed.
> 
> ::: dom/vr/VREventObserver.cpp:59
> (Diff revision 2)
> > +  if (mWindow->AsInner()->IsCurrentInnerWindow() && !mWindow->GetOuterWindow()->IsBackground()) {
> > +    mWindow->GetOuterWindow()->DispatchCustomEvent(NS_LITERAL_STRING("vrdisplaypresentchange"));
> > +  }
> 
> ditto.
Fixed.
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:464
> (Diff revision 2)
> >  void
> >  VRManagerChild::FireDOMVRDisplayConnectedEvent()
> >  {
> > +  for (auto& listener : mListeners) {
> > +    listener->NotifyVRDisplayConnected();
> > +  }
> >  }
> >  
> >  void
> >  VRManagerChild::FireDOMVRDisplayDisconnectedEvent()
> >  {
> > +  for (auto& listener : mListeners) {
> > +    listener->NotifyVRDisplayDisconnected();
> > +  }
> >  }
> >  
> >  void
> >  VRManagerChild::FireDOMVRDisplayPresentChangeEvent()
> >  {
> > +  for (auto& listener : mListeners) {
> > +    listener->NotifyVRDisplayPresentChange();
> > +  }
> > +}
> 
> Hmm, I'm not sure these methods are always called at safe to dispatch DOM
> events. That's the most important point you to dispatch DOM events from C++
> code.
> 
> You can check it with nsContentUtils::IsSafeToRunScript(). If it's false,
> your event may cause some security issues. If these methods are not
> guaranteed to be called only when it's safe, please use
> nsContentUtils::AddScriptRunner() for dispatching the events.
> 
> The second hunk of this changeset muse be a good reference for you:
> https://hg.mozilla.org/mozilla-central/diff/e4d4400c93b1/editor/libeditor/
> base/nsEditor.cpp

Thanks!  I will update the patch again with this change before assigning it back for review.

> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:499
> (Diff revision 2)
> > +    return; // already exists
> > +  }
> > +
> > +  mListeners.AppendElement(aObserver);
> > +  if (mListeners.Length() == 1) {
> > +    Unused << SendSetHaveEventListener(true);
> 
> I'm not sure what this does...

The actual hardware enumeration that triggers the events occurs in a separate
process and thread.  The active VREventObserver's are stored in mListeners.
The SendSetHaveEventListener(true) is an IPC call to start the enumeration of
hardware.  When we receive a response back, VRManagerChild will dispatch
the events to the observers in mListeners.

> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:515
> (Diff revision 2)
> > -}
> > +  }
> > +
> > +  mListeners.RemoveElement(aObserver);
> > +
> > +  if (mListeners.Length() == 0) {
> > +    Unused << SendSetHaveEventListener(false);
> 
> ditto.
This reverses the actions of AddListener.  Note that the parent process
will keep count of how many children need to be notified of events,
so we won't shut down the events for other documents.
(Assignee)

Comment 148

10 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/14-15/
(Assignee)

Updated

10 months ago
Attachment #8768982 - Attachment is obsolete: true
(Assignee)

Comment 149

10 months ago
Created attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

- DOM Mochitests ensure that a dom peer has reviewed
  new interfaces listed in the test_interfaces.html file.
- This patch includes the changes to this file that
  will require DOM Peer review.

Review commit: https://reviewboard.mozilla.org/r/63782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63782/
(Assignee)

Comment 150

10 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/13-14/
Attachment #8770318 - Flags: review?(bzbarsky)
(Assignee)

Comment 151

10 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/14-15/
(Assignee)

Comment 152

10 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/10-11/
(Assignee)

Comment 153

10 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/10-11/
(Assignee)

Comment 154

10 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/14-15/
(Assignee)

Comment 155

10 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/14-15/
(Assignee)

Comment 156

10 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/27-28/
(Assignee)

Comment 157

10 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/15-16/
(Assignee)

Comment 158

10 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/1-2/
(Assignee)

Comment 159

10 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/28-29/
Attachment #8722648 - Attachment description: Bug 1250244 - Part 7: (WIP, Don't commit) Implement WebVR 1.0 API → Bug 1250244 - Part 7: Implement WebVR 1.0 API
(Assignee)

Comment 160

10 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/16-17/
(Assignee)

Comment 161

10 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/2-3/
(Assignee)

Comment 162

10 months ago
I have updated the Part 7 patch (Implement WebVR 1.0 API) to handle issues with multiple tabs that were found in testing.
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

https://reviewboard.mozilla.org/r/63782/#review61424
Attachment #8770318 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 164

10 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/14-15/
(Assignee)

Comment 165

10 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/15-16/
(Assignee)

Comment 166

10 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/11-12/
(Assignee)

Comment 167

10 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/11-12/
(Assignee)

Comment 168

10 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/15-16/
(Assignee)

Comment 169

10 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/15-16/
(Assignee)

Comment 170

10 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/29-30/
(Assignee)

Comment 171

10 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/17-18/
(Assignee)

Comment 172

10 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/3-4/
(Assignee)

Updated

10 months ago
Attachment #8722648 - Flags: review?(bas)
(Assignee)

Comment 173

9 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/15-16/
(Assignee)

Comment 174

9 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/16-17/
(Assignee)

Comment 175

9 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/12-13/
(Assignee)

Comment 176

9 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/12-13/
(Assignee)

Comment 177

9 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/16-17/
(Assignee)

Comment 178

9 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/16-17/
(Assignee)

Comment 179

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/30-31/
(Assignee)

Comment 180

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/18-19/
(Assignee)

Comment 181

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/4-5/
(Assignee)

Updated

9 months ago
Attachment #8741953 - Flags: review?(masayuki)
(Assignee)

Comment 182

9 months ago
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #107)
> Hmm, I'm not sure these methods are always called at safe to dispatch DOM
> events. That's the most important point you to dispatch DOM events from C++
> code.
> 
> You can check it with nsContentUtils::IsSafeToRunScript(). If it's false,
> your event may cause some security issues. If these methods are not
> guaranteed to be called only when it's safe, please use
> nsContentUtils::AddScriptRunner() for dispatching the events.
> 
> The second hunk of this changeset muse be a good reference for you:
> https://hg.mozilla.org/mozilla-central/diff/e4d4400c93b1/editor/libeditor/
> base/nsEditor.cpp
>

Thank you for finding this.  I have updated it to fall back to AddScriptRunner() when IsSafeToRunScript() returns false now.

I have corrected all of the items in your original comment #107 and assigned it back to you for the next review.

The mozreview comments were lost in a prior push and attached to the wrong patch.  It seems to also show "r+", even though you have not R+'ed.  Sorry for the confusion.

Thank you very much for your reviews and helpful feedback.
(Assignee)

Comment 183

9 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/16-17/
(Assignee)

Comment 184

9 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/17-18/
(Assignee)

Comment 185

9 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/13-14/
(Assignee)

Comment 186

9 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/13-14/
(Assignee)

Comment 187

9 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/17-18/
(Assignee)

Comment 188

9 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/17-18/
(Assignee)

Comment 189

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/31-32/
(Assignee)

Comment 190

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/19-20/
(Assignee)

Comment 191

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/5-6/
(Assignee)

Comment 192

9 months ago
I have re-based the patches to match the current mozilla-central
(Assignee)

Comment 193

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f647ca4d1d83
(Assignee)

Comment 194

9 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/17-18/
(Assignee)

Comment 195

9 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/18-19/
(Assignee)

Comment 196

9 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/14-15/
(Assignee)

Comment 197

9 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/14-15/
(Assignee)

Comment 198

9 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/18-19/
(Assignee)

Comment 199

9 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/18-19/
(Assignee)

Comment 200

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/32-33/
(Assignee)

Comment 201

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/20-21/
(Assignee)

Comment 202

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/6-7/
(Assignee)

Comment 203

9 months ago
Re-based patches.
(Assignee)

Comment 204

9 months ago
This is intended to land in Firefox 50.  Could you let me know if you will have time to review the patch?
Flags: needinfo?(bas)
(Assignee)

Comment 205

9 months ago
I am not certain if the erraneous "r+" status on the Part 8 patch is blocking you from getting notifications.  I have updated the patch with the changes from your previous review.  Please advise if you will have time to review my updated patch in time to land for Firefox 50?
Flags: needinfo?(masayuki)
(In reply to :kip (Kearwood Gilbert) from comment #205)
> I am not certain if the erraneous "r+" status on the Part 8 patch is
> blocking you from getting notifications.  I have updated the patch with the
> changes from your previous review.  Please advise if you will have time to
> review my updated patch in time to land for Firefox 50?

When is FX50? I'm at SIGGRAPH now but I've already done part of the review and should be able to finish it before the end of the week. Let me know if you need it done earlier and I'll make sure it is.
Flags: needinfo?(bas)
Oh, sorry. I missed to catch the review request. Feel free to ping me if I don't review patches in a couple of days. I'll review it soon.
Flags: needinfo?(masayuki)
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review63850

I'd like to check new patch again. So, temporarily, -'ing.

::: dom/base/nsGlobalWindow.cpp:1718
(Diff revision 21)
>  
>  #if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)
>    mOrientationChangeObserver = nullptr;
>  #endif
>  
> +  mVREventObserver = nullptr;

Hmm, you set mVREventObserver to nullptr here, but...

::: dom/base/nsGlobalWindow.cpp:1758
(Diff revision 21)
>  #ifdef MOZ_GAMEPAD
>    DisableGamepadUpdates();
>    mHasGamepad = false;
>    mGamepads.Clear();
>  #endif
> +  DisableVRUpdates();

You call DisableVRUpdates() here again for clearing mVREventObserver.

::: dom/base/nsGlobalWindow.cpp:9962
(Diff revision 21)
> +nsGlobalWindow::EnableVRUpdates()
> +{
> +  MOZ_ASSERT(IsInnerWindow());
> +
> +  if (mHasVREvents && !mVREventObserver) {
> +    mVREventObserver = new mozilla::dom::VREventObserver(this);

nit: do you really need to specify the namespace explicitly here?

::: dom/vr/VREventObserver.cpp:16
(Diff revision 21)
> +* This class is used by nsGlobalWindow to implement window.onvrdisplayconnected,
> +* window.onvrdisplaydisconnected, and window.onvrdisplaypresentchange.
> +*/

nit: could you insert a whitespace at each line's head. Typically, we use this comment style ("*" is at each line's second column):

/**
 *
 */

::: dom/vr/VREventObserver.cpp:19
(Diff revision 21)
> +VREventObserver::VREventObserver(
> +  nsGlobalWindow* aGlobalWindow)

nit: you can write this in a line:

> VREventObserver::VREventObserver(nsGlobalWindow* aGlobalWindow)

::: dom/vr/VREventObserver.cpp:25
(Diff revision 21)
> +  nsGlobalWindow* aGlobalWindow)
> +  : mWindow(aGlobalWindow)
> +{
> +  MOZ_ASSERT(aGlobalWindow && aGlobalWindow->IsInnerWindow());
> +
> +  gfx::VRManagerChild* vmc = gfx::VRManagerChild::Get();

nit: if you don't like to use |gfx::| in this file, I don't mind you to add |using namespace gfx;| between |namespace mozilla {| and |namespace dom {| with empty lines.

::: dom/vr/VREventObserver.cpp:43
(Diff revision 21)
> +    mWindow->GetOuterWindow()->DispatchCustomEvent(
> +      NS_LITERAL_STRING("vrdisplayconnected"));

Could you add |MOZ_ASSERT(nsContentUtils::IsSafeToRunScript())| before the call of DispatchCustomeEvent()?

::: dom/vr/VREventObserver.cpp:52
(Diff revision 21)
> +    mWindow->GetOuterWindow()->DispatchCustomEvent(
> +      NS_LITERAL_STRING("vrdisplaydisconnected"));

Same here.

::: dom/vr/VREventObserver.cpp:60
(Diff revision 21)
> +  // Background tabs should not be allowed to see when a foreground tab
> +  // has started or ended vr presentation.

Sounds like that background tab's window won't receive vrdisplaypresentchange event. However, it seems that if the state is different between when it becomes a background table and becomes a foreground tab again, doesn't the window want this event?

If so, I think that this class should store the state and notify the window of pending change events when it becomes a foreground tab.

Anyway, even if this is necessary, you should add another patch because it makes this patch complicated.

::: dom/vr/VREventObserver.cpp:64
(Diff revision 21)
> +    mWindow->GetOuterWindow()->DispatchCustomEvent(
> +      NS_LITERAL_STRING("vrdisplaypresentchange"));

And same here. (MOZ_ASSERT())

::: gfx/vr/ipc/VRManagerChild.h:38
(Diff revision 21)
>                       , public layers::TextureForwarder
>                       , public layers::ShmemAllocator
>  {
>  public:
>    static VRManagerChild* Get();
> +  

nit: Please remove this unnecessary (trailing) whitespaces before landing.

::: gfx/vr/ipc/VRManagerChild.cpp:21
(Diff revision 21)
> +namespace {
> +const nsTArray<RefPtr<dom::VREventObserver> >::index_type NoIndex =
> +  nsTArray<RefPtr<dom::VREventObserver> >::NoIndex;
> +} // namespace

nit: you don't need to take care old compilers with |> >| hack. Gecko's code already has |>>| in a lot of places.

And this is const value. So, I like kNoIndex (with |k| prefix).

::: gfx/vr/ipc/VRManagerChild.cpp:476
(Diff revision 21)
> +  if (nsContentUtils::IsSafeToRunScript()) {
> +    FireDOMVRDisplayConnectedEventInternal();
> +  } else {
> +    // If it's not safe to run scripts right now, schedule this to run later
> +    nsContentUtils::AddScriptRunner(NewRunnableMethod(this,
> +      &VRManagerChild::FireDOMVRDisplayConnectedEventInternal));
> +  }

You don't need to check nsContentUtils::IsSafeToRunScript() by yourself because AddScriptRunner runs given runnable class immediately if it's safe. So, only the else block is enough.

::: gfx/vr/ipc/VRManagerChild.cpp:488
(Diff revision 21)
> +  if (nsContentUtils::IsSafeToRunScript()) {
> +    FireDOMVRDisplayDisconnectedEventInternal();
> +  } else {
> +    // If it's not safe to run scripts right now, schedule this to run later
> +    nsContentUtils::AddScriptRunner(NewRunnableMethod(this,
> +      &VRManagerChild::FireDOMVRDisplayDisconnectedEventInternal));
> +  }

ditto.

::: gfx/vr/ipc/VRManagerChild.cpp:500
(Diff revision 21)
> +  if (nsContentUtils::IsSafeToRunScript()) {
> +    FireDOMVRDisplayPresentChangeEventInternal();
> +  } else {
> +    // If it's not safe to run scripts right now, schedule this to run later
> +    nsContentUtils::AddScriptRunner(NewRunnableMethod(this,
> +      &VRManagerChild::FireDOMVRDisplayPresentChangeEventInternal));

ditto.

::: gfx/vr/ipc/VRManagerChild.cpp:509
(Diff revision 21)
> +void
> +VRManagerChild::FireDOMVRDisplayConnectedEventInternal()
> +{
> +  for (auto& listener : mListeners) {
> +    listener->NotifyVRDisplayConnected();
> +  }
> +}
> +
> +void
> +VRManagerChild::FireDOMVRDisplayDisconnectedEventInternal()
> +{
> +  for (auto& listener : mListeners) {
> +    listener->NotifyVRDisplayDisconnected();
> +  }
> +}
> +
> +void
> +VRManagerChild::FireDOMVRDisplayPresentChangeEventInternal()
> +{
> +  for (auto& listener : mListeners) {
> +    listener->NotifyVRDisplayPresentChange();
> +  }
>  }

I assume that these methods never cause DOM events at shutting down of XPCOM since mListeners should be empty in such case. If not, please manage the state with a flag or something like |mDestroyed|.

::: gfx/vr/ipc/VRManagerChild.cpp:557
(Diff revision 21)
> +  mListeners.RemoveElement(aObserver);
> +
> +  if (mListeners.Length() == 0) {
> +    Unused << SendSetHaveEventListener(false);
> +  }

It seems that you *might* need to send this with synchronous IPC before removing the observer actually. If not, please drop this issue.
Attachment #8741953 - Flags: review?(masayuki) → review-
(Assignee)

Comment 209

9 months ago
https://reviewboard.mozilla.org/r/46871/#review63850

> Sounds like that background tab's window won't receive vrdisplaypresentchange event. However, it seems that if the state is different between when it becomes a background table and becomes a foreground tab again, doesn't the window want this event?
> 
> If so, I think that this class should store the state and notify the window of pending change events when it becomes a foreground tab.
> 
> Anyway, even if this is necessary, you should add another patch because it makes this patch complicated.

I have changed this so that even background tabs will receive this event right away.  This was working around javascript content that was making assumptions, but the content should in fact be fixed.
(Assignee)

Comment 210

9 months ago
https://reviewboard.mozilla.org/r/46871/#review63850

> I assume that these methods never cause DOM events at shutting down of XPCOM since mListeners should be empty in such case. If not, please manage the state with a flag or something like |mDestroyed|.

mListeners should be empty during XPCOM shutdown.
(Assignee)

Comment 211

9 months ago
https://reviewboard.mozilla.org/r/46871/#review63850

> It seems that you *might* need to send this with synchronous IPC before removing the observer actually. If not, please drop this issue.

I have now made the IPC call "sync" in the ipdl.
(Assignee)

Comment 212

9 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/18-19/
Attachment #8741953 - Flags: review- → review?(masayuki)
(Assignee)

Comment 213

9 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/19-20/
(Assignee)

Comment 214

9 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/15-16/
(Assignee)

Comment 215

9 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/15-16/
(Assignee)

Comment 216

9 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/19-20/
(Assignee)

Comment 217

9 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/19-20/
(Assignee)

Comment 218

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/33-34/
(Assignee)

Comment 219

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/21-22/
(Assignee)

Comment 220

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/7-8/
(Assignee)

Comment 221

9 months ago
Thanks again for your very helpful feedback, Masayuki!

I have made the changes you recommended and have updated the patch.

As I'm not sure if reviewboard will send you a notification, I'll NI you here.


(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #208)
> Comment on attachment 8741953 [details]
> Bug 1250244 - Part 8: Implement WebVR DOM Events
> 
> https://reviewboard.mozilla.org/r/46871/#review63850
> 
> I'd like to check new patch again. So, temporarily, -'ing.
> 
> ::: dom/base/nsGlobalWindow.cpp:1718
> (Diff revision 21)
> >  
> >  #if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)
> >    mOrientationChangeObserver = nullptr;
> >  #endif
> >  
> > +  mVREventObserver = nullptr;
> 
> Hmm, you set mVREventObserver to nullptr here, but...
> 
> ::: dom/base/nsGlobalWindow.cpp:1758
> (Diff revision 21)
> >  #ifdef MOZ_GAMEPAD
> >    DisableGamepadUpdates();
> >    mHasGamepad = false;
> >    mGamepads.Clear();
> >  #endif
> > +  DisableVRUpdates();
> 
> You call DisableVRUpdates() here again for clearing mVREventObserver.
> 
> ::: dom/base/nsGlobalWindow.cpp:9962
> (Diff revision 21)
> > +nsGlobalWindow::EnableVRUpdates()
> > +{
> > +  MOZ_ASSERT(IsInnerWindow());
> > +
> > +  if (mHasVREvents && !mVREventObserver) {
> > +    mVREventObserver = new mozilla::dom::VREventObserver(this);
> 
> nit: do you really need to specify the namespace explicitly here?
> 
> ::: dom/vr/VREventObserver.cpp:16
> (Diff revision 21)
> > +* This class is used by nsGlobalWindow to implement window.onvrdisplayconnected,
> > +* window.onvrdisplaydisconnected, and window.onvrdisplaypresentchange.
> > +*/
> 
> nit: could you insert a whitespace at each line's head. Typically, we use
> this comment style ("*" is at each line's second column):
> 
> /**
>  *
>  */
> 
> ::: dom/vr/VREventObserver.cpp:19
> (Diff revision 21)
> > +VREventObserver::VREventObserver(
> > +  nsGlobalWindow* aGlobalWindow)
> 
> nit: you can write this in a line:
> 
> > VREventObserver::VREventObserver(nsGlobalWindow* aGlobalWindow)
> 
> ::: dom/vr/VREventObserver.cpp:25
> (Diff revision 21)
> > +  nsGlobalWindow* aGlobalWindow)
> > +  : mWindow(aGlobalWindow)
> > +{
> > +  MOZ_ASSERT(aGlobalWindow && aGlobalWindow->IsInnerWindow());
> > +
> > +  gfx::VRManagerChild* vmc = gfx::VRManagerChild::Get();
> 
> nit: if you don't like to use |gfx::| in this file, I don't mind you to add
> |using namespace gfx;| between |namespace mozilla {| and |namespace dom {|
> with empty lines.
> 
> ::: dom/vr/VREventObserver.cpp:43
> (Diff revision 21)
> > +    mWindow->GetOuterWindow()->DispatchCustomEvent(
> > +      NS_LITERAL_STRING("vrdisplayconnected"));
> 
> Could you add |MOZ_ASSERT(nsContentUtils::IsSafeToRunScript())| before the
> call of DispatchCustomeEvent()?
> 
> ::: dom/vr/VREventObserver.cpp:52
> (Diff revision 21)
> > +    mWindow->GetOuterWindow()->DispatchCustomEvent(
> > +      NS_LITERAL_STRING("vrdisplaydisconnected"));
> 
> Same here.
> 
> ::: dom/vr/VREventObserver.cpp:60
> (Diff revision 21)
> > +  // Background tabs should not be allowed to see when a foreground tab
> > +  // has started or ended vr presentation.
> 
> Sounds like that background tab's window won't receive
> vrdisplaypresentchange event. However, it seems that if the state is
> different between when it becomes a background table and becomes a
> foreground tab again, doesn't the window want this event?
> 
> If so, I think that this class should store the state and notify the window
> of pending change events when it becomes a foreground tab.
> 
> Anyway, even if this is necessary, you should add another patch because it
> makes this patch complicated.
> 
> ::: dom/vr/VREventObserver.cpp:64
> (Diff revision 21)
> > +    mWindow->GetOuterWindow()->DispatchCustomEvent(
> > +      NS_LITERAL_STRING("vrdisplaypresentchange"));
> 
> And same here. (MOZ_ASSERT())
> 
> ::: gfx/vr/ipc/VRManagerChild.h:38
> (Diff revision 21)
> >                       , public layers::TextureForwarder
> >                       , public layers::ShmemAllocator
> >  {
> >  public:
> >    static VRManagerChild* Get();
> > +  
> 
> nit: Please remove this unnecessary (trailing) whitespaces before landing.
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:21
> (Diff revision 21)
> > +namespace {
> > +const nsTArray<RefPtr<dom::VREventObserver> >::index_type NoIndex =
> > +  nsTArray<RefPtr<dom::VREventObserver> >::NoIndex;
> > +} // namespace
> 
> nit: you don't need to take care old compilers with |> >| hack. Gecko's code
> already has |>>| in a lot of places.
> 
> And this is const value. So, I like kNoIndex (with |k| prefix).
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:476
> (Diff revision 21)
> > +  if (nsContentUtils::IsSafeToRunScript()) {
> > +    FireDOMVRDisplayConnectedEventInternal();
> > +  } else {
> > +    // If it's not safe to run scripts right now, schedule this to run later
> > +    nsContentUtils::AddScriptRunner(NewRunnableMethod(this,
> > +      &VRManagerChild::FireDOMVRDisplayConnectedEventInternal));
> > +  }
> 
> You don't need to check nsContentUtils::IsSafeToRunScript() by yourself
> because AddScriptRunner runs given runnable class immediately if it's safe.
> So, only the else block is enough.
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:488
> (Diff revision 21)
> > +  if (nsContentUtils::IsSafeToRunScript()) {
> > +    FireDOMVRDisplayDisconnectedEventInternal();
> > +  } else {
> > +    // If it's not safe to run scripts right now, schedule this to run later
> > +    nsContentUtils::AddScriptRunner(NewRunnableMethod(this,
> > +      &VRManagerChild::FireDOMVRDisplayDisconnectedEventInternal));
> > +  }
> 
> ditto.
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:500
> (Diff revision 21)
> > +  if (nsContentUtils::IsSafeToRunScript()) {
> > +    FireDOMVRDisplayPresentChangeEventInternal();
> > +  } else {
> > +    // If it's not safe to run scripts right now, schedule this to run later
> > +    nsContentUtils::AddScriptRunner(NewRunnableMethod(this,
> > +      &VRManagerChild::FireDOMVRDisplayPresentChangeEventInternal));
> 
> ditto.
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:509
> (Diff revision 21)
> > +void
> > +VRManagerChild::FireDOMVRDisplayConnectedEventInternal()
> > +{
> > +  for (auto& listener : mListeners) {
> > +    listener->NotifyVRDisplayConnected();
> > +  }
> > +}
> > +
> > +void
> > +VRManagerChild::FireDOMVRDisplayDisconnectedEventInternal()
> > +{
> > +  for (auto& listener : mListeners) {
> > +    listener->NotifyVRDisplayDisconnected();
> > +  }
> > +}
> > +
> > +void
> > +VRManagerChild::FireDOMVRDisplayPresentChangeEventInternal()
> > +{
> > +  for (auto& listener : mListeners) {
> > +    listener->NotifyVRDisplayPresentChange();
> > +  }
> >  }
> 
> I assume that these methods never cause DOM events at shutting down of XPCOM
> since mListeners should be empty in such case. If not, please manage the
> state with a flag or something like |mDestroyed|.
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:557
> (Diff revision 21)
> > +  mListeners.RemoveElement(aObserver);
> > +
> > +  if (mListeners.Length() == 0) {
> > +    Unused << SendSetHaveEventListener(false);
> > +  }
> 
> It seems that you *might* need to send this with synchronous IPC before
> removing the observer actually. If not, please drop this issue.
Flags: needinfo?(masayuki)
(Assignee)

Comment 222

9 months ago
(In reply to Bas Schouten (:bas.schouten) from comment #206)
> (In reply to :kip (Kearwood Gilbert) from comment #205)
> > I am not certain if the erraneous "r+" status on the Part 8 patch is
> > blocking you from getting notifications.  I have updated the patch with the
> > changes from your previous review.  Please advise if you will have time to
> > review my updated patch in time to land for Firefox 50?
> 
> When is FX50? I'm at SIGGRAPH now but I've already done part of the review
> and should be able to finish it before the end of the week. Let me know if
> you need it done earlier and I'll make sure it is.

The merge date will be 2016-08-01; however, it would need to land some time earlier I suppose.

I know the patch is quite big, so thank you for looking into it!

I've been reading some great papers coming from SIGGRAPH, tell me if you find anything interesting to apply to WebVR!
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review64340

With the last issue fix, r=masayuki.

::: gfx/vr/ipc/PVRManager.ipdl:53
(Diff revision 22)
>    // sensor state is the "Zero" position.
>    async ResetSensor(uint32_t aDisplayID);
>  
>    sync GetSensorState(uint32_t aDisplayID) returns(VRHMDSensorState aState);
>    sync GetImmediateSensorState(uint32_t aDisplayID) returns(VRHMDSensorState aState);
> -  async SetHaveEventListener(bool aHaveEventListener);
> +  sync SetHaveEventListener(bool aHaveEventListener);

I hope that this doesn't harm the performance. But if this is async, probably it causes race between the chrome process and the remote process.

::: gfx/vr/ipc/VRManagerChild.cpp:542
(Diff revision 22)
> +  mListeners.RemoveElement(aObserver);
> +
> +  if (mListeners.Length() == 0) {
> +    Unused << SendSetHaveEventListener(false);
> +  }

I think that you need to send to notify the chrome process of removing observer first, no? (i.e., when mListeners.Length() == 1, you should call SendSetHaveEventListener(false), then, you should remove the last listener after that.

With this code, even after mListeners becomes empty, the chrome process thinks that the remote process still have some listeners.
Attachment #8741953 - Flags: review?(masayuki) → review+
(Assignee)

Comment 224

9 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/19-20/
Attachment #8741953 - Attachment description: Bug 1250244 - Part 8: Implement WebVR DOM Events → Bug 1250244 - Part 8: Implement WebVR DOM Events,
(Assignee)

Comment 225

9 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/20-21/
(Assignee)

Comment 226

9 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/16-17/
(Assignee)

Comment 227

9 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/16-17/
(Assignee)

Comment 228

9 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/20-21/
(Assignee)

Comment 229

9 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/20-21/
(Assignee)

Comment 230

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/34-35/
(Assignee)

Comment 231

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/22-23/
(Assignee)

Comment 232

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/8-9/
(Assignee)

Comment 233

9 months ago
Rebased patches and fixed last issue in Masayuki's review of the part 8 patch.
https://reviewboard.mozilla.org/r/46871/#review64724

::: gfx/vr/ipc/VRManagerChild.cpp:542
(Diff revisions 22 - 23)
>      return; // doesn't exist
>    }
>  
> -  mListeners.RemoveElement(aObserver);
> -
>    if (mListeners.Length() == 0) {

As I said, you need to check if there is only one item to be removed.
(Assignee)

Comment 235

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/23-24/
(Assignee)

Comment 236

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/9-10/
(Assignee)

Comment 237

9 months ago
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #234)
> https://reviewboard.mozilla.org/r/46871/#review64724
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:542
> (Diff revisions 22 - 23)
> >      return; // doesn't exist
> >    }
> >  
> > -  mListeners.RemoveElement(aObserver);
> > -
> >    if (mListeners.Length() == 0) {
> 
> As I said, you need to check if there is only one item to be removed.

Good catch, thanks!  I've corrected it again.
(Assignee)

Comment 238

9 months ago
Changes in bug 1282364 have broken this patch set.  I'll update the Part 7 patch to match.
(Assignee)

Comment 239

9 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/20-21/
(Assignee)

Comment 240

9 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/21-22/
(Assignee)

Comment 241

9 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/17-18/
(Assignee)

Comment 242

9 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/17-18/
(Assignee)

Comment 243

9 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/21-22/
(Assignee)

Comment 244

9 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/21-22/
(Assignee)

Comment 245

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/35-36/
(Assignee)

Comment 246

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/24-25/
(Assignee)

Comment 247

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/10-11/
(Assignee)

Comment 248

9 months ago
(In reply to :kip (Kearwood Gilbert) from comment #238)
> Changes in bug 1282364 have broken this patch set.  I'll update the Part 7
> patch to match.
I have re-based the patch set and updated the part 7 patch to match the changes in Bug 1282364.
I'm about half-way through reviewing this and will do the rest tomorrow, note that I'm not very knowledgeable on WebIDL and it would probably be a good idea to get another set of eyes on that.
Attachment #8722648 - Flags: review?(bas) → review+
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review65254

In the future, it would be really nice if patches moving things around and renaming things are split up separarely. That makes it way harder to miss real changes embedded in bits of the code where a lot of renaming is going on. I've gone a little easy on bits of the code that are only actually touched by VR users (which is most), I'm hoping all this added IPDL doesn't add renewed shutdown issues for those users. That sort of stuff is hard to predict.

::: dom/html/HTMLCanvasElement.cpp:1413
(Diff revision 36)
> +
> +already_AddRefed<layers::SharedSurfaceTextureClient>
> +HTMLCanvasElement::GetVRFrame()
> +{
> +  if (GetCurrentContextType() != CanvasContextType::WebGL1
> +      && GetCurrentContextType() != CanvasContextType::WebGL2) {

nit: I believe our coding style requires && on the end of the previous line.

::: dom/vr/VRDisplay.cpp:113
(Diff revision 36)
> +  aDisplays = displays;
>  }
>  
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(VRFieldOfView, mParent)
> +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(VRFieldOfView, AddRef)
> +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(VRFieldOfView, Release)

We have to be very careful here, since final Cycle Collection occurs after layers & gfx shutdown, we -have- to ensure no graphics resources are being kept alive by this after a certain shutdown stage and must implement shutdown observers to prevent this if it would be the case. (as far as I can tell, for the moment, it's not)

::: gfx/layers/d3d11/TextureD3D11.cpp:702
(Diff revision 36)
>  DXGITextureHostD3D11::Lock()
>  {
> -  if (!mCompositor) {
> -    NS_WARNING("no suitable compositor");
> -    return false;
> -  }
> +  /**
> +   * Note: This function may be called when mCompositor is null
> +   *       such as during WebVR frame submission.
> +   **/

Hrm, I wonder if this will give problems during Device Reset, have you tested this?

::: gfx/layers/d3d11/TextureD3D11.cpp:702
(Diff revision 36)
>  DXGITextureHostD3D11::Lock()
>  {
> -  if (!mCompositor) {
> -    NS_WARNING("no suitable compositor");
> -    return false;
> -  }
> +  /**
> +   * Note: This function may be called when mCompositor is null
> +   *       such as during WebVR frame submission.
> +   **/

Hrm, I wonder if this will give problems during Device Reset, have you tested this?

::: gfx/vr/VRManager.h:65
(Diff revision 36)
>  
> -  typedef nsTArray<RefPtr<VRHMDManager>> VRHMDManagerArray;
> -  VRHMDManagerArray mManagers;
> +  typedef nsTArray<RefPtr<VRDisplayManager>> VRDisplayManagerArray;
> +  VRDisplayManagerArray mManagers;
>  
> -  typedef nsRefPtrHashtable<nsUint32HashKey, gfx::VRHMDInfo> VRHMDInfoHashMap;
> -  VRHMDInfoHashMap mVRDisplays;
> +  typedef nsRefPtrHashtable<nsUint32HashKey, gfx::VRDisplayHost> VRDisplayHostHashMap;
> +  VRDisplayHostHashMap mVRDisplays;

Personally I'd much rather see us use std::vector here, but that might just be me.

::: gfx/vr/gfxVROculus.cpp:483
(Diff revision 36)
> -    ovrTextureSwapChainDesc desc;
> +  ovrTextureSwapChainDesc desc;
> -    memset(&desc, 0, sizeof(desc));
> +  memset(&desc, 0, sizeof(desc));
> -    desc.Type = ovrTexture_2D;
> +  desc.Type = ovrTexture_2D;
> -    desc.ArraySize = 1;
> +  desc.ArraySize = 1;
> -    desc.Format = OVR_FORMAT_B8G8R8A8_UNORM_SRGB;
> +  desc.Format = OVR_FORMAT_B8G8R8A8_UNORM_SRGB;
> -    desc.Width = aSize.width;
> +  desc.Width = mDisplayInfo.mEyeResolution.width * 2;

Although this is obvious to us. Maybe somehow annotate this differently, it's not necessarily obvious to everyone the presentation format is a backbuffer with both eyes horizontally next to each other. (even a comment could be good enough)

::: gfx/vr/gfxVROculus.cpp:505
(Diff revision 36)
> +    NS_WARNING("Failed to get immediate context for Oculus");
> +    return;
>    }
> -#endif
>  
> -  if (aCompositor->GetBackendType() == layers::LayersBackend::LAYERS_OPENGL) {
> +  if (!SUCCEEDED(mDevice->CreateVertexShader(sLayerQuadVS.mData, sLayerQuadVS.mLength, nullptr, &mQuadVS))) {

nit: Is there a reason to use !SUCCEEDED instead of FAILED? Personally I prefer FAILED. Also we should consider using gfxWarning would be my personal preference in gfx/ but I'll leave that up to you.

::: gfx/vr/ipc/VRLayerChild.cpp:71
(Diff revision 36)
> +
> +  mFront = mShSurfClient;
> +  mShSurfClient = nullptr;
> +
> +  mFront->SetAddedToCompositableClient();
> +  //mFront->SyncWithObject(VRManagerChild::Get()->GetSyncObject());

This seems to be left-over debug code. Although technically I believe SyncWithObject should probably be there (in practice it won't matter for this texture client type). FinalizeFrame definitely should not.

::: gfx/vr/ipc/VRManagerChild.h:48
(Diff revision 36)
>    static void InitWithGPUProcess(Endpoint<PVRManagerChild>&& aEndpoint);
>    static bool InitForContent(Endpoint<PVRManagerChild>&& aEndpoint);
>    static void ShutDown();
>  
>    static bool IsCreated();
> +  

nit: whitespace

::: gfx/vr/ipc/VRManagerChild.cpp:206
(Diff revision 36)
>      }
>    }
>  
> -  mDevices = devices;
> +  // mDisplays could be a hashed container for more scalability, but not worth
> +  // it now as we expect < 10 entries.
> +  nsTArray<RefPtr<VRDisplayClient> > displays;

nit: We no longer support compilers which don't accept just doing >>

::: gfx/vr/ipc/VRManagerChild.cpp:207
(Diff revision 36)
>    }
>  
> -  mDevices = devices;
> +  // mDisplays could be a hashed container for more scalability, but not worth
> +  // it now as we expect < 10 entries.
> +  nsTArray<RefPtr<VRDisplayClient> > displays;
> +  for (auto& displayUpdate: aDisplayUpdates) {

Personally, not a big van of using auto here when the type is so short, that doesn't improve readability.

::: gfx/vr/ipc/VRManagerChild.cpp:413
(Diff revision 36)
> +
> +  bool alreadyRegistered = !mFrameRequestCallbacks.IsEmpty();
> +  DebugOnly<FrameRequest*> request =
> +    mFrameRequestCallbacks.AppendElement(FrameRequest(aCallback, newHandle));
> +  NS_ASSERTION(request, "This is supposed to be infallible!");
> +  if (!alreadyRegistered) {

I think you didn't mean to have this here. Or you meant to have the lines above this inside here.
(Assignee)

Comment 251

9 months ago
https://reviewboard.mozilla.org/r/36163/#review65254

> We have to be very careful here, since final Cycle Collection occurs after layers & gfx shutdown, we -have- to ensure no graphics resources are being kept alive by this after a certain shutdown stage and must implement shutdown observers to prevent this if it would be the case. (as far as I can tell, for the moment, it's not)

The gfx resources are cleared during gfx shutdown even if these objects are still allocated.

> Personally I'd much rather see us use std::vector here, but that might just be me.

This is a remnant from prior refactoring.  Rationale for using the hashmap is that due to hardware hot-swapping, there will not always be a 1:1 relationship between the vector index and the displayid used for the entry.  The benefits of the hashmap are limited; however, as there is expected to be < 10 entries in the most extreme case.  Iterating over the small std::vector is probably faster than the hashmap.

In the pre-WebVR 1.0 API, this map was also going to hold individual sensors and controllers which could greatly increase the length of the array.  This is no longer the case as these have been moved out to Gamepad API.

I have added a separate bug to clean this up:

Bug 1291434 - [webvr] Replace nsRefPtrHashtable with std::vector for VRManager::mVRDisplays

> Although this is obvious to us. Maybe somehow annotate this differently, it's not necessarily obvious to everyone the presentation format is a backbuffer with both eyes horizontally next to each other. (even a comment could be good enough)

I have added comments describing how content describes the desired presentation format.  A separate bug captures an opportunity to optimize the resolution of this buffer based on the format requested by content:

Bug 1291443 - [webvr] Dynamically resize the Oculus render buffer to optimize for content VRLayer eye rectangles

> This seems to be left-over debug code. Although technically I believe SyncWithObject should probably be there (in practice it won't matter for this texture client type). FinalizeFrame definitely should not.

Have uncommented call to SyncWithObject and removed call to FinalizeFrame.

> I think you didn't mean to have this here. Or you meant to have the lines above this inside here.

Dead code no longer needed.  Originally had something special to do for the first one added.
(Assignee)

Comment 252

9 months ago
https://reviewboard.mozilla.org/r/36163/#review65254

> Hrm, I wonder if this will give problems during Device Reset, have you tested this?

Oculus will only be used with DirectX 11 and can only be initialized with specific NVidia and AMD graphics drivers on discrete GPUs only.

Microsoft lists the cases that can cause a Device Removal with DX11:

https://msdn.microsoft.com/en-us/windows/uwp/gaming/handling-device-lost-scenarios

As this code can only be hit on discrete desktop GPU's, the chances are quite rare.  For the remaining cases, the Oculus compositor runtime appears to shut down or crashes if in use.

- The graphics driver is upgraded.
  - This is theoretically possible; however, the user would almost certainly be removing their headset to do the update and the Oculus software would need to restart, disconnecting the HMD.
- The system changes from a power-saving graphics adapter to a performance graphics adapter.
  - Oculus only supports discrete NVidia and AMD graphics and will not initialize on laptops or Intel GPUs.
- The graphics device stops responding and is reset.
  - Unfortunately in these cases, it appears that the current Oculus compositor runtimes will also crash.
- A graphics adapter is physically attached or removed.
  - Oculus does not officially support this; however, it is theoretically possible if someone is using an external Thunderbolt GPU.
(Assignee)

Comment 253

9 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/21-22/
(Assignee)

Comment 254

9 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/22-23/
(Assignee)

Comment 255

9 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/18-19/
(Assignee)

Comment 256

9 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/18-19/
(Assignee)

Comment 257

9 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/22-23/
(Assignee)

Comment 258

9 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/22-23/
(Assignee)

Comment 259

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/36-37/
(Assignee)

Comment 260

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/25-26/
(Assignee)

Comment 261

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/11-12/
(Assignee)

Comment 262

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/37-38/
(Assignee)

Comment 263

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/26-27/
(Assignee)

Comment 264

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/12-13/
(Assignee)

Comment 265

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=659d1590535c
(Assignee)

Comment 266

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/38-39/
(Assignee)

Comment 267

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/27-28/
(Assignee)

Comment 268

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/13-14/
(Assignee)

Comment 269

9 months ago
I have fixed a static analysis error (duplicate mRefCnt) found in a try push.  I've fixed it and am running a new try run as a sanity check before committing.
(Assignee)

Comment 270

9 months ago
Comment on attachment 8741952 [details]
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46869/diff/22-23/
Attachment #8722648 - Attachment description: Bug 1250244 - Part 7: Implement WebVR 1.0 API → Bug 1250244 - Part 7: Implement WebVR 1.0 API,
Attachment #8770318 - Attachment description: Bug 1250244 - Part 9: Update test_interfaces.html → Bug 1250244 - Part 9: Update test_interfaces.html,
(Assignee)

Comment 271

9 months ago
Comment on attachment 8741942 [details]
Bug 1250244 - Part 2: Remove old VR rendering paths,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46859/diff/23-24/
(Assignee)

Comment 272

9 months ago
Comment on attachment 8757452 [details]
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55870/diff/19-20/
(Assignee)

Comment 273

9 months ago
Comment on attachment 8757453 [details]
Bug 1250244 - Part 4: Remove Cardboard VR Support,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55872/diff/19-20/
(Assignee)

Comment 274

9 months ago
Comment on attachment 8741940 [details]
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46855/diff/23-24/
(Assignee)

Comment 275

9 months ago
Comment on attachment 8741941 [details]
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46857/diff/23-24/
(Assignee)

Comment 276

9 months ago
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36163/diff/39-40/
(Assignee)

Comment 277

9 months ago
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46871/diff/28-29/
(Assignee)

Comment 278

9 months ago
Comment on attachment 8770318 [details]
Bug 1250244 - Part 9: Update test_interfaces.html,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63782/diff/14-15/
(Assignee)

Updated

9 months ago
Attachment #8722648 - Flags: review?(bzbarsky)
Attachment #8741953 - Flags: review?(bzbarsky)
(Assignee)

Comment 279

9 months ago
I was unable to commit due to a hook that warned me that I need DOM peer review on the webidl files.

bz: Would you be able to review the webidl changes in Patch 7 and 8?

The spec containing the webidl changes:

https://w3c.github.io/webvr/
Flags: needinfo?(masayuki) → needinfo?(bzbarsky)
(Assignee)

Updated

9 months ago
Blocks: 1291777
(Assignee)

Updated

9 months ago
Blocks: 1260937
Attachment #8722648 - Flags: review?(bzbarsky) → review-
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

Note that I did not really review the impl except for the parts the IDL-generated bindings would call into, and even that was mostly a cursory overview...

::: dom/vr/VRDisplay.h:128
(Diff revision 40)
>  
>    double mTimeStamp;
> +  uint32_t mFrameId;
>    gfx::VRHMDSensorState mVRState;
>  
> -  RefPtr<DOMPoint> mPosition;
> +  JS::Heap<JSObject*> mPosition;

You need to trace all your `JS::Heap<JSObject*>` members; otherwise the GC will happily collect them for you.

That means expanding out `NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE`, then expanding out `NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE`, then nulling out the members in the unlink bits and tracing (via `NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK`) the memebers inside the trace bits.

See what the `Exception` class in DOMException.cpp does, though you can pass multiple pointers to `NS_IMPL_CYCLE_COLLECTION_TRAVERSE`/`NS_IMPL_CYCLE_COLLECTION_UNLINK`.

::: dom/vr/VRDisplay.cpp:147
(Diff revision 40)
>  }
>  
> -VRFieldOfView*
> -VREyeParameters::MaximumFieldOfView()
> +void
> +VREyeParameters::GetOffset(JSContext* aCx, JS::MutableHandle<JSObject*> aRetval)
>  {
> -  return mMaxFOV;
> +  if (mOffset == nullptr) {

You can just do `if (mOffset) {`, no?

::: dom/vr/VRDisplay.cpp:149
(Diff revision 40)
> -VRFieldOfView*
> -VREyeParameters::MaximumFieldOfView()
> +void
> +VREyeParameters::GetOffset(JSContext* aCx, JS::MutableHandle<JSObject*> aRetval)
>  {
> -  return mMaxFOV;
> +  if (mOffset == nullptr) {
> +    // Lazily create the Float32Array
> +    mOffset = dom::Float32Array::Create(aCx, this, 3, mEyeTranslation.components);

Create() can return null on OOM (and leave an exception on the JSContext in the process).  If it does, you want to NoteJSContextException on the ErrorResult bindings pass you, which you should have because this getter should be marked [Throws].

Same thing for all the other getters that call Float32Array::Create.

::: dom/webidl/VRDisplay.webidl:27
(Diff revision 40)
> - Constructor(optional VRFieldOfViewInit fov),
> - Constructor(double upDegrees, double rightDegrees, double downDegrees, double leftDegrees)]
> -interface VRFieldOfView : VRFieldOfViewReadOnly {
> -  inherit attribute double upDegrees;
> -  inherit attribute double rightDegrees;
> -  inherit attribute double downDegrees;
> +dictionary VRLayer {
> +  /**
> +   * XXX - When WebVR in WebWorkers is implemented, HTMLCanvasElement below
> +   *       should be replaced with VRSource.
> +   */
> +  HTMLCanvasElement? source = null;

This IDL (which you faithfully copied from the spec) doesn't make any sense, especially when paired with the prose in the spec.

First of all, having no source for a VRLayer makes no sense, as I understand this stuff.

Second, https://w3c.github.io/webvr/#vrlayer-attributes says: "The leftBounds MUST default to [0.0, 0.0, 0.5, 1.0]." but default _when_?  Why is null a valid value to pass here at all?

It seems to me that the right IDL should be something like this:

    dictionary VRLayer {
      required HTMLCanvasElement source; // or VRSource, whatever
      sequence<float> leftBounds;
      sequence<float> rightBounds;
    };

and then https://w3c.github.io/webvr/#dom-vrdisplay-requestpresent needs to define an actual processing model that describes what happens when leftBounds/rightBounds are missing.  Or what happens if the passed-in sequences don't have 4 values, for that matter.  And whether the return value from getLayers returns stuff fixed up with the default values for leftBounds/rightBounds or the original (missing, or too short, or whatever) value that was passed in.  Please raise a spec issue to this effect.

I guess you could use a default value of [] if you want to handle "missing" and zero-length list the same way.  Again, the spec actually needs to define how they're handled.

::: dom/webidl/VRDisplay.webidl:68
(Diff revision 40)
> +   * or update non-VR UI because that content will not be visible.
> +   */
> +  readonly attribute boolean hasExternalDisplay;
> +
> +  /**
> +   * Whether or not the VRDisplay is capable of presenting content to an HMD or similar device.

s/or not //

::: dom/webidl/VRDisplay.webidl:71
(Diff revision 40)
> +
> +  /**
> +   * Whether or not the VRDisplay is capable of presenting content to an HMD or similar device.
> +   * Can be used to indicate “magic window” devices that are capable of 6DoF tracking but for
> +   * which requestPresent is not meaningful. If false then calls to requestPresent should
> +   * always fail, and getEyeParameters should return null. 

Trailing space here.

::: dom/webidl/VRDisplay.webidl:90
(Diff revision 40)
> + */
>  [Pref="dom.vr.enabled",
>   HeaderFile="mozilla/dom/VRDisplay.h"]
> -interface VREyeParameters {
> -  /* These values are expected to be static per-device/per-user */
> -  [Constant, Cached] readonly attribute VRFieldOfView minimumFieldOfView;
> +interface VRStageParameters {
> +  /**
> +   * A 16-element array containing the components of a 4x4 transform

This is what the spec says... but that's not enough to figure out what the value should actually be.  Is this list in column-major order or row-major order?  Or equivalently, is it acting on row vectors or column vectors?

Also, the vectors this acts on are length t3 vectors.  Presumably the idea is that this is an affine transform matrix, and to get its action on the vector you extend it with a "1", act, and then drop the fourth entry.  But none of this is in the spec, and it needs to be.  Again, please raise issues.

Also, is there a reason this is not a DOMMatrix?  Again, that's a spec-level issue...

::: dom/webidl/VRDisplay.webidl:114
(Diff revision 40)
>  };
>  
> -[Pref="dom.vr.enabled"]
> -interface VRDisplay {
> +[Pref="dom.vr.enabled",
> + HeaderFile="mozilla/dom/VRDisplay.h"]
> +interface VRPose {
> +  readonly attribute DOMHighResTimeStamp timestamp;

The spec doesn't say what this timestamp represents, and in particular whether it's a difference of two other timestamps or time since the time origin, and if so which global's time origin.  For example, is this timestamp comparable to `Performance.now()` values?

Looking at our impl, it looks like this value eventually comes from a PR_Now() value divided by 1e6.  This has two problems: 1) It's not relative to the time origin, nor a difference of two timestamps, and 2) it's not a monotonic clock like the spec says.

I guess this is preexisting brokenness, but please file a spec bug to define the timebase here, and an implementation bug on us to use a monotonic clock.  How the latter gets done depends on how the spec defines its timebase, of course.

::: dom/webidl/VRDisplay.webidl:121
(Diff revision 40)
>    /**
> -   * An identifier for the distinct hardware unit that this
> -   * VR Device is a part of.  All VRDisplay/Sensors that come
> -   * from the same hardware will have the same hardwareId
> +   * position, linearVelocity, and linearAcceleration are 3-component vectors.
> +   * position is relative to a sitting space. Transforming this point with
> +   * VRStageParameters.sittingToStandingTransform converts this to standing space.
>     */
> -  [Constant] readonly attribute DOMString hardwareUnitId;
> +  readonly attribute Float32Array? position;

Is there a reason this is not a DOMPoint?  Again, that's a spec-level issue.

::: dom/webidl/VRDisplay.webidl:140
(Diff revision 40)
>    /**
> -   * An identifier for this distinct sensor/device on a physical
> -   * hardware device.  This shouldn't change across browser
> -   * restrats, allowing configuration data to be saved based on it.
> +   * offset is a 3-component vector representing an offset to
> +   * translate the eye. This value may vary from frame
> +   * to frame if the user adjusts their headset ipd.
>     */
> -  [Constant] readonly attribute DOMString deviceId;
> +  [Constant, Cached] readonly attribute Float32Array offset;

Why is this cached?  I don't see a reason for it to be cached, nor for the other things on this interface.  Making them `[Constant]` does make sense, if they are, of course.

The old code used to do `[Cached]` stuff because the return value types were dictionaries, which do have to be cached in the binding layer because they're pass by value, not pass by reference.  But all your stff on this new interface version is either pass by reference or turns into JS primitives, which are also pass by value, so doesn't need caching.

::: dom/webidl/VRDisplay.webidl:170
(Diff revision 40)
> +
> +  /**
> +   * If this VRDisplay supports room-scale experiences, the optional
> +   * stage attribute contains details on the room-scale parameters.
> +   */
> +  readonly attribute VRStageParameters? stageParameters;

Please raise a spec issue.  The spec needs to describe clearly when this is null and when it's not null, and when the value can change.  You're not marking this [Constant], so I assume it _can_ change.

::: dom/webidl/VRDisplay.webidl:188
(Diff revision 40)
> -
> -[Pref="dom.vr.enabled" ,
> - HeaderFile="mozilla/dom/VRDisplay.h"]
> -interface PositionSensorVRDisplay : VRDisplay {
> -  /*
> -   * Return a VRPositionState dictionary containing the state of this position sensor
> +   */
> +  [Constant] readonly attribute DOMString displayName;
> +
> +  /**
> +   * Return a VRPose containing the future predicted pose of the VRDisplay
> +   * when the current frame will be presented. The value returned will not

This comment, and the spec's normative "MUST NOT change" text is inconsistent with the [NewObject] annotation.  Please file a spec issue to clarify what it means by "MUST NOT change", if it doesn't mean that each call will produce the same object (in terms of object identity) until control is returned to the browser.  We seem to be creating a new object each time, and giving it new typed arrays as members, so the only thing that "does not change" is the actual values in those typed arrays, and the timestamp....

::: dom/webidl/VRDisplay.webidl:197
(Diff revision 40)
> -   * and acceleration of each of these properties.  Use "hasPosition" and "hasOrientation"
> -   * to check if the associated members are valid; if these are false, those members
> -   * will be null.
> +   * and acceleration of each of these properties.
> +   */
> +  [NewObject] VRPose getPose();
> +
> +  /**
> +   * Return the current instantaneous pose of the VRDisplay, with no

So just to make sure... this one _can_ change before JS gives up control?
Attachment #8741953 - Flags: review?(bzbarsky) → review-
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review66538

::: dom/events/EventListenerManager.cpp:429
(Diff revision 29)
>               aTypeAtom == nsGkAtoms::oncompositionupdate ||
>               aTypeAtom == nsGkAtoms::oninput) {
>      if (!aFlags.mInSystemGroup) {
>        mMayHaveInputOrCompositionEventListener = true;
>      }
> +  } else if (aTypeAtom == nsGkAtoms::onvrdisplayconnected ||

Why can't this use the existing EventListenerAdded hook we have, which is called later in this method?  The only target these events will fire on is the window itself, right?  Or are there other targets that the listeners can get added to and then require notifying the window?

::: dom/webidl/Window.webidl:507
(Diff revision 29)
>    [Throws, Func="nsGlobalWindow::IsPrivilegedChromeWindow"]
>    void beginWindowMove(Event mouseDownEvent, optional Element? panel = null);
>  };
>  
> +partial interface Window {
> +  [Pref="dom.vr.enabled"]

Per spec, these first two should be named vrdisplayconnect and vrdisplaydisconnect.  Why are we using different names?

Also what about vrdisplayactivate and vrdisplaydeactivate?  Worth at least a comment in the IDL...

::: gfx/vr/ipc/VRManagerChild.cpp:535
(Diff revision 29)
> +void
> +VRManagerChild::RemoveListener(dom::VREventObserver* aObserver)
> +{
> +  MOZ_ASSERT(aObserver);
> +
> +  if (mListeners.IndexOf(aObserver) == kNoIndex) {

Might be better to save the index returned from this call and then use RemoveElementAt, so we don't have to search the list twice.

Or juts RemoveElement and then if mListeners.IsEmpty(), do the SendSetHaveEventListener bit.
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

9 months ago
Blocks: 1293333
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 291

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> You need to trace all your `JS::Heap<JSObject*>` members; otherwise the GC will happily collect them for you.
> 
> That means expanding out `NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE`, then expanding out `NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE`, then nulling out the members in the unlink bits and tracing (via `NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK`) the memebers inside the trace bits.
> 
> See what the `Exception` class in DOMException.cpp does, though you can pass multiple pointers to `NS_IMPL_CYCLE_COLLECTION_TRAVERSE`/`NS_IMPL_CYCLE_COLLECTION_UNLINK`.

Fixed.  Would appreciate if you could verify that I did the right thing here, thanks!

> Create() can return null on OOM (and leave an exception on the JSContext in the process).  If it does, you want to NoteJSContextException on the ErrorResult bindings pass you, which you should have because this getter should be marked [Throws].
> 
> Same thing for all the other getters that call Float32Array::Create.

Also added [Throws] to all of the attributes returning a Float32Array.

> Why is this cached?  I don't see a reason for it to be cached, nor for the other things on this interface.  Making them `[Constant]` does make sense, if they are, of course.
> 
> The old code used to do `[Cached]` stuff because the return value types were dictionaries, which do have to be cached in the binding layer because they're pass by value, not pass by reference.  But all your stff on this new interface version is either pass by reference or turns into JS primitives, which are also pass by value, so doesn't need caching.

Misguided intent to maintain performance.  I've removed the [Cached] where not necessary.
> Misguided intent to maintain performance.

Are the relevant getters likely to be called enough that the difference between 5 machine instructions and 100 machine instructions is important?  If so, [Cached] can be useful for performance...  If not, probably not.
(Assignee)

Comment 293

9 months ago
(In reply to Boris Zbarsky [:bz] from comment #292)
> > Misguided intent to maintain performance.
> 
> Are the relevant getters likely to be called enough that the difference
> between 5 machine instructions and 100 machine instructions is important? 
> If so, [Cached] can be useful for performance...  If not, probably not.

These getters will be used within rendering engines based on WebGL.  It's possible that such an engine may be calling these on every draw call, but most likely the value is being copied off to some other engine-specific structure or uploaded to the GPU in a uniform value that is recycled between draw calls.
> It's possible that such an engine may be calling these on every draw call

How often do those happen?
(Assignee)

Comment 295

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> Please raise a spec issue.  The spec needs to describe clearly when this is null and when it's not null, and when the value can change.  You're not marking this [Constant], so I assume it _can_ change.

This is intended to be null when the VRDisplay does not support room-scale experiences.  When room-scale experiences are possible with the VRDisplay, the value will be non-null.

In our implementation, this won't change from null to non-null or vice-versa once a VRDisplay is enumerated; however, it is theoretically possible that the user re-calibrates the room setup for their device while a page is open.
(Assignee)

Comment 296

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> This comment, and the spec's normative "MUST NOT change" text is inconsistent with the [NewObject] annotation.  Please file a spec issue to clarify what it means by "MUST NOT change", if it doesn't mean that each call will produce the same object (in terms of object identity) until control is returned to the browser.  We seem to be creating a new object each time, and giving it new typed arrays as members, so the only thing that "does not change" is the actual values in those typed arrays, and the timestamp....

More precisely, the values of the pose must not change.  The "VRPose" is considered a snapshot of the sampled (GetImmediatePose) or predicted (GetPose) sensor state.

Additionally, "until JavaScript has returned control to the browser" is not accurate.  Javascript may in fact give up control to the browser and later call "VRDisplay.submitFrame" later.  The predicted VRPose values will be the same for each frame, which is not advanced through vsync.  It is advanced only through VRDisplay.submitFrame.

I agree that normative text is misleading here.
(Assignee)

Comment 297

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> So just to make sure... this one _can_ change before JS gives up control?

Exactly.  This will almost certainly return a different value on every call, even within a single frame.
(Assignee)

Comment 298

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> This IDL (which you faithfully copied from the spec) doesn't make any sense, especially when paired with the prose in the spec.
> 
> First of all, having no source for a VRLayer makes no sense, as I understand this stuff.
> 
> Second, https://w3c.github.io/webvr/#vrlayer-attributes says: "The leftBounds MUST default to [0.0, 0.0, 0.5, 1.0]." but default _when_?  Why is null a valid value to pass here at all?
> 
> It seems to me that the right IDL should be something like this:
> 
>     dictionary VRLayer {
>       required HTMLCanvasElement source; // or VRSource, whatever
>       sequence<float> leftBounds;
>       sequence<float> rightBounds;
>     };
> 
> and then https://w3c.github.io/webvr/#dom-vrdisplay-requestpresent needs to define an actual processing model that describes what happens when leftBounds/rightBounds are missing.  Or what happens if the passed-in sequences don't have 4 values, for that matter.  And whether the return value from getLayers returns stuff fixed up with the default values for leftBounds/rightBounds or the original (missing, or too short, or whatever) value that was passed in.  Please raise a spec issue to this effect.
> 
> I guess you could use a default value of [] if you want to handle "missing" and zero-length list the same way.  Again, the spec actually needs to define how they're handled.

Regarding behavior with no source, an issue has been raised in the spec:

https://github.com/w3c/webvr/issues/58

Our implementation currently ignores these layers, maching the functionality of Chrome until this is decided.
(Assignee)

Comment 299

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> Regarding behavior with no source, an issue has been raised in the spec:
> 
> https://github.com/w3c/webvr/issues/58
> 
> Our implementation currently ignores these layers, maching the functionality of Chrome until this is decided.

Perhaps the wrong terminology was used in the prose regarding "default". (Which I wrote...)

"The leftBounds MUST default to [0.0, 0.0, 0.5, 1.0]"

What I meant to say is that null values should be treated as [0.0, 0.0, 0.5, 1.0].

I'll file an issue in the spec to change the text.
(Assignee)

Comment 300

9 months ago
(In reply to Boris Zbarsky [:bz] from comment #294)
> > It's possible that such an engine may be calling these on every draw call
> 
> How often do those happen?

Current VR displays run at 90hz and 500 to 5,000 draw calls per frame are typical.

So effectively 45,000 to 450,000 times per second.
OK.  So at 30ns per uncached get (which is about as slow as I expect it to go on laptop or desktop hardware), we're talking about 13.5ms at the high end there, out of an entire second.  At the low end we're talking 1.5ms.  Probably not worth caching, though it would be interesting to measure this in a real-life scenario.
(Assignee)

Comment 302

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> This is what the spec says... but that's not enough to figure out what the value should actually be.  Is this list in column-major order or row-major order?  Or equivalently, is it acting on row vectors or column vectors?
> 
> Also, the vectors this acts on are length t3 vectors.  Presumably the idea is that this is an affine transform matrix, and to get its action on the vector you extend it with a "1", act, and then drop the fourth entry.  But none of this is in the spec, and it needs to be.  Again, please raise issues.
> 
> Also, is there a reason this is not a DOMMatrix?  Again, that's a spec-level issue...

I'll file an issue on the spec to clarify the order of the components within the transform and regarding the homogenous coordinates expected.

DOMMatrix and others defined in the geometry API were initially considered, but was veto'ed.

In the majority of the use cases, the values returned by the WebVR API will be used directly by WebGL shaders as uniform values.

WebGL uniform values are uploaded as Float32Arrays:

https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.8
> In our implementation, this won't change from null to non-null or vice-versa
> once a VRDisplay is enumerated; however, it is theoretically possible that
> the user re-calibrates the room setup for their device while a page is open.

Right, the spec just needs to make this clear.

> More precisely, the values of the pose must not change.  The "VRPose" is
> considered a snapshot of the sampled (GetImmediatePose) or predicted
> (GetPose) sensor state.

Again, the spec needs to make this clear.  Please do file spec issues.

> What I meant to say is that null values should be treated as [0.0, 0.0, 0.5, 1.0].

Why are we allowing null at all?  I don't see any reason to allow that offhand; we should either allow nothing passed or allow a sequence, but allowing null is weird.

As for the default value when nothing is passed, the way this _should_ work is that the actual processing model text at https://w3c.github.io/webvr/#dom-vrdisplay-requestpresent should describe the processing model.  Again, please raise a spec issue as needed.
(Assignee)

Comment 304

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> The spec doesn't say what this timestamp represents, and in particular whether it's a difference of two other timestamps or time since the time origin, and if so which global's time origin.  For example, is this timestamp comparable to `Performance.now()` values?
> 
> Looking at our impl, it looks like this value eventually comes from a PR_Now() value divided by 1e6.  This has two problems: 1) It's not relative to the time origin, nor a difference of two timestamps, and 2) it's not a monotonic clock like the spec says.
> 
> I guess this is preexisting brokenness, but please file a spec bug to define the timebase here, and an implementation bug on us to use a monotonic clock.  How the latter gets done depends on how the spec defines its timebase, of course.

"timestamp Monotonically increasing value that allows the author to determine if position state data been updated from the hardware. Since values are monotonically increasing, they can be compared to determine the ordering of updates, as newer values will always be greater than or equal to older values."

The intent of this wording is that content should not make assumptions about the origin of the timestamp values.  It should only be used for comparison to determine the ordering or duration between pose samples.

These timestamp values in our implementation are currently originating from PR_Now(); however, in some cases an implementation may report timestamp values taken from hardware without non-volatile memory.

Perhaps the wording could be extended to say that "timestamp values from one VRDisplay must only be compared with timestamp values of the same VRDisplay"?
> Fixed.  Would appreciate if you could verify that I did the right thing here, thanks!

Looks good to me. (re: tracing gc things.)
(Assignee)

Comment 306

9 months ago
(In reply to Boris Zbarsky [:bz] from comment #301)
> OK.  So at 30ns per uncached get (which is about as slow as I expect it to
> go on laptop or desktop hardware), we're talking about 13.5ms at the high
> end there, out of an entire second.  At the low end we're talking 1.5ms. 
> Probably not worth caching, though it would be interesting to measure this
> in a real-life scenario.

1.5ms is a very long time in a real-time rendering context; however, if someone is implementing more than a trivial "hello world" scene, I would expect them to follow WebGL best practices such as "reducing state changes".  This would imply that their engine should fetch the value once, bind it to a uniform value, and re-use it for multiple draw calls.

I don't think there is much benefit to [Cached] for well written rendering engines on WebGL which would be only accessing this at 90hz.
> The intent of this wording is that content should not make assumptions about the origin of the timestamp values.

The best way to ensure this would be to use a random origin of some sort for the timestamp values.  What we do now will absolutely lead content to assume that they can compare these values to Date.now(), no?

What do other UAs do here in practice?

> These timestamp values in our implementation are currently originating from PR_Now()

PR_Now is not a monotonically increasing value, so its use here is broken anyway.  As I said, that's a preexisting issue.

> Perhaps the wording could be extended to say that

The wording in the spec can be all you want, but that won't affect how people use the API.  In practice, if all UAs agree that the values returned are more or less Unix epoch times, content _will_ depend on that.  If that's not desirable, don't return Unix epoch times.

Various timestamps in the platform use the document timeline as their time origin.  If you don't want that, you presumably want to define, in some consistent way, a time origin per VRDisplay.  But I will bet you $50 people will want to compare VRDisplay times to document timeline times in the end...
> 1.5ms is a very long time in a real-time rendering context

This is 1.5ms per _second_, not per frame, right?  Is that still a very long time?
(Assignee)

Comment 309

9 months ago
(In reply to Boris Zbarsky [:bz] from comment #308)
> > 1.5ms is a very long time in a real-time rendering context
> 
> This is 1.5ms per _second_, not per frame, right?  Is that still a very long
> time?

I would still consider that a long time for a property access, and have spent weeks trying to trim an extra 0.1ms per frame before.  This shouldn't be an issue in this case; however, as the property should not be called on every draw call in a well built engine.
(Assignee)

Comment 310

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> Is there a reason this is not a DOMPoint?  Again, that's a spec-level issue.

See above comment about DOMMatrix.

Essentially, it was considered initially but later settled on Float32Array.
(Assignee)

Comment 311

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> I'll file an issue on the spec to clarify the order of the components within the transform and regarding the homogenous coordinates expected.
> 
> DOMMatrix and others defined in the geometry API were initially considered, but was veto'ed.
> 
> In the majority of the use cases, the values returned by the WebVR API will be used directly by WebGL shaders as uniform values.
> 
> WebGL uniform values are uploaded as Float32Arrays:
> 
> https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.8

I have added an issue on the spec to clarify the kind of matrix we are returning:

https://github.com/w3c/webvr/issues/64

I have also described the matrix member ordering in the webidl comments.
(Assignee)

Comment 312

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> More precisely, the values of the pose must not change.  The "VRPose" is considered a snapshot of the sampled (GetImmediatePose) or predicted (GetPose) sensor state.
> 
> Additionally, "until JavaScript has returned control to the browser" is not accurate.  Javascript may in fact give up control to the browser and later call "VRDisplay.submitFrame" later.  The predicted VRPose values will be the same for each frame, which is not advanced through vsync.  It is advanced only through VRDisplay.submitFrame.
> 
> I agree that normative text is misleading here.

I have added an issue on the spec here:

https://github.com/w3c/webvr/issues/65
(Assignee)

Comment 313

9 months ago
(In reply to Boris Zbarsky [:bz] from comment #303)
> > In our implementation, this won't change from null to non-null or vice-versa
> > once a VRDisplay is enumerated; however, it is theoretically possible that
> > the user re-calibrates the room setup for their device while a page is open.
> 
> Right, the spec just needs to make this clear.
> 
> > More precisely, the values of the pose must not change.  The "VRPose" is
> > considered a snapshot of the sampled (GetImmediatePose) or predicted
> > (GetPose) sensor state.
> 
> Again, the spec needs to make this clear.  Please do file spec issues.

Added issue:
https://github.com/w3c/webvr/issues/65

> 
> > What I meant to say is that null values should be treated as [0.0, 0.0, 0.5, 1.0].
> 
> Why are we allowing null at all?  I don't see any reason to allow that
> offhand; we should either allow nothing passed or allow a sequence, but
> allowing null is weird.
The majority of content will be using these "default" values and do not need the extra verbosity.  Additional optional attributes are expected to be added to VRLayer in order to support many kinds of presentation.  Essentially VRLayer is intended as an extension mechanism where the parameters of presentation are not expected to be listed in full.

> 
> As for the default value when nothing is passed, the way this _should_ work
> is that the actual processing model text at
> https://w3c.github.io/webvr/#dom-vrdisplay-requestpresent should describe
> the processing model.  Again, please raise a spec issue as needed.

Added issue for processing model text improvement:
https://github.com/w3c/webvr/issues/66
(Assignee)

Comment 314

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> Perhaps the wrong terminology was used in the prose regarding "default". (Which I wrote...)
> 
> "The leftBounds MUST default to [0.0, 0.0, 0.5, 1.0]"
> 
> What I meant to say is that null values should be treated as [0.0, 0.0, 0.5, 1.0].
> 
> I'll file an issue in the spec to change the text.

Please see my reply to your comments in the Bugzilla bug.

Also, I have added issues in the spec to capture normative text changes:

https://github.com/w3c/webvr/issues/65
https://github.com/w3c/webvr/issues/66
(Assignee)

Comment 315

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> This is intended to be null when the VRDisplay does not support room-scale experiences.  When room-scale experiences are possible with the VRDisplay, the value will be non-null.
> 
> In our implementation, this won't change from null to non-null or vice-versa once a VRDisplay is enumerated; however, it is theoretically possible that the user re-calibrates the room setup for their device while a page is open.

I added an issue with my suggested text change:

https://github.com/w3c/webvr/issues/67
> The majority of content will be using these "default" values and do not need the extra verbosity.

I have no idea what "extra verbosity" you're talking about.

Let me summarize the issue I'm seeing.  You (or the spec draft, at least) and I both agree that it should be possible to pass these object literals as a VRLayer:

  { source: someCanvas }
  { source: someCanvas, rightBounds: someArray }
  { source: someCanvas, leftBounds: undefined }
  { source: someCanvas, leftBounds: undefined, rightBounds: someArray }

and in all four of those cases leftBounds will end up with the default value.  That all makes sense.

The question I'm asking is why you want to allow passing { source: someCanvas, leftBounds: null } and have that end up with the default leftBounds instead of throwing an exception.  Why is this desirable?

> Essentially VRLayer is intended as an extension mechanism where the
> parameters of presentation are not expected to be listed in full.

Yes, it's an options object, I get it.  What I _think_ is happening is that you're conflating the "not present" state that a dictionary property can have (and which is represented by a missing property or explicit undefined value in JS) with the quite different null value in JS.  That makes sense in some situations, but I'm not entirely convinced this is one of them.
(Assignee)

Updated

9 months ago
Blocks: 1293793
(Assignee)

Comment 317

9 months ago
mozreview-review-reply
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review66514

> "timestamp Monotonically increasing value that allows the author to determine if position state data been updated from the hardware. Since values are monotonically increasing, they can be compared to determine the ordering of updates, as newer values will always be greater than or equal to older values."
> 
> The intent of this wording is that content should not make assumptions about the origin of the timestamp values.  It should only be used for comparison to determine the ordering or duration between pose samples.
> 
> These timestamp values in our implementation are currently originating from PR_Now(); however, in some cases an implementation may report timestamp values taken from hardware without non-volatile memory.
> 
> Perhaps the wording could be extended to say that "timestamp values from one VRDisplay must only be compared with timestamp values of the same VRDisplay"?

Added issue to describe time-base of VRPose.timestamp in spec:

https://github.com/w3c/webvr/issues/68

Bugzilla bug to change PR_Now() usage in WebVR to a monotonic clock source:

Bug 1293793 - [webvr] VRPose.timebase is not a monotonically increasing value
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 327

9 months ago
I have updated the patch set to include changes from bz's review of the part 7 patch and to rebase it on the current m-c.
(Assignee)

Comment 328

9 months ago
mozreview-review-reply
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review66538

> Per spec, these first two should be named vrdisplayconnect and vrdisplaydisconnect.  Why are we using different names?
> 
> Also what about vrdisplayactivate and vrdisplaydeactivate?  Worth at least a comment in the IDL...

vrdisplayactivate and vrdisplaydeactivate were added recently, after these patches were completed.

I have added a bug to add these separately:

Bug 1293333 - [webvr] Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events
(Assignee)

Comment 329

9 months ago
mozreview-review-reply
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review66538

> vrdisplayactivate and vrdisplaydeactivate were added recently, after these patches were completed.
> 
> I have added a bug to add these separately:
> 
> Bug 1293333 - [webvr] Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events

These have changed in the spec mid-flight during my implementation.  I have updated the patchset to match the new naming.

> Might be better to save the index returned from this call and then use RemoveElementAt, so we don't have to search the list twice.
> 
> Or juts RemoveElement and then if mListeners.IsEmpty(), do the SendSetHaveEventListener bit.

[Deleted comment added to incorrect thread]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 332

9 months ago
mozreview-review-reply
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review66538

> Why can't this use the existing EventListenerAdded hook we have, which is called later in this method?  The only target these events will fire on is the window itself, right?  Or are there other targets that the listeners can get added to and then require notifying the window?

These events will only fire on the window itself.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 335

9 months ago
mozreview-review-reply
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review66538

> These events will only fire on the window itself.

I have updated the patch to use the EventListenerAdded hook instead.
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review67908

::: dom/base/nsGlobalWindow.h:500
(Diff revisions 32 - 33)
>                             nsIWidget* aWidget, nsIScreen* aScreen);
>    bool FullScreen() const;
>  
>    // Inner windows only.
>    virtual void SetHasGamepadEventListener(bool aHasGamepad = true) override;
> -  virtual void NotifyOfAddingVREventListener() override;
> +  virtual void NotifyOfAddingVREventListener();

Why does this need to be virtual?

For that matter, why does it even need to exist?  EventListenerAdded should only be called on inner windows, so I don't see why you need this function.
Attachment #8741953 - Flags: review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 346

9 months ago
mozreview-review-reply
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review67908

> Why does this need to be virtual?
> 
> For that matter, why does it even need to exist?  EventListenerAdded should only be called on inner windows, so I don't see why you need this function.

Indeed, it no longer needs to be virtual.

I've removed the virtual keyword from the patch and added an assert to ensure it's only called for inner windows.

The function is also called from Navigator::GetVRDisplays, so I haven't inlined it into nsGlobalWindow::EventListenerAdded.
(Assignee)

Comment 347

9 months ago
I have updated the patches to rebase on the current m-c and to fix the issue BZ found in the review on the part 8 patch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 357

9 months ago
The patches have been rebased and updated with bz's feedback on VRLayer.leftBounds and VRLayer.rightbounds.

VRLayer.leftBounds and VRLayer.rightBounds no longer accept a null value.

Issues have been submitted to the WebVR Spec to update the webidl and normative text.
Comment on attachment 8741953 [details]
Bug 1250244 - Part 8: Implement WebVR DOM Events,

https://reviewboard.mozilla.org/r/46871/#review68740

::: dom/base/nsGlobalWindow.h:500
(Diff revisions 29 - 35)
>                             nsIWidget* aWidget, nsIScreen* aScreen);
>    bool FullScreen() const;
>  
>    // Inner windows only.
>    virtual void SetHasGamepadEventListener(bool aHasGamepad = true) override;
> -  virtual void NotifyOfAddingVREventListener() override;
> +  void NotifyOfAddingVREventListener();

Now that I read this again, this name is pretty awkward.  Why not NotifyVREventListenerAdded?

r=me with that fixed, I think.
Attachment #8741953 - Flags: review+
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review68756

r=me with these changes.  Thank you!
Attachment #8722648 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 369

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d055dafd4a32609df8f33fa0caa8d06c2fbd864f
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/80977f3eaea67a95efa327e18f8534276ddf51ac
Bug 1250244 - Part 2: Remove old VR rendering paths,r=gw280

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f822e39e679a0e96f454a8b8eb51b2dbcc1041
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,r=gw280

https://hg.mozilla.org/integration/mozilla-inbound/rev/320c566370cce837b525db6f433a34c0b7e6b4b5
Bug 1250244 - Part 4: Remove Cardboard VR Support,r=gw280

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dddf6d0a9f6bfa6034f30df8d7a896035df6f49
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4140fefc590400e6fc644b16361bc8c19a8035f
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,r=gw280

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b1349cb7487ce1b03a6f91c016315f9e8fdd0ec
Bug 1250244 - Part 7: Implement WebVR 1.0 API,r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc38fe6bde478931ea9e64fd156fdc8eed32b7a5
Bug 1250244 - Part 8: Implement WebVR DOM Events,r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec7229cceafcb353f5293347aa653f4242219c88
Bug 1250244 - Part 9: Update test_interfaces.html,r=bz

Comment 370

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d055dafd4a32
https://hg.mozilla.org/mozilla-central/rev/80977f3eaea6
https://hg.mozilla.org/mozilla-central/rev/a7f822e39e67
https://hg.mozilla.org/mozilla-central/rev/320c566370cc
https://hg.mozilla.org/mozilla-central/rev/6dddf6d0a9f6
https://hg.mozilla.org/mozilla-central/rev/a4140fefc590
https://hg.mozilla.org/mozilla-central/rev/7b1349cb7487
https://hg.mozilla.org/mozilla-central/rev/cc38fe6bde47
https://hg.mozilla.org/mozilla-central/rev/ec7229cceafc
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1295951

Updated

8 months ago
Depends on: 1296381
(Assignee)

Updated

8 months ago
See Also: → bug 1136734

Comment 371

8 months ago
mozreview-review
Comment on attachment 8722648 [details]
Bug 1250244 - Part 7: Implement WebVR 1.0 API,

https://reviewboard.mozilla.org/r/36163/#review70488

Please get review from me when you touch WebGL code.

::: dom/canvas/WebGLContext.cpp:2358
(Diff revision 45)
>  }
>  
> +already_AddRefed<layers::SharedSurfaceTextureClient>
> +WebGLContext::GetVRFrame()
> +{
> +  VRManagerChild *vrmc = VRManagerChild::Get();

Use 4-spaces, like the rest of the file.
Also * to the left, against type.

::: dom/canvas/WebGLContext.cpp:2363
(Diff revision 45)
> +  PresentScreenBuffer();
> +  mDrawCallsSinceLastFlush = 0;
> +
> +  MarkContextClean();
> +  UpdateLastUseIndex();

These should really have been made into functions, and not copy-pasted.

::: dom/canvas/WebGLContext.cpp:2417
(Diff revision 45)
> +      vrmc,
> +      vrmc->GetBackendType(),
> +      TextureFlags::ORIGIN_BOTTOM_LEFT);
> +
> +  screen->Morph(Move(factory));
> +  mVRPresentationActive = true;

Is this var ever used? It looks like it's only ever set to true here, never read.
(Assignee)

Comment 372

8 months ago
Thanks for spotting these, Jeff.  I'll be sure to include you in anything else that touches these files.

I've created Bug 1296745 to hold the patch that will clean this up.


(In reply to Jeff Gilbert [:jgilbert] from comment #371)
> Comment on attachment 8722648 [details]
> Bug 1250244 - Part 7: Implement WebVR 1.0 API,
> 
> https://reviewboard.mozilla.org/r/36163/#review70488
> 
> Please get review from me when you touch WebGL code.
> 
> ::: dom/canvas/WebGLContext.cpp:2358
> (Diff revision 4