- From: Matt Giuca <notifications@github.com>
- Date: Tue, 03 Oct 2017 03:33:11 +0000 (UTC)
- To: w3c/manifest <manifest@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/manifest/pull/613/review/66640754@github.com>
mgiuca requested changes on this pull request.
Hi Ken,
I've done a fairly thorough review of the changes. Here are my thoughts.
> - </li>
- <li>Let <var>service worker registration</var> of <var>parsed
- manifest</var> be the result of running the <a>steps for processing
- the <code>serviceworker</code> member</a> with <var>manifest</var>,
- <var>manifest URL</var>, and <var>serviceworker</var> as arguments.
- </li>
- <li>Let <var>display mode</var> of <var>parsed manifest</var> be the
- result of running the <a>steps for processing the
- <code>display</code> member</a> with <var>manifest</var> as the
- argument.
- </li>
- <li>Let <var>orientation</var> of <var>parsed manifest</var> be the
- result of running the <a>steps for processing the
- <code>orientation</code> member</a> with <var>manifest</var> and
- <var>display mode</var> as arguments.
+ <li>Let <a>WebAppManifest</a> <var>manifest</var> be the result of
I think you should not literally say "converting json to IDL dictionary value"; although it's obvious from context, that doesn't explicitly state which dictionary type you are converting to. (In WebIDL, the algorithm is called "An ECMAScript value V is converted to **an IDL dictionary type value**" -- emphasis mine, meaning, this is the algorithm whenever a value is converted to a type which is declared as a dictionary).
So you should write:
Let <var>manifest</var> be the result of <a data-dfn="to-idl-dictionary-value">converting</a> <var>json</var> to a <a>WebAppManifest</a>.
> </li>
- <li>Let <var>language</var> of <var>parsed manifest</var> be the
- result of running the <a>steps for processing the <code>lang</code>
+ <li>Let <var>manifest["<a>display</a>"]</var> be the result of
+ running the <a>steps for post-processing the <code>display</code>
Why not have the "steps for post-processing" each field take the field value, instead of the entire manifest?
So here it would say "running the steps for post-processing the `display` member with *manifest["display"]* as the argument", and then later on where that is defined, it would say "The algorithm takes a DisplayModeType *value* as an argument, and returns a DisplayModeType." Seems a bit cleaner this way.
> </h2>
+ <pre class="idl">
+ dictionary WebAppManifest {
+ TextDirectionType? dir;
Why are all of these types nullable (ending in `?`)?
Dictionary members are always optional unless otherwise specified. Adding `?` means the user can explicitly specify `null` as a value, which I don't think we want. So I would remove all the `?`s.
> </li>
- <li>Let <var>name</var> of <var>parsed manifest</var> be the result
- of running the <a>steps for processing the <code>name</code>
- member</a> with <var>manifest</var> as the argument.
- </li>
- <li>Let <var>description</var> of <var>parsed manifest</var> be the
- result of running the <a>steps for processing the
- <code>description</code> member</a> with <var>manifest</var> as the
- argument.
+ <li>Let <var>manifest["<a>start_url</a>"]</var> be the result of
So we're removing the Trim() logic for all the strings (except categories... why?). I'm not really comfortable with this until we know whether it will break sites in the wild.
Chromium, at least, [certainly does trim the strings](https://cs.chromium.org/chromium/src/content/renderer/manifest/manifest_parser.cc), so we would presumably remove that logic if it was removed from the manifest. I think we need to do an analysis of how many real-world manifests contain leading or trailing whitespace in these strings, and whether it would be safe to remove the trim() step. **I am looking into this now.**
For now, I think we should leave it, which unfortunately means each of the strings needs a post-processing step to trim it. However, for all the trim-only members, how about instead of writing "running the steps for post-processing X", you just write "Let *manifest[X]* be the result of running Trim(*manifest[X]*)". That's reasonably short.
Unfortunately, you can't trim enum values like "display" after converting to the WebAppManifest dictionary type, because it will already be a TypeError. Instead, you need to pre-process by trimming the string values in "json" before converting.
> </h2>
+ <pre class="idl">
+ dictionary WebAppManifest {
+ TextDirectionType? dir;
+ DOMString? lang;
+ USVString? name;
+ USVString? short_name;
+ USVString? description;
+ sequence<ImageResource>? icons;
+ sequence<ImageResource>? screenshots;
+ sequence<USVString>? categories;
+ DOMString? iarc_rating_id;
+ USVString? start_url;
+ DisplayModeType? display;
I think this can have a default "`= "browser"`". See discussion below.
> <p>
- The <dfn id="member-display"><code>display</code> member</dfn> is a
- <a>string</a>, whose value is one of <a>display modes values</a>. The
- item represents the developer's preferred <a>display mode</a> for the
- web application. When the member is missing or erroneous, the user
- agent MUST use the <a>fallback display mode</a>.
+ The <dfn>display</dfn> member is a <a>DisplayModeType</a>, whose
+ value is one of <a>display modes values</a>. The item represents the
+ developer's preferred <a>display mode</a> for the web application.
+ When the member is missing or erroneous, the user agent MUST use the
This sentence isn't necessary; it's covered by the algorithm below (and/or the default above if one is added).
> </li>
- <li>If <a>Type</a>(<var>value</var>) is not "string":
- <ol>
- <li>If <a>Type</a>(<var>value</var>) is not "undefined", <a>issue
- a developer warning</a> that the type is unsupported.
- </li>
- <li>Return the <a>fallback display mode</a>'s value.
- </li>
- </ol>
- </li>
- <li>Otherwise, <a>Trim</a>(<var>value</var>), convert to lower-case,
- and set <var>value</var> to be the result.
+ <li>If <a>Type</a>(<var>V</var>) is <code>null</code> return the <a>
+ fallback display mode</a>'s value.
I'm confused about this (both the old and the new text, but since we're changing it, let's consider it): I think possibly this text has always been wrong, and should've said "default display mode", not "fallback display mode". Each display mode has a defined "fallback display mode" (e.g., "fullscreen"'s FDM is "standalone"), so it doesn't make sense to say "the fallback display mode" when it's undefined.
Rather it should say "the default display mode", but since that is defined as "browser", why not just say "browser"? And if you're going to say that, why not just set a default of "browser" in the IDL, and delete this entire post-processing step?
> with <var>manifest</var>, <var>manifest URL</var>, and "screenshots"
as arguments.
</li>
- <li>Return <var>parsed manifest</var>.
+ <li>Let <var>manifest["<a>related_applications</a>"]</var> be the
+ result of running the <a>steps for processing the
Why are this, and the next, ones called "processing" rather than "post-processing"?
> <ol>
- <li>Let <var>platform</var> be the result of running the <a>
- steps for processing the <code>platform</code> member of an
- application</a> with <var>potential application</var>.
- </li>
- <li>If <var>platform</var> is <code>undefined</code>, move
- onto the next item if any are left.
- </li>
- <li>Let <var>id</var> be the result of running the <a>steps
- for processing the <code>id</code> member of an
- application</a> with <var>potential application</var>.
+ <li>Let <var>V'</var> be a new object created as if by the
Can this just destructively update V?
Just delete Steps 1--4, and use *V* instead of *V'*. Should be equivalent, and simpler.
> </li>
- <li>If <var>unprocessed categories</var>is an array, then:
- <ol>
- <li>For each <var>potential category</var> in the array:
- <ol>
- <li>
- <a>Trim</a>(<var>value</var>) and convert <a>to ASCII
- lowercase</a>
- </li>
- <li>Append <var>potential category</var> to
- <var>categories</var>
- </li>
- </ol>
- </li>
- </ol>
+ <li>Initialize <var>i</var> to be 0.
Does this have to be an index-based loop? Can you just write "For each element of *A*, *e*: Trim(*e*) and convert to ASCII lowercase"? (Optionally, "updating the value in *A*" if you are worried about someone taking it too literally.)
> <li>
- <a>Issue a developer warning</a> that the type is not
- supported.
+ <a>Trim</a>(<var>V</var>) and convert <a>to ASCII lowercase</a>
Curious that this is the last remaining Trim(). As I said above, I think we should keep all the Trims, but if you are certain of taking them out, take this one out too.
> @@ -2689,56 +2383,33 @@ <h3 id="applying">
Manifest authors are encouraged to use lower-case.
</p>
<p class="note">
- This specification does not define the particular values for a the
- <a><code>categories</code> member</a>. However, the working group
- maintains a <a href=
+ This specification does not define the particular values for
+ <dfn>USVString</dfn>s for a the <a>categories</a> member. However,
Drop the "a".
--
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/613#pullrequestreview-66640754
Received on Tuesday, 3 October 2017 03:33:33 UTC