Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor getDevices into requestDevice #289

Merged
merged 6 commits into from
Oct 20, 2017
Merged

Refactor getDevices into requestDevice #289

merged 6 commits into from
Oct 20, 2017

Conversation

toji
Copy link
Member

@toji toji commented Sep 28, 2017

Got an initial pull request up, but this is going to take some discussion.

The basic idea is to drop getDevices in favor of a requestDevice call that can take an optional set of filters. Credit where credit is due, this is basically what @cvan suggested we do way back in January. I felt that we should try and keep the API simpler. Jokes on me, I guess. ;)

Some of the important points:

This follows very closely the pattern already happily established by WebUSB and WebBluetooth, right down the the exact function name. That immediately puts my mind at ease a bit, in that it means we feel more at home next to those APIs and means we're able to borrow from the expertise that went into establishing those standards.

Our filters are much more minimal than the other two APIs, however, since there is no perceived benefit and several real downsides to being overly specific in this case. We generally want to encourage developers to take whatever device matches their most coarse requirements (ie: needs to support exclusive presentation, in the future needs to support AR passthrough?). We definitely don't want to allow or encourage "I want a device with at least a screen resolution of X, FoV no smaller than Y, and that supports at least Z simultaneous layers."

There was a question on the most recent call about if we should drop the VRDevice object, since we could feasibly just request sessions directly from navigator.vr with this pattern. After stewing on it for a bit I think the answer is "Yes... but there's an issue." Specifically, we still need a way to ensure that WebGL contexts are created with a compatible adapter, and I like the pattern we've established of using the VRDevice to indicate what adapter the context needs to be compatible with. (I personally feel it's a hard requirement to allow developers to create compatible contexts prior to the creation of a session.) So in my opinion VRDevice stays and simply loses the name string.

That creates a minor complication when it comes to multiple devices connected to a single PC, though. I had this fanciful idea that the VRDevice didn't need to represent a physical piece of hardware, and could instead represent all the potential devices that could satisfy the filters, and only when the user attempted to create a VRSession would the system throw up a prompt (if necessary) and say "Hey, which of these two headsets do you want to present to?" That breaks the ability to use VRDevice as the handle for context compatibility, though, since it's entirely possible those two headsets are connected to different adapters. (It's also just kinda weird when set next to WebUSB and WebBluetooth, both of whom treat their respective devices as a physical piece of hardware.)

So I think a VRDevice needs to continue representing just one piece of hardware, which then begs the question of how the poor power user with 4 GPUs chooses between their Rift and Vive when viewing WebVR content.

However, I don't think it's a big enough issue for us to try and bend over backwards for. A few things to consider:

  • Most users will only have one possible device, either because it's their phone, it's a standalone HMD, or because headsets are expensive and power hungry so it doesn't make much sense to have more than one per machine.
  • Even in the case that you do have N devices available, you'll often have a clear indication that the user wants to use a specific device. (If the user is viewing a VR browser in one HMD, they definitely don't want WebVR content to show up on the other.)
  • Hardware trends and basic financial factors seem to be favoring standalone and mobile systems long term anyway.
  • In the case that it truly is ambiguous the browser could have some internal settings page where the default VR device is set.

Something I don't want to see happen, though, is for pages to pop up a "Which device do you mean?" dialog upon calls to requestDevice, because I would expect that to happen early on in most pages lifetime as they try to determine if they should advertise VR features. A UA-driven selection dialog at that step would mean that many pages would immediately ask you to pick a headset upon navigation even though you've made no indication you want to use VR. That's severely user-hostile behavior.

One thing I'd really appreciate is hearing from @grorg if these changes would satisfy Apple's fingerprinting concerns sufficiently. I think we can work out the above issues, but if this change doesn't get us closer to enabling eventual implementation in WebKit then it's something of a moot point.

I need to add a section about how to use the filters, so this PR isn't quite complete even if everyone agrees on the functionality, but I wanted to get the conversation going. As a heads up, I'll be unavailable for the entirety of the week of Oct 2nd, but I hope that the rest of the community group can work towards a resolution in the meantime. This is an important change to finalize soon so that we don't get stuck in spec limbo.

Need to add a section about how to use the filters.
Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thanks for proposing this change! 👍

see my comments on the IDL, which I'm curious for your input on.

I'm happy with this change. first though, primarily, I have some concerns about how the event listener is expected to be used. I could be missing the obvious, but a code sample would also help.

explainer.md Outdated

