-
Notifications
You must be signed in to change notification settings - Fork 386
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
Enable optional/required features to be checked before session creation #739
Conversation
Missed one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! One nit and one bigger concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thanks!
Thanks, Brandon! This is a fairly important PR, so I plan to leave this open for a few days to give folks a chance to review and leave feedback. If nothing showstopping comes up, I'm aiming to merge by Friday. |
a43a774
to
28a4df0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Nell! Suggestions and a request for clarification on one paragraph inline...
explainer.md
Outdated
* **Required** This feature must be available in order for the experience to function at all. If [explicit consent](privacy-security-explainer.md#explicit-consent) is necessary, users will be prompted in response to `xr.requestSession()`. Session creation will be rejected if the feature is unavailable for the XR device or if the UA determines the user does not wish the feature enabled. | ||
* **Optional** The experience would like to use this feature for the entire session, but can function without it. Again, if [explicit consent](privacy-security-explainer.md#explicit-consent) is necessary, users will be prompted in response to `xr.requestSession()`. However, session creation will succeed regardless of the feature's hardware support or user intent. Developers must not assume optional features are available in the session and check the result from attempting to use them. | ||
|
||
If the UA does not recognize an entry in `XRSession.requiredFeatures` the session will not be created. Under some circumstances, features recognized by the UA but not explicitly listed in these arrays may be implicitly added to `XRSessionInit.optionalFeatures`. For example, if a feature does not require a signal of [user intent](privacy-security-explainer.md#user-intent). Or, if the requested session mode implies consent, such as `immersive-vr` does for `local` and `local-floor` reference spaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest adding two additional concepts here:
- Performance might affect whether a feature is added implicitly
- Which features get added implicitly might vary by user agent. Don't count on anything not
required
Also, I think I know what the last sentence in the paragraph means, but the phrase 'implies consent' could be interpreted as 'implies user consent' so I'm not certain. I think the intention is that session mode could imply that certain features are available.
In case it helps, here's a suggested alternative including the above concepts and clarifying the meaning around session mode (note: this isn't intended to be blocking, I'm just including it in case it's useful):
If the UA does not recognize an entry in
XRSession.requiredFeatures
the session will not be created.Features not listed in either array may or may not be available during the session. Under some circumstances the UA may implicitly add features to
XRSessionInit.optionalFeatures
, enabling them during the session.For example, a user agent may choose to enable a feature that does not require a signal of user intent and which has no adverse performance implications. This behavior is likely to vary between user agents, and developers should not depend upon the availability of features not explicitly listed as
required
.The user agent may also enable features that are implied by session type, or implied by other requested features. For example, as any immersive session must support both
local
andlocal-floor
reference spaces, a user agent may add both references spaces toXRSessionInit.optionalFeatures
when animmersive-vr
session is created.
On the last bit, I'm curious whether this is a firm requirement. For example, consider a scenario where a developer requests an immersive session with a local
reference space, but not local-floor
. Suppose the user has configured their viewer height. #638 proposes that exposing actual viewer height requires explicit consent because it is sensitive data. In this case, should the user agent:
- Require explicit consent because
local-floor
is implied? - Add an implicit
local-floor
reference space, but use emulated floor height to avoid explicit consent? - Not add
local-floor
at all, even though it's an immersive session? - Any of the above, at the discretion of the user agent (and if a developer needs
local-floor
they should make itrequired
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about potential performance feedback! And I definitely didn't make it clear enough, that it is not a choice to be left to the UA discretion about which features qualify for being automatically enabled. Without consistency on this, we'd end up in a world of hurt.
I also, better understand your concern now about local-floor
technically having a different threat profile than simply local
. While it is a requirement that local
and local-floor
be possible in any session that supports immersive-vr
and immersive-ar
, it doesn't mean we necessarily want to expose user height without the UA having the opportunity to manage that risk differently.
Hopefully my latest commit has addressed both your concerns and @toji's. If so, I'd appreciate an "Approve" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table definitely helps clarify things, thanks!
To make sure I understand your above comment, Nell: Sounds like the intent is that local-floor
is no longer one of the implicit features, but (as is currently required by the spec) would still be available to any immersive session via an emulated floor height. The only way to get a non-emulated floor height would be to explicitly request local-floor
in either the requiredFeatures
or optionalFeatures
lists. Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change came from my discussion with @johnpallett on Friday, so please correct me if I've misunderstood. I was under the impression that we couldn't automatically allow a local-floor with even an emulated height because it could be used for fingerprinting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concern with local-floor
is when the viewer height is not emulated. The user's height is sensitive data (i.e. 'are you a child') so would likely require explicit consent on some platforms.
In contrast, local-floor
with emulated height doesn't have this concern, and the UA can round the data to mitigate the fingerprinting risk.
@NellWaliczek If I understood on Friday the primary concern was making sure user agents were consistent in their behavior; it seems like requiring an emulated local-floor
in this scenario could achieve that consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right now I remember. We have two competing problems to solve:
- We definitely don't want
local-floor
to be forced to use an emulated height when a real one is available (either via sensor data or a user configuration setting). - We definitely don't want to have inconsistency about some hardware needing to have
local-floor
requested up front and others not.
The intent behind this approach was simply to settle on the lowest common denominator to ensure consistency and user safety.
@toji, you on board with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not speaking for @toji but that makes sense to me and I agree with the approach as written
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure I'm on the same page: My reading of this is that local-floor
must be explicitly requested to be used, and local
is the only reference space that will be implicitly available to immersive sessions. If that's correct, I'm on board with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct
28a4df0
to
6deb520
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question below regarding whether added features are at the user agent's discretion (the phrasing might suggest they are).
Updated. Waiting on the response to #739 (comment) from @johnpallett |
Ok, this PR's been open for a bit and has been iterated based on the feedback provided. At this point, I'm inclined to merge and address any subsequent feedback in a separate PR so that we are unblocked on finishing the "VR complete" milestone. |
Engage! |
This is the corresponding spec text for the functionality described in PR #739. There's some hand waving going on around user consent and such because those will be better defined in a follow-up PR. The primary goal of this PR is to establish the algorithims that drive how the feature requests are processed.
This is the corresponding spec text for the functionality described in PR #739. There's some hand waving going on around user consent and such because those will be better defined in a follow-up PR. The primary goal of this PR is to establish the algorithims that drive how the feature requests are processed.
This is the corresponding spec text for the functionality described in PR #739. There's some hand waving going on around user consent and such because those will be better defined in a follow-up PR. The primary goal of this PR is to establish the algorithims that drive how the feature requests are processed.
This is the corresponding spec text for the functionality described in PR #739. There's some hand waving going on around user consent and such because those will be better defined in a follow-up PR. The primary goal of this PR is to establish the algorithims that drive how the feature requests are processed.
This PR contains explainer text intended to describe the fixes for #723, #591, #423, #417, and #727. I'm holding off on the spec text until it's clear we've got consensus on this approach.
First off! The main purpose of this proposal is to ensure that developers have a way to block session creation if a feature they absolute require is unavailable. While this may occasionally have to do with consent, it also may simply be due to lack of hardware support. Following the same pattern, this design allows developers to indicate up front that there are optional features they would very much like to use. This allows any necessary explicit consent to be requested before a session begins. For now, this PR only lists reference-space related features, but we've heard feedback from folks working on new functionality that a similar system is needed.
Second! There are several main considerations in the reasoning behind this approach:
XRFeatureName
(or something). However that would have meant optional features that the UA did not recognize would have caused sessions to be rejected. Alternatively, we considered using dictionary keys that could be ignored when processing the optional array. However, that would have meant required features the UA did not recognize would have allowed sessions to be created anyway. We considered having these two arrays be made up of different things (as was tried and abandoned in Added session feature handling to the explainer. #433), but the difference seemed to cause unnecessary cognitive load and confusion for developers.DOMStrings
are insufficient to communicate feature requirements, we can easily and transparently switch the arrays to be of typeAny
or a union type with whatever new configuration requirements we have identified.(And yes, there's a few more PRs ready to be posted once we have consensus established on this one)