Make Metadata use ContentType #5544

boghison · 2015-04-06T18:02:38Z

highfive · 2015-04-06T18:02:41Z

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

hoppipolla-critic-bot · 2015-04-06T18:02:42Z

Critic review: https://critic.hoppipolla.co.uk/r/4531

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

Manishearth · 2015-04-06T18:19:02Z

components/script/parse/html.rs

                    let page = format!("<html><body><img src='{}' /></body></html>", url.serialize());
                    parser.parse_chunk(page);
                },
-                Some((ref t, ref st)) if t.as_slice().eq_ignore_ascii_case("text") &&
-                                         st.as_slice().eq_ignore_ascii_case("plain") => {
+                Some(ContentType(Mime(ref t, ref st, _))) if *t == TopLevel::Text &&


Can't we just do Mime(TopLevel::Text, SubLevel::Plain, _)?

Oh, yes, totally, I was just keeping the original structure :)

Manishearth · 2015-04-06T18:19:11Z

components/script/parse/html.rs

@@ -291,12 +293,12 @@ pub fn parse_html(document: JSRef<Document>,
        }
        HTMLInput::InputUrl(load_response) => {
            match load_response.metadata.content_type {
-                Some((ref t, _)) if t.as_slice().eq_ignore_ascii_case("image") => {
+                Some(ContentType(Mime(ref t, _, _))) if *t == TopLevel::Image => {


(same here)

Manishearth · 2015-04-06T18:19:18Z

components/script/script_task.rs

@@ -1067,8 +1069,8 @@ impl ScriptTask {
        });

        let content_type = match response.metadata.content_type {
-            Some((ref t, ref st)) if t.as_slice().eq_ignore_ascii_case("text") &&
-                                    st.as_slice().eq_ignore_ascii_case("plain") => {
+            Some(ContentType(Mime(ref t, ref st, _))) if *t == TopLevel::Text &&


(same here)

Manishearth · 2015-04-06T18:19:47Z

Looks good, aside from some minor fixes in the match.

Thanks!

Manishearth · 2015-04-06T23:32:32Z

@bors-servo: r+

bors-servo · 2015-04-06T23:32:32Z

📌 Commit 84e9b5d has been approved by Manishearth

bors-servo · 2015-04-08T03:59:14Z

⌛ Testing commit 84e9b5d with merge 76ec67f...


        Auto merge of #5544 - boghison:master, r=Manishearth

Fixes #5538

bors-servo · 2015-04-08T04:04:11Z

💔 Test failed - mac2

jdm · 2015-04-08T04:18:01Z

Sorry, looks like #5005 added some code that needs to be adjusted now.

/Users/servo/buildbot/slave/mac2/build/components/net/resource_task.rs:82:53: 82:75 error: mismatched types:
 expected `&core::option::Option<(collections::string::String, collections::string::String)>`,
    found `&core::option::Option<hyper::header::common::content_type::ContentType>`
(expected tuple,
    found struct `hyper::header::common::content_type::ContentType`) [E0308]
/Users/servo/buildbot/slave/mac2/build/components/net/resource_task.rs:82                                                     &metadata.content_type, &partial_body);
                                                                                                                              ^~~~~~~~~~~~~~~~~~~~~~
/Users/servo/buildbot/slave/mac2/build/components/net/resource_task.rs:81:33: 82:91 error: mismatched types:
 expected `core::option::Option<hyper::header::common::content_type::ContentType>`,
    found `core::option::Option<(collections::string::String, collections::string::String)>`
(expected struct `hyper::header::common::content_type::ContentType`,
    found tuple) [E0308]
/Users/servo/buildbot/slave/mac2/build/components/net/resource_task.rs:81         metadata.content_type = classifier.classify(nosniff, check_for_apache_bug,
/Users/servo/buildbot/slave/mac2/build/components/net/resource_task.rs:82                                                     &metadata.content_type, &partial_body);
error: aborting due to 2 previous errors

boghison · 2015-04-08T12:49:35Z

@jdm so what am I supposed to do, edit the whole mime classifier or just send toplevel and sublevel as strings?

jdm · 2015-04-08T15:45:24Z

I would go ahead and edit the classifier code.

boghison · 2015-04-08T18:29:54Z

@jdm That is going to need a lot of rewriting, mostly because almost everything has been fixed to use strings there

jdm · 2015-04-08T18:38:35Z

Oh, hmm. Go ahead and pass it strings, in that case. We can file a follow-up to rewrite it separately.

boghison · 2015-04-08T18:40:27Z

@jdm Yes, that would be best, the hardest part is probably dealing with non standard types like application+xml

boghison · 2015-04-09T16:42:27Z

@jdm How do I actually convert a mime to a string?

jdm · 2015-04-09T17:05:14Z

You might need to resort to format!("{}", mime) for the full a/b format, or format!("{}", mime.0) for the subparts in particular.

jdm · 2015-04-14T14:15:48Z

@bors-servo: r+

bors-servo · 2015-04-14T14:15:49Z

📌 Commit 214c9d9 has been approved by jdm

bors-servo · 2015-04-14T14:22:11Z

⌛ Testing commit 214c9d9 with merge 55d19d1...


        Auto merge of #5544 - boghison:master, r=jdm

Fixes #5538  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5544)

bors-servo · 2015-04-14T14:44:20Z

💔 Test failed - linux1

jdm · 2015-04-14T15:27:45Z

<std macros>:5:10: 5:35 error: the trait `core::cmp::PartialEq<core::option::Option<(collections::string::String, collections::string::String)>>` is not implemented for the type `core::option::Option<hyper::header::common::content_type::ContentType>` [E0277]
<std macros>:5 if ! ( ( * left_val == * right_val ) && ( * right_val == * left_val ) ) {
                        ^~~~~~~~~~~~~~~~~~~~~~~~~
<std macros>:1:1: 9:39 note: in expansion of assert_eq!
/home/servo/buildbot/slave/linux1/build/tests/unit/net/data_loader.rs:21:5: 21:64 note: expansion site
<std macros>:5:43: 5:68 error: the trait `core::cmp::PartialEq<core::option::Option<hyper::header::common::content_type::ContentType>>` is not implemented for the type `core::option::Option<(collections::string::String, collections::string::String)>` [E0277]
<std macros>:5 if ! ( ( * left_val == * right_val ) && ( * right_val == * left_val ) ) {
                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~
<std macros>:1:1: 9:39 note: in expansion of assert_eq!
/home/servo/buildbot/slave/linux1/build/tests/unit/net/data_loader.rs:21:5: 21:64 note: expansion site

You can reproduce this with ./mach test-unit.


        Make Metadata use ContentType


        Reduce duplication

boghison · 2015-04-14T19:41:03Z

@jdm To fix this test I'd have to add hyper as an external dependency, is it ok?

Manishearth · 2015-04-14T19:41:53Z

Yes, to the unit tests crate.

boghison · 2015-04-14T19:43:08Z

@Manishearth Yes, obviously


        Fix content type unit test

jdm · 2015-04-14T20:11:17Z

@bors-servo: r+

bors-servo · 2015-04-14T20:11:17Z

📌 Commit 3dd48d2 has been approved by jdm

bors-servo · 2015-04-14T20:11:20Z

⌛ Testing commit 3dd48d2 with merge 7f422e2...


        Auto merge of #5544 - boghison:master, r=jdm

Fixes #5538  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5544)

bors-servo · 2015-04-14T20:42:39Z

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

Manishearth reviewed Apr 6, 2015
View changes

jdm added the S-awaiting-review label Apr 6, 2015

bors-servo pushed a commit that referenced this pull request Apr 8, 2015

Auto merge of #5544 - boghison:master, r=Manishearth

76ec67f

Fixes #5538

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 8, 2015

metajack assigned Manishearth Apr 9, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 13, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 13, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 13, 2015

jdm added S-awaiting-merge and removed S-awaiting-review labels Apr 14, 2015

jdm added S-tests-failed and removed S-awaiting-merge labels Apr 14, 2015

boghison added 2 commits Apr 13, 2015

Make Metadata use ContentType

19ffab8

Reduce duplication

ce4b8a0

highfive added S-awaiting-review and removed S-tests-failed labels Apr 14, 2015

Fix content type unit test

Loading status checks…

3dd48d2

jdm added S-awaiting-merge and removed S-awaiting-review labels Apr 14, 2015

bors-servo merged commit 3dd48d2 into servo:master Apr 14, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

Make Metadata use ContentType #5544

Make Metadata use ContentType #5544

boghison commented Apr 6, 2015

highfive commented Apr 6, 2015

hoppipolla-critic-bot commented Apr 6, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Apr 6, 2015

Manishearth commented Apr 6, 2015

bors-servo commented Apr 6, 2015

bors-servo commented Apr 8, 2015

bors-servo commented Apr 8, 2015

jdm commented Apr 8, 2015

boghison commented Apr 8, 2015

jdm commented Apr 8, 2015

boghison commented Apr 8, 2015

jdm commented Apr 8, 2015

boghison commented Apr 8, 2015

boghison commented Apr 9, 2015

jdm commented Apr 9, 2015

jdm commented Apr 14, 2015

bors-servo commented Apr 14, 2015

bors-servo commented Apr 14, 2015

bors-servo commented Apr 14, 2015

jdm commented Apr 14, 2015

boghison commented Apr 14, 2015

Manishearth commented Apr 14, 2015

boghison commented Apr 14, 2015

jdm commented Apr 14, 2015

bors-servo commented Apr 14, 2015

bors-servo commented Apr 14, 2015

bors-servo commented Apr 14, 2015

servo / servo

Join GitHub today

Make Metadata use ContentType #5544

Make Metadata use ContentType #5544

Conversation

boghison commented Apr 6, 2015

highfive commented Apr 6, 2015

hoppipolla-critic-bot commented Apr 6, 2015

This comment has been minimized.

Manishearth Apr 6, 2015

This comment has been minimized.

boghison Apr 6, 2015

This comment has been minimized.

Manishearth Apr 6, 2015

This comment has been minimized.

Manishearth Apr 6, 2015

Manishearth commented Apr 6, 2015

Manishearth commented Apr 6, 2015

bors-servo commented Apr 6, 2015

bors-servo commented Apr 8, 2015

bors-servo commented Apr 8, 2015

jdm commented Apr 8, 2015

boghison commented Apr 8, 2015

jdm commented Apr 8, 2015

boghison commented Apr 8, 2015

jdm commented Apr 8, 2015

boghison commented Apr 8, 2015

boghison commented Apr 9, 2015

jdm commented Apr 9, 2015

jdm commented Apr 14, 2015

bors-servo commented Apr 14, 2015

bors-servo commented Apr 14, 2015

bors-servo commented Apr 14, 2015

jdm commented Apr 14, 2015

boghison commented Apr 14, 2015

Manishearth commented Apr 14, 2015

boghison commented Apr 14, 2015

jdm commented Apr 14, 2015

bors-servo commented Apr 14, 2015

bors-servo commented Apr 14, 2015

bors-servo commented Apr 14, 2015