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.

Sign up for GitHub

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

Jump to bottom

Made binary files show an info message instead of garbled text. #7956

Merged
merged 1 commit into from Oct 31, 2015
Merged

Made binary files show an info message instead of garbled text. #7956

merged 1 commit into from Oct 31, 2015

Conversation

gkbrk
Copy link
Contributor

@gkbrk gkbrk commented Oct 10, 2015

Content-types with the TopLevel "Application" such as

  • application/octet-stream
  • application/pdf

now show an info message instead of trying to view binary data as html.

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 10, 2015
@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

@gkbrk
Copy link
Contributor Author

gkbrk commented Oct 10, 2015

This should solve #7706 I think.

@jdm jdm self-assigned this Oct 10, 2015
@jdm
Copy link
Member

jdm commented Oct 10, 2015

Thanks for doing this work!
-S-awaiting-review +S-needs-code-changes

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

components/script/dom/servohtmlparser.rs, line 135 [r1] (raw file):
This does solve the problem, but I think we should move to a whitelisting model instead, ie. only continue parsing HTML for text/html, and display this for all others that we don't explicitly support.

components/script/dom/servohtmlparser.rs, line 136 [r1] (raw file):
Let's rename this to is_synthesized_content.

Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 10, 2015
@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 Oct 10, 2015
@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 10, 2015
@jdm
Copy link
Member

jdm commented Oct 10, 2015

-S-awaiting-review +S-needs-code-changes

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

components/script/dom/servohtmlparser.rs, line 142 [r2] (raw file):
This should be a None branch with a comment that it should be merged with the previous branch when #4212 is fixed.

Comments from the review on Reviewable.io

@jdm jdm added the S-fails-tidy `./mach test-tidy` reported errors. label Oct 10, 2015
@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 Oct 11, 2015
@gkbrk
Copy link
Contributor Author

gkbrk commented Oct 11, 2015

@jdm anything else I should change?

@jdm
Copy link
Member

jdm commented Oct 12, 2015

Nope! Just squash these three commits into one and we're good to merge!
-S-awaiting-review -S-fails-tidy +S-needs-squash

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

@jdm jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-fails-tidy `./mach test-tidy` reported errors. S-awaiting-review There is new code that needs to be reviewed. labels Oct 12, 2015
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 12, 2015
@gkbrk
Copy link
Contributor Author

gkbrk commented Oct 12, 2015

@jdm I might have done the squash incorrectly, doesn't seem like there's a problem though.

@jdm
Copy link
Member

jdm commented Oct 12, 2015

@bors-servo: r+
Lovely!

@bors-servo
Copy link
Contributor

📌 Commit 93721d8 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-squash Some (or all) of the commits in the PR should be combined. labels Oct 12, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 93721d8 with merge db04716...

bors-servo pushed a commit that referenced this pull request Oct 12, 2015
Auto merge of #7956 - gkbrk:binary_mime, r=jdm
db04716 
Made binary files show an info message instead of garbled text.

Content-types with the TopLevel "Application" such as

* application/octet-stream
* application/pdf

now show an info message instead of trying to view binary data as html.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7956)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@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 Oct 12, 2015
@jdm
Copy link
Member

jdm commented Oct 12, 2015

Ah, presumably the server is giving us text/xhtml for .xhtml files. We should probably claim that we can deal with those, even though we don't have an XML parser yet.

@gkbrk
Copy link
Contributor Author

gkbrk commented Oct 12, 2015

@jdm So I'm adding text/xhtml to the match statement right?

@gkbrk
Copy link
Contributor Author

gkbrk commented Oct 12, 2015

@jdm http://www.w3.org/International/articles/serving-xhtml/ says xhtml has 3 possible mime types.

  • application/xhtml+xml
  • application/xml
  • text/xml

Which ones should I add?

@eefriedman
Copy link
Contributor

Hopefully just application/xhtml+xml is sufficient. Treating arbitrary text/xml as HTML is likely to lead to weird results.

@bors-servo
Copy link
Contributor

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 16, 2015
@eefriedman eefriedman added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 29, 2015
@Manishearth
Copy link
Member

Any updates?

@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 Oct 31, 2015
@gkbrk
Copy link
Contributor Author

gkbrk commented Oct 31, 2015

@jdm @Manishearth Just updated the PR to compile and hopefully pass the xhtml tests.

@jdm
Copy link
Member

jdm commented Oct 31, 2015

@bors-servo: r+
Thanks!

@bors-servo
Copy link
Contributor

📌 Commit aa7a391 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 Oct 31, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit aa7a391 with merge f432c3c...

bors-servo pushed a commit that referenced this pull request Oct 31, 2015
Auto merge of #7956 - gkbrk:binary_mime, r=jdm
f432c3c 
Made binary files show an info message instead of garbled text.

Content-types with the TopLevel "Application" such as

* application/octet-stream
* application/pdf

now show an info message instead of trying to view binary data as html.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7956)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit aa7a391 into servo:master Oct 31, 2015
@jdm jdm mentioned this pull request Nov 17, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@gkbrk @highfive @jdm @bors-servo @eefriedman @Manishearth