Skip to content

/ servo

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

interpret empty data URI as plain text #7836

Merged
merged 1 commit into from Oct 3, 2015
Merged

interpret empty data URI as plain text #7836

merged 1 commit into from Oct 3, 2015

Conversation

@6112
Copy link
Contributor

6112 commented Oct 2, 2015

Fixes #7803. As @eefriedman pointed out, RFC 2397 says:

If <mediatype> is omitted, it defaults to text/plain;charset=US-ASCII. As a shorthand, "text/plain" can be omitted but the charset parameter supplied.

Review on Reviewable
tests/unit/net/data_loader.rs Outdated
@@ -69,8 +72,23 @@ fn plain_charset() {
}

#[test]
fn plain_only_charset() {
assert_parse(
"data:charset=utf-8,hello",

This comment has been minimized.

Sign in to view
Copy link
@eefriedman

eefriedman Oct 2, 2015

Contributor

The correct shorthand is data:;charset=utf-8,hello. data:charset=utf-8,<b>hello gets interpreted as HTML by Firefox.
tests/unit/net/data_loader.rs Outdated
assert_parse("data:,hello%20world", None, None, Some(b"hello world".iter().map(|&x| x).collect()));
assert_parse(
"data:,hello%20world",
Some(ContentType(Mime(TopLevel::Text, SubLevel::Plain, vec!()))),

This comment has been minimized.

Sign in to view
Copy link
@eefriedman

eefriedman Oct 2, 2015

Contributor

This should be Some(ContentType(Mime(TopLevel::Text, SubLevel::Plain, vec!((Attr::Charset, Value::Ext("US-ASCII".to_owned()))))), or something like that.
@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 3, 2015

-S-awaiting-review +S-needs-squash

This looks good to me. Added one minor/optional style comment. After addressing that (if you choose), please rebase and squash all the commits, and this can land with r=mbrubeck. Thanks!

Reviewed 2 of 2 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

components/net/data_loader.rs, line 64 [r1] (raw file):
Nit-picking: Personally I prefer if content_type.is_none() or if content_type == None. But if you find this version more readable, you can keep it.

Comments from the review on Reviewable.io
@mbrubeck mbrubeck self-assigned this Oct 3, 2015
@6112 6112 force-pushed the 6112:master branch from f9d031d to eefa405 Oct 3, 2015
@6112
make mediatype default to text/plain in data URIs
Loading status checks…
d4ebec6 
change `data:charset=` to `data:;charset=` and set US-ASCII as default encoding

style change
@6112 6112 force-pushed the 6112:master branch from eefa405 to d4ebec6 Oct 3, 2015
@6112
Copy link
Contributor Author

6112 commented Oct 3, 2015

I think I did the right thing. I'm still getting used to git.
@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 3, 2015

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io
@mbrubeck mbrubeck removed the S-needs-squash label Oct 3, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 3, 2015

@bors-servo r+

Looks great, thanks!
@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2015

📌 Commit d4ebec6 has been approved by mbrubeck
@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2015

⌛ Testing commit d4ebec6 with merge f30f40b...
bors-servo pushed a commit that referenced this pull request Oct 3, 2015
bors-servo
Auto merge of #7836 - 6112:master, r=mbrubeck
Loading status checks…
f30f40b 
interpret empty data URI as plain text

Fixes #7803. As @eefriedman pointed out, RFC 2397 says:

> If &lt;mediatype&gt; is omitted, it defaults to text/plain;charset=US-ASCII.  As a shorthand, "text/plain" can be omitted but the charset parameter supplied.

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

bors-servo commented Oct 3, 2015

💔 Test failed - mac-dev-ref-unit
@jdm
Copy link
Member

jdm commented Oct 3, 2015

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2015

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-rel-wpt are reusable. Rebuilding only mac-dev-ref-unit, mac-rel-css...
@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit d4ebec6 into servo:master Oct 3, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mbrubeck mbrubeck

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@6112 @mbrubeck @bors-servo @jdm @eefriedman @SimonSapin @highfive
You can’t perform that action at this time.