Skip to content

Use data_url::Mime to parse the MIME Types#27725

Merged
bors-servo merged 1 commit intoservo:masterfrom
ghostd:extract-mime-type
Nov 18, 2020
Merged

Use data_url::Mime to parse the MIME Types#27725
bors-servo merged 1 commit intoservo:masterfrom
ghostd:extract-mime-type

Conversation

@ghostd
Copy link
Copy Markdown
Contributor

@ghostd ghostd commented Oct 29, 2020 •
edited
Loading

This commit follows the spectification https://fetch.spec.whatwg.org/#concept-header-extract-mime-type

This commit partially addresses #24923 (only the content type part). I'll try to fix the parser stuff in an other commit.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

@ghostd ghostd force-pushed the extract-mime-type branch from fcc1cea to 390e456 Compare October 29, 2020 20:11
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 30, 2020

☔ The latest upstream changes (presumably #27726) made this pull request unmergeable. Please resolve the merge conflicts.

@ghostd ghostd force-pushed the extract-mime-type branch from 390e456 to 55804da Compare October 30, 2020 17:00
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 31, 2020

☔ The latest upstream changes (presumably #27728) made this pull request unmergeable. Please resolve the merge conflicts.

@ghostd ghostd force-pushed the extract-mime-type branch from 55804da to 7602350 Compare October 31, 2020 23:22
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 1, 2020

☔ The latest upstream changes (presumably #27730) made this pull request unmergeable. Please resolve the merge conflicts.

jdm
jdm requested changes Nov 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

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

These changes look sensible! Thanks!

components/script/dom/xmlhttprequest.rs Outdated
/// <https://xhr.spec.whatwg.org/#response-mime-type>
fn response_mime_type(&self) -> Option<Mime> {
return extract_mime_type(&self.response_headers.borrow())
.map(|mime_as_bytes| String::from_utf8(mime_as_bytes).unwrap().parse().ok())
Copy link
Copy Markdown
Member

@jdm jdm Nov 13, 2020

Choose a reason for hiding this comment

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

Let's use unwrap_or_default() instead.

Copy link
Copy Markdown
Contributor Author

@ghostd ghostd Nov 14, 2020

Choose a reason for hiding this comment

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

Fixed

components/script/dom/headers.rs Outdated
continue;
}

let temp_charset = &temp_mime.get_parameter(&"charset".to_string());
Copy link
Copy Markdown
Member

@jdm jdm Nov 13, 2020

Choose a reason for hiding this comment

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

The to_string shouldn't be necessary, I believe.

Copy link
Copy Markdown
Contributor Author

@ghostd ghostd Nov 14, 2020

Choose a reason for hiding this comment

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

You're right! Fixed

@jdm jdm self-assigned this Nov 13, 2020
@jdm jdm added the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Nov 13, 2020
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 14, 2020
@ghostd
Copy link
Copy Markdown
Contributor Author

ghostd commented Nov 14, 2020

I rebased the branch.

@ghostd ghostd requested a review from jdm November 14, 2020 23:22
@jdm
Copy link
Copy Markdown
Member

jdm commented Nov 15, 2020 •
edited
Loading

Looks like a mach fmt is required.

@ghostd
Copy link
Copy Markdown
Contributor Author

ghostd commented Nov 15, 2020

My bad :-/ Fixed

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 15, 2020

☔ The latest upstream changes (presumably #27763) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Nov 15, 2020
@jdm
Copy link
Copy Markdown
Member

jdm commented Nov 15, 2020

@bors-servo r+

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 15, 2020

📌 Commit 2b297a3 has been approved by jdm

@highfive highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 15, 2020
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 15, 2020

⌛ Testing commit 2b297a3 with merge adb02b8...

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-rebase There are merge conflict errors. labels Nov 15, 2020
bors-servo added a commit that referenced this pull request Nov 15, 2020
@bors-servo
Auto merge of #27725 - ghostd:extract-mime-type, r=jdm
adb02b8 
Use data_url::Mime to parse the MIME Types

This commit follows the spectification https://fetch.spec.whatwg.org/#concept-header-extract-mime-type

This commit partially addresses #24923 (only the content type part). I'll try to fix the parser stuff in an other commit.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 15, 2020

💔 Test failed - status-taskcluster

@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 15, 2020
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Nov 18, 2020
@ghostd
Copy link
Copy Markdown
Contributor Author

ghostd commented Nov 18, 2020

Done

Should I open an issue to follow this "regression"?

@jdm
Copy link
Copy Markdown
Member

jdm commented Nov 18, 2020

Yes please!
@bors-servo try=wpt

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 18, 2020

⌛ Trying commit 7fee619 with merge 33e72de...

bors-servo added a commit that referenced this pull request Nov 18, 2020
@bors-servo
Auto merge of #27725 - ghostd:extract-mime-type, r=<try>
33e72de 
Use data_url::Mime to parse the MIME Types

This commit follows the spectification https://fetch.spec.whatwg.org/#concept-header-extract-mime-type

This commit partially addresses #24923 (only the content type part). I'll try to fix the parser stuff in an other commit.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@ghostd ghostd mentioned this pull request Nov 18, 2020
Open
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 18, 2020

☔ The latest upstream changes (presumably #27779) made this pull request unmergeable. Please resolve the merge conflicts.

@ghostd
Copy link
Copy Markdown
Contributor Author

ghostd commented Nov 18, 2020

Rebased

@jdm
Copy link
Copy Markdown
Member

jdm commented Nov 18, 2020

@bors-servo r+

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 18, 2020

📌 Commit 43b3d93 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Nov 18, 2020
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 18, 2020

⌛ Testing commit 43b3d93 with merge 3a6a088...

bors-servo added a commit that referenced this pull request Nov 18, 2020
@bors-servo
Auto merge of #27725 - ghostd:extract-mime-type, r=jdm
3a6a088 
Use data_url::Mime to parse the MIME Types

This commit follows the spectification https://fetch.spec.whatwg.org/#concept-header-extract-mime-type

This commit partially addresses #24923 (only the content type part). I'll try to fix the parser stuff in an other commit.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 18, 2020

💔 Test failed - status-taskcluster

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 18, 2020
@ghostd
Copy link
Copy Markdown
Contributor Author

ghostd commented Nov 18, 2020 

UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 57: ordinal not in range(128)

  File "/Users/worker/tasks/task_1605728842/repo/python/servo/testing_commands.py", line 600, in filter_intermittents
    format(actual_failures, description)
  File "/Users/worker/tasks/task_1605728842/repo/python/servo/testing_commands.py", line 588, in format
    file.write(formatted.encode("utf-8"))
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 357, in write

For the Mac build :-/

@jdm
Copy link
Copy Markdown
Member

jdm commented Nov 18, 2020

Yeah, you have to look at the filtered summary instead (#25233).

  ▶ TIMEOUT [expected ERROR] /_webgl/conformance/textures/image_bitmap_from_canvas/tex-2d-luminance_alpha-luminance_alpha-unsigned_byte.html
  ▶ FAIL [expected PASS] /css/CSS2/normal-flow/width-040.xht
  │   → /css/CSS2/normal-flow/width-040.xht ['7b6d4715bf74ba79b0c5f8cd6bbc78ebba36a265']
  └   → /css/CSS2/normal-flow/max-width-006-ref.xht ['d9da3f1daeb78b8416383c9c83c9dfdcb349fd38']

I believe those are known intermittent failures.

@bors-servo retry

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 18, 2020

⌛ Testing commit 43b3d93 with merge e2687b5...

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 18, 2020
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 18, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing e2687b5 to master...

@bors-servo bors-servo merged commit e2687b5 into servo:master Nov 18, 2020
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 18, 2020
@ghostd ghostd deleted the extract-mime-type branch November 18, 2020 21:43
@jdm jdm mentioned this pull request Oct 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdm jdm Awaiting requested review from jdm

Assignees

@jdm jdm

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ghostd @bors-servo @jdm @CYBAI @highfive