1250244 - [meta] [webvr] WebVR 1.0 API

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[:kip (Kearwood Gilbert)]

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.

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 7: Implement WebVR 1.0 API,

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

Comment 2

[:kip (Kearwood Gilbert)]

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.

Comment 3

[:kip (Kearwood Gilbert)]

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/

Comment 4

[:kip (Kearwood Gilbert)]

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/

Comment 5

[:kip (Kearwood Gilbert)]

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/

Comment 6

[:kip (Kearwood Gilbert)]

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/

Comment 7

[:kip (Kearwood Gilbert)]

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/

Comment 8

[:kip (Kearwood Gilbert)]

Attachment: MozReview Request: Initial OpenVR/SteamVR support

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

Comment 9

[:kip (Kearwood Gilbert)]

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/

Comment 10

[:kip (Kearwood Gilbert)]

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/

Comment 11

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,

Review commit: https://reviewboard.mozilla.org/r/46855/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46855/

Comment 12

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,

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

Comment 13

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 2: Remove old VR rendering paths,

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

Comment 14

[:kip (Kearwood Gilbert)]

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/

Comment 15

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,

Review commit: https://reviewboard.mozilla.org/r/46869/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46869/

Comment 16

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 8: Implement WebVR DOM Events,

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

Comment 17

[:kip (Kearwood Gilbert)]

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/

Comment 18

[:kip (Kearwood Gilbert)]

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/

Comment 19

[:kip (Kearwood Gilbert)]

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/

Comment 20

[:kip (Kearwood Gilbert)]

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/

Comment 21

[:kip (Kearwood Gilbert)]

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/

Comment 22

[:kip (Kearwood Gilbert)]

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/

Comment 23

[:kip (Kearwood Gilbert)]

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/

Comment 24

[:kip (Kearwood Gilbert)]

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/

Comment 25

[:kip (Kearwood Gilbert)]

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/

Comment 27

[:kip (Kearwood Gilbert)]

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/

Comment 28

[:kip (Kearwood Gilbert)]

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/

Comment 29

[:kip (Kearwood Gilbert)]

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/

Comment 30

[:kip (Kearwood Gilbert)]

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/

Comment 31

[:kip (Kearwood Gilbert)]

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/

Comment 32

[:kip (Kearwood Gilbert)]

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/

Comment 33

[:kip (Kearwood Gilbert)]

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/

Comment 34

[:kip (Kearwood Gilbert)]

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/

Comment 35

[:kip (Kearwood Gilbert)]

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/

Comment 36

[:kip (Kearwood Gilbert)]

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/

Comment 37

[:kip (Kearwood Gilbert)]

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/

Comment 38

[:kip (Kearwood Gilbert)]

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/

Comment 39

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,

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

Review commit: https://reviewboard.mozilla.org/r/55870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55870/

Comment 40

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 4: Remove Cardboard VR Support,

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

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

Comment 41

[:kip (Kearwood Gilbert)]

Attachment: MozReview Request: Bug 1250244 - Part 8: Implement TextureForwarder

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

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

Comment 42

[:kip (Kearwood Gilbert)]

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/

Comment 43

[:kip (Kearwood Gilbert)]

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/

Comment 44

[:kip (Kearwood Gilbert)]

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/

Comment 45

[:kip (Kearwood Gilbert)]

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/

Comment 46

[:kip (Kearwood Gilbert)]

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/

Comment 47

[:kip (Kearwood Gilbert)]

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/

Comment 48

[:kip (Kearwood Gilbert)]

I am re-basing the patches on top of Bug 1198518, which is currently in m-i.

Comment 49

[:kip (Kearwood Gilbert)]

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.

Comment 50

[:kip (Kearwood Gilbert)]

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/

Comment 51

[:kip (Kearwood Gilbert)]

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/

Comment 52

[:kip (Kearwood Gilbert)]

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/

Comment 53

[:kip (Kearwood Gilbert)]

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/

Comment 54

[:kip (Kearwood Gilbert)]

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/

Comment 55

[:kip (Kearwood Gilbert)]

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/

Comment 56

[:kip (Kearwood Gilbert)]

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/

Comment 57

[:kip (Kearwood Gilbert)]

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/

Comment 58

[:kip (Kearwood Gilbert)]

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/

Comment 59

[:kip (Kearwood Gilbert)]

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/

Comment 60

[:kip (Kearwood Gilbert)]

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/

Comment 61

[:kip (Kearwood Gilbert)]

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/

Comment 62

[:kip (Kearwood Gilbert)]

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/

Comment 63

[:kip (Kearwood Gilbert)]

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/

Comment 64

[:kip (Kearwood Gilbert)]

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/

Comment 65

[:kip (Kearwood Gilbert)]

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.

Comment 66

[:kip (Kearwood Gilbert)]

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

Comment 67

[:kip (Kearwood Gilbert)]

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/

Comment 68

