Add permission for Accelerometer, Gyroscope, Magnetometer and Orientation Sensor #142

riju · 2017-03-31T12:31:32Z

Fixes #141.

riju · 2017-03-31T12:33:14Z

tobie · 2017-03-31T12:48:55Z

I think this needs more discussion. What's the relationship between those permissions? i.e. what happens when the orientation-sensor permission is granted and then further along the gyroscope permission is requested? Etc.

anssiko · 2017-03-31T13:21:29Z

Currently no relationship, each one is considered separately. Based on the learnings from the recent WebVR Origin Trial, I feel we actually should ship this as an experiment in order to have definitive answer to this kind of ergonomics issues.

I leave it to the editors to decide whether to land this spec patch now and possibly adjust later, or leave this open until we have gathered such web developer feedback by shipping an experiment.

tobie · 2017-03-31T13:49:50Z

I feel we actually should ship this as an experiment in order to have definitive answer to this kind of ergonomics issues.

I doubt you'll have anything clear as an answer here unless you ship all motion sensors API together. Not to mention that some of the concerns are related to yet to be released permissions APIs.

riju · 2017-04-03T08:21:12Z

Something concrete as proposed by the Chrome Privacy/Security team after some analysis is as follows:

AMBIENT_LIGHT_SENSOR: opt-out UI. granted by default.
MAGNETOMETER: opt-out UI. granted by default.
ORIENTATION_SENSOR: opt-out UI. granted by default.

ACCELEROMETER: auto-grant. no UI.
GYROSCOPE: auto grant. no UI.

We would go into origin trial with this model, get feedback from developers, and are open to change the policy based on feedback.

riju · 2017-04-03T08:36:58Z

Regarding this PR, iirc it was agreed in TPAC that we use granular permission names.
Details : w3c/sensors#22 (comment)

So that implementation can later use whatever privacy policy them deem fit on top of this
w3c/sensors#174 (comment)

tobie · 2017-04-03T08:40:35Z

Regarding this PR, iirc it was agreed in TPAC that we use granular permission names.

Yes. That was the conclusion then. There's now evidence that this probably won't work well in practice, especially for motion sensors, will cause issues with new APIs (such as revoking permissions), and needs further research/work.

anssiko · 2017-04-03T14:37:24Z

I doubt you'll have anything clear as an answer here unless you ship all motion sensors API together.

That's the plan for the Chrome Origin Trial.

@marcoscaceres any concerns? We'd like to land this to have it documented before we enter Origin Trial to gather web developers' feedback on the model. Follows 4f95684

tobie · 2017-04-03T15:31:10Z

I doubt you'll have anything clear as an answer here unless you ship all motion sensors API together.

That's the plan for the Chrome Origin Trial.

I meant all motion sensors not just these four.

marcoscaceres · 2017-04-04T07:33:40Z

@anssiko, maybe we can add a note to the spec to say that these are under an origin trial - and that we are testing how the combination works together? @tobie, would that help a bit?

anssiko · 2017-04-04T09:54:10Z

@marcoscaceres, that is indeed a very good idea.

@tobie, please propose a good wording for the note, perhaps linking to your related research and/or the issue in w3c/sensors so people know where to go to provide further feedback on the topic.

anssiko · 2017-04-05T11:43:57Z

@riju, please add the following note below the enum PermissionName block:

Note: The enumeration values {{accelerometer}}, {{gyroscope}}, {{magnetometer}}, and
{{orientation-sensor}} are considered provisional and are subject to change based on
feedback from early implementations. See
<a href="https://github.com/w3c/sensors/issues/22">w3c/sensors#22</a> issue for
more information.

With this addition we should be good to land this PR, right @tobie and @marcoscaceres?

tobie · 2017-04-05T11:47:41Z

LGTM.

raymeskhoury · 2017-04-06T04:57:17Z

index.bs

+    <p>
+      The <dfn for="PermissionName" enum-value>"magnetometer"</dfn>
+      permission is the permission associated with the usage of the
+      [[ambient-light]] API.


I think this should be magnetometer?

raymeskhoury · 2017-04-06T04:57:31Z

index.bs

+    <p>
+      The <dfn for="PermissionName" enum-value>"orientation-sensor"</dfn>
+      permission is the permission associated with the usage of the
+      [[ambient-light]] API.


orientation sensor?

pozdnyakov · 2017-04-12T10:17:43Z

@riju think it's worth dropping "orientation-sensor" from the list due to w3c/orientation-sensor#33

raymeskhoury · 2017-04-19T02:58:51Z

index.bs

      <a enum-value>"clipboard"</a>,
    };
  </pre>
+  <p class="note">
+  The enumeration values {{accelerometer}}, {{gyroscope}}, {{magnetometer}}, and
+  {{orientation-sensor}} are considered provisional and are subject to change


