-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Use the fetched unsafe response for network status and response bodies #9362
base: main
Are you sure you want to change the base?
Conversation
source
Outdated
@@ -35843,6 +35845,8 @@ interface <dfn interface>MediaError</dfn> { | |||
<span data-x="concept-response">response</span> <var>response</var>:</p> | |||
|
|||
<ol> | |||
<li><p>Set <var>response</var> to <var>response</var>'s <span>unsafe response</span>.</p></li> |
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 think this is right. Later, e.g. here, we check if the response is CORS-cross-origin. This would return false for the internal response. We should only unwrap the internal response when actually wanting to use the data - e.g. when decoding, presenting etc.
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.
Ah that's right. I've updated HTMLMediaElement to use the unsafe response just when reading its body.
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 the same applies to images though
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.
For images, there weren't really any spec steps that run on response
directly, it just originally said "The resource obtained in this fashion, if any, is image request's image data".
For links, it seemed like all downstream users also need the unsafe response, as it's used when:
- Checking its status
- Checking its Content-Type header for manifest links
- Checking its Content-Type header for CSS links
I can update those to instead all check the unsafe response's status / Content-Type inline, if that's preferable.
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.
Oh, but for CSS links, we do need to know downstream if it is CORS-same-origin, so I'll go ahead and inline the use of unsafe response.
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.
Yes, but later there are places where we check if image data is CORS-cross-origin. So we should assume image-data is something response-like (this should really be refactored) and use the unsafe response only when we need direct access to the data
cd6c93c
to
e46173d
Compare
After using Fetch infrastructure to retrieve a resource, go through the response's unsafe response to inspect the network status of the response and read its body. When the response is CORS-cross-origin, Fetch will provide an opaque filtered response, which will always have a null body and network status of 0.
e46173d
to
090bdbf
Compare
@trflynn89 could you look into signing https://participate.whatwg.org/agreement? We'll need that eventually in order to be able to merge the PR. |
After using Fetch infrastructure to retrieve a resource, go through the response's unsafe response to inspect the network status of the response and read its body. When the response is CORS-cross-origin, Fetch will provide an opaque filtered response, which will always have a null body and network status of 0.
Fixes #9355
(See WHATWG Working Mode: Changes for more details.)
/acknowledgements.html ( diff )
/links.html ( diff )
/media.html ( diff )
/semantics.html ( diff )