[:kip (Kearwood Gilbert)]

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/

Comment 69

[:kip (Kearwood Gilbert)]

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/

Comment 70

[:kip (Kearwood Gilbert)]

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/

Comment 71

[:kip (Kearwood Gilbert)]

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/

Comment 72

[:kip (Kearwood Gilbert)]

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/

Comment 73

[:kip (Kearwood Gilbert)]

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/

Comment 74

[:kip (Kearwood Gilbert)]

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/

Comment 75

[:kip (Kearwood Gilbert)]

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

Comment 76

[:kip (Kearwood Gilbert)]

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

Comment 77

[:kip (Kearwood Gilbert)]

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/

Comment 78

[:kip (Kearwood Gilbert)]

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/

Comment 79

[:kip (Kearwood Gilbert)]

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/

Comment 80

[:kip (Kearwood Gilbert)]

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/

Comment 81

[:kip (Kearwood Gilbert)]

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/

Comment 82

[:kip (Kearwood Gilbert)]

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/

Comment 83

[:kip (Kearwood Gilbert)]

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/

Comment 84

[:kip (Kearwood Gilbert)]

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/

Comment 85

[George Wright (:gw280) (needinfo me!)]

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 :)

Comment 86

[:kip (Kearwood Gilbert)]

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 87

[:kip (Kearwood Gilbert)]

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

Comment 88

[George Wright (:gw280) (needinfo me!)]

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

Comment 89

[George Wright (:gw280) (needinfo me!)]

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?

Comment 90

[George Wright (:gw280) (needinfo me!)]

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 91

[George Wright (:gw280) (needinfo me!)]

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

https://reviewboard.mozilla.org/r/46855/#review56192

Comment 92

[George Wright (:gw280) (needinfo me!)]

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/

Comment 93

[George Wright (:gw280) (needinfo me!)]

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.

Comment 94

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 9: Implement WebVR DOM Events

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

Comment 95

[:kip (Kearwood Gilbert)]

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/

Comment 96

[:kip (Kearwood Gilbert)]

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/

Comment 97

[:kip (Kearwood Gilbert)]

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/

Comment 98

[:kip (Kearwood Gilbert)]

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/

Comment 99

[:kip (Kearwood Gilbert)]

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/

Comment 100

[:kip (Kearwood Gilbert)]

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/

Comment 101

[:kip (Kearwood Gilbert)]

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/

Comment 102

[:kip (Kearwood Gilbert)]

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/

Comment 103

[:kip (Kearwood Gilbert)]

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.

Comment 104

[:kip (Kearwood Gilbert)]

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/

Comment 105

[:kip (Kearwood Gilbert)]

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/

Comment 106

[:kip (Kearwood Gilbert)]

Implementation of Navigator.activeVRDisplays has been split off into Bug 1284357.

Comment 107

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

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.

Comment 108

[:kip (Kearwood Gilbert)]

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/

Comment 109

[:kip (Kearwood Gilbert)]

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/

Comment 110

[:kip (Kearwood Gilbert)]

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/

Comment 111

[:kip (Kearwood Gilbert)]

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/

Comment 112

[:kip (Kearwood Gilbert)]

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/

Comment 113

[:kip (Kearwood Gilbert)]

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/

Comment 114

[:kip (Kearwood Gilbert)]

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/

Comment 115

[:kip (Kearwood Gilbert)]

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/

Comment 116

[:kip (Kearwood Gilbert)]

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/

Comment 117

[:kip (Kearwood Gilbert)]

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/

Comment 118

[:kip (Kearwood Gilbert)]

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/

Comment 119

[:kip (Kearwood Gilbert)]

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/

Comment 120

[:kip (Kearwood Gilbert)]

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/

Comment 121

[:kip (Kearwood Gilbert)]

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/

Comment 122

[:kip (Kearwood Gilbert)]

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/

Comment 123

[:kip (Kearwood Gilbert)]

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/

Comment 124

[:kip (Kearwood Gilbert)]

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

r+'ed by gw280 earlier.

Comment 125

[:kip (Kearwood Gilbert)]

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.

Comment 126

[:kip (Kearwood Gilbert)]

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

Comment 127

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 9: Update test_interfaces.html

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

Review commit: https://reviewboard.mozilla.org/r/62956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62956/

Comment 128

[:kip (Kearwood Gilbert)]

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/

Comment 129

[:kip (Kearwood Gilbert)]

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/

Comment 130

[:kip (Kearwood Gilbert)]

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/

Comment 131

[:kip (Kearwood Gilbert)]

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/

Comment 132

[:kip (Kearwood Gilbert)]

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/

Comment 133

[:kip (Kearwood Gilbert)]

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/

Comment 134

[:kip (Kearwood Gilbert)]

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/

Comment 135

[:kip (Kearwood Gilbert)]

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/

Comment 136

[:kip (Kearwood Gilbert)]

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/