nit: this still mentions orientation-sensor

Add note for the new sensor permission types. Note that we considered using a permission type for orientation-sensor, but finally decided against it. Details: w3c/orientation-sensor#33

anssiko · 2017-04-19T08:09:05Z

@marcoscaceres, thanks for your patience with this one.

We now have LGTMs from @tobie and me, and all the review comments from @raymeskhoury have been addressed. @riju squashed the multiple commits into one so you can maintain your beautiful commit history :-)

@marcoscaceres, feel free to land this so we can advance with the implementation.

anssiko · 2017-04-19T10:03:50Z

Thanks @marcoscaceres! We owe you a 🍻

marcoscaceres · 2017-04-19T10:06:38Z

I'll be taking you up on that at TPAC! 💃

This is the first part of adding permission guard for sensors based on Generic Sensor Framework. There was consensus to add granular permissions for sensors here w3c/sensors#22 This CL adds permission name for the different sensors in the permission module. Spec discussion : w3c/permissions#142 BUG=606766 Review-Url: https://codereview.chromium.org/2791623004 Cr-Commit-Position: refs/heads/master@{#491303}

riju changed the title ~~Add permission for Accelerometer, Gyroscope, Magnetometer Orientation Sensor~~ Add permission for Accelerometer, Gyroscope, Magnetometer and Orientation Sensor Apr 3, 2017

raymeskhoury reviewed Apr 6, 2017

View reviewed changes

riju force-pushed the addSensorPermissions branch from f0ed1b5 to 59c35af Compare April 11, 2017 10:38

raymeskhoury reviewed Apr 19, 2017

View reviewed changes

riju force-pushed the addSensorPermissions branch from 196f56e to 4a891ee Compare April 19, 2017 08:04

Add new permission types for Accelerometer, Gyroscope and Magnetometer.

c5eb2b8

Add note for the new sensor permission types. Note that we considered using a permission type for orientation-sensor, but finally decided against it. Details: w3c/orientation-sensor#33

riju force-pushed the addSensorPermissions branch from 4a891ee to c5eb2b8 Compare April 19, 2017 08:05

marcoscaceres merged commit e9623d7 into w3c:master Apr 19, 2017

anssiko mentioned this pull request Apr 19, 2017

Simplify extension of the Permissions API for concrete sensor implementations w3c/sensors#22

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add permission for Accelerometer, Gyroscope, Magnetometer and Orientation Sensor #142

Add permission for Accelerometer, Gyroscope, Magnetometer and Orientation Sensor #142

riju commented Mar 31, 2017

riju commented Mar 31, 2017

tobie commented Mar 31, 2017

anssiko commented Mar 31, 2017

tobie commented Mar 31, 2017 •

edited

Loading

riju commented Apr 3, 2017

riju commented Apr 3, 2017

tobie commented Apr 3, 2017

anssiko commented Apr 3, 2017

tobie commented Apr 3, 2017 •

edited

Loading

marcoscaceres commented Apr 4, 2017

anssiko commented Apr 4, 2017

anssiko commented Apr 5, 2017

tobie commented Apr 5, 2017

raymeskhoury Apr 6, 2017

raymeskhoury Apr 6, 2017

pozdnyakov commented Apr 12, 2017

raymeskhoury Apr 19, 2017

anssiko commented Apr 19, 2017

anssiko commented Apr 19, 2017

marcoscaceres commented Apr 19, 2017

Add permission for Accelerometer, Gyroscope, Magnetometer and Orientation Sensor #142

Add permission for Accelerometer, Gyroscope, Magnetometer and Orientation Sensor #142

Conversation

riju commented Mar 31, 2017

riju commented Mar 31, 2017

tobie commented Mar 31, 2017

anssiko commented Mar 31, 2017

tobie commented Mar 31, 2017 • edited Loading

riju commented Apr 3, 2017

riju commented Apr 3, 2017

tobie commented Apr 3, 2017

anssiko commented Apr 3, 2017

tobie commented Apr 3, 2017 • edited Loading

marcoscaceres commented Apr 4, 2017

anssiko commented Apr 4, 2017

anssiko commented Apr 5, 2017

tobie commented Apr 5, 2017

raymeskhoury Apr 6, 2017

Choose a reason for hiding this comment

raymeskhoury Apr 6, 2017

Choose a reason for hiding this comment

pozdnyakov commented Apr 12, 2017

raymeskhoury Apr 19, 2017

Choose a reason for hiding this comment

anssiko commented Apr 19, 2017

anssiko commented Apr 19, 2017

marcoscaceres commented Apr 19, 2017

tobie commented Mar 31, 2017 •

edited

Loading

tobie commented Apr 3, 2017 •

edited

Loading