-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return a content-encoding
header for resource timing and more
#1796
base: main
Are you sure you want to change the base?
Conversation
fetch.bs
Outdated
<li><p>Let <var>accept-coding</var> be the result of <a for="header list">getting</a> | ||
`<code>Accept-Encoding</code>` from <var>request</var>'s <a for=request>header list</a>. | ||
If <var>accept-encoding</var> is not null and the server selects one of the encoding options, | ||
let <var>coding</var> be the selected encoding option; otherwise, i.e., no encoding is used, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't have "let" in the middle of a phrase, see style in the rest of the document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I see why the "let" in the middle of the sentence is a bad idea. However, I didn't find the style guide in this document. I found this page: https://github.com/guohuideng2024/fetch/tree/main instead, so I think the style guide may have been moved? I wonder if there is more style guide that I can study. Thanks!
Thanks for taking the time to pick this up. However, it doesn't seem like this addresses all the issues with #1742? I recommend studying the feedback on that PR. |
Hi Anne! I think I should have put up some background information here.
Therefore, in this Does this sound right to you? I am new to |
This CL introduce a contentEncoding field to Performance resource timing object. This field is behind a feature flag. PR to resource timing specification: w3c/resource-timing#411 PR to fetch specification: whatwg/fetch#1796 Bug: 327941462 Change-Id: I70cad190fe658fb3dbf8b401ff8393bc1d0782f0
This CL introduce a contentEncoding field to Performance resource timing object. This field is behind a feature flag. PR to resource timing specification: w3c/resource-timing#411 PR to fetch specification: whatwg/fetch#1796 Bug: 327941462 Change-Id: I70cad190fe658fb3dbf8b401ff8393bc1d0782f0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6098321 Commit-Queue: Guohui Deng <[email protected]> Reviewed-by: Noam Rosenthal <[email protected]> Reviewed-by: Matthew Denton <[email protected]> Reviewed-by: Yoav Weiss (@Shopify) <[email protected]> Cr-Commit-Position: refs/heads/main@{#1407331}
This CL introduce a contentEncoding field to Performance resource timing object. This field is behind a feature flag. PR to resource timing specification: w3c/resource-timing#411 PR to fetch specification: whatwg/fetch#1796 Bug: 327941462 Change-Id: I70cad190fe658fb3dbf8b401ff8393bc1d0782f0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6098321 Commit-Queue: Guohui Deng <[email protected]> Reviewed-by: Noam Rosenthal <[email protected]> Reviewed-by: Matthew Denton <[email protected]> Reviewed-by: Yoav Weiss (@Shopify) <[email protected]> Cr-Commit-Position: refs/heads/main@{#1407331}
This CL introduce a contentEncoding field to Performance resource timing object. This field is behind a feature flag. PR to resource timing specification: w3c/resource-timing#411 PR to fetch specification: whatwg/fetch#1796 Bug: 327941462 Change-Id: I70cad190fe658fb3dbf8b401ff8393bc1d0782f0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6098321 Commit-Queue: Guohui Deng <[email protected]> Reviewed-by: Noam Rosenthal <[email protected]> Reviewed-by: Matthew Denton <[email protected]> Reviewed-by: Yoav Weiss (@Shopify) <[email protected]> Cr-Commit-Position: refs/heads/main@{#1407331}
fetch.bs
Outdated
<var>value</var>. | ||
|
||
<li><p>Otherwise, if <var>value</var> is not <var>candidateValue</var>, return failure. | ||
</ol> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the idea here is that duplicate values are okay as long as they are fully identical? So gzip
and GZIP
is not okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! I changed the text. Now something like {gzip, GZIP} (case-insensitive match) would be ok. What do you think?
fetch.bs
Outdated
</ol> | ||
|
||
<li><p>If <var>candidateValue</var> is the empty string or has a <a for=/>code point</a> that is | ||
not an <a for=/>ASCII digit</a>, then return null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it only contain digits? Typical values for Content-Encoding
are gzip
or deflate
as I understand it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean ASCII code point
? Is unicode not allowed in Content-Encoding
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry my bad. I removed this.
fetch.bs
Outdated
If <var>accept-encoding</var> is not null and the server selects one of the encoding options, | ||
set <var>coding</var> to the selected encoding option; otherwise, i.e., no encoding is used, | ||
set <var>coding</var> to <a href=https://httpwg.org/specs/rfc9110.html#field.accept-encoding> | ||
<code>"identity"</code></a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation here doesn't follow the style guide.
How would we know what the server selects here? This doesn't make much sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks guys for pointing out this is on client side. I thought I was on server side. :(
Also Noam and I had discussion offline. All the filtering happens after fetch. So we don't filter the values here.
I changed the text accordingly.
fetch.bs
Outdated
<a for="data: URL struct">body</a> <a for="byte sequence">as a body</a>. | ||
<var>mimeType</var>), (`<code>Content-Encoding</code>`, <var>coding</var>) », and | ||
<a for=response>body</a> is <var>dataURLStruct</var>'s <a for="data: URL struct"> | ||
body</a> <a for="byte sequence">as a body</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also doesn't follow the formatting guidelines. Also, do we really want to return Content-Encoding
for data:
URLs? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now understand what data url is and contentEncoding
doesn't apply to it.
So I removed the text.
…ourceTiming, a=testonly Automatic update from web-platform-tests Expose contentEncoding in PerformanceResourceTiming This CL introduce a contentEncoding field to Performance resource timing object. This field is behind a feature flag. PR to resource timing specification: w3c/resource-timing#411 PR to fetch specification: whatwg/fetch#1796 Bug: 327941462 Change-Id: I70cad190fe658fb3dbf8b401ff8393bc1d0782f0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6098321 Commit-Queue: Guohui Deng <[email protected]> Reviewed-by: Noam Rosenthal <[email protected]> Reviewed-by: Matthew Denton <[email protected]> Reviewed-by: Yoav Weiss (@Shopify) <[email protected]> Cr-Commit-Position: refs/heads/main@{#1407331} -- wpt-commits: 1df2c3e47bcb6379ecf3a07735bd967101d02a5b wpt-pr: 50115
1) formatting; 2) "gzip, GZIP" is ok for they case-insensitive match. 3) there is a mistake saying that the "contentEncoding" consists of digits; 4) no longer returns "contentEncoding" for data url.
fetch.bs
Outdated
@@ -5113,6 +5160,7 @@ steps: | |||
`<code>OK</code>`, <a for=response>header list</a> is « (`<code>Content-Type</code>`, | |||
<var>mimeType</var>) », and <a for=response>body</a> is <var>dataURLStruct</var>'s | |||
<a for="data: URL struct">body</a> <a for="byte sequence">as a body</a>. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spurious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
fetch.bs
Outdated
@@ -5092,7 +5138,8 @@ steps: | |||
<li><p>Set <var>response</var>'s <a for=response>header list</a> to « | |||
(`<code>Content-Length</code>`, <var>serializedSlicedLength</var>), | |||
(`<code>Content-Type</code>`, <var>type</var>), (`<code>Content-Range</code>`, | |||
<var>contentRange</var>) ». | |||
<var>contentRange</var>), (`<code>Content-Encoding</code>`, <var>coding</var>) ». | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
That's on the client side getting the reponse header.
fetch.bs
Outdated
<li><p>Let <var>candidateValue</var> be null. | ||
|
||
<li> | ||
<p><a for=list>For each</a> <var>value</var> of <var>values</var>: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically a no-duplicates algorithm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know changed the text to allow duplicates. (as long as they case-insensitive match), I am not sure if this is the best option. what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. What do browsers currently do with duplicates and multiple values? There should be WPTs for this
Updated the patch, I just added the content encoding to the body info struct, and add the clause that updates it. |
@@ -3945,7 +3948,6 @@ Content-Type: | |||
</div> | |||
</div> | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the new line
@@ -1176,6 +1178,7 @@ is a <a>byte-case-insensitive</a> match for one of | |||
<li>`<a http-header><code>Access-Control-Request-Headers</code></a>` | |||
<li>`<a http-header><code>Access-Control-Request-Method</code></a>` | |||
<li>`<code>Connection</code>` | |||
<li>`<code>Content-Encoding</code>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
@@ -6319,6 +6321,9 @@ optional boolean <var>forceNewConnection</var> (default false), run these steps: | |||
<li><p>Let <var>codings</var> be the result of <a>extracting header list values</a> given | |||
`<code>Content-Encoding</code>` and <var>response</var>'s <a for=response>header list</a>. | |||
|
|||
<li><p>Set <var>response</var>'s <a for=response>body info</a>'s | |||
<a for="response body info">content encoding</a> to <var>codings</var>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do UAs today do any observable processing to this "codings" value? What happens when there are multiple values? Resource timing would make this more observable so it needs to be specified
The major change is to add
content-encoding
to response header list. This PR also adds description on howcontent-encoding
is determined. (content negotiation)The purpose is to pass such value to resource timing. Further details are available at
w3c/resource-timing#381.
Note: Per discussion at 12/05/2024 webPerWG call (https://docs.google.com/document/d/1mpFDrAWuV6IgvJ1KiL9sgIlcboC5uArtF8r_oqS1Sco/edit?tab=t.0#heading=h.af6v74wysf4m), we decided to allow arbitrary "content-encoding" value at "fetch". We only filter such value at client side, before passing the value to resource timing.
Related PR to modify resource timing specification:
w3c/resource-timing#411
At least two implementers are interested (and none opposed):
Likely, Discussed and agreed at Feb 29, 2024 W3C WebPerf call, Chromium already receiving content-encoding through fetch. But not sure about other browsers.
Tests are written and can be reviewed and commented upon at:
[to be updated with a new link] https://chromium-review.googlesource.com/c/chromium/src/+/5958411
Implementation bugs are filed:
Chromium: https://issues.chromium.org/issues/327941462
Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1886107
WebKit: https://bugs.webkit.org/show_bug.cgi?id=271632
Deno (not for CORS changes): …
MDN issue is filed:
New PerformanceResourceTiming.contentEncoding field mdn/content#32823
The top of this comment includes a clear commit message to use.
(See WHATWG Working Mode: Changes for more details.)
Bug: w3c/resource-timing#381
Preview | Diff