Comment 137

[:kip (Kearwood Gilbert)]

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/

Comment 138

[:kip (Kearwood Gilbert)]

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/

Comment 139

[:kip (Kearwood Gilbert)]

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/

Comment 140

[:kip (Kearwood Gilbert)]

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/

Comment 141

[:kip (Kearwood Gilbert)]

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/

Comment 142

[:kip (Kearwood Gilbert)]

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/

Comment 143

[:kip (Kearwood Gilbert)]

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/

Comment 144

[:kip (Kearwood Gilbert)]

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/

Comment 145

[:kip (Kearwood Gilbert)]

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/

Comment 146

[:kip (Kearwood Gilbert)]

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

Comment 147

[:kip (Kearwood Gilbert)]

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.

Comment 148

[:kip (Kearwood Gilbert)]

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/

Comment 149

[:kip (Kearwood Gilbert)]

Attachment: Bug 1250244 - Part 9: Update test_interfaces.html,

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

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

Comment 150

[:kip (Kearwood Gilbert)]

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/

Comment 151

[:kip (Kearwood Gilbert)]

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/

Comment 152

[:kip (Kearwood Gilbert)]

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/

Comment 153

[:kip (Kearwood Gilbert)]

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/

Comment 154

[:kip (Kearwood Gilbert)]

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/

Comment 155

[:kip (Kearwood Gilbert)]

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/

Comment 156

[:kip (Kearwood Gilbert)]

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/

Comment 157

[:kip (Kearwood Gilbert)]

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/

Comment 158

[:kip (Kearwood Gilbert)]

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/

Comment 159

[:kip (Kearwood Gilbert)]

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/

Comment 160

[:kip (Kearwood Gilbert)]

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/

Comment 161

[:kip (Kearwood Gilbert)]

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/

Comment 162

[:kip (Kearwood Gilbert)]

I have updated the Part 7 patch (Implement WebVR 1.0 API) to handle issues with multiple tabs that were found in testing.

Comment 163

[Boris Zbarsky [:bzbarsky]]

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

https://reviewboard.mozilla.org/r/63782/#review61424

Comment 164

[:kip (Kearwood Gilbert)]

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/

Comment 165

[:kip (Kearwood Gilbert)]

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/

Comment 166

[:kip (Kearwood Gilbert)]

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/

Comment 167

[:kip (Kearwood Gilbert)]

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/

Comment 168

[:kip (Kearwood Gilbert)]

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/

Comment 169

[:kip (Kearwood Gilbert)]

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/

Comment 170

[:kip (Kearwood Gilbert)]

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/

Comment 171

[:kip (Kearwood Gilbert)]

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/

Comment 172

[:kip (Kearwood Gilbert)]

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/

Comment 173

[:kip (Kearwood Gilbert)]

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/

Comment 174

[:kip (Kearwood Gilbert)]

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/

Comment 175

[:kip (Kearwood Gilbert)]

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/

Comment 176

[:kip (Kearwood Gilbert)]

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/

Comment 177

[:kip (Kearwood Gilbert)]

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/

Comment 178

[:kip (Kearwood Gilbert)]

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/

Comment 179

[:kip (Kearwood Gilbert)]

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/

Comment 180

[:kip (Kearwood Gilbert)]

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/

Comment 181

[:kip (Kearwood Gilbert)]

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/

Comment 182

[:kip (Kearwood Gilbert)]

