[meta] [webvr] WebVR 1.0 API

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kip, Assigned: kip)

Tracking

(Blocks 2 bugs, {dev-doc-needed})

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [webvr])

Attachments

(9 attachments, 4 obsolete attachments)

58 bytes, text/x-review-board-request
bas.schouten
: review+
Details
58 bytes, text/x-review-board-request
gw280
: review+
Details
58 bytes, text/x-review-board-request
gw280
: review+
Details
58 bytes, text/x-review-board-request
gw280
: review+
Details
58 bytes, text/x-review-board-request
gw280
: review+
Details
58 bytes, text/x-review-board-request
masayuki
: review+
Details
58 bytes, text/x-review-board-request
gw280
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
(Assignee)

Description

3 years 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

3 years ago
Summary: [webvr] Implement WebVR 1.0 API → [meta] [webvr] WebVR 1.0 API
(Assignee)

Comment 2

3 years 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

3 years 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

3 years 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/
(Assignee)

Updated

3 years ago
Depends on: 1196672
(Assignee)

Updated

3 years ago
Depends on: 1254776
(Assignee)

Updated

3 years ago
Depends on: 1256444
(Assignee)

Comment 5

3 years 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

3 years 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

3 years ago
Blocks: 1258504
(Assignee)

Comment 7

3 years 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

3 years ago
Blocks: 1186578

Updated

3 years ago
Whiteboard: [webvr]
(Assignee)

Comment 9

3 years 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

3 years 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

3 years ago
Attachment #8736520 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
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 14

3 years 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

3 years ago
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 17

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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/
(Assignee)

Comment 27

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
Depends on: 1276066
(Assignee)

Comment 39

3 years ago
- 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

3 years ago
- 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

3 years ago
- 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
Depends on: 1276359
(Assignee)

Updated

3 years ago
Blocks: 1276711
(Assignee)

Updated

3 years ago
Blocks: 1269460
(Assignee)

Comment 48

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

Updated

3 years ago
Depends on: 1276811
(Assignee)

Comment 49

3 years 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

3 years ago
Blocks: 1277040
(Assignee)

Comment 50

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
Attachment #8757454 - Attachment is obsolete: true
(Assignee)

Comment 58

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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 67

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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 77

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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/
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

3 years ago
Depends on: 1284292
(Assignee)

Updated

3 years ago
Depends on: 1284324
(Assignee)

Comment 95

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
Attachment #8767761 - Flags: review?(masayuki)
(Assignee)

Comment 103

3 years 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

3 years ago
Depends on: 1284357
(Assignee)

Comment 104

3 years 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

3 years 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

3 years ago
Implementation of Navigator.activeVRDisplays has been split off into Bug 1284357.
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

3 years ago
Depends on: 1284984
(Assignee)

Comment 108

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
Attachment #8767761 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8757453 - Flags: review+
(Assignee)

Comment 116

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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 127

3 years ago
- 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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 147

3 years 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

3 years 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

3 years ago
Attachment #8768982 - Attachment is obsolete: true
(Assignee)

Comment 149

3 years ago
- 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
Attachment #8722648 - Flags: review?(bas)
(Assignee)

Comment 173

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
Attachment #8741953 - Flags: review?(masayuki)
(Assignee)

Comment 182

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

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

Comment 194

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
Re-based patches.
(Assignee)

Comment 204

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

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

Comment 239

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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 266

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
Attachment #8722648 - Flags: review?(bzbarsky)
Attachment #8741953 - Flags: review?(bzbarsky)
(Assignee)

Comment 279

3 years 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

3 years ago
Blocks: 1291777
(Assignee)

Updated

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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