- From: Matt Giuca <notifications@github.com>
- Date: Wed, 29 Jul 2020 01:13:20 -0700
- To: w3c/manifest <manifest@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/manifest/pull/932/review/457272018@github.com>
@mgiuca commented on this pull request.
Hi Daniel,
Great! Thanks for submitting this PR and getting your feet wet!
I've put a number of comments. My review was somewhat hasty so I didn't give you the precise syntax or wording in a lot of my suggestions, but I think you can figure it out and ping me if you need exact suggested wording or syntax advice.
Apologies: As I said in one of the comments, it's a bit hard to cargo-cult the syntax from elsewhere in this file, since Respec changed the syntax since this was written.
Also I think you already found this, but the language that these algorithms is written in is HTML [Infra](https://infra.spec.whatwg.org/); it's a semi-formal language and we should try to follow those conventions rather than just writing English pseudo-code.
Cheers!
Matt
> + </pre>
+ <p>
+ The following shows a <a>manifest</a> that prefers the <code>
+ minimal-ui</code> <a data-link-for="DisplayModeType">display mode</a>
+ to <code>standalone</code>, but wants to support both.
+ </p>
+ <pre class="example json" title="Advanced display requirements manifest">
+ {
+ "name": "Recipe Madness",
+ "description": "All of the recipes!",
+ "icons": [{
+ "src": "icon/hd_hi",
+ "sizes": "128x128"
+ }],
+ "start_url": "/index.html",
+ "display_override": ["minimal-ui", "standalone"],
I'm not quite clear on what this site's intentions are (maybe make that more clear in the description above).
They want minimal-ui, but if that's not supported fall back to standalone, but if display-override isn't supported, fall back to browser? I don't really see why you'd ever want this.
Wouldn't they either want display-override: minimal, standalone, and display: minimal, or display-override: minimal, display: standalone?
> @@ -586,17 +606,72 @@ <h2>
with the <a>default display mode</a> (<code>browser</code>) being the
last item in the chain.
</p>
+ <p>
I think you should edit the paragraph above this (from the existing text), since it's somewhat redundant with the algorithm you propose below (it describes the fallback chain normatively). Either delete parts of it, or put them into a non-normative note, with the below algorithm becoming the normative description of the fallback chain.
> @@ -586,17 +606,72 @@ <h2>
with the <a>default display mode</a> (<code>browser</code>) being the
last item in the chain.
</p>
+ <p>
+ For more advanced usages, the <a data-link-for="WebAppManifest">
+ display_override</a> member can be used to specify a custom fallback
+ order. This member is valid IFF <a data-link-for="WebAppManifest">display</a> is also present.
+ </p>
+ <p>
+ The algorithm for determing the web app's requested display mode is:
+ <ol>
+ <li>[=list/For each=] <var>considering_display_mode</var> of <var>
Perhaps rename `considering_display_mode` to `candidate_display_mode`?
Also instead of `<var>`, use `|...|`. (Sorry .. cargo culting in this file is hard because the syntax has changed over time, so a lot of the file uses legacy HTML-like syntax when we prefer the new respec syntax.)
> @@ -586,17 +606,72 @@ <h2>
with the <a>default display mode</a> (<code>browser</code>) being the
last item in the chain.
</p>
+ <p>
+ For more advanced usages, the <a data-link-for="WebAppManifest">
+ display_override</a> member can be used to specify a custom fallback
+ order. This member is valid IFF <a data-link-for="WebAppManifest">display</a> is also present.
+ </p>
+ <p>
+ The algorithm for determing the web app's requested display mode is:
This should be defined and called "steps" not algorithm:
```html
The steps for <dfn>determining the web app's requested display mode</dfn>
```
(Also note s/determing/determining)
> @@ -586,17 +606,72 @@ <h2>
with the <a>default display mode</a> (<code>browser</code>) being the
last item in the chain.
</p>
+ <p>
+ For more advanced usages, the <a data-link-for="WebAppManifest">
+ display_override</a> member can be used to specify a custom fallback
+ order. This member is valid IFF <a data-link-for="WebAppManifest">display</a> is also present.
+ </p>
+ <p>
+ The algorithm for determing the web app's requested display mode is:
Also define the inputs and outputs. See another algorithm as an example: https://www.w3.org/TR/appmanifest/#color-parsing
This algorithm should take a `[=processed manifest=] |manifest:processed manifest|` and return a display mode.
(Note that the syntax `|name:type|` optionally specifies a type for the variable, which shows when you mouse over it.)
> @@ -586,17 +606,72 @@ <h2>
with the <a>default display mode</a> (<code>browser</code>) being the
last item in the chain.
</p>
+ <p>
+ For more advanced usages, the <a data-link-for="WebAppManifest">
+ display_override</a> member can be used to specify a custom fallback
+ order. This member is valid IFF <a data-link-for="WebAppManifest">display</a> is also present.
+ </p>
+ <p>
+ The algorithm for determing the web app's requested display mode is:
+ <ol>
+ <li>[=list/For each=] <var>considering_display_mode</var> of <var>
+ <a data-link-for="WebAppManifest">display_override</a></var>:
Instead of just saying "of `display_override`", say `|manifest|.display_override` (I forget the exact syntax to link it). So like real code, we're referring to a member of the *manifest* argument to this algorithm.
> @@ -586,17 +606,72 @@ <h2>
with the <a>default display mode</a> (<code>browser</code>) being the
last item in the chain.
</p>
+ <p>
+ For more advanced usages, the <a data-link-for="WebAppManifest">
+ display_override</a> member can be used to specify a custom fallback
+ order. This member is valid IFF <a data-link-for="WebAppManifest">display</a> is also present.
+ </p>
+ <p>
+ The algorithm for determing the web app's requested display mode is:
+ <ol>
+ <li>[=list/For each=] <var>considering_display_mode</var> of <var>
+ <a data-link-for="WebAppManifest">display_override</a></var>:
+ <ol>
+ <li>
+ If the user agent supports the |considering_display_mode|,
+ then that display mode is used (exit).
s/then that display mode is used (exit)/return `|considering_display_mode|`.
> + <li>
+ If the user agent supports the |considering_display_mode|,
+ then that display mode is used (exit).
+ </li>
+ </ol>
+ </li>
+ <li>
+ Let |considering_display_mode| be the the value specified by the
+ <a data-link-for="WebAppManifest">display</a> property.
+ </li>
+ <li>
+ If the user agent supports the |considering_display_mode|, then
+ that display mode is used (exit).
+ </li>
+ <li>
+ While |considering_display_mode| has <a>fallback display mode</a>,
I'd like to find a way to express this in a normal while loop rather than effectively using a goto.
I think the right approach here is to essentially allow considering display mode to be undefined. Hasty pseudo-code:
1. Let *considering* be the display property.
2. While *considering* is not undefined:
1. If the user agent supports *considering*, return *considering*.
2. Set *considering* to the fallback display mode of *considering* (which is undefined if it has none).
Alternatively, you don't really need a condition, since as your note below says, we're guaranteed to terminate with browser. So you could just do:
1. Let *considering* be the display property.
2. While true:*
1. If the user agent supports *considering*, return *considering*.
2. Set *considering* to the fallback display mode of *considering*.
\* I'm not sure what the idiomatic way to do a while-true loop is in [Infra](https://infra.spec.whatwg.org/). There doesn't seem to be any special syntax for "Loop until broken" or "Loop forever". (I've been writing Rust code and I quite like that they have a special keyword `loop {}` for this case, to avoid the somewhat ugly `while true {}` in other languages.)
> + </ol>
+ </li>
+ <li>
+ Let |considering_display_mode| be the the value specified by the
+ <a data-link-for="WebAppManifest">display</a> property.
+ </li>
+ <li>
+ If the user agent supports the |considering_display_mode|, then
+ that display mode is used (exit).
+ </li>
+ <li>
+ While |considering_display_mode| has <a>fallback display mode</a>,
+ assign that to |considering_display_mode| and go back to step 3.
+ </li>
+ </ol>
+ This algorithm will end with the final <a>fallback display mode</a> of
Put this in a p class="note" so it's non-normative.
> <div class="example">
<p>
- For example, SuperSecure Browser (a fictitious browser) only supports
+ Example: SuperSecure Browser (a fictitious browser) only supports
Can just drop "Example:"
> @@ -1374,6 +1449,20 @@ <h3>
developer's preferred <a>display mode</a> for the web application.
</p>
</section>
+ <section>
+ <h3>
+ <code>display_override</code> member
+ </h3>
+ <p>
+ The <dfn>display_override</dfn> member is a <a>sequence</a> of<a>
+ DisplayModeType</a>. This item represents the developer's preferred
+ fallback chain for display modes. This field overrides the
Link "fallback chain"
> supports <code>fullscreen</code> (it doesn't), so it falls back to
<code>standalone</code> (which it also doesn't support), and
ultimately falls back to <code>minimal-ui</code>.
</p>
</div>
+ <div class="example">
+ <p>
+ Example: Same browser as above (SuperSecure Browser),
+ but the developer instead specified the following manifest using
+ display_override:
+ <pre class="example json" title="Using display_override">
+ {
+ display_override: ["fullscreen"],
+ display: "browser"
+ }
+ </pre>
+ This declares that she only wants <code>fullscreen</code>, and if
Avoid using gendered pronouns (in general, best to avoid talking about the developer and just the relationship between the site and the user agent, which is what this spec cares about). Just "This declares that the user agent should use fullscreen, and if that is not supported, it should fall back to browser without considering the other modes."
> @@ -1374,6 +1449,20 @@ <h3>
developer's preferred <a>display mode</a> for the web application.
</p>
</section>
+ <section>
+ <h3>
+ <code>display_override</code> member
You also need to add an entry for `display_override` in the Web App Manifest WebIDL (search for `dictionary WebAppManifest`).
--
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-457272018
Received on Wednesday, 29 July 2020 08:13:34 UTC