Re: [w3c/manifest] Add shortcuts feature to the explainer and spec (#768)

mgiuca requested changes on this pull request.

Hi Aaron,

I just saw this! Sorry (I was on leave in June). This is great, I'm glad we're moving ahead on this proposal.

I have some comments and questions but largely this is pretty polished.

> @@ -2368,6 +2373,98 @@ <h3>
           can get their games and apps rated with IARC</a>.
         </p>
       </section>
+      <section>
+        <h3>
+          <code>shortcuts</code> member
+        </h3>
+        <p>
+          The <dfn>shortcuts</dfn> member is an <a>array</a> of
+          <a>ShortcutItem</a>s that provide access to key tasks within a web
+          application. For example, they can be used to link directly to a
+          user’s timeline within a social media application or their recent

nit: There's a fancy apostrophe here; I think this should just be a regular ASCII apostrophe (maybe search the document for other instances of this).

> +          The <a>icons</a> member of an <a>ShortcutItem</a> member is an
+          <a>array</a> of <a>ImageResource</a>s that can serve as iconic
+          representations of the shortcut in various contexts.
+        </p>
+      </section>
+      <section>
+        <h3>
+          <dfn>Launching a shortcut</dfn>
+        </h3>
+        <p>
+          When <a>ShortcutItem</a> <var>shortcut</var> having
+          <a>WebAppManifest</a> <var>manifest</var> is invoked, run the
+          following steps:
+        </p>
+        <ol>
+          <li>Let <var>url</var> be a copy of <var>shortcut.url</var>.

I'm not sure we need it there either.

> +        <p>
+          The steps for <dfn>processing the <code>shortcuts</code> member</dfn>
+          are given by the following algorithm. The algorithm takes a
+          <a data-cite=
+          "WEBIDL#sequence-type">sequence</a>&lt;<a>ShortcutItem</a>&gt;
+          <var>shortcuts</var> as an argument. This algorithm returns an
+          <a data-cite=
+          "WEBIDL#sequence-type">sequence</a>&lt;<a>ShortcutItem</a>&gt;. For
+          each <var>shortcut</var> (<a>ShortcutItem</a>) in the sequence, set
+          <var>shortcut.icons</var> to the result of running <a>processing
+          `ImageResource` members</a> given <var>shortcut.icons</var> and
+          <var>manifest URL</var>.
+        </p>
+        <p>
+          A user agent MUST expose shortcuts via interactions that are
+          consistent with exposure of an application icon’s context menu in the

here too

> +        </p>
+        <p>
+          The steps for <dfn>processing the <code>shortcuts</code> member</dfn>
+          are given by the following algorithm. The algorithm takes a
+          <a data-cite=
+          "WEBIDL#sequence-type">sequence</a>&lt;<a>ShortcutItem</a>&gt;
+          <var>shortcuts</var> as an argument. This algorithm returns an
+          <a data-cite=
+          "WEBIDL#sequence-type">sequence</a>&lt;<a>ShortcutItem</a>&gt;. For
+          each <var>shortcut</var> (<a>ShortcutItem</a>) in the sequence, set
+          <var>shortcut.icons</var> to the result of running <a>processing
+          `ImageResource` members</a> given <var>shortcut.icons</var> and
+          <var>manifest URL</var>.
+        </p>
+        <p>
+          A user agent MUST expose shortcuts via interactions that are

These MUSTs should be SHOULDs. We can't mandate what the user agent does with its own (or the OS's) UI. From a spec point of view, we treat the UA and OS as a single entity --- if the UA wants to invent a whole new mechanism for displaying the icon that's different to how the OS normally does it, well that's fine. It could also display these things inside the browser UI, not the OS at all.

We also can't impose a MUST on the ordering; the user agent could provide the list of shortcuts to the OS and then the OS may sort them alphabetically, or sort them based on how often the user presses each one.

> +          <a>top-level browsing context</a>.
+          </li>
+          <li>
+            <a>Navigate</a> <var>browsing context</var> to a new <a>request</a>
+            whose url is <var>url</var>, header list is <var>header list</var>,
+            and body is <var>body</var>.
+          </li>
+        </ol>
+      </section>
+    </section>
+    <section>
+      <h2>
+        <dfn>RequestParams</dfn>
+      </h2>
+      <pre class="idl">
+        dictionary RequestParams {

Why is this an empty dictionary? I guess you intend this to contain arbitrary key/values (not specifying which ones)? I don't think a dictionary is appropriate here. This should be an IDL [record](https://heycam.github.io/webidl/#idl-record).

However, I don't think we want RequestParams at all --- see comment above.

> +      <section>
+        <h3>
+          <dfn>Launching a shortcut</dfn>
+        </h3>
+        <p>
+          When <a>ShortcutItem</a> <var>shortcut</var> having
+          <a>WebAppManifest</a> <var>manifest</var> is invoked, run the
+          following steps:
+        </p>
+        <ol>
+          <li>Let <var>url</var> be a copy of <var>shortcut.url</var>.
+          </li>
+          <li>Let <var>header list</var> be a new empty <a data-xref-for=
+          "request">header list</a>.
+          </li>
+          <li>If <var>shortcut.params</var> <a data-cite="infra/#list-contain">

Why do we need `shortcut.params`? This doesn't let the user, user agent, or site dynamically inject parameters into the URL, does it? Like, it seems that the params are a static set of key/value pairs defined in the manifest alongside `url`. Thus, why do we need to specify this as a separate dictionary that gets encoded into a query string? Why not just let the manifest specify query parameters in the `url` directly?

As an example, say you had this:

```json
"shortcuts": [
  {
      "name": "Subscriptions",
      "url": "/subscriptions",
      "params": {
        "sortby": "ascending"
      }
  }
]
```

why not just express it as this:

```json
"shortcuts": [
  {
      "name": "Subscriptions",
      "url": "/subscriptions?sortby=ascending"
  }
]
```

-- 
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/manifest/pull/768#pullrequestreview-285285220