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

@mgiuca commented on this pull request.



> +        by the following algorithm. The algorithm takes a
+        [=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>
+            Define the |fallback_chain| for {{browser}} to be [{{browser}}]

Nit: End each of these with a period.

> @@ -577,30 +598,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.
+      </p>
+      <p>
+        The steps for determing the web app's chosen display mode is given

Put "steps for determing the web app's chosen display mode" in a `<dfn>` even though it isn't invoked from anywhere (makes the algorithm name bold and potentially allows linking).

> +        by the following algorithm. The algorithm takes a
+        [=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>
+            Define the |fallback_chain| for {{browser}} to be [{{browser}}]

Don't make *fallback_chain* a variable, since it doesn't act like a variable, it's more like a concept (strictly speaking, you could think of it as a function, `fallback_chain(display_mode) = ...`, but I think it's not worth being so formal about it).

So I think move these definitions outside of this algorithm, and express them just in plain English: "The fallback chain for `browser` is «`browser`»"., etc. (Note: Use «» to denote a literal list, not, [], per [Infra](https://infra.spec.whatwg.org/#lists).)

Then down below, replace "the *fallback_chain* of" with "the fallback chain of" and you're good.

> +            Define the |fallback_chain| for <a data-link-for="DisplayModeType">
+            minimal-ui</a> to be [<a data-link-for="DisplayModeType">minimal-ui
+            </a>, {{browser}}]
+          </li>
+          <li>
+            Define the |fallback_chain| for {{standalone}} to be [
+            {{standalone}}, <a data-link-for="DisplayModeType">minimal-ui</a>,
+            {{browser}}]
+          </li>
+          <li>
+            Define the |fallback_chain| for {{fullscreen}} to be [
+            {{fullscreen}}, {{standalone}}, <a data-link-for="DisplayModeType">
+            minimal-ui</a>, {{browser}}]
+          </li>
+          <li>
+            Let |candidate_display_mode| be the the value specified by the

Can you combine this and the next item into one line, to avoid reassigning the *candidate_display_mode* variable?

i.e.: "For each *fallback_mode* of the fallback chain of *manifest*.`display`."

> +            Define the |fallback_chain| for {{fullscreen}} to be [
+            {{fullscreen}}, {{standalone}}, <a data-link-for="DisplayModeType">
+            minimal-ui</a>, {{browser}}]
+          </li>
+          <li>
+            Let |candidate_display_mode| be the the value specified by the
+            |manifest|.{{WebAppManifest/display}} property.
+          </li>
+          <li>
+            [=list/For each=] |fallback_mode:DisplayModeType| of the
+            |fallback_chain| of the |candidate_display_mode|:
+            <ol>
+              <li>
+                If the user agent supports the |fallback_mode|, then
+                return |fallback_mode|.
+              </li>

The above suggestions would collapse this algorithm into just two loops: one to do display_override and one to do the fallback chain of display. I think that would look quite nice.

Arguably it's still cleaner to do my original suggestion, which is to do a single loop over "the [concatenation](https://infra.spec.whatwg.org/#string-concatenate) of *manifest*.`display_override` and the fallback chain of *manifest*.`display`".

But I will leave that up to you, since I can see how it's maybe easier to read having two loops, each with a distinct purpose.

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

Technically, this algorithm is missing a final return at the end of the loop (as in, something that many compilers would complain about). Infra doesn't require that we return something here, but can we add a `<p class="note">` acknowledging it:

"The above loop is guaranteed to return a value before it terminates, due to the fact that `browser` is in every mode's fallback chain, and the requirement that all user agents support the `browser` display mode."

> @@ -653,9 +720,8 @@ <h2>
           window).
         </dd>
         <dd>
-          The <code>browser</code> <a>display mode</a> doesn't have a
-          <a>fallback display mode</a> (conforming user agents are required to
-          support the <code>browser</code> <a>display mode</a>).
+          Conforming user agents are required to

I think you can just delete this paragraph. It's redundant for the sentence above: "As such, the user agent MUST support the browser display mode."

I think it was redundantly here before just to explain why it's OK to not have a fallback display mode. That's equivalent to the new note that I suggested you add above.

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