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 2 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>.

> +        <li>If |manifest|'s [=application manifest/client=] is null, return.
+          <aside class="note">
+            When a manifest is processed without an associated [=document=],
+            |client| is null and no image fetch is performed. This ensures
+            that [=enforced|enforcement=] of CSP and [=service worker=] fetch
+            interception, which depend on the [=document=]'s [=relevant
+            settings object=], remain in effect.

In the “fetch a manifest image resource” algorithm, step 1 returns early when the manifest’s client is null, but the algorithm otherwise returns the result of “fetching an image resource”. This makes the algorithm’s return value inconsistent/underspecified for the null-client case (and can prevent icon fetching entirely for manifests processed without a document). Consider defining an explicit return value for the null-client branch (e.g., return failure / return a resolved promise with null, or define how to fetch with a null request client) so callers have a well-defined outcome.
```suggestion
        <li>If |manifest|'s [=application manifest/client=] is null, return
        null.
          <aside class="note">
            When a manifest is processed without an associated [=document=],
            |client| is null, no image fetch is performed, and the algorithm
            returns null. This ensures that [=enforced|enforcement=] of CSP
            and [=service worker=] fetch interception, which depend on the
            [=document=]'s [=relevant settings object=], remain in effect.
```

> +            |client| is null and no image fetch is performed. This ensures
+            that [=enforced|enforcement=] of CSP and [=service worker=] fetch
+            interception, which depend on the [=document=]'s [=relevant
+            settings object=], remain in effect.

The note under the null-client branch refers to “|client| is null”, but |client| is not a variable in scope for this algorithm (only |image| and |manifest| are). To avoid ambiguity, refer to “|manifest|’s client” (or “|manifest|’s application manifest/client”) instead, and consider rewording the rationale so it’s clear this is about avoiding bypassing CSP/service worker interception rather than “enforcement … remain[ing] in effect” when no fetch occurs.
```suggestion
            |manifest|'s [=application manifest/client=] is null and no image
            fetch is performed. This avoids fetching the image without the
            [=document=] context needed for CSP and [=service worker=] fetch
            interception, which depend on the [=document=]'s [=relevant
            settings object=].
```

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

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