Re: [w3c/manifest] Adding field `display_override` to the manifest (#932)

@mgiuca approved this pull request.

Great work, Daniel! Sorry this has been so long in the making.

I've approved but added some minor comments. One is a type problem we're going to have in the future, but isn't currently a problem, so I've added a long comment but the effect of it is just to add a note about it, for now.

> @@ -207,7 +207,28 @@ <h3>
               "type": "image/jpeg"
             }]
           }
-      </pre>
+        </pre>
+        <p>
+          The following shows a <a>manifest</a> that prefers the <code>
+          minimal-ui</code> [=display modes|display mode=]
+          to <code>standalone</code>, but if minimal-ui isn't supported, fall
+          back to standalone as opposed to browser.
+        </p>
+        <pre class="example json" title="Advanced display usage manifest">
+          {
+            "name": "Recipe Madness",

I'm not sure how big of a deal this particular one is, but given the recent efforts around inclusive language, it might be best to avoid the term "madness" here and perhaps just go "Recipe Book", or if you want to spice it up, "Recipe Time", "Recipe Bonanza" or "Recipe Zone", etc.

> +        [=processed manifest=] |manifest:processed manifest| and returns a
+        [=display mode=].
+        <ol>
+          <li>
+            [=list/For each=] |candidate_display_mode:DisplayModeType| of
+            |manifest|.{{WebAppManifest/display_override}}:
+            <ol>
+              <li>
+                If the user agent supports the |candidate_display_mode|, then
+                return |candidate_display_mode|.
+              </li>
+            </ol>
+          </li>
+          <li>
+            [=list/For each=] |fallback_mode:DisplayModeType| of the
+            fallback_chain of |manifest|.{{WebAppManifest/display}}:

Remove the underscore in "`fallback_chain`" since this is a prose term defined above without an underscore.

And link to the above definition (see above comment to use `<dfn>`). I think you can use `{{fallback chain}}` for this. Or maybe `[=fallback chain=]`. I usually guess until it works :)

> @@ -577,30 +598,92 @@ <h2>
         mode</a>.
       </p>
       <p>
-        Each <a>display mode</a>, except <code>browser</code>, has a
-        <dfn>fallback display mode</dfn>, which is the <a>display mode</a> that
-        the user agent can try to use if it doesn't support a particular
-        <a>display mode</a>. If the user agent does support a <a>fallback
-        display mode</a>, then it checks to see if it can use that <a>display
-        mode</a>'s <a>fallback display mode</a>. This creates a fallback chain,
-        with the <a>default display mode</a> (<code>browser</code>) being the
-        last item in the chain.
+        The fallback chain for:

Use `<dfn>` around "fallback chain" to define it for use below.

> @@ -1044,6 +1110,7 @@ <h2>
              DOMString iarc_rating_id;
              USVString start_url;
              DisplayModeType display = "browser";
+             sequence&lt;DisplayModeType&gt; display_override;

I just thought of a problem (which is not a problem right now, but will be as soon as we add a new display mode): we need a way to say for new display modes that they are allowed in `display_override` but not `display`.

We _either_ need to do this in the type system (so it is type-impossible to use, say, `"display": "tabbed"`, but you can use `"display_override": ["tabbed"]`), or have steps in the algorithm to fail if you use a "new" display mode in `display`. My preference is the former.

I think what we should do is have two enums, for now let's just call them "DisplayModeType" and "EnhancedDisplayModeType". The IDL should have:

```webidl
             DisplayModeType display = "browser";
             sequence<DisplayModeType or EnhancedDisplayModeType> display_override;
```

And never put a new value in `DisplayModeType`.

That allows you to use the new ones in `display_override` but not `display`, and we don't need to mention this in the algorithms at all. Does that sound good? We could also rename the old one to `LegacyDisplayModeType` or something, I don't really mind.

If we do that now, `EnhancedDisplayModeType` will be empty, so maybe we just add a note to do that in the future? Let's add a note just under the listing of `DisplayModeType`'s possible values, that no new values will be added to this, and future display modes will be added to a new enum that can only be used with `display_override` but not `display`, for forwards-compatibility reasons.

> +              <li>
+                If the user agent supports the |candidate_display_mode|, then
+                return |candidate_display_mode|.
+              </li>
+            </ol>
+          </li>
+          <li>
+            [=list/For each=] |fallback_mode:DisplayModeType| of the
+            fallback_chain of |manifest|.{{WebAppManifest/display}}:
+            <ol>
+              <li>
+                If the user agent supports the |fallback_mode|, then
+                return |fallback_mode|.
+              </li>
+            </ol>
+          </li>

Add "[Assert](https://infra.spec.whatwg.org/#assert): false", to explicitly state that you should not be able to exit the second loop.

> +              </li>
+            </ol>
+          </li>
+          <li>
+            [=list/For each=] |fallback_mode:DisplayModeType| of the
+            fallback_chain of |manifest|.{{WebAppManifest/display}}:
+            <ol>
+              <li>
+                If the user agent supports the |fallback_mode|, then
+                return |fallback_mode|.
+              </li>
+            </ol>
+          </li>
+        </ol>
+        <p class="note">
+          The above loop is guaranteed to return a value before it terminates,

s/it terminates/the assertion/

-- 
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/932#pullrequestreview-491139048