Re: [w3c/permissions] Introduce "Automation" section (#151) from Jeffrey Yasskin on 2017-09-22 (public-webapps-github@w3.org from September 2017)

From: Jeffrey Yasskin <notifications@github.com>
Date: Fri, 22 Sep 2017 23:49:56 +0000 (UTC)
To: w3c/permissions <permissions@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <w3c/permissions/pull/151/review/64715364@github.com>
jyasskin requested changes on this pull request.

I think it's reasonable to leave out [prompt the user to choose](https://w3c.github.io/permissions/#prompt-the-user-to-choose) for now. To handle that, we'll actually need to define the options that each permission offers in its prompts (e.g. bluetooth or USB devices, and in the future particular cameras, microphones, or speakers), and have the automated test pick out which one to accept.

> +  </p>
+
+  <section>
+    <h3 id="set-permission-command">
+      Set Permission
+    </h3>
+    <table>
+      <tbody>
+        <tr>
+          <th>HTTP Method</th>
+          <th><a lt="extension command prefix">Prefix</a></th>
+          <th><a lt="extension command name">Name</a></th>
+        </tr>
+        <tr>
+          <td>POST</td>
+          <td>permissions</td>

https://w3c.github.io/webdriver/webdriver-spec.html#dfn-extension-command-prefix says this should be a URI template, and gives the example of `/session/{session id}/ms/edge`.

> +          <th><a lt="extension command name">Name</a></th>
+        </tr>
+        <tr>
+          <td>POST</td>
+          <td>permissions</td>
+          <td>set</td>
+        </tr>
+      </tbody>
+    </table>
+ <p>The <dfn>Set Permission</dfn> <a>extension command</a> simulates user
+    modification of a {{PermissionDescriptor}}'s <a>permission state</a>.</p>
+    <p>The <a>remote end steps</a> are:</p>
+    <ol>
+      <li>Let |permissionDesc| be the result of <a>getting the property</a>
+      "`descriptor`" from the |parameters| argument.</li>
+      <li>If |permissionDesc| is not a JSON object, return <a>error</a> with

I'm uncomfortable with WebDriver reserving the term "error" globally. Can you call this "WebDriver error" (by changing the `text: error; url: dfn-error` line at the top of the file)?

I'd also rather include the article between "return" and the thing: "return a WebDriver error" reads better than "return WebDriver error". This is inconsistently followed in https://w3c.github.io/webdriver/webdriver-spec.html.

> +    <p>The <a>remote end steps</a> are:</p>
+    <ol>
+      <li>Let |permissionDesc| be the result of <a>getting the property</a>
+      "`descriptor`" from the |parameters| argument.</li>
+      <li>If |permissionDesc| is not a JSON object, return <a>error</a> with
+      <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Let |rootDesc| be the object |permissionDesc| refers to, <a>converted
+      to an IDL value</a> of type {{PermissionDescriptor}}. If this throws an
+      exception, return <a>error</a> with <a>error code</a> <a>invalid
+      argument</a>.</li>
+      <li>Let |typedDescriptor| be the object |permissionDesc| refers to,
+      <a>converted to an IDL value</a> of
+      <code>|rootDesc|.{{PermissionDescriptor/name}}</code>'s <a>permission
+      descriptor type</a>. If this throws an exception, return <a>error</a>
+      with <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Let |settings| be the <a>current settings object</a>.</li>

I think you need to pull the settings object out of the [current session](https://w3c.github.io/webdriver/webdriver-spec.html#dfn-current-session)'s [current browsing context](https://w3c.github.io/webdriver/webdriver-spec.html#dfn-current-browsing-context).

> +          <th><a lt="extension command prefix">Prefix</a></th>
+          <th><a lt="extension command name">Name</a></th>
+        </tr>
+        <tr>
+          <td>POST</td>
+          <td>permissions</td>
+          <td>set</td>
+        </tr>
+      </tbody>
+    </table>
+ <p>The <dfn>Set Permission</dfn> <a>extension command</a> simulates user
+    modification of a {{PermissionDescriptor}}'s <a>permission state</a>.</p>
+    <p>The <a>remote end steps</a> are:</p>
+    <ol>
+      <li>Let |permissionDesc| be the result of <a>getting the property</a>
+      "`descriptor`" from the |parameters| argument.</li>

Please indent continuation lines inside `<li>` elements.

> +      <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Let |rootDesc| be the object |permissionDesc| refers to, <a>converted
+      to an IDL value</a> of type {{PermissionDescriptor}}. If this throws an
+      exception, return <a>error</a> with <a>error code</a> <a>invalid
+      argument</a>.</li>
+      <li>Let |typedDescriptor| be the object |permissionDesc| refers to,
+      <a>converted to an IDL value</a> of
+      <code>|rootDesc|.{{PermissionDescriptor/name}}</code>'s <a>permission
+      descriptor type</a>. If this throws an exception, return <a>error</a>
+      with <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Let |settings| be the <a>current settings object</a>.</li>
+      <li>If |settings| is a <a>non-secure context</a> and
+      <code>|rootDesc|.{{PermissionDescriptor/name}}</code> isn't <a>allowed in
+      non-secure contexts</a>, return <a>error</a> with <a>error code</a>
+      <a>invalid argument</a>.</li>
+      <li>Let |state| by the result of <a>getting the property</a> "`state`"

s/by/be/

> +      <a>converted to an IDL value</a> of
+      <code>|rootDesc|.{{PermissionDescriptor/name}}</code>'s <a>permission
+      descriptor type</a>. If this throws an exception, return <a>error</a>
+      with <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Let |settings| be the <a>current settings object</a>.</li>
+      <li>If |settings| is a <a>non-secure context</a> and
+      <code>|rootDesc|.{{PermissionDescriptor/name}}</code> isn't <a>allowed in
+      non-secure contexts</a>, return <a>error</a> with <a>error code</a>
+      <a>invalid argument</a>.</li>
+      <li>Let |state| by the result of <a>getting the property</a> "`state`"
+      from the |parameters| argument.</li>
+      <li>If |state| is neither "`granted`", "`prompt`", nor "`denied`", return
+      <a>error</a> with <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Let |allRealms| be the result of <a>getting the property</a>
+      "`allRealms`" from the |parameters| argument.</a></li>
+      <li>If |allRealms| is `undefined`, let |allRealms| be false.</li>

It would probably be simpler to write a [WebIDL dictionary](https://heycam.github.io/webidl/#idl-dictionaries) that *parameters* needs to be an instance of, and convert it all at once. That'd make it easy to default *allRealms*, typecheck *state*, etc.

> +      <code>|rootDesc|.{{PermissionDescriptor/name}}</code> isn't <a>allowed in
+      non-secure contexts</a>, return <a>error</a> with <a>error code</a>
+      <a>invalid argument</a>.</li>
+      <li>Let |state| by the result of <a>getting the property</a> "`state`"
+      from the |parameters| argument.</li>
+      <li>If |state| is neither "`granted`", "`prompt`", nor "`denied`", return
+      <a>error</a> with <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Let |allRealms| be the result of <a>getting the property</a>
+      "`allRealms`" from the |parameters| argument.</a></li>
+      <li>If |allRealms| is `undefined`, let |allRealms| be false.</li>
+      <li>If |allRealms| is neither true nor false, return <a>error</a> with
+      <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Interpret the value of |state| as <a>new information about the user's
+      intent</a>. If |allRealms| is true, this information applies to other
+      <a>realms</a> with the <a>same origin</a>.
+      <li>Return <a>success</a> with data `null`.</li>

* Chrome has trouble implementing a 1-realm permission grant
* Firefox defaults to ephemeral grants, meaning the permission state stays "prompt" even if the user grants permission. If you override that, it becomes an all-realm grant. I don't know if they can do a non-ephemeral 1-realm grant.
* Safari defaults to 1-realm grants, and if you override that it becomes an all-realm grant.

In order for the automation API to work, it has to select Firefox's persistent mode, so I think I'd vote for making all-realm the default. @jan-ivar, got any counter-proposals?

I agree that we shouldn't have optional arguments that default to `true`.

> +      with <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Let |settings| be the <a>current settings object</a>.</li>
+      <li>If |settings| is a <a>non-secure context</a> and
+      <code>|rootDesc|.{{PermissionDescriptor/name}}</code> isn't <a>allowed in
+      non-secure contexts</a>, return <a>error</a> with <a>error code</a>
+      <a>invalid argument</a>.</li>
+      <li>Let |state| by the result of <a>getting the property</a> "`state`"
+      from the |parameters| argument.</li>
+      <li>If |state| is neither "`granted`", "`prompt`", nor "`denied`", return
+      <a>error</a> with <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Let |allRealms| be the result of <a>getting the property</a>
+      "`allRealms`" from the |parameters| argument.</a></li>
+      <li>If |allRealms| is `undefined`, let |allRealms| be false.</li>
+      <li>If |allRealms| is neither true nor false, return <a>error</a> with
+      <a>error code</a> <a>invalid argument</a>.</li>
+      <li>Interpret the value of |state| as <a>new information about the user's

We indirect a lot of the permission API stuff through ["new information about the user's intent"](https://w3c.github.io/permissions/#new-information-about-the-users-intent) in order to give UAs lots of freedom about how to infer the right permission state. However, in an automation API, we don't want to give that freedom, so I think you should just say 
> The UA must act as if it returned *state* from an invocation of *typedDescriptor*'s [permission state](https://w3c.github.io/permissions/#permission-state) for *settings* at this point.

They may be better ways to word that, but I'm trying to hook into the "If there was a previous invocation of this algorithm ..." wording.

If *allRealms* is `true`, you'll post a task to all other realms with the same origin to do the same thing.

We'll have some trouble implementing this for the always-granted APIs like `"midi"`, so you may want to let the UA explicitly reject the update.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/permissions/pull/151#pullrequestreview-64715364
Received on Friday, 22 September 2017 23:50:19 UTC