Re: [w3c/manifest] Clarify how manifest image resources are fetched (PR #1171)

@Copilot commented on this pull request.

## Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.





---

💡 <a href="/w3c/manifest/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

> +            [=URL=] |manifest URL:URL|, a [=byte sequence=] |bodyBytes|, and an
+            optional [=environment settings object=] |client|.

The algorithm signature says the `client` parameter is optional, but later steps treat it as always present (stored on the processed manifest and later compared to null). In this spec, “optional” parameters are handled via “was not given” checks (see `launch a web application`), whereas `client` is described as being explicitly null. Please either (a) make `client` a required parameter whose value can be an environment settings object or null, or (b) add a step up front that sets `client` to null when it was not given, and update downstream checks accordingly.
```suggestion
            [=URL=] |manifest URL:URL|, a [=byte sequence=] |bodyBytes|, and a
            |client| whose value is either null or an
            [=environment settings object=].
```

> @@ -1751,9 +1755,11 @@ <h3>
           </h3>
           <p>
             The [=processing a manifest=] steps are invoked by [[HTML]]'s
-            processing steps for the [^link^] element, but MAY also be invoked
-            by the user agent to process a manifest without an associated
-            [=document=].
+            processing steps for the [^link^] element, in which case
+            |client| is the [=document=]'s [=relevant settings object=].
+            The steps MAY also be invoked by the user agent to process a
+            manifest without an associated [=document=], in which case
+            |client| is null.

This section says that when processing a manifest without an associated document, `client` is null. That conflicts with the earlier parameter type of “optional environment settings object” (an environment settings object is not null). Please align the contract: either describe `client` as “an environment settings object or null” everywhere, or keep it optional and avoid using null as the sentinel value.
```suggestion
            |client| is omitted.
```

> +        <li>If |manifest|'s [=application manifest/client=] is null, return.
+        </li>
+        <li>Let |request| be a new [=Request=].
+        </li>
+        <li>Set |request|'s [=request/URL=] to |image|'s {{ImageResource/src}}.
+        </li>
+        <li>Set |request|'s [=request/Destination=] to "`image`".
+        </li>
+        <li>Set |request|'s [=request/Client=] to |manifest|'s [=application

`fetch a manifest image resource` returns early when the manifest’s stored `client` is null, which would make it impossible to fetch icons/shortcut icons in “processing without a document” scenarios (where this PR defines `client` as null). If fetching should still occur in that case, drop the early return and instead proceed with a request that has no client (or an appropriate default client), while still setting the client when available for CSP/service worker interception.
```suggestion
        <li>Let |request| be a new [=Request=].
        </li>
        <li>Set |request|'s [=request/URL=] to |image|'s {{ImageResource/src}}.
        </li>
        <li>Set |request|'s [=request/Destination=] to "`image`".
        </li>
        <li>If |manifest|'s [=application manifest/client=] is not null, set
        |request|'s [=request/Client=] to |manifest|'s [=application
```

> +        <li>Set |request|'s [=request/Destination=] to "`image`".
+        </li>
+        <li>Set |request|'s [=request/Client=] to |manifest|'s [=application

The Fetch internal slots are typically referenced as `request/destination` and `request/client` (lowercase). Using `[=request/Destination=]` and `[=request/Client=]` is likely to break xrefs and is inconsistent with Fetch terminology. Please switch these xrefs to the correct slot names so the spec links resolve and the text matches Fetch’s definitions.
```suggestion
        <li>Set |request|'s [=request/destination=] to "`image`".
        </li>
        <li>Set |request|'s [=request/client=] to |manifest|'s [=application
```

-- 
Reply to this email directly or view it on GitHub:
https://github.com/w3c/manifest/pull/1171#pullrequestreview-4094454466
You are receiving this because you are subscribed to this thread.

Message ID: <w3c/manifest/pull/1171/review/4094454466@github.com>