- From: Jake Archibald <notifications@github.com>
- Date: Mon, 04 Oct 2021 05:03:27 -0700
- To: w3c/manifest <manifest@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/manifest/pull/1005/review/770116469@github.com>
@jakearchibald requested changes on this pull request.
> + <li>Let |file_handler:ordered map| be [=process a file handler
+ item=] with |entry|.
It looks like **process a file handler item** takes more than one arg
> + On [=installation=], a user agent SHOULD <dfn>register file
+ handlers</dfn> with the operating system via interactions that are
+ consistent with:
Fwiw, I'd untangle the definition and call of this algorithm.
> + "file handler">icons</dfn></code> member lists images that serve as
+ graphical representations of a [=file type=] on a platform. This MAY be
+ replaced by the user agent for privacy & security reasons.
+ </p>
+ </section>
+ <section>
+ <h2>
+ Processing file handler items
+ </h2>
+ <p>
+ To <dfn>process a file handler item</dfn>, given [=ordered map=]
+ |item:ordered map|, [=URL=] |start_url:URL|, [=URL=] |scope:URL|, and
+ [=URL=] |manifest URL:URL|.
+ </p>
+ <ol class="algorithm">
+ <li>Return failure if it's the case that:
Make it clear that it's a failure if _any_ of the following are true.
> + replaced by the user agent for privacy & security reasons.
+ </p>
+ </section>
+ <section>
+ <h2>
+ Processing file handler items
+ </h2>
+ <p>
+ To <dfn>process a file handler item</dfn>, given [=ordered map=]
+ |item:ordered map|, [=URL=] |start_url:URL|, [=URL=] |scope:URL|, and
+ [=URL=] |manifest URL:URL|.
+ </p>
+ <ol class="algorithm">
+ <li>Return failure if it's the case that:
+ <ul>
+ <li>|item| is not [=ordered map=].
It's typed as an ordered map in the params, so if you want to do a check like this, the type needs to become something vaguer, or the type check needs to be done elsewhere.
> + [=URL=] |manifest URL:URL|.
+ </p>
+ <ol class="algorithm">
+ <li>Return failure if it's the case that:
+ <ul>
+ <li>|item| is not [=ordered map=].
+ </li>
+ <li>|item|["action"] doesn't [=map/exist=] or is not a [=string=].
+ </li>
+ <li>|item|["name"] [=map/exists=] and is not a [=string=].
+ </li>
+ <li>|item|["accept"] doesn't [=map/exist=].
+ </li>
+ <li>|item|["accept"] is not a [=dictionary=].
+ </li>
+ <li>|item|["accept"] is [=list/empty].
There's a markup and type issue here. Presumably the type should be 'map'?
> + <li>|item|["name"] [=map/exists=] and is not a [=string=].
+ </li>
+ <li>|item|["accept"] doesn't [=map/exist=].
+ </li>
+ <li>|item|["accept"] is not a [=dictionary=].
+ </li>
+ <li>|item|["accept"] is [=list/empty].
+ </li>
+ </ul>
+ </li>
+ <li>Let |url:URL| be the result of [=URL Parser|parsing=]
+ |item|["action"] with |start_url URL| as the base URL.
+ </li>
+ <li>If |url| is failure, return failure.
+ </li>
+ <li>If |url| is not [=manifest/within scope=] of |scope|, return
That algorithm takes a manifest, not a scope
> + </li>
+ <li>|item|["accept"] is not a [=dictionary=].
+ </li>
+ <li>|item|["accept"] is [=list/empty].
+ </li>
+ </ul>
+ </li>
+ <li>Let |url:URL| be the result of [=URL Parser|parsing=]
+ |item|["action"] with |start_url URL| as the base URL.
+ </li>
+ <li>If |url| is failure, return failure.
+ </li>
+ <li>If |url| is not [=manifest/within scope=] of |scope|, return
+ failure.
+ </li>
+ <li>Let |accept:ordered map| be an [=ordered map=].
Nit: make it clear this is a new ordered map
> + </li>
+ <li>|item|["accept"] is [=list/empty].
+ </li>
+ </ul>
+ </li>
+ <li>Let |url:URL| be the result of [=URL Parser|parsing=]
+ |item|["action"] with |start_url URL| as the base URL.
+ </li>
+ <li>If |url| is failure, return failure.
+ </li>
+ <li>If |url| is not [=manifest/within scope=] of |scope|, return
+ failure.
+ </li>
+ <li>Let |accept:ordered map| be an [=ordered map=].
+ </li>
+ <li>[=map/iterate=] |mime_type_string:string| → |extensions| of
The term to use here is "For each" https://infra.spec.whatwg.org/#map-iterate
> + <li>Let |url:URL| be the result of [=URL Parser|parsing=]
+ |item|["action"] with |start_url URL| as the base URL.
+ </li>
+ <li>If |url| is failure, return failure.
+ </li>
+ <li>If |url| is not [=manifest/within scope=] of |scope|, return
+ failure.
+ </li>
+ <li>Let |accept:ordered map| be an [=ordered map=].
+ </li>
+ <li>[=map/iterate=] |mime_type_string:string| → |extensions| of
+ |item|["accept"]
+ <ol>
+ <li>If |extensions| is not a [=list=], return failure.
+ </li>
+ <li>If |extensions| is an [=list/empty=] [=list=], return failure.
Nit
```suggestion
<li>If |extensions| is [=list/empty=], return failure.
```
> + </li>
+ <li>If |url| is failure, return failure.
+ </li>
+ <li>If |url| is not [=manifest/within scope=] of |scope|, return
+ failure.
+ </li>
+ <li>Let |accept:ordered map| be an [=ordered map=].
+ </li>
+ <li>[=map/iterate=] |mime_type_string:string| → |extensions| of
+ |item|["accept"]
+ <ol>
+ <li>If |extensions| is not a [=list=], return failure.
+ </li>
+ <li>If |extensions| is an [=list/empty=] [=list=], return failure.
+ </li>
+ <li>If |extensions| is a [=list=] with items that are not
You've already established that it's a list
> + <li>If |mime_type_parsed| is not defined in [[rfc2046]], return
+ failure.
I'm not sure this is right. rfc2046 doesn't define eg `image/avif` specifically, so I think this working need to change. a bit. Also it feels like "parse a mime type" returns a different kind of struct.
> + |item|["accept"]
+ <ol>
+ <li>If |extensions| is not a [=list=], return failure.
+ </li>
+ <li>If |extensions| is an [=list/empty=] [=list=], return failure.
+ </li>
+ <li>If |extensions| is a [=list=] with items that are not
+ [=string=]s, return failure.
+ </li>
+ <li>Let |mime_type_parsed:mime type| be the result of running the
+ steps of [=parse a mime type=] on |mime_type_string|.
+ </li>
+ <li>If |mime_type_parsed| is not defined in [[rfc2046]], return
+ failure.
+ </li>
+ <li>If |mime_type_parsed|.type is not listed in as a top-level type
We don't use dot notation like this as far as I'm aware. It should be `|mime_type_parsed| [=MIME type/type=]` or whatever the respec equivalent is.
> + </li>
+ <li>If |extensions| is an [=list/empty=] [=list=], return failure.
+ </li>
+ <li>If |extensions| is a [=list=] with items that are not
+ [=string=]s, return failure.
+ </li>
+ <li>Let |mime_type_parsed:mime type| be the result of running the
+ steps of [=parse a mime type=] on |mime_type_string|.
+ </li>
+ <li>If |mime_type_parsed| is not defined in [[rfc2046]], return
+ failure.
+ </li>
+ <li>If |mime_type_parsed|.type is not listed in as a top-level type
+ in [[IANA_MEDIA_TYPES]], return failure.
+ </li>
+ <li>Add |mime_type_string| → |extensions| to |accept|.
They syntax isn't quite right here. I guess it's supposed to be
```
Set |accept|[|mime_type_string|] to |extensions|.
```
Maybe?
> + </li>
+ <li>Let |mime_type_parsed:mime type| be the result of running the
+ steps of [=parse a mime type=] on |mime_type_string|.
+ </li>
+ <li>If |mime_type_parsed| is not defined in [[rfc2046]], return
+ failure.
+ </li>
+ <li>If |mime_type_parsed|.type is not listed in as a top-level type
+ in [[IANA_MEDIA_TYPES]], return failure.
+ </li>
+ <li>Add |mime_type_string| → |extensions| to |accept|.
+ </li>
+ </ol>
+ </li>
+ <li>Let |file_handler:ordered map| be |ordered map| «[ "action" →
+ |url|, "name" → |item|["name"], "accept" → |accept| ]».
I think this is an error since |item| might not contain "name".
> + </p>
+ <p>
+ A {{Window/window}} has an associated [=list=] of {{LaunchParams}}
+ <dfn>unconsumed launch params</dfn>.
+ </p>
+ <p>
+ When a [=file type=] is registered with a web app and one or more of
+ these registered files are launched on the platform, run the following
+ steps, given the launched {{FrozenArray}} of {{FileSystemHandle}}s
+ |files:FrozenArray<FileSystemHandle>| and the corresponding
+ [=file handler|file_handler=].
+ </p>
+ <ol>
+ <li>Let |url| be the |file_handler|.[=file handler/action=].
+ </li>
+ <li>Let |browsing context| be the result of creating a new [=top-level
Nit: We don't typically use spaces in variables
> + <li>If [=assigned launch consumer=] is set, call [=assigned launch
+ consumer=] with |params| and return
+ </li>
+ <li>[=list/Append=] |params| to [=unconsumed launch params=].
These should clarify which "assigned launch consumer" and "unconsumed launch params" they're talking about.
> + <p>
+ When a [=file type=] is registered with a web app and one or more of
+ these registered files are launched on the platform, run the following
+ steps, given the launched {{FrozenArray}} of {{FileSystemHandle}}s
+ |files:FrozenArray<FileSystemHandle>| and the corresponding
+ [=file handler|file_handler=].
+ </p>
+ <ol>
+ <li>Let |url| be the |file_handler|.[=file handler/action=].
+ </li>
+ <li>Let |browsing context| be the result of creating a new [=top-level
+ browsing context=].
+ </li>
+ <li>[=Navigate=] |browsing context| to |url|.
+ </li>
+ <li>Let |params:LaunchParams| be {{LaunchParams}} with
A new {{LaunchParams}} I guess.
> + </ol>
+ </section>
+ <section data-cite="file-system-access">
+ <h2>
+ <dfn>Execute a file handler launch</dfn>
+ </h2>
+ <p>
+ A {{Window/window}} has an associated {{LaunchConsumer}} <dfn>assigned
+ launch consumer</dfn>.
+ </p>
+ <p>
+ A {{Window/window}} has an associated [=list=] of {{LaunchParams}}
+ <dfn>unconsumed launch params</dfn>.
+ </p>
+ <p>
+ When a [=file type=] is registered with a web app and one or more of
I think it'd be clearer to give this algorithm a definition, then spec when that algorithm is called.
> + <section data-cite="file-system-access">
+ <h2>
+ <dfn>Execute a file handler launch</dfn>
+ </h2>
+ <p>
+ A {{Window/window}} has an associated {{LaunchConsumer}} <dfn>assigned
+ launch consumer</dfn>.
+ </p>
+ <p>
+ A {{Window/window}} has an associated [=list=] of {{LaunchParams}}
+ <dfn>unconsumed launch params</dfn>.
+ </p>
+ <p>
+ When a [=file type=] is registered with a web app and one or more of
+ these registered files are launched on the platform, run the following
+ steps, given the launched {{FrozenArray}} of {{FileSystemHandle}}s
It's unclear where this object comes from (eg what JS realm)
> + A {{Window/window}} has an associated [=list=] of {{LaunchParams}}
+ <dfn>unconsumed launch params</dfn>.
What causes this list to contain more than one item?
> + </section>
+ <section data-cite="file-system-access">
+ <h2>
+ <dfn>Execute a file handler launch</dfn>
+ </h2>
+ <p>
+ A {{Window/window}} has an associated {{LaunchConsumer}} <dfn>assigned
+ launch consumer</dfn>.
+ </p>
+ <p>
+ A {{Window/window}} has an associated [=list=] of {{LaunchParams}}
+ <dfn>unconsumed launch params</dfn>.
+ </p>
+ <p>
+ When a [=file type=] is registered with a web app and one or more of
+ these registered files are launched on the platform, run the following
> these steps are also run after every same-origin page navigation
Can you explain that in a bit more detail? Let's say I launch `//origin1/foo/` using a file, then I click a link in the resulting page to `//origin1/bar/` (creating a same-origin navigation), are you saying the next page gets the file handler too? Or is it just to handle reloads?
Either way, it seems like this data needs to go on the session history entry in some way.
--
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/1005#pullrequestreview-770116469
Received on Monday, 4 October 2021 12:03:41 UTC