`navigator.vr.getDevices` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a list of available devices. Each `VRDevice` represents a physical unit of VR hardware that can present imagery to the user somehow, referred to here as a "VR hardware device". On desktop clients this will usually be a headset peripheral; on mobile clients it may represent the mobile device itself in conjunction with a viewer harness (e.g., Google Cardboard or Samsung Gear VR). It may also represent devices without stereo presentation capabilities but more advanced tracking, such as Tango devices.
`navigator.vr.requestDevice` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to an available device which matches the given filter criteria. A `VRDevice` represents a physical unit of VR hardware that can present imagery to the user somehow, referred to here as a "VR hardware device". On desktop clients this will usually be a headset peripheral; on mobile clients it may represent the mobile device itself in conjunction with a viewer harness (e.g., Google Cardboard or Samsung Gear VR). It may also represent devices without stereo presentation capabilities but more advanced tracking, such as Tango devices.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tango -> ARCore/ARKit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

explainer.md Outdated
}, err => {
// An error occurred querying VR hardware. May be the result of blocked
// permissions by a parent frame.
navigator.vr.requestDevice().then((device) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: don't need the parentheses since this is a single, non-expanded argument

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

explainer.md Outdated
// permissions by a parent frame.
navigator.vr.requestDevice().then((device) => {
vrDevice = device;
onVRAvailable();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think we might as well have this example pass vrDevice as a parameter to the onVRAvailable function? as "best practices" sample code, having the onVRAvailable example function accept a single argument could be nicer than exposing a global variable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, though I still set the global variable in onVRAvailable simply because it's easier to track over the various example snippets than constantly passing the device around.

explainer.md Outdated
};

dictionary VRDeviceRequestOptions {
required sequence<VRDeviceFilter> filters;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they're simple, but can we add a sample code block showing usage of filters?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why's this required if it's optional as an argument?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that we need example code that uses filters, I'll try to add that. As for the required on an optional, it's a pattern I've seen elsewhere before. Simply saying that the dictionary itself is optional, but if you include one then you must specify filters. (Otherwise why are you passing the dictionary at all?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion with the rest of the implementers we've decided to remove the filter criteria in the initial version so that we can have more detailed discussions about what's appropriate to filter on before we add it back in down the road. As such the need for example code is now moot. 😉

explainer.md Outdated

Promise<sequence<VRDevice>> getDevices();
[SecureContext, Exposed=Window] interface VR : EventTarget {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have this (and others) exposed on (Window,Worker)? or do you feel that warrants a separate issue and discussion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely want to get there, but I do feel it's worth having it's own issue. I don't expect it to be controversial, but any conversation that does crop up around it deserves its own space rather than being intermixed with this change.

explainer.md Outdated

Promise<sequence<VRDevice>> getDevices();
[SecureContext, Exposed=Window] interface VR : EventTarget {
attribute EventHandler ondeviceschanged;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we don't have the plural vs. singular and active vs. past tenses, do you think onchange might read better? (the event names in the event interfaces of the Web Bluetooth and Sensor APIs are inconsistent, but the Permissions API uses onchange, which I feel is pretty unambiguous. still doesn't disambiguate from change vs. changed, but it's still better I think.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on second thought, perhaps it's better to be explicit. but, if only one event is emitted per VRDevice that is changed, IMO this should be ondevicechanged (singular) instead of ondeviceschanged (plural).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't there be some state associated with the event emitted? from a web developer's POV this is a bit hard to work with:

navigator.vr.addEventListener('devicechange', evt => {
  // Which VR device changed? And to which state did it change?
});

see the Permissions API for similar cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too opinionated on the exact verbiage (though I think "devices" is more accurate than "device", since there may in fact be more than one.)

I don't know what we could appropriately surface in the event without brushing up against the fingerprinting concerns that prompted this change, though. If we deliver the devices in question via this event then developers could presumably just monitor the event to get a more complete view of the system than the query delivers.

The purpose of the event is simply to indicate to developers that if they had previously asked for a device and didn't get an appropriate one back they may now want to try again. Given that, is there an appropriate bit of contextual information that you can think of that would be useful here without describing the available hardware explicitly? (It's a bit of an annoying song and dance, I know, but: Web.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to ondevicechanged

explainer.md Outdated
Promise<sequence<VRDevice>> getDevices();
[SecureContext, Exposed=Window] interface VR : EventTarget {
attribute EventHandler ondeviceschanged;
Promise<VRDevice> requestDevice(optional VRDeviceRequestOptions options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid adding the new dictionary interface, VRDeviceRequestOptions, can this instead be a plain object? (inspiration: navigator.permissions.query) I seem to recall @annevk's making a similar suggestion in prior discussions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd try to avoid object as it means you have to write your own binding logic in prose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha, makes sense. I assume that was the rationale behind this issue you filed: w3c/permissions#158

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that had a different reason (and doesn't seem to be going anywhere).

@cvan
Copy link
Contributor

cvan commented Oct 3, 2017

Apologies in advance for talking about this yet again! I know we've talked about this in the past; both other web specs and my thinking have changed (for the better). (Spoiler: I still think No UI is the best path forward.)

But to address your concerns about the Permissions aspect of navigator.vr.requestDevice

I found this comment in the Permissions API spec: w3c/permissions#158 (comment)

In my view, it's less a request for permission elevation, more a request to turn on the accelerometer (now or forever).

From w3c/permissions#158 (comment):

We could also encourage UAs to merge prompts within a narrow window of time.

If a script calls getUserMedia() multiple times before reaching a stable state, this document advises the UI designer that the permission dialogs should be merged, so that the user can give permission for the use of multiple cameras and/or media sources in one dialog interaction. The constraints on each getUserMedia call can be used to decide which stream gets which media sources.

(Note: these comments aren't directly related to the parent issue in w3c/permissions#158 [https://github.com/w3c/permissions/issues/158#issue-259866485], but they're still useful for the context of this discussion.)


This reminds me of how the Fullscreen API works with Pointer Lock. Neither API is spec'd to use the Permissions API (I'll assume for historical, legacy reasons), and I do also share the concern about compounding the problem with introducing mandatory UI prompts.

Today, browsers don't require two clicks for web sites to engage Pointer Lock (requestPointerLock) after engaging Full Screen (requestPointerLock). This behaviour isn't yet defined in the Permissions API (but could be).

From the Pointer Lock spec:

Pointer lock is commonly combined with fullscreen [FULLSCREEN], and if an engagement gesture is used to enter fullscreen it is sufficient for a subsequent requestPointerLock.


I would suggest that we build navigator.vr.requestDevice with the Permissions API in mind (but at this time a navigator.permissions.query({name: 'vr'}) request ought not to be required).

Similar to the ongoing spec changes to the Sensor API (i.e., deviceorientation, devicemotion), I do not think we need to introduce UI prompts but access could be gated by the Permissions API.

Access to sensor readings are controlled by the Permissions API [PERMISSIONS]. User agents use a number of criteria to grant access to the readings. Note that access can be granted without prompting the user.

If we can add similar text in this PR, I'd be a happy camper!

Thanks for taking this one on. It's a tricky issue that hasn't yet been fully addressed in other APIs either, so thanks. 👍

@annevk
Copy link

annevk commented Oct 3, 2017

How could navigator.permissions.query({name: 'vr'}) be required? It's just a permission check. If the browser grants that permission by default, it would end up returning "granted".

Also, to be clear, you cannot require any kind of specific UX. User agents have to be free to implement a flow they think makes sense. (It helps enormously to sketch these out in advance and verify them with security UX folks, as it can influence the API a bit.)

@cvan
Copy link
Contributor

cvan commented Oct 3, 2017

How could navigator.permissions.query({name: 'vr'}) be required? It's just a permission check. If the browser grants that permission by default, it would end up returning "granted".

  1. User loads a WebVR site for the first time
  2. navigator.permissions.query({name: 'vr'}) gets called
  3. UA shows Permissions dialog box
  4. User clicks Deny
  5. Promise from navigator.permissions.query({name: 'vr'}) gets resolved with {state: 'rejected'}
  6. Content should subsequently fail whenever navigator.vr.getDevices() is called

Am I correct in thinking that the behaviour in the last step (6) should be spec'd?

How it's spec'd right now and implemented is, as long as the action was user initiated, a call to navigator.vr.getDevices() is always granted, regardless of browser permissions.

FWIW, this is what the Sensor API spec says:

Access to sensor readings are controlled by the Permissions API [PERMISSIONS].
User agents use a number of criteria to grant access to the readings.
Note that access can be granted without prompting the user.


Also, to be clear, you cannot require any kind of specific UX. User agents have to be free to implement a flow they think makes sense. (It helps enormously to sketch these out in advance and verify them with security UX folks, as it can influence the API a bit.)

Understood, thanks. It's certainly something we've been careful to not describe, even in non-normative text. I see this passage repeated in the Permissions API:

This is intentionally vague about the details of the permission UI and how the UA infers user intent. UAs should be able to explore lots of UI within this framework.


As an aside, but related:

Firefox 55+ always requires a user-initiated event (can be manually disabled with the dom.vr.require-gesture flag) for the first time content presents to VR.

In Chrome for Android, navigator.getVRDisplays() was gated on a user-initiated gesture (usually a touch* event) . That's changed slightly in Chrome 62:

The synthetic click event at viewport (0, 0) has been removed (for both Cardboard and the Daydream controller touchpad) (bug). The vrdisplayactivate event is now considered a user gesture, and may be used to request presentation and begin media playback, without relying on the click event.

@annevk
Copy link

annevk commented Oct 3, 2017

I think you misunderstand query(). query() never triggers UI.

Note that requiring "user activation" (that's the HTML Standard term for "gesture") should be written down: https://html.spec.whatwg.org/#triggered-by-user-activation. E.g., Fullscreen does require that.

@cvan
Copy link
Contributor

cvan commented Oct 4, 2017

I think you misunderstand query(). query() never triggers UI.

From your original comment, I now see what you mean.

The Permissions API's navigator.permissions.query(…) just resolves a Promise with a state of 'prompt' if the API is blocked on the browser's permissions UI prompt. Chrome, for example, will only return 'granted' or 'denied' after a user has clicked the Allow/Block buttons in the permissions UI prompt. And the only way Chrome's permissions UI prompt will show up is when a "powerful API" is invoked upon a user-initiated event.

Thanks for the clarification. This helps a ton.

@toji
Copy link
Member Author

toji commented Oct 11, 2017

FYI: I just got back from vacation so I haven't had a chance to parse all this yet but thank you for the detailed discussion! I'll respond to things individually as I get a chance.

@toji
Copy link
Member Author

toji commented Oct 13, 2017

Okay, catching up now. The permissions discussion is very interesting to me, because I also had some misconceptions about what it was doing. To clarify, this is how I think it works now based on the above discussion:

  • Calling navigator.permissions.query({name: 'vr'}) would return "granted" if the page is allowed to use VR features, "denied" if the page has been prevented from using VR features (likely from a previously shown prompt), or "prompt" if the next attempt to use VR features would result in UI being shown. The call to query itself will never cause UI to be shown.
  • Any UI that does show up will be the result of designated WebVR API calls (such as potentially vrDisplay.requestSession()), and although we can specify that it requires user activation we must only suggest places where UI may be appropriate, not require it.
  • The WebVR spec can independently outline what actions have permissions associated with them without blocking on inclusion or browser implementation of the permissions API, which merely provides a way to surface the current state of any permissions we define.

Correct?

@annevk
Copy link

annevk commented Oct 16, 2017

Yeah, that looks correct to me. @jyasskin you might want to double check the previous comment in this issue.

Removing filters for the moment, since they’re easy to add in after the
fact and we’re still not clear on what we want to allow developers to
filter on.
@toji
Copy link
Member Author

toji commented Oct 18, 2017

Pushed a new update with changes based on both the feedback here and the discussion we had on the call yesterday. Big change is removal of filter criteria for now. Also added some explicit text about when device selection UI should be used.

@toji
Copy link
Member Author

toji commented Oct 18, 2017

Should also mention that we got a tentative "OK" from Apple on this route addressing the core of their fingerprinting concerns. Dean is going to get additional confirmation, but for the moment it looks like this gets us to a better place in terms of cross vendor implementability.

@toji
Copy link
Member Author

toji commented Oct 18, 2017

One final note: My impression at this point is that the latest version of this proposal has broad vendor support so unless concerns are raised I'm inclined to merge it within a day or so. As always we can iterate on individual details in separate issues after that.

Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very well done! I like the small, incremental change. forgive my nitpicky comments (though they're mostly about language and ergonomics).

I approve of this change. in fact, I think perhaps the event changes (the removal of the ondeviceconnect and ondevicedisconnect event and addition of the ondevicechanged event) warrant their own PR.

I'm not strongly opposed to it, but I don't think there's enough documentation about the nuances of the changes.

let me know your feedback. and let me know when you're ready for me to take another look. I'll be quick on it. thanks for your patience and taking this on. 👍

explainer.md Outdated
}, (err) => {
// Could not find any available VR hardware or an error occurred querying VR
// hardware.
});
```

If there are multiple VR hardware devices available the UA will need to pick which one to return. The UA is allowed to use any criteria it wishes to select which device is returned, including settings UI that allows users to manage device priority. Calling `requestDevice` should not trigger device selection UI, however, as this would cause many sites to display VR-specific dialogs early in the page lifetime when the user has not made any indication that they wish to use VR features.

Future iterations of the API may add filter criteria to `requestDevice`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you feel this needs to be noted? (perhaps it could be in a “tip”-styled box)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to explicitly say navigator.vr.requestDevice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iterations -> revisions?

explainer.md Outdated
7. Continue producing frames until the user indicates that they wish to exit VR mode.
8. End the VR session.
1. Request a VR device that supports the presentation modes the application needs.
1. If a device is available, application advertises VR functionality to the user.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mentioning availability is good, but what about permission? want to make a mention here?

explainer.md Outdated
8. End the VR session.
1. Request a VR device that supports the presentation modes the application needs.
1. If a device is available, application advertises VR functionality to the user.
1. User performs an action that indicates they want to enter VR mode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you say “triggered by user activation” explicitly? and can you link to this https://html.spec.whatwg.org/#triggered-by-user-activation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“enter VR mode” or “request presentation to a VR device”?

explainer.md Outdated
1. Request a VR device that supports the presentation modes the application needs.
1. If a device is available, application advertises VR functionality to the user.
1. User performs an action that indicates they want to enter VR mode.
1. Request a VR session from the device to present VR content with.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“from the device with which to present”?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“from” -> “to”?

explainer.md Outdated
1. If a device is available, application advertises VR functionality to the user.
1. User performs an action that indicates they want to enter VR mode.
1. Request a VR session from the device to present VR content with.
1. Begin a render loop that produces graphical frames to be displayed on the VR device.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“graphical frames” -> “visual frames”? admittedly, that’s not a whole lot better, but I can’t think of a more descriptive name atm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we ought to be consistent with the “presented” vs. “displayed” vs. “requested” terminology

@@ -93,7 +86,11 @@ If a `VRDevice` is available and able to create an exclusive session, the applic
In the following examples we will focus on using exclusive sessions, and cover non-exclusive session use in the [`Advanced Functionality`](#non-exclusive-sessions-magic-windows) section. With that in mind, we ask here if the `VRDevice` supports sessions with `exclusive` access (the default), since we want the ability to display imagery on the headset.

```js
async function onVRAvailable() {
let vrDevice = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do without the global variable?

explainer.md Outdated
attribute EventHandler ondevicedisconnect;

Promise<sequence<VRDevice>> getDevices();
attribute EventHandler ondevicechanged;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will documentation of this be handled in a separate PR? can you file an issue to track?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I've asked this like a dozen different times, sorry lol…

should this be past or activetense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this change is probably deserving of its own PR. but I think perhaps this needs to be a bit more nuanced. a state property (example) seems like it'd be valuable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW: Changed this to devicechange after reading through it a few more times. I think that is indeed the preferred tense. Not sure if I agree this should be pulled out into it's own PR, though. I feel like the individual connect/disconnect events don't make sense once the function has changed over.

@@ -623,18 +620,15 @@ partial interface Navigator {
};

[SecureContext, Exposed=Window] interface VR : EventTarget {
attribute EventHandler ondeviceconnect;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will documentation be updated to reflect the removal of these in a separate PR? can you file an issue to track?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which documentation are you referring to in this case? There's not much in the way of docs for the 2.0 API yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I had read a passage on these events - but I was wrong. I was thinking of the spec.

this is fine 👍

explainer.md Outdated

Promise<sequence<VRDevice>> getDevices();
attribute EventHandler ondevicechanged;
Promise<VRDevice> requestDevice();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there other APIs with similar property naming? the reason I ask is because like with the Permissions API, navigator.permissions.query(…) is what you call to get a Promise back of the permissions state. request to me can be ambiguous; without reading the IDL here that says Promise<VRDevice> or the explainer text above, as a web developer, I could see myself getting confused thinking it’s going to return the availability of a VR Device or begin requesting VR presentation.

that all make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you be opposed to getAvailableDevice? (borrowing a bit from the Remote Playback API which is making its way to CR - which obviously doesn't mean it has absolutely the best spec API but it's probably a good signal.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both WebUSB and WebBluetooth use getDevice. (navigator.usb.getDevice and navigator.bluetooth.getDevice respectively.) As a result I'd really like to carry that verbiage through to this API to reinforce the sense that this is part of the same Web API family.

};

//
// Device
//

[SecureContext, Exposed=Window] interface VRDevice : EventTarget {
readonly attribute DOMString deviceName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this need to be removed? I assume that this is to avoid fingerprinting? without the ability to query or filter VR devices, this seems to perhaps be a premature change. I don’t have metrics in front of me (nor do I think it’d be easy to inspect existing WebVR content to determine), but I can see this complicating content.

I know there’s an issue on file to handle fingerprinting. do you want to handle that in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I see you mention in the PR description that this change is to satisfy Apple’s fingerprinting concerns. I suppose this doesn’t completely. and if anything, hopefully the removal of this encourages developers to not sniff and exclude particular headsets (à la the abuse of User-Agent response headers and navigator.userAgent).

after having thought about it, I think this change is acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would understand the impact to developer on making "responsive" webvr apps cross HMDs. I mean webvr apps are able to check the devcieName and load corresponding assets, let's say different complexity of models, different size of textures etc., to deliver best experience for that HMD. Are there any other ways to do this without deviceName?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a great answer for you, unfortunately. (This is something that WebGL in general struggles with.) But I can say with confidence that scaling scene complexity based on the device name is a bad idea. At the very least it would mean that you'd have to update your code every time a new device hit the market, and it's very easy to get wrong (Not only would browser possibly expose different names but developers might check for, say, the string "Vive" and load high-end assets not realizing that HTC has plans to use the Vive branding for standalone devices as well.)

There's a variety of methods out there to determine if you're on a mobile device or not, so that could be used as a baseline performance differentiator. Beyond that you could ask users to pick a detail level (high, medium, low) or try to scale it dynamically as your experience runs.

I wish I had a better answer, but this is a problem that extends well beyond WebVR and we wouldn't be doing the platform any favors by encouraging another form of user agent sniffing.

@jyasskin
Copy link

#289 (comment) looks correct to me. I haven't fully digested the rest of this thread, or the rest of WebVR.

I'll point out that the Permissions Spec defines a couple operations around requesting permission, which are intended for use from other specs, and which you can use even in browsers that don't implement the Permissions API (permissions.query()). For a requestDevice() kind of operation, you probably want https://w3c.github.io/permissions/#prompt-the-user-to-choose, which gives the UA a list of options for the user to choose between, and leaves it up to the UA how it wants to let the user choose.

You may instead want https://w3c.github.io/permissions/#request-permission-to-use if you intend a yes/no kind of request.

Using these makes sure that you don't have to change your spec if folks later want to give UAs more flexibility in how they show prompts.

@toji
Copy link
Member Author

toji commented Oct 19, 2017

Thanks for the additional feedback, @cvan! Made some updates and left some responses. And thanks for the clarification @jyasskin. I'll definitely make use of those definitions in the spec text.

Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for taking the time to address everything and for making the additions.

my comments here are just to cover a typo or two. I, otherwise, have no objections to these changes.

explainer.md Outdated
8. End the VR session.
1. Request a VR device.
1. If a device is available, application advertises VR functionality to the user.
1. Request a VR session from the device in response to a user activation event.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user activation event -> user-activation event

and can you link to https://html.spec.whatwg.org/multipage/interaction.html#activation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the link would be helpful, because this topic seems to come up a lot of How is user intent defined in WebVR?

explainer.md Outdated

The first thing that any VR-enabled page will want to do is enumerate the available VR hardware and, if present, advertise VR functionality to the user.
The first thing that any VR-enabled page will want to do is request a `VRDevice` and, if one is available, advertise VR functionality to the user. (For example, by adding a button to the DOM that the user can click to start VR content.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you're already saying page above, I'd say page instead of DOM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or document?

explainer.md Outdated

`navigator.vr.getDevices` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a list of available devices. Each `VRDevice` represents a physical unit of VR hardware that can present imagery to the user somehow, referred to here as a "VR hardware device". On desktop clients this will usually be a headset peripheral; on mobile clients it may represent the mobile device itself in conjunction with a viewer harness (e.g., Google Cardboard or Samsung Gear VR). It may also represent devices without stereo presentation capabilities but more advanced tracking, such as Tango devices.
`navigator.vr.requestDevice` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a `VRDevice` if one is available. In no `VRDevice` is available it will resolve to null. The promise will be rejected if an error occurs, such as the the page not having the appropriate permissions to access VR capabilities.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comma after available?

explainer.md Outdated
`navigator.vr.getDevices` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a list of available devices. Each `VRDevice` represents a physical unit of VR hardware that can present imagery to the user somehow, referred to here as a "VR hardware device". On desktop clients this will usually be a headset peripheral; on mobile clients it may represent the mobile device itself in conjunction with a viewer harness (e.g., Google Cardboard or Samsung Gear VR). It may also represent devices without stereo presentation capabilities but more advanced tracking, such as Tango devices.
`navigator.vr.requestDevice` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a `VRDevice` if one is available. In no `VRDevice` is available it will resolve to null. The promise will be rejected if an error occurs, such as the the page not having the appropriate permissions to access VR capabilities.

A `VRDevice` represents a physical unit of VR hardware that can present imagery to the user somehow, referred to here as a "VR hardware device". On desktop clients this will usually be a headset peripheral; on mobile clients it may represent the mobile device itself in conjunction with a viewer harness (e.g., Google Cardboard/Daydream or Samsung GearVR). It may also represent devices without stereo-presentation capabilities but with more advanced tracking, such as ARCore/ARKit-compatible devices.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks

function checkForVR() {
navigator.vr.requestDevice().then(device => {
if (device) {
onVRAvailable(device);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good 👍

explainer.md Outdated
});
Future revisions of the API may add filter criteria to `navigator.vr.requestDevice`.

> **Non-normative Note:** If there are multiple VR devices available, the UA will need to pick which one to return. The UA is allowed to use any criteria it wishes to select which device is returned, including settings UI that allows users to manage device priority. Calling `navigator.vr.requestDevice` should not trigger device selection UI, however, as this would cause many sites to display VR-specific dialogs early in the document lifecycle without and user activation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: device selection UI -> device-selection UI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: without and

explainer.md Outdated

> **Non-normative Note:** If there are multiple VR devices available, the UA will need to pick which one to return. The UA is allowed to use any criteria it wishes to select which device is returned, including settings UI that allows users to manage device priority. Calling `navigator.vr.requestDevice` should not trigger device selection UI, however, as this would cause many sites to display VR-specific dialogs early in the document lifecycle without and user activation.

It's possible that even if no VR device is available initially one may become available while the application is running, or that a previously available device becomes unavailable. This will be most common with PC peripherals that can be connected at disconnected at any time. To respond to these changes pages can listen to the `devicechange` event on the `navigator.vr` object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comma after initially?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connected at disconnected -> connected or disconnected?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps alternate phrasing:

To respond to these changes pages can listen to the devicechange event on the navigator.vr object.

to

Pages can listen to the devicechange event emitted on navigator.vr to respond to new information from the device.

explainer.md Outdated
It's possible that even if no VR device is available initially one may become available while the application is running, or that a previously available device becomes unavailable. This will be most common with PC peripherals that can be connected at disconnected at any time. To respond to these changes pages can listen to the `devicechange` event on the `navigator.vr` object.

```js
navigator.vr.addEventListener('devicechange', checkForVR);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will devicechange always fire when a document loads and a VR device is detected? or in the presence of an event listener on navigator.vr? what's the trigger?

if so, when? in relation to DOMContentLoaded, load, etc.?

explainer.md Outdated
```

### Sessions

A `VRDevice` indicates the presence of a VR hardware device but provides very little information about it outside of a name that could be used to select it from a list. In order to do anything that involves the hardware's presentation or tracking capabilities the application will need to request a `VRSession` from the `VRDevice`.
A `VRDevice` only indicates the availabilty of a VR device. In order to do anything that involves the device's presentation or tracking capabilities, the application will need to request a `VRSession` from the `VRDevice`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only indicates the availabilty -> indicates only the availability

(typo: availability; and the only should be placed next to the noun, which is the availability)

@@ -623,18 +620,15 @@ partial interface Navigator {
};

[SecureContext, Exposed=Window] interface VR : EventTarget {
attribute EventHandler ondeviceconnect;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I had read a passage on these events - but I was wrong. I was thinking of the spec.

this is fine 👍

Thanks for iterating on the text with me!
Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! just one typo - the rest are nits.

explainer.md Outdated
8. End the VR session.
1. Request a VR device.
1. If a device is available, application advertises VR functionality to the user.
1. Request a VR session from the device in response to a [user-activation event](https://html.spec.whatwg.org/multipage/interaction.html#activation).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 👍

explainer.md Outdated
`navigator.vr.getDevices` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a list of available devices. Each `VRDevice` represents a physical unit of VR hardware that can present imagery to the user somehow, referred to here as a "VR hardware device". On desktop clients this will usually be a headset peripheral; on mobile clients it may represent the mobile device itself in conjunction with a viewer harness (e.g., Google Cardboard or Samsung Gear VR). It may also represent devices without stereo presentation capabilities but more advanced tracking, such as Tango devices.
`navigator.vr.requestDevice` returns a [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) that resolves to a `VRDevice` if one is available. If no `VRDevice` is available, it will resolve to null. The promise will be rejected if an error occurs, such as the the page not having the appropriate permissions to access VR capabilities.

A `VRDevice` represents a physical unit of VR hardware that can present imagery to the user somehow, referred to here as a "VR hardware device". On desktop clients this will usually be a headset peripheral; on mobile clients it may represent the mobile device itself in conjunction with a viewer harness (e.g., Google Cardboard/Daydream or Samsung GearVR). It may also represent devices without stereo-presentation capabilities but with more advanced tracking, such as ARCore/ARKit-compatible devices.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nittiest nit: GearVR -> Gear VR

explainer.md Outdated
});
Future revisions of the API may add filter criteria to `navigator.vr.requestDevice`.

> **Non-normative Note:** If there are multiple VR devices available, the UA will need to pick which one to return. The UA is allowed to use any criteria it wishes to select which device is returned, including settings UI that allows users to manage device priority. Calling `navigator.vr.requestDevice` should not trigger device selection UI, however, as this would cause many sites to display VR-specific dialogs early in the document lifecycle without and user activation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: device selection UI -> device-selection UI

explainer.md Outdated
});
Future revisions of the API may add filter criteria to `navigator.vr.requestDevice`.

> **Non-normative Note:** If there are multiple VR devices available, the UA will need to pick which one to return. The UA is allowed to use any criteria it wishes to select which device is returned, including settings UI that allows users to manage device priority. Calling `navigator.vr.requestDevice` should not trigger device selection UI, however, as this would cause many sites to display VR-specific dialogs early in the document lifecycle without and user activation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: without and

explainer.md Outdated

> **Non-normative Note:** If there are multiple VR devices available, the UA will need to pick which one to return. The UA is allowed to use any criteria it wishes to select which device is returned, including settings UI that allows users to manage device priority. Calling `navigator.vr.requestDevice` should not trigger device selection UI, however, as this would cause many sites to display VR-specific dialogs early in the document lifecycle without and user activation.

It's possible that even if no VR device is available initially, one may become available while the application is running, or that a previously available device becomes unavailable. This will be most common with PC peripherals that can be connected or disconnected at any time. Pages can listen to the `devicechange` event emitted on `navigator.vr` to respond to changes in device availability after the page loads. (VR devices already available when the page loads will not cause a `devicechange` event to be fired.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding the last sentence - it’s helpful

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem fine to me, though I'm not certain I see how the fingerprinting concern has been adequately addressed (other than removing the device name?). I'm not objecting to this PR :) Just a bit confused.
The PR description states the premise that users are unlikely to have more than one VR headset connected at a time and therefore differentiating between them isn't a problem worth trying to solve right now. So, we've basically just renamed getDevice to requestDevice and are allowing the UA to be clever in picking a single one (which will probably be the only one anyway?)
Perhaps I'm just not understanding the fingerprinting concern well? In 2.0, we'd already moved all of the sensitive information (other than the name, which is now removed) to the VRSession object.

explainer.md Outdated
8. End the VR session.
1. Request a VR device.
1. If a device is available, application advertises VR functionality to the user.
1. Request a VR session from the device in response to a [user-activation event](https://html.spec.whatwg.org/multipage/interaction.html#activation).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are user-activation events now also required for non-exclusive sessions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the verbiage just got confusing after applying some suggested edits. I was kind of viewing this list as the steps of a "basic" VR experience, which is mostly about getting the exclusive session running. I've made some tweaks to try and clarify.

@toji
Copy link
Member Author

toji commented Oct 19, 2017

@NellWaliczek: The fingerprinting concern was two fold as I understood it. The device name string was certainly part of it, but there was also the fact that if I did happen to have multiple VR devices this identifies me as one of a very small group, making it far more effective as a fingerprinting bit. Just returning "Yup, here's a device" on the other hand probably just means that you have a newish phone, and that will become less of a unique bit as time goes on.

Then again, maybe Apple was thinking of something completely different. 😅 Either way this change appears to address the concern.

explainer.md Outdated
});
Future revisions of the API may add filter criteria to `navigator.vr.requestDevice`.

> **Non-normative Note:** If there are multiple VR devices available, the UA will need to pick which one to return. The UA is allowed to use any criteria it wishes to select which device is returned, including settings UI that allows users to manage device priority. Calling `navigator.vr.requestDevice` should not trigger device-selection UI, however, as this would cause many sites to display VR-specific dialogs early in the document lifecycle without and user activation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still typo here: and, right? or am I misreading it?

@toji toji merged commit a8b98ba into master Oct 20, 2017
@toji toji deleted the request_device branch October 20, 2017 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants