Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve logic for media elements #16
Improve logic for media elements #16
Changes from 2 commits
c41b654
fcc5a88
16aeece
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 step seems a bit undesirable
This special case for 206 seems to mean that a video served as
text/html
will not work with 206 responses, right? This seems undesirable.Maybe we can remove this special case?
For non-media, "no-cors media request state" will be "N/A" and ORB will block the response in the following 2 new-ish steps:
If 206 is for the beginning of the body, ORB would still block the response after it doesn't sniff as image/audio/video/javascript (in later steps).
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 step stems from https://fetch.spec.whatwg.org/#corb-check. Did Chrome end up removing it?
I'm not sure how the first quoted step is relevant.
As for the second step, earlier you mentioned that this was useful to have and would happen in the implementation before sniffing. Are you suggesting to only do it when no-cors media request state is "initial"? I suppose we could make that change. It would allow the server to serve random 206 responses as JavaScript (if they happen to parse), but I guess that's 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.
Doh... I forgot about it. Yes, Chrome's CORB implementation still blocks 206 responses with html/json/xml MIME types. Given this, I think we should just land the PR in its current shape.
I was just trying to argue why I think the earlier step (blocking 206 for html/xml/json) might be redundant, because latter steps (without any changes) would block such html/xml/json responses.