Update behavior, name of Feature Policy #842

toji · 2019-09-12T21:40:42Z

Not intended for merge until discussed at the TPAC /facetoface

Would fix #823 if the working group agreed to the suggested behavioral change from that issue. For a broader look at the editors proposed approach to Feature Policy handling within the Immersive Web APIs, please see https://docs.google.com/document/d/1RZTL69JsTxoJUyXNnu_2v0PPILqrDpYW3ZxDjMAqQ-M/edit?usp=sharing, which will also be discussed at the face to face.

alcooper91 · 2019-09-13T16:32:41Z

index.bs

@@ -2205,7 +2206,7 @@ Integrations {#integrations}
 Feature Policy {#feature-policy}
 --------------

-This specification defines a [=policy-controlled feature=] that controls whether the {{Navigator/xr}} attribute is exposed on the {{Navigator}} object.
+This specification defines a [=policy-controlled feature=] that controls whether the any {{XRSession}}s may be returned by {{XR/requestSession()}} and whether {{XR/supportsSession()}} indicates that any session mode is supported.


We should also consider the behavior of device change events

Great catch! I've updated the PR to also indicate that devicechange events shouldn't fire when the feature policy is not set.

alcooper91 · 2019-09-16T14:30:12Z

index.bs

@@ -273,6 +273,7 @@ The <dfn method for="XR">supportsSession(|mode|)</dfn> method queries if a given
 When this method is invoked, it MUST run the following steps:

  1. Let |promise| be [=a new Promise=].
+  1. If the requesting document's origin is not allowed to use the WebXR [[#feature-policy|feature policy]], [=reject=] |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.


It looks like this is the supportsSession algorithm, should this be done for requestSession as well?

Ahh, requestSession I see that it's buried in the "Trustworthy" and "request inline" sessions. Perhaps it should be unified up here to match the early bail out of supportsSession?
Also, if the decision is made to not make this change for whatever reason, should that logic be removed?

toji · 2019-09-19T09:46:59Z

After discussion at TPAC, updated this to conform to what sounds like the general direction the group wants to go. Proposed feature policy is now xr-spatial-tracking and, as the name implies, more narrowly only blocks features that attempt to use spatial tracking. (This happens to be all immersive sessions, as should be clearer from the refactored required features text.) This has the side effect of allowing inline sessions without tracking to be used everywhere, which we're asserting is the desired behavior. (This following comments from the TAG that relying on a polyfill for these scenarios as I had described earlier was incredibly strange.) PTAL!

index.bs

NellWaliczek

I'm really happy with the direction we've taken this. Thanks to @dontcallmedom for the suggestion!

Overall I think this PR is great; just a few nits. However, I do think some of the reasoning behind why immersive sessions are always impacted by this feature policy gets a bit lost in the details. Perhaps some non-normative text that explicitly points out the connection would be all that's needed to remedy it.

NellWaliczek · 2019-09-19T23:58:05Z

index.bs

@@ -417,53 +418,61 @@ Future iterations of this specification and additional modules may expand the li

 Note: Features are accepted as an array of <code>any</code> values to ensure forwards compatibility. It allows unrecognized optional values to be properly ignored as new [=feature descriptor=] types are added.

-Some {{XRSessionMode}}s enable certain [=feature descriptors=] as [=optional features=] by default. This is only done if the feature does not require a signal of [=user intent=] nor impact performance or the behavior of other features when enabled. The following table describes the <dfn>default features</dfn> associated with each session:
+Depending on the {{XRSessionMode}} requested, certain [=feature descriptors=] are added to the {{XRSessionInit/requiredFeatures}} or {{XRSessionInit/optionalFeatures}} lists by default. This is only done if the feature does not require a signal of [=user intent=] nor impact performance or the behavior of other features when enabled. The following table describes the <dfn>default features</dfn> associated with each session type and feature list:


This is only done if the feature does not require a signal of [=user intent=] nor impact performance or the behavior of other features when enabled

We should probably make this sentence a bit more flexible

Feeling like we should just drop that line entirely? We can put whatever criteria we want in the table below.

index.bs

toji · 2019-09-20T02:54:27Z

Updated to address review feedback. Thanks!

NellWaliczek

One small language nit, but otherwise looks good so I'm approving! I'd suggest leaving this open for a few days so that the wider group has an opportunity chime in on this breaking change.

NellWaliczek · 2019-09-20T03:29:10Z

index.bs


 The feature identifier for this feature is <code>"xr-spatial-tracking"</code>.

 The [=default allowlist=] for this feature is <code>["self"]</code>.

+Note: If the document's origin is not allowed to use the <code>"xr-spatial-tracking"</code> feature policy any [=immersive sessions=] will be blocked, because all [=immersive sessions=] require some use of spatial tracking. [=Inline sessions=] will still be allowed, but restricted to only using the {{XRReferenceSpaceType/"viewer"}} {{XRReferenceSpace}}.


[=Inline sessions=] will still be allowed, but restricted to only using the {{XRReferenceSpaceType/"viewer"}} {{XRReferenceSpace}}.

how about

[=Inline sessions=] will still be allowed, but requestSession will only succeed if no reference spaces are listed in the [=required features=] array. These sessions will only be capable of creating an {{XRReferenceSpaceType/"viewer"}} {{XRReferenceSpace}}.

dontcallmedom · 2019-09-20T06:00:35Z

Just a thought following one of my comments during the meeting: I wonder if we can and should grant feature-id that are needed to run a WebXR polyfill (e.g. accelerometer, magnetomer?, gyroscope) when xr-spatial-tracking is granted - this would enable the polyfill to run in that situation.

("should" is whether this is a useful use case to cater for, "can" is whether this is a concept that either already exists or would be considered by the feature policy team)

NellWaliczek · 2019-09-21T04:57:43Z

/fixes #823, #770, #768, #765, #763, #729

toji · 2019-09-23T20:19:49Z

Updated to incorporate Nell's suggestion.

I wonder if we can and should grant feature-id that are needed to run a WebXR polyfill (e.g. accelerometer, magnetomer?, gyroscope) when xr-spatial-tracking is granted - this would enable the polyfill to run in that situation.

That would only work for browsers that actually have a WebXR implementation, and my impression from talking with the TAG last week was that using polyfills to prop up the functionality of a native implementation was "weird". :)

/agenda to ensure that all WG members have an opportunity to look over this change prior to landing it, which will ideally happen this week.

Artyom17

Ship it!

rcabanier · 2019-09-24T20:11:30Z

Is there a tracking bug to add this to the feature policy spec?
I looked their github repo but I didn't find any references to WebXR

toji · 2019-09-24T20:33:01Z

That's a good point. Given how the feature policy spec is structured, we'll actually want to update https://github.com/w3c/webappsec-feature-policy/blob/master/features.md, but regardless we should put up a PR to make that change. I'll file it now.

…terfaces.

As a result of TPAC there were some spec changes to the xr feature policy (FP). These were made in: immersive-web/webxr#842 The following changes were needed to become spec compliant: * rename FP from "xr" to "xr-spatial-tracking" * supportsSession always resolves for "inline" * supportsSession rejects for immersive-vr with a SecurityError if FP not set * default features per mode are now required features for that mode * requestSession does not unconditionally reject if FP is not set, instead resolving specific features now depends on FP status. * While resolving features, if any required features depend on a missing FP, the requestSession call will fail with a "NotSupportedError" * While resolving features, if any optional features depend on a missing FP, those features will be silently ignored. * devicechange events will not fire for documents that do not have FP set. Bug:1003842 Change-Id: Ie9ae16b5c1d863b730e556b511c024bae8a4503c

As a result of TPAC there were some spec changes to the xr feature policy (FP). These were made in: immersive-web/webxr#842 The following changes were needed to become spec compliant: * rename FP from "xr" to "xr-spatial-tracking" * supportsSession always resolves for "inline" * supportsSession rejects for immersive-vr with a SecurityError if FP not set * default features per mode are now required features for that mode * requestSession does not unconditionally reject if FP is not set, instead resolving specific features now depends on FP status. * While resolving features, if any required features depend on a missing FP, the requestSession call will fail with a "NotSupportedError" * While resolving features, if any optional features depend on a missing FP, those features will be silently ignored. * devicechange events will not fire for documents that do not have FP set. Bug: 1003842 Change-Id: Ie9ae16b5c1d863b730e556b511c024bae8a4503c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808800 Commit-Queue: Alexander Cooper <alcooper@chromium.org> Reviewed-by: Brandon Jones <bajones@chromium.org> Reviewed-by: Ian Clelland <iclelland@chromium.org> Reviewed-by: Jacob DeWitt <jacde@chromium.org> Auto-Submit: Alexander Cooper <alcooper@chromium.org> Cr-Commit-Position: refs/heads/master@{#701740}

As a result of TPAC there were some spec changes to the xr feature policy (FP). These were made in: immersive-web/webxr#842 The following changes were needed to become spec compliant: * rename FP from "xr" to "xr-spatial-tracking" * supportsSession always resolves for "inline" * supportsSession rejects for immersive-vr with a SecurityError if FP not set * default features per mode are now required features for that mode * requestSession does not unconditionally reject if FP is not set, instead resolving specific features now depends on FP status. * While resolving features, if any required features depend on a missing FP, the requestSession call will fail with a "NotSupportedError" * While resolving features, if any optional features depend on a missing FP, those features will be silently ignored. * devicechange events will not fire for documents that do not have FP set. Bug: 1003842 Change-Id: Ie9ae16b5c1d863b730e556b511c024bae8a4503c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808800 Commit-Queue: Alexander Cooper <alcooper@chromium.org> Reviewed-by: Brandon Jones <bajones@chromium.org> Reviewed-by: Ian Clelland <iclelland@chromium.org> Reviewed-by: Jacob DeWitt <jacde@chromium.org> Auto-Submit: Alexander Cooper <alcooper@chromium.org> Cr-Commit-Position: refs/heads/master@{#701740}

…or to match spec, a=testonly Automatic update from web-platform-tests Update xr feature policy name and behavior to match spec As a result of TPAC there were some spec changes to the xr feature policy (FP). These were made in: immersive-web/webxr#842 The following changes were needed to become spec compliant: * rename FP from "xr" to "xr-spatial-tracking" * supportsSession always resolves for "inline" * supportsSession rejects for immersive-vr with a SecurityError if FP not set * default features per mode are now required features for that mode * requestSession does not unconditionally reject if FP is not set, instead resolving specific features now depends on FP status. * While resolving features, if any required features depend on a missing FP, the requestSession call will fail with a "NotSupportedError" * While resolving features, if any optional features depend on a missing FP, those features will be silently ignored. * devicechange events will not fire for documents that do not have FP set. Bug: 1003842 Change-Id: Ie9ae16b5c1d863b730e556b511c024bae8a4503c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808800 Commit-Queue: Alexander Cooper <alcooper@chromium.org> Reviewed-by: Brandon Jones <bajones@chromium.org> Reviewed-by: Ian Clelland <iclelland@chromium.org> Reviewed-by: Jacob DeWitt <jacde@chromium.org> Auto-Submit: Alexander Cooper <alcooper@chromium.org> Cr-Commit-Position: refs/heads/master@{#701740} -- wpt-commits: dc17f8c8e3372dd30ce98d542874843f87726e8e wpt-pr: 19119

…or to match spec, a=testonly Automatic update from web-platform-tests Update xr feature policy name and behavior to match spec As a result of TPAC there were some spec changes to the xr feature policy (FP). These were made in: immersive-web/webxr#842 The following changes were needed to become spec compliant: * rename FP from "xr" to "xr-spatial-tracking" * supportsSession always resolves for "inline" * supportsSession rejects for immersive-vr with a SecurityError if FP not set * default features per mode are now required features for that mode * requestSession does not unconditionally reject if FP is not set, instead resolving specific features now depends on FP status. * While resolving features, if any required features depend on a missing FP, the requestSession call will fail with a "NotSupportedError" * While resolving features, if any optional features depend on a missing FP, those features will be silently ignored. * devicechange events will not fire for documents that do not have FP set. Bug: 1003842 Change-Id: Ie9ae16b5c1d863b730e556b511c024bae8a4503c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808800 Commit-Queue: Alexander Cooper <alcooperchromium.org> Reviewed-by: Brandon Jones <bajoneschromium.org> Reviewed-by: Ian Clelland <iclellandchromium.org> Reviewed-by: Jacob DeWitt <jacdechromium.org> Auto-Submit: Alexander Cooper <alcooperchromium.org> Cr-Commit-Position: refs/heads/master{#701740} -- wpt-commits: dc17f8c8e3372dd30ce98d542874843f87726e8e wpt-pr: 19119 UltraBlame original commit: 0d34ec0cb3b41dbc57243f1ebc1e77a840bba80e

alcooper91 reviewed Sep 13, 2019

View reviewed changes

toji marked this pull request as ready for review September 16, 2019 01:20

alcooper91 reviewed Sep 16, 2019

View reviewed changes

NellWaliczek added this to the September 2019 milestone Sep 19, 2019

Manishearth assigned Manishearth and toji and unassigned Manishearth Sep 19, 2019

toji force-pushed the new-feature-policy branch from 48e923f to 7cbd425 Compare September 19, 2019 09:42

toji requested a review from NellWaliczek September 19, 2019 09:47

Manishearth approved these changes Sep 19, 2019

View reviewed changes

index.bs Outdated Show resolved Hide resolved

toji changed the title ~~Update behavior of 'xr' Feature Policy~~ Update behavior, name of Feature Policy Sep 19, 2019

toji force-pushed the new-feature-policy branch from 7cbd425 to 89b3c30 Compare September 19, 2019 14:05

alcooper91 reviewed Sep 19, 2019

View reviewed changes

index.bs Show resolved Hide resolved

NellWaliczek reviewed Sep 20, 2019

View reviewed changes

NellWaliczek approved these changes Sep 20, 2019

View reviewed changes

NellWaliczek mentioned this pull request Sep 21, 2019

Some edits and questions. #818

Closed

Artyom17 approved these changes Sep 24, 2019

View reviewed changes

toji added 5 commits September 25, 2019 21:08

Update behavior of 'xr' Feature Policy

2126b90

Added devicechange events to the list of feature policy-controlled in…

e49dcea

…terfaces.

Refactored to use a feature policy of 'xr-spatial-tracking'

9071bae

Typo correction from @Manishearth

bc255dc

Addressing feedback

e685a41

toji force-pushed the new-feature-policy branch from e152456 to e685a41 Compare September 26, 2019 04:13

toji merged commit aabd770 into master Sep 26, 2019

chromium-wpt-export-bot mentioned this pull request Sep 30, 2019

Update xr feature policy name and behavior to match spec web-platform-tests/wpt#19119

Merged

toji deleted the new-feature-policy branch October 1, 2019 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update behavior, name of Feature Policy #842

Update behavior, name of Feature Policy #842

toji commented Sep 12, 2019

alcooper91 Sep 13, 2019

toji Sep 13, 2019

alcooper91 Sep 16, 2019

alcooper91 Sep 17, 2019

toji commented Sep 19, 2019

NellWaliczek left a comment

NellWaliczek Sep 19, 2019

toji Sep 20, 2019

toji commented Sep 20, 2019

NellWaliczek left a comment

NellWaliczek Sep 20, 2019

dontcallmedom commented Sep 20, 2019

NellWaliczek commented Sep 21, 2019

toji commented Sep 23, 2019

Artyom17 left a comment

rcabanier commented Sep 24, 2019

toji commented Sep 24, 2019

Update behavior, name of Feature Policy #842

Update behavior, name of Feature Policy #842

Conversation

toji commented Sep 12, 2019

alcooper91 Sep 13, 2019

Choose a reason for hiding this comment

toji Sep 13, 2019

Choose a reason for hiding this comment

alcooper91 Sep 16, 2019

Choose a reason for hiding this comment

alcooper91 Sep 17, 2019

Choose a reason for hiding this comment

toji commented Sep 19, 2019

NellWaliczek left a comment

Choose a reason for hiding this comment

NellWaliczek Sep 19, 2019

Choose a reason for hiding this comment

toji Sep 20, 2019

Choose a reason for hiding this comment

toji commented Sep 20, 2019

NellWaliczek left a comment

Choose a reason for hiding this comment

NellWaliczek Sep 20, 2019

Choose a reason for hiding this comment

dontcallmedom commented Sep 20, 2019

NellWaliczek commented Sep 21, 2019

toji commented Sep 23, 2019

Artyom17 left a comment

Choose a reason for hiding this comment

rcabanier commented Sep 24, 2019

toji commented Sep 24, 2019