(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.

Comment 183

[:kip (Kearwood Gilbert)]

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/

Comment 184

[:kip (Kearwood Gilbert)]

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/

Comment 185

[:kip (Kearwood Gilbert)]

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/

Comment 186

[:kip (Kearwood Gilbert)]

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/

Comment 187

[:kip (Kearwood Gilbert)]

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/

Comment 188

[:kip (Kearwood Gilbert)]

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/

Comment 189

[:kip (Kearwood Gilbert)]

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/

Comment 190

[:kip (Kearwood Gilbert)]

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/

Comment 191

[:kip (Kearwood Gilbert)]

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/

Comment 192

[:kip (Kearwood Gilbert)]

I have re-based the patches to match the current mozilla-central

Comment 193

[:kip (Kearwood Gilbert)]

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

Comment 194

[:kip (Kearwood Gilbert)]

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/

Comment 195

[:kip (Kearwood Gilbert)]

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/

Comment 196

[:kip (Kearwood Gilbert)]

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/

Comment 197

[:kip (Kearwood Gilbert)]

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/

Comment 198

[:kip (Kearwood Gilbert)]

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/

Comment 199

[:kip (Kearwood Gilbert)]

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/

Comment 200

[:kip (Kearwood Gilbert)]

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/

Comment 201

[:kip (Kearwood Gilbert)]

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/

Comment 202

[:kip (Kearwood Gilbert)]

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/

Comment 203

[:kip (Kearwood Gilbert)]

Re-based patches.

Comment 204

[:kip (Kearwood Gilbert)]

This is intended to land in Firefox 50.  Could you let me know if you will have time to review the patch?

Comment 205

[:kip (Kearwood Gilbert)]

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?

Comment 206

[Bas Schouten (:bas.schouten)]

(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.

Comment 207

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

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.

Comment 208

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

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.

Comment 209

[:kip (Kearwood Gilbert)]

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.

Comment 210

[:kip (Kearwood Gilbert)]

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.

Comment 211

[:kip (Kearwood Gilbert)]

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.

Comment 212

[:kip (Kearwood Gilbert)]

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/

Comment 213

[:kip (Kearwood Gilbert)]

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/

Comment 214

[:kip (Kearwood Gilbert)]

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/

Comment 215

[:kip (Kearwood Gilbert)]

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/

Comment 216

[:kip (Kearwood Gilbert)]

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/

Comment 217

[:kip (Kearwood Gilbert)]

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/

Comment 218

[:kip (Kearwood Gilbert)]

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/

Comment 219

[:kip (Kearwood Gilbert)]

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/

Comment 220

[:kip (Kearwood Gilbert)]

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/

Comment 221

[:kip (Kearwood Gilbert)]

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.

Comment 222

[:kip (Kearwood Gilbert)]

(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 223

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

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.

Comment 224

[:kip (Kearwood Gilbert)]

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/

Comment 225

[:kip (Kearwood Gilbert)]

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/

Comment 226

[:kip (Kearwood Gilbert)]

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/

Comment 227

[:kip (Kearwood Gilbert)]

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/

Comment 228

[:kip (Kearwood Gilbert)]

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/

Comment 229

[:kip (Kearwood Gilbert)]

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/

Comment 230

[:kip (Kearwood Gilbert)]

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/

Comment 231

[:kip (Kearwood Gilbert)]

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/

Comment 232

[:kip (Kearwood Gilbert)]

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/

Comment 233

[:kip (Kearwood Gilbert)]

Rebased patches and fixed last issue in Masayuki's review of the part 8 patch.

Comment 234

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

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.

Comment 235

[:kip (Kearwood Gilbert)]

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/

Comment 236

[:kip (Kearwood Gilbert)]

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/

Comment 237

[:kip (Kearwood Gilbert)]

(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.

Comment 238

[:kip (Kearwood Gilbert)]

Changes in bug 1282364 have broken this patch set.  I'll update the Part 7 patch to match.

Comment 239

[:kip (Kearwood Gilbert)]

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/

Comment 240

[:kip (Kearwood Gilbert)]

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/

Comment 241

[:kip (Kearwood Gilbert)]

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/

Comment 242

[:kip (Kearwood Gilbert)]

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/

Comment 243

[:kip (Kearwood Gilbert)]

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/

Comment 244

[:kip (Kearwood Gilbert)]

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/

Comment 245

[:kip (Kearwood Gilbert)]

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/

Comment 246

[:kip (Kearwood Gilbert)]

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/

Comment 247

[:kip (Kearwood Gilbert)]

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/

Comment 248

[:kip (Kearwood Gilbert)]

(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.

Comment 249

[Bas Schouten (:bas.schouten)]

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.

Comment 250

[Bas Schouten (:bas.schouten)]

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.

Comment 251

[:kip (Kearwood Gilbert)]

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.

Comment 252

[:kip (Kearwood Gilbert)]

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.

Comment 253

[:kip (Kearwood Gilbert)]

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/

Comment 254

[:kip (Kearwood Gilbert)]

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/

Comment 255

[:kip (Kearwood Gilbert)]

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/

Comment 256

[:kip (Kearwood Gilbert)]

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/

Comment 257

[:kip (Kearwood Gilbert)]

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/

Comment 258

[:kip (Kearwood Gilbert)]

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/

Comment 259

[:kip (Kearwood Gilbert)]

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/

Comment 260

[:kip (Kearwood Gilbert)]

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/

Comment 261

[:kip (Kearwood Gilbert)]

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/

Comment 262

[:kip (Kearwood Gilbert)]

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/

Comment 263

[:kip (Kearwood Gilbert)]

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/

Comment 264

[:kip (Kearwood Gilbert)]

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/

Comment 265

[:kip (Kearwood Gilbert)]

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

Comment 266

[:kip (Kearwood Gilbert)]

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/

Comment 267

[:kip (Kearwood Gilbert)]

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/

Comment 268

[:kip (Kearwood Gilbert)]

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/

Comment 269

[:kip (Kearwood Gilbert)]

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.

Comment 270

[:kip (Kearwood Gilbert)]

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/

Comment 271

[:kip (Kearwood Gilbert)]

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/

Comment 272

[:kip (Kearwood Gilbert)]

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/

Comment 273

[:kip (Kearwood Gilbert)]

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/

Comment 274

[:kip (Kearwood Gilbert)]

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/

Comment 275

[:kip (Kearwood Gilbert)]

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/

Comment 276

[:kip (Kearwood Gilbert)]

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/

Comment 277

[:kip (Kearwood Gilbert)]

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/

Comment 278

[:kip (Kearwood Gilbert)]

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/

Comment 279

[:kip (Kearwood Gilbert)]

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/

Comment 280

[Boris Zbarsky [:bzbarsky]]

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?

Comment 281

[Boris Zbarsky [:bzbarsky]]

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.

Comment 282

[:kip (Kearwood Gilbert)]

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/23-24/

Comment 283

[:kip (Kearwood Gilbert)]

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/24-25/

Comment 284

[:kip (Kearwood Gilbert)]

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/20-21/

Comment 285

[:kip (Kearwood Gilbert)]

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/20-21/

Comment 286

[:kip (Kearwood Gilbert)]

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/24-25/

Comment 287

[:kip (Kearwood Gilbert)]

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/24-25/

Comment 288

[:kip (Kearwood Gilbert)]

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/40-41/

Comment 289

[:kip (Kearwood Gilbert)]

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/29-30/

Comment 290

[:kip (Kearwood Gilbert)]

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/15-16/

Comment 291

[:kip (Kearwood Gilbert)]

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.

Comment 292

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 293

[:kip (Kearwood Gilbert)]

(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.

Comment 294

[Boris Zbarsky [:bzbarsky]]

> It's possible that such an engine may be calling these on every draw call

How often do those happen?

Comment 295

[:kip (Kearwood Gilbert)]

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.

Comment 296

[:kip (Kearwood Gilbert)]

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.

Comment 297

[:kip (Kearwood Gilbert)]

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.

Comment 298

[:kip (Kearwood Gilbert)]

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.

Comment 299

[:kip (Kearwood Gilbert)]

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.

Comment 300

[:kip (Kearwood Gilbert)]

(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.

Comment 301

[Boris Zbarsky [:bzbarsky]]

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.

Comment 302

[:kip (Kearwood Gilbert)]

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

Comment 303

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 304

[:kip (Kearwood Gilbert)]

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"?

Comment 305

[Boris Zbarsky [:bzbarsky]]

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

Looks good to me. (re: tracing gc things.)

Comment 306

[:kip (Kearwood Gilbert)]

(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.

Comment 307

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 308

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 309

[:kip (Kearwood Gilbert)]

(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.

Comment 310

[:kip (Kearwood Gilbert)]

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.

Comment 311

[:kip (Kearwood Gilbert)]

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.

Comment 312

[:kip (Kearwood Gilbert)]

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

Comment 313

[:kip (Kearwood Gilbert)]

(In reply to Boris Zbarsky [:bz] from comment #303)
> > In our implementation, this won't change from null to non-null or vice-versa
> > once a VRDisplay is enumerated; however, it is theoretically possible that
> > the user re-calibrates the room setup for their device while a page is open.
> 
> Right, the spec just needs to make this clear.
> 
> > More precisely, the values of the pose must not change.  The "VRPose" is
> > considered a snapshot of the sampled (GetImmediatePose) or predicted
> > (GetPose) sensor state.
> 
> Again, the spec needs to make this clear.  Please do file spec issues.

Added issue:
https://github.com/w3c/webvr/issues/65

> 
> > What I meant to say is that null values should be treated as [0.0, 0.0, 0.5, 1.0].
> 
> Why are we allowing null at all?  I don't see any reason to allow that
> offhand; we should either allow nothing passed or allow a sequence, but
> allowing null is weird.
The majority of content will be using these "default" values and do not need the extra verbosity.  Additional optional attributes are expected to be added to VRLayer in order to support many kinds of presentation.  Essentially VRLayer is intended as an extension mechanism where the parameters of presentation are not expected to be listed in full.

> 
> As for the default value when nothing is passed, the way this _should_ work
> is that the actual processing model text at
> https://w3c.github.io/webvr/#dom-vrdisplay-requestpresent should describe
> the processing model.  Again, please raise a spec issue as needed.

Added issue for processing model text improvement:
https://github.com/w3c/webvr/issues/66

Comment 314

[:kip (Kearwood Gilbert)]

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

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

> Perhaps the wrong terminology was used in the prose regarding "default". (Which I wrote...)
> 
> "The leftBounds MUST default to [0.0, 0.0, 0.5, 1.0]"
> 
> What I meant to say is that null values should be treated as [0.0, 0.0, 0.5, 1.0].
> 
> I'll file an issue in the spec to change the text.

Please see my reply to your comments in the Bugzilla bug.

Also, I have added issues in the spec to capture normative text changes:

https://github.com/w3c/webvr/issues/65
https://github.com/w3c/webvr/issues/66

Comment 315

[:kip (Kearwood Gilbert)]

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

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

> This is intended to be null when the VRDisplay does not support room-scale experiences.  When room-scale experiences are possible with the VRDisplay, the value will be non-null.
> 
> In our implementation, this won't change from null to non-null or vice-versa once a VRDisplay is enumerated; however, it is theoretically possible that the user re-calibrates the room setup for their device while a page is open.

I added an issue with my suggested text change:

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

Comment 316

[Boris Zbarsky [:bzbarsky]]

> The majority of content will be using these "default" values and do not need the extra verbosity.

I have no idea what "extra verbosity" you're talking about.

Let me summarize the issue I'm seeing.  You (or the spec draft, at least) and I both agree that it should be possible to pass these object literals as a VRLayer:

  { source: someCanvas }
  { source: someCanvas, rightBounds: someArray }
  { source: someCanvas, leftBounds: undefined }
  { source: someCanvas, leftBounds: undefined, rightBounds: someArray }

and in all four of those cases leftBounds will end up with the default value.  That all makes sense.

The question I'm asking is why you want to allow passing { source: someCanvas, leftBounds: null } and have that end up with the default leftBounds instead of throwing an exception.  Why is this desirable?

> Essentially VRLayer is intended as an extension mechanism where the
> parameters of presentation are not expected to be listed in full.

Yes, it's an options object, I get it.  What I _think_ is happening is that you're conflating the "not present" state that a dictionary property can have (and which is represented by a missing property or explicit undefined value in JS) with the quite different null value in JS.  That makes sense in some situations, but I'm not entirely convinced this is one of them.

Comment 317

[:kip (Kearwood Gilbert)]

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

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

> "timestamp Monotonically increasing value that allows the author to determine if position state data been updated from the hardware. Since values are monotonically increasing, they can be compared to determine the ordering of updates, as newer values will always be greater than or equal to older values."
> 
> The intent of this wording is that content should not make assumptions about the origin of the timestamp values.  It should only be used for comparison to determine the ordering or duration between pose samples.
> 
> These timestamp values in our implementation are currently originating from PR_Now(); however, in some cases an implementation may report timestamp values taken from hardware without non-volatile memory.
> 
> Perhaps the wording could be extended to say that "timestamp values from one VRDisplay must only be compared with timestamp values of the same VRDisplay"?

Added issue to describe time-base of VRPose.timestamp in spec:

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

Bugzilla bug to change PR_Now() usage in WebVR to a monotonic clock source:

Bug 1293793 - [webvr] VRPose.timebase is not a monotonically increasing value

Comment 318

[:kip (Kearwood Gilbert)]

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/24-25/

Comment 319

[:kip (Kearwood Gilbert)]

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/25-26/

Comment 320

[:kip (Kearwood Gilbert)]

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/21-22/

Comment 321

[:kip (Kearwood Gilbert)]

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/21-22/

Comment 322

[:kip (Kearwood Gilbert)]

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/25-26/

Comment 323

[:kip (Kearwood Gilbert)]

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/25-26/

Comment 324

[:kip (Kearwood Gilbert)]

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/41-42/

Comment 325

[:kip (Kearwood Gilbert)]

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/30-31/

Comment 326

[:kip (Kearwood Gilbert)]

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/16-17/

Comment 327

[:kip (Kearwood Gilbert)]

I have updated the patch set to include changes from bz's review of the part 7 patch and to rebase it on the current m-c.

Comment 328

[:kip (Kearwood Gilbert)]

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

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

> Per spec, these first two should be named vrdisplayconnect and vrdisplaydisconnect.  Why are we using different names?
> 
> Also what about vrdisplayactivate and vrdisplaydeactivate?  Worth at least a comment in the IDL...

vrdisplayactivate and vrdisplaydeactivate were added recently, after these patches were completed.

I have added a bug to add these separately:

Bug 1293333 - [webvr] Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events

Comment 329

[:kip (Kearwood Gilbert)]

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

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

> vrdisplayactivate and vrdisplaydeactivate were added recently, after these patches were completed.
> 
> I have added a bug to add these separately:
> 
> Bug 1293333 - [webvr] Implement window.onvrdisplayactivate and window.onvrdisplaydeactivate events

These have changed in the spec mid-flight during my implementation.  I have updated the patchset to match the new naming.

> Might be better to save the index returned from this call and then use RemoveElementAt, so we don't have to search the list twice.
> 
> Or juts RemoveElement and then if mListeners.IsEmpty(), do the SendSetHaveEventListener bit.

[Deleted comment added to incorrect thread]

Comment 330

[:kip (Kearwood Gilbert)]

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/31-32/

Comment 331

[:kip (Kearwood Gilbert)]

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/17-18/

Comment 332

[:kip (Kearwood Gilbert)]

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

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

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

These events will only fire on the window itself.

Comment 333

[:kip (Kearwood Gilbert)]

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/32-33/

Comment 334

[:kip (Kearwood Gilbert)]

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/18-19/

Comment 335

[:kip (Kearwood Gilbert)]

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

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

> These events will only fire on the window itself.

I have updated the patch to use the EventListenerAdded hook instead.

Comment 336

[Boris Zbarsky [:bzbarsky]]

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

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

::: dom/base/nsGlobalWindow.h:500
(Diff revisions 32 - 33)
>                             nsIWidget* aWidget, nsIScreen* aScreen);
>    bool FullScreen() const;
>  
>    // Inner windows only.
>    virtual void SetHasGamepadEventListener(bool aHasGamepad = true) override;
> -  virtual void NotifyOfAddingVREventListener() override;
> +  virtual void NotifyOfAddingVREventListener();

Why does this need to be virtual?

For that matter, why does it even need to exist?  EventListenerAdded should only be called on inner windows, so I don't see why you need this function.

Comment 337

[:kip (Kearwood Gilbert)]

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/25-26/

Comment 338

[:kip (Kearwood Gilbert)]

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/26-27/

Comment 339

[:kip (Kearwood Gilbert)]

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/22-23/

Comment 340

[:kip (Kearwood Gilbert)]

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/22-23/

Comment 341

[:kip (Kearwood Gilbert)]

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/26-27/

Comment 342

[:kip (Kearwood Gilbert)]

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/26-27/

Comment 343

[:kip (Kearwood Gilbert)]

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/42-43/

Comment 344

[:kip (Kearwood Gilbert)]

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/33-34/

Comment 345

[:kip (Kearwood Gilbert)]

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/19-20/

Comment 346

[:kip (Kearwood Gilbert)]

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

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

> Why does this need to be virtual?
> 
> For that matter, why does it even need to exist?  EventListenerAdded should only be called on inner windows, so I don't see why you need this function.

Indeed, it no longer needs to be virtual.

I've removed the virtual keyword from the patch and added an assert to ensure it's only called for inner windows.

The function is also called from Navigator::GetVRDisplays, so I haven't inlined it into nsGlobalWindow::EventListenerAdded.

Comment 347

[:kip (Kearwood Gilbert)]

I have updated the patches to rebase on the current m-c and to fix the issue BZ found in the review on the part 8 patch.

Comment 348

[:kip (Kearwood Gilbert)]

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/26-27/

Comment 349

[:kip (Kearwood Gilbert)]

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/27-28/

Comment 350

[:kip (Kearwood Gilbert)]

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/23-24/

Comment 351

[:kip (Kearwood Gilbert)]

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/23-24/

Comment 352

[:kip (Kearwood Gilbert)]

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/27-28/

Comment 353

[:kip (Kearwood Gilbert)]

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/27-28/

Comment 354

[:kip (Kearwood Gilbert)]

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/43-44/

Comment 355

[:kip (Kearwood Gilbert)]

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/34-35/

Comment 356

[:kip (Kearwood Gilbert)]

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/20-21/

Comment 357

[:kip (Kearwood Gilbert)]

The patches have been rebased and updated with bz's feedback on VRLayer.leftBounds and VRLayer.rightbounds.

VRLayer.leftBounds and VRLayer.rightBounds no longer accept a null value.

Issues have been submitted to the WebVR Spec to update the webidl and normative text.

Comment 358

[Boris Zbarsky [:bzbarsky]]

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

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

::: dom/base/nsGlobalWindow.h:500
(Diff revisions 29 - 35)
>                             nsIWidget* aWidget, nsIScreen* aScreen);
>    bool FullScreen() const;
>  
>    // Inner windows only.
>    virtual void SetHasGamepadEventListener(bool aHasGamepad = true) override;
> -  virtual void NotifyOfAddingVREventListener() override;
> +  void NotifyOfAddingVREventListener();

Now that I read this again, this name is pretty awkward.  Why not NotifyVREventListenerAdded?

r=me with that fixed, I think.

Comment 359

[Boris Zbarsky [:bzbarsky]]

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

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

r=me with these changes.  Thank you!

Comment 360

[:kip (Kearwood Gilbert)]

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/27-28/

Comment 361

[:kip (Kearwood Gilbert)]

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/28-29/

Comment 362

[:kip (Kearwood Gilbert)]

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/24-25/

Comment 363

[:kip (Kearwood Gilbert)]

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/24-25/

Comment 364

[:kip (Kearwood Gilbert)]

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/28-29/

Comment 365

[:kip (Kearwood Gilbert)]

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/28-29/

Comment 366

[:kip (Kearwood Gilbert)]

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/44-45/

Comment 367

[:kip (Kearwood Gilbert)]

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/35-36/

Comment 368

[:kip (Kearwood Gilbert)]

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/21-22/

Comment 369

[:kip (Kearwood Gilbert)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d055dafd4a32609df8f33fa0caa8d06c2fbd864f
Bug 1250244 - Part 1: Remove FullScreenOptions parameter from Element.RequestFullScreen,r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/80977f3eaea67a95efa327e18f8534276ddf51ac
Bug 1250244 - Part 2: Remove old VR rendering paths,r=gw280

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f822e39e679a0e96f454a8b8eb51b2dbcc1041
Bug 1250244 - Part 3: Remove Oculus 0.5 Runtime support,r=gw280

https://hg.mozilla.org/integration/mozilla-inbound/rev/320c566370cce837b525db6f433a34c0b7e6b4b5
Bug 1250244 - Part 4: Remove Cardboard VR Support,r=gw280

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dddf6d0a9f6bfa6034f30df8d7a896035df6f49
Bug 1250244 - Part 5: Rename VRDevice to VRDisplay,r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4140fefc590400e6fc644b16361bc8c19a8035f
Bug 1250244 - Part 6: Replace VRStateValidFlags with VRDisplayCapabilityFlags,r=gw280

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b1349cb7487ce1b03a6f91c016315f9e8fdd0ec
Bug 1250244 - Part 7: Implement WebVR 1.0 API,r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc38fe6bde478931ea9e64fd156fdc8eed32b7a5
Bug 1250244 - Part 8: Implement WebVR DOM Events,r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec7229cceafcb353f5293347aa653f4242219c88
Bug 1250244 - Part 9: Update test_interfaces.html,r=bz

Comment 370

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d055dafd4a32
https://hg.mozilla.org/mozilla-central/rev/80977f3eaea6
https://hg.mozilla.org/mozilla-central/rev/a7f822e39e67
https://hg.mozilla.org/mozilla-central/rev/320c566370cc
https://hg.mozilla.org/mozilla-central/rev/6dddf6d0a9f6
https://hg.mozilla.org/mozilla-central/rev/a4140fefc590
https://hg.mozilla.org/mozilla-central/rev/7b1349cb7487
https://hg.mozilla.org/mozilla-central/rev/cc38fe6bde47
https://hg.mozilla.org/mozilla-central/rev/ec7229cceafc

Comment 371

[Kelsey Gilbert [:jgilbert]]

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

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

Please get review from me when you touch WebGL code.

::: dom/canvas/WebGLContext.cpp:2358
(Diff revision 45)
>  }
>  
> +already_AddRefed<layers::SharedSurfaceTextureClient>
> +WebGLContext::GetVRFrame()
> +{
> +  VRManagerChild *vrmc = VRManagerChild::Get();

Use 4-spaces, like the rest of the file.
Also * to the left, against type.

::: dom/canvas/WebGLContext.cpp:2363
(Diff revision 45)
> +  PresentScreenBuffer();
> +  mDrawCallsSinceLastFlush = 0;
> +
> +  MarkContextClean();
> +  UpdateLastUseIndex();

These should really have been made into functions, and not copy-pasted.

::: dom/canvas/WebGLContext.cpp:2417
(Diff revision 45)
> +      vrmc,
> +      vrmc->GetBackendType(),
> +      TextureFlags::ORIGIN_BOTTOM_LEFT);
> +
> +  screen->Morph(Move(factory));
> +  mVRPresentationActive = true;

Is this var ever used? It looks like it's only ever set to true here, never read.

Comment 372

[:kip (Kearwood Gilbert)]

Thanks for spotting these, Jeff.  I'll be sure to include you in anything else that touches these files.

I've created Bug 1296745 to hold the patch that will clean this up.


(In reply to Jeff Gilbert [:jgilbert] from comment #371)
> Comment on attachment 8722648 [details]
> Bug 1250244 - Part 7: Implement WebVR 1.0 API,
> 
> https://reviewboard.mozilla.org/r/36163/#review70488
> 
> Please get review from me when you touch WebGL code.
> 
> ::: dom/canvas/WebGLContext.cpp:2358
> (Diff revision 45)
> >  }
> >  
> > +already_AddRefed<layers::SharedSurfaceTextureClient>
> > +WebGLContext::GetVRFrame()
> > +{
> > +  VRManagerChild *vrmc = VRManagerChild::Get();
> 
> Use 4-spaces, like the rest of the file.
> Also * to the left, against type.
> 
> ::: dom/canvas/WebGLContext.cpp:2363
> (Diff revision 45)
> > +  PresentScreenBuffer();
> > +  mDrawCallsSinceLastFlush = 0;
> > +
> > +  MarkContextClean();
> > +  UpdateLastUseIndex();
> 
> These should really have been made into functions, and not copy-pasted.
> 
> ::: dom/canvas/WebGLContext.cpp:2417
> (Diff revision 45)
> > +      vrmc,
> > +      vrmc->GetBackendType(),
> > +      TextureFlags::ORIGIN_BOTTOM_LEFT);
> > +
> > +  screen->Morph(Move(factory));
> > +  mVRPresentationActive = true;
> 
> Is this var ever used? It looks like it's only ever set to true here, never
> read.