Skip to content
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

Introduce get the MIME type for request and response #1657

Merged
merged 4 commits into from
Jun 2, 2023

Conversation

CYBAI
Copy link
Contributor

@CYBAI CYBAI commented May 18, 2023

Fix #1630


When the Request or Response has a Blob body with type and MIME type exists in its headers, browsers should always return the MIME type in headers instead of the body's type.

However, previously, the spec didn't mention this clearly so this PR will help to clarify by introducing an algorithm of get the MIME type.


Preview | Diff

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty close now!

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@CYBAI
Copy link
Contributor Author

CYBAI commented May 26, 2023

I've tried to address those review comments and rewrap the switch cases for formData() in the fixup commit. If the PR is okay, I can squash the fixup commit before merge 🙏 Also, thanks again for your review! Your comments are very helpful to me!

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, only nits remain. \o/

You don't have to squash as I'll do that through the GitHub UI. If you could work out a tentative commit message that would help though.

And perhaps we should no longer mark this as Editorial as you've fixed a null-pointer dereference.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented May 31, 2023

Finally, I suspect you're not in the Acknowledgments section yet. Feel free to add yourself!

CYBAI added 2 commits June 1, 2023 22:51
Previously, the spec didn't clarify how to get the MIME type for both
request and response. Thus, this patch will introduce the "get the MIME
type" algorithm to clarify how to get it; then, other places which
requires MIME type can reuse this algorithm.

Also, with moving to use the algorithm, it will fix a potential
null-pointer dereference issue in the `formData()` method because a
request or a response's MIME type could potentially be null.
@CYBAI
Copy link
Contributor Author

CYBAI commented Jun 1, 2023

I've tried to reword the commit message and add more description in it. If it looks good, I can rename the PR title as well 🙏

@annevk annevk merged commit 5eaaf66 into whatwg:main Jun 2, 2023
2 checks passed
@annevk
Copy link
Member

annevk commented Jun 2, 2023

Thanks @CYBAI for tackling this! I noticed a few more things at the end that I cleaned up before merging. Hope that's okay.

@CYBAI CYBAI deleted the clarify-get-mime branch June 2, 2023 12:54
@CYBAI
Copy link
Contributor Author

CYBAI commented Jun 2, 2023

Sure! Thank you so much for the cleanup! I just learned more about how to write the spec :)

@CYBAI CYBAI changed the title Editorial: clarify how to get MIME type for request and response Introduce get the MIME type for request and response Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Clarify priority between blob.type and Content-Type in headers for request/response.blob()
2 participants