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

@mgiuca commented on this pull request.

Thanks Daniel. Looking great now.

A few minor comments, and then a suggestion to rewrite the algorithm which you can either accept or reject. Not sure which is cleaner, so have a think about it.

And a good weekend!

> @@ -207,7 +207,27 @@ <h3>
               "type": "image/jpeg"
             }]
           }
-      </pre>
+        </pre>
+        <p>
+          The following shows a <a>manifest</a> that prefers the <code>
+          minimal-ui</code> {{DisplayModeType}}

s/`{{DisplayModeType}}`/display mode/ (since this section is quite informal and doesn't refer to specific pieces of spec-language).

> @@ -207,7 +207,27 @@ <h3>
               "type": "image/jpeg"
             }]
           }
-      </pre>
+        </pre>
+        <p>
+          The following shows a <a>manifest</a> that prefers the <code>
+          minimal-ui</code> {{DisplayModeType}}
+          to <code>standalone</code>, but wants to support both.

s/wants to support both/if `minimal-ui` isn't supported, fall back to `standalone` as opposed to `browser`.

> @@ -577,30 +597,88 @@ <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.
+        For more advanced usages, the {{WebAppManifest/display_override}}
+        member can be used to specify a custom fallback order. This member is
+        valid IFF {{WebAppManifest/display}} is also present.

I don't think we can have this requirement. Since `display` defaults to "browser" (at the IDL level), having it omitted is exactly the same as writing "display: browser". So we can't write a rule that says "if display is "browser" then display_override is valid, but if display is missing, then display_override is invalid." Thus, just delete this sentence.

Corollary: It is actually valid to have display_override without display, as long as you're happy for legacy browsers to use the browser display mode.

`----` I wrote the below earlier but I don't think it's relevant given what I just said above:

"IFF" isn't technical language in this place. We should reword this as a normative requirement for the user agent, e.g. "If `display` is undefined, set `display_override` to undefined." I think that language belongs in the **steps for processing a manifest**, not here (it's part of the algorithm).

Maybe also add a `<p class="note">` here with the human-understandable form, "The `display_override` member is ignored if `display` is not present."

> @@ -577,30 +597,88 @@ <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.
+        For more advanced usages, the {{WebAppManifest/display_override}}
+        member can be used to specify a custom fallback order. This member is
+        valid IFF {{WebAppManifest/display}} is also present.
+      </p>
+      <p>
+        The steps for determing the web app's requested display mode is given

"web app's requested display mode" is kinda the wrong name, given that it's the *resolved* display mode you're talking about. (The "requested" display mode is the first element of display_override.)

How about "the effective display mode" or "the chosen display mode"?

> +          <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>
+            Let |candidate_display_mode| be the the value specified by the
+            |manifest|.{{WebAppManifest/display}} property.
+          </li>
+          <li>
+            [=iteration/while=] |considering_display_mode| is not {{browser}}

1. I think you can capitalize the W of while and it will still link properly.
2. You forgot to update `considering_display_mode` to `candidate_display_mode` here.

> +            <ol>
+              <li>
+                If the user agent supports the |candidate_display_mode|, then
+                return |candidate_display_mode|.
+              </li>
+            </ol>
+          </li>
+          <li>
+            Let |candidate_display_mode| be the the value specified by the
+            |manifest|.{{WebAppManifest/display}} property.
+          </li>
+          <li>
+            [=iteration/while=] |considering_display_mode| is not {{browser}}
+            <ol>
+              <li>
+                If the user agent supports the |candidate_display_mode|, then

I'm thinking about this while loop. I think it's correct but it's still a bit messy to do a loop with a switch in it when essentially you're just doing a linear scan through a list.

How's this (thinking out loud ... this might be messier, feel free to ignore this and keep the while loop).

First, some definitions:

1. The fallback chain for browser is [browser].
2. The fallback chain for minimal-ui is [minimal-ui] + the fallback chain for browser.
3. The fallback chain for standalone is [standalone] + the fallback chain for minimal-ui.
4. The fallback chain for fullscreen is [fullscreen] + the fallback chain for standalone.

(Or you could explicitly spell out the full fallback chain for each mode.)

Then, in the algorithm:

1. First thing you do is go "the display fallback chain is `display_override` + the fallback chain for `display`".
2. For each candidate of display fallback chain, if the user agent supports candidate, return it.

(Effectively, you combine the entire fallback chain into a single list combining both fields, just like you do in the explainer, and scan over it in one general iteration.)

WDYT?

-- 
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-463057007