Response API #13058

malisas · 2016-08-26T16:05:27Z

This PR adds the dom::Response implementation and addresses #11896.

The relevant passing tests` expectations have been updated.

In order to allow non-UTF-8-encoded status messages, net_traits::response::Response's raw_status field has been changed from type Option<RawStatus> to type Option<(u16, Vec<u8>)>. As a result, a few other files which rely on the raw_status field were affected and updated.

TODOs:

The body and trailer methods. Relies on implementation of ReadableStream and Promises.
Similarly, replace the dummy constructor _body: Option<USVString> argument with body: ResponseBodyInit.
Currently, whenever r's response's header list or r's Headers object are mentioned, I always modify the headers_reflector field (of type dom::Headers, or r's Headers object) and not the corresponding hyper::Headers list in the net_traits::Response field. A completely accurate interpretation of the spec might consider making both of these lists the same thing via a reference. Discussion was had.

./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 _____

This change is

highfive · 2016-08-26T16:05:30Z

Heads up! This PR modifies the following files:

@fitzgen: components/devtools/Cargo.toml, components/devtools/actors/network_event.rs, components/devtools/lib.rs, components/devtools_traits/lib.rs, components/devtools_traits/lib.rs
@KiChjang: components/script/dom/request.rs, components/script/dom/webidls/Response.webidl, components/script/dom/mod.rs, components/net_traits/response.rs, components/net_traits/response.rs, components/script/dom/htmllinkelement.rs, components/net/http_loader.rs, components/script/dom/xmlhttprequest.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/htmlmediaelement.rs, components/net/about_loader.rs, components/net/blob_loader.rs, components/script/dom/headers.rs, components/script/dom/response.rs, components/net/fetch/methods.rs, components/script/dom/htmlscriptelement.rs, components/script/dom/bindings/trace.rs

malisas · 2016-08-26T16:06:25Z

r? @jdm

fitzgen · 2016-08-26T18:11:55Z

Oops, looks like there's still some compilation errors:

error[E0308]: mismatched types
   --> /home/travis/build/servo/servo/tests/unit/net/fetch.rs:832:22
    |
832 |         status: Some(RawStatus(200, Cow::Borrowed("OK"))),
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected tuple, found struct `hyper::http::RawStatus`
    |
    = note: expected type `(u16, std::vec::Vec<u8>)`
    = note:    found type `hyper::http::RawStatus`

https://travis-ci.org/servo/servo/jobs/155384418#L2693

etc

jdm · 2016-08-26T19:56:58Z

Great start!
-S-awaiting-review +S-needs-code-changes

Reviewed 27 of 27 files at r1.
Review status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

components/devtools/actors/network_event.rs, line 366 [r1] (raw file):

    pub fn add_response(&mut self, response: DevtoolsHttpResponse) {
        self.response.headers = response.headers.clone();
        self.response.status = match response.status {

self.response.status = response.status.as_ref().map(|&(s, ref st)| {
    let status_text = UTF_8.decode(st, DecoderTrap::Replace).unwrap();
    RawStatus(s, Cow::from(status_text))
});

components/net/http_loader.rs, line 1085 [r1] (raw file):

        metadata.headers = Some(Serde(adjusted_headers));
        let response_status = response.status_raw().clone();
        metadata.status = Some((response_status.0, response_status.1.into_owned().as_bytes().to_vec()));

I think we can use response_status.1.as_bytes().to_vec(), and remove the clone() on the previous line.

components/net/fetch/methods.rs, line 977 [r1] (raw file):

            response.status = Some(res.response.status);
            let response_status = res.response.status_raw().clone();
            response.raw_status = Some((response_status.0, response_status.1.into_owned().as_bytes().to_vec()));

I think we can remove the into_owned() and the clone() on the previous line.

components/script/dom/headers.rs, line 232 [r1] (raw file):

        self.Get(ByteString::new(b"Content-Type".to_vec())).
            unwrap_or(Some(ByteString::new(b"".to_vec()))).
            unwrap_or(ByteString::new(b"".to_vec())).to_lower()

Why are we making this change?

components/script/dom/htmlscriptelement.rs, line 160 [r1] (raw file):

        let status_code = self.metadata.as_ref().and_then(|m| {
            match m.status {
                Some((c, _)) => Some((c)),

No need for the () here.

components/script/dom/request.rs, line 44 [r1] (raw file):

    body_used: Cell<bool>,
    headers: MutNullableHeap<JS<Headers>>,
    mime_type: DOMRefCell<ByteString>,

I don't understand why we're making this change.

components/script/dom/response.rs, line 25 [r1] (raw file):

pub struct Response {
    reflector_: Reflector,
    #[ignore_heap_size_of = "NetTraitsResponse is defined in net_traits"]

We should be able to derive HeapSizeOf on the original type and remove this.

components/script/dom/response.rs, line 29 [r1] (raw file):

    //TODO: Figure out what the spec means by "A Response object's body is its response's body."
    headers_reflector: MutNullableHeap<JS<Headers>>,
    mime_type: DOMRefCell<ByteString>,

Why use a ByteString here?

components/script/dom/response.rs, line 47 [r1] (raw file):

    // https://fetch.spec.whatwg.org/#dom-response
    pub fn new(global: GlobalRef)
               -> Root<Response> {

nit: merge this with the previous line.

components/script/dom/response.rs, line 68 [r1] (raw file):

        // Step 3
        let r = Response::new(global);
        r.headers_reflector.or_init(|| Headers::for_response(r.global().r()));

I don't think this line should be necessary. When we call r.Headers() later, we'll get the correct initialization.

components/script/dom/response.rs, line 92 [r1] (raw file):

        // Step 7
        // TODO: For now always false until ResponseBodyInit argument is fully implemented
        if false {

It seems reasonable to use if let Some(body) = body { now.

components/script/dom/response.rs, line 107 [r1] (raw file):

            // Step 7.4
            if let Some(content_type_contents) = content_type {
                if (*r.Headers()).Get(ByteString::new(b"Content-Type".to_vec())).unwrap() == None {

Can we use Headers::Has instead? Also, is the (*) necessary here?

components/script/dom/response.rs, line 127 [r1] (raw file):

    // https://fetch.spec.whatwg.org/#concept-response-clone
    fn clone_net_traits_response(&self) -> Fallible<NetTraitsResponse> {

I believe this operation should be defined in net_traits/response.rs instead.

components/script/dom/response.rs, line 134 [r1] (raw file):

        // TODO: what is meant by a limited view?
        if net_traits_response.response_type != NetTraitsResponseType::Error {
            return Ok(net_traits_response.clone());

This should check for whether internal_response is non-null, then create a new NetTraitsResponse object that contains a clone of the internal response.

components/script/dom/response.rs, line 179 [r1] (raw file):

        // Step 2
        if parsed_url.is_err() {

let url = match parsed_url {
    Ok(url) => url,
    Err(_) => return Err(...),
};

components/script/dom/response.rs, line 239 [r1] (raw file):

    fn Url(&self) -> USVString {
        let response = self.response.borrow();
        let url_list_len = response.url_list.borrow().len();

I believe USVString(String::from(response.url.as_ref().map(|u| u.as_str()).unwrap_or(""))) is equivalent to the current implementation.

components/script/dom/response.rs, line 243 [r1] (raw file):

            let url_string = String::from(response.url_list.borrow()[url_list_len - 1].as_str());
            return USVString(url_string);
            // TODO: not sure if returned value is "serialized with the exclude-fragment flag set"

as_str() will include the fragment; there does not appear to be an API for Url that does what we want yet, so we should file an issue at https://github.com/servo/rust-url/

components/script/dom/response.rs, line 252 [r1] (raw file):

    fn Redirected(&self) -> bool {
        let response = self.response.borrow();
        let url_list_len = response.url_list.borrow().len();

let response = self.response.borrow();
response.url_list.borrow().len() > 1

components/script/dom/response.rs, line 291 [r1] (raw file):

    // https://fetch.spec.whatwg.org/#dom-response-headers
    fn Headers(&self) -> Root<Headers> {
        self.headers_reflector.or_init(|| Headers::new(self.global().r()))

This should use Headers::for_response, or the guard will be wrong.

components/script/dom/response.rs, line 301 [r1] (raw file):

        // Step 2
        let response_response_clone = try!(self.clone_net_traits_response());
        let cloned_response = reflect_dom_object(box Response {

This should probably delegate to Response::new_inherited/Response::new instead.

components/script/dom/webidls/Response.webidl, line 37 [r1] (raw file):

enum ResponseType { "basic", "cors", "default", "error", "opaque", "opaqueredirect" };

// TODO: BodyInit is already defined in XMLHttpRequest, so compiler complains if we include it here

Let's remove this comment and the commented out code, in that case.

components/servo/Cargo.lock, line 490 [r1] (raw file):

dependencies = [
 "devtools_traits 0.0.1",
 "encoding 0.2.32 (registry+https://github.com/rust-lang/crates.io-index)",

We'll need to run ./mach cargo-update -p devtools_traits in order to have the other Cargo.lock file get updated, too.

tests/wpt/web-platform-tests/fetch/api/response/response-static-redirect.html, line 26 [r1] (raw file):

        test(function() {
      redirectResponse = Response.redirect(url, status);
      assert_equals(redirectResponse.status, status, "Redirect status is " + status);

nit: let's undo the indentation change that happened here.

Comments from Reviewable

bors-servo · 2016-08-27T11:32:29Z

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

malisas · 2016-08-29T22:45:05Z

components/servo/Cargo.lock, line 490 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

We'll need to run ./mach cargo-update -p devtools_traits in order to have the other Cargo.lock file get updated, too.

I got `error: package id specification devtools_traits matched no packages`, so I just ran `./mach cargo-update -a` instead.

Comments from Reviewable

malisas · 2016-08-29T22:45:38Z

components/script/dom/response.rs, line 243 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

as_str() will include the fragment; there does not appear to be an API for Url that does what we want yet, so we should file an issue at https://github.com/servo/rust-url/

The filed issue: https://github.com/servo/rust-url/issues/221

Comments from Reviewable

malisas · 2016-08-30T03:52:40Z

components/servo/Cargo.lock, line 490 [r1] (raw file):

Previously, malisas (Malisa) wrote…

I got error: package id specification devtools_traits matched no packages, so I just ran
./mach cargo-update -a instead.

Unfortunately the huge number of package updates ending up breaking other code, so I reverted that change for the time being. Not sure which command I should run.

Comments from Reviewable

malisas · 2016-08-30T03:54:27Z

r? @Manishearth
Thanks!

Manishearth · 2016-08-30T03:58:33Z

@bors-servo try

bors-servo · 2016-08-30T03:58:36Z

⌛ Trying commit dd0b5b3 with merge dff4a7d...


        Auto merge of #13058 - malisas:malisa-responseAPI, r=<try>

Response API  This PR adds the [dom::Response](https://fetch.spec.whatwg.org/#response-class) implementation and addresses #11896. The relevant passing tests` expectations have been updated. In order to allow non-UTF-8-encoded status messages, `net_traits::response::Response`'s `raw_status` field has been changed from type [`Option<RawStatus>`](https://doc.servo.org/hyper/http/struct.RawStatus.html) to type `Option<(u16, Vec<u8>)>`. As a result, a few other files which rely on the `raw_status` field were affected and updated. TODOs: - The `body` and `trailer` methods. Relies on implementation of `ReadableStream` and `Promise`s. - Similarly, replace the dummy constructor `_body: Option<USVString>` argument with `body: ResponseBodyInit`. - Currently, whenever `r's response's header list` or `r's Headers object` are mentioned, I always modify the `headers_reflector` field (of type dom::Headers, or `r's Headers object`) and not the corresponding hyper::Headers list in the net_traits::Response field. A completely accurate interpretation of the spec might consider making both of these lists the same thing via a reference. [Discussion](whatwg/fetch#358) was [had](#12884). ---  - [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).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13058)

bors-servo · 2016-08-30T04:09:13Z

💔 Test failed - linux-dev

Manishearth · 2016-08-30T04:12:23Z

You'll need to apply this diff:

diff --git a/ports/cef/Cargo.lock b/ports/cef/Cargo.lock
index 1d41dde..e8cc5e1 100644
--- a/ports/cef/Cargo.lock
+++ b/ports/cef/Cargo.lock
@@ -445,6 +445,7 @@ name = "devtools"
 version = "0.0.1"
 dependencies = [
  "devtools_traits 0.0.1",
+ "encoding 0.2.32 (registry+https://github.com/rust-lang/crates.io-index)",
  "hyper 0.9.10 (registry+https://github.com/rust-lang/crates.io-index)",
  "hyper_serde 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "ipc-channel 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)",

let's see what wpt says: http://build.servo.org/builders/mac-rel-wpt/builds/2899

Manishearth · 2016-08-30T07:07:59Z

components/net/about_loader.rs

@@ -38,7 +37,7 @@ pub fn factory(mut load_data: LoadData,
                    Some(Serde(ContentType(Mime(TopLevel::Text, SubLevel::Html, vec![])))),
                charset: Some("utf-8".to_owned()),
                headers: None,
-                status: Some(Serde(RawStatus(200, "OK".into()))),
+                status: Some((200, "OK".as_bytes().to_vec())),


b"OK".to_vec() will work

Manishearth · 2016-08-30T07:24:35Z

tests/wpt/web-platform-tests/fetch/api/response/response-static-redirect.html

-          redirectResponse = Response.redirect(url, status);
-          assert_equals(redirectResponse.status, status, "Redictect status is " + status);
+        redirectResponse = Response.redirect(url, status);
+        assert_equals(redirectResponse.status, status, "Redirect status is " + status);


This indentation still needs fixing

highfive · 2016-09-08T05:17:52Z

  ▶ ERROR [expected OK] /webgl/conformance-1.0.3/conformance/ogles/GL/build/build_001_to_008.html
  └   → gl.getProgramInfoLog is not a function

Manishearth · 2016-09-08T05:20:31Z

@bors-servo retry

#13165

bors-servo · 2016-09-08T08:21:23Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only mac-rel-wpt...

Ms2ger · 2016-09-08T09:35:34Z

@bors-servo r- force

(testing)

jdm · 2016-09-08T15:25:34Z

@bors-servo: r=Manishearth,jdm

bors-servo · 2016-09-08T15:25:35Z

📌 Commit faf32a7 has been approved by Manishearth,jdm

bors-servo · 2016-09-08T21:47:02Z

⌛ Testing commit faf32a7 with merge c5be617...


        Auto merge of #13058 - malisas:malisa-responseAPI, r=Manishearth,jdm

Response API  This PR adds the [dom::Response](https://fetch.spec.whatwg.org/#response-class) implementation and addresses #11896. The relevant passing tests` expectations have been updated. In order to allow non-UTF-8-encoded status messages, `net_traits::response::Response`'s `raw_status` field has been changed from type [`Option<RawStatus>`](https://doc.servo.org/hyper/http/struct.RawStatus.html) to type `Option<(u16, Vec<u8>)>`. As a result, a few other files which rely on the `raw_status` field were affected and updated. TODOs: - The `body` and `trailer` methods. Relies on implementation of `ReadableStream` and `Promise`s. - Similarly, replace the dummy constructor `_body: Option<USVString>` argument with `body: ResponseBodyInit`. - Currently, whenever `r's response's header list` or `r's Headers object` are mentioned, I always modify the `headers_reflector` field (of type dom::Headers, or `r's Headers object`) and not the corresponding hyper::Headers list in the net_traits::Response field. A completely accurate interpretation of the spec might consider making both of these lists the same thing via a reference. [Discussion](whatwg/fetch#358) was [had](#12884). ---  - [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).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13058)

bors-servo · 2016-09-08T22:43:49Z

💔 Test failed - mac-rel-wpt

highfive · 2016-09-08T22:43:51Z

  ▶ ERROR [expected OK] /webgl/conformance-1.0.3/conformance/ogles/GL/build/build_001_to_008.html
  └   → gl.getProgramInfoLog is not a function

jdm · 2016-09-08T23:05:13Z

@bors-servo: retry

#13165


        Auto merge of #13058 - malisas:malisa-responseAPI, r=Manishearth,jdm

Response API  This PR adds the [dom::Response](https://fetch.spec.whatwg.org/#response-class) implementation and addresses #11896. The relevant passing tests` expectations have been updated. In order to allow non-UTF-8-encoded status messages, `net_traits::response::Response`'s `raw_status` field has been changed from type [`Option<RawStatus>`](https://doc.servo.org/hyper/http/struct.RawStatus.html) to type `Option<(u16, Vec<u8>)>`. As a result, a few other files which rely on the `raw_status` field were affected and updated. TODOs: - The `body` and `trailer` methods. Relies on implementation of `ReadableStream` and `Promise`s. - Similarly, replace the dummy constructor `_body: Option<USVString>` argument with `body: ResponseBodyInit`. - Currently, whenever `r's response's header list` or `r's Headers object` are mentioned, I always modify the `headers_reflector` field (of type dom::Headers, or `r's Headers object`) and not the corresponding hyper::Headers list in the net_traits::Response field. A completely accurate interpretation of the spec might consider making both of these lists the same thing via a reference. [Discussion](whatwg/fetch#358) was [had](#12884). ---  - [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).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13058)

bors-servo · 2016-09-08T23:58:06Z

⌛ Testing commit faf32a7 with merge 5a5a76c...

bors-servo · 2016-09-09T00:52:16Z

💔 Test failed - linux-rel

highfive · 2016-09-09T00:52:18Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

KiChjang · 2016-09-09T00:53:27Z

@bors-servo retry

#11574

bors-servo · 2016-09-09T01:40:51Z

⚡ Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only linux-rel, mac-rel-wpt...

bors-servo · 2016-09-09T02:37:53Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned nox Aug 26, 2016

highfive added the S-awaiting-review label Aug 26, 2016

highfive assigned jdm and unassigned nox Aug 26, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Aug 26, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 26, 2016

highfive added the S-needs-rebase label Aug 27, 2016

malisas force-pushed the malisas:malisa-responseAPI branch from b06aa4e to dd0b5b3 Aug 30, 2016

highfive assigned Manishearth and unassigned jdm Aug 30, 2016

highfive added the S-tests-failed label Aug 30, 2016

highfive removed the S-tests-failed label Aug 30, 2016

Manishearth reviewed Aug 30, 2016
View changes

Ms2ger closed this Sep 8, 2016

Ms2ger reopened this Sep 8, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 8, 2016

highfive removed the S-awaiting-merge label Sep 8, 2016

highfive added the S-tests-failed label Sep 8, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 8, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 9, 2016

bors-servo merged commit faf32a7 into servo:master Sep 9, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

bors-servo mentioned this pull request Sep 9, 2016

Tidy: Fix ordering use statements with braces #13205

Merged

servo / servo

Response API #13058

Response API #13058

malisas commented Aug 26, 2016 •

edited by larsbergstrom

highfive commented Aug 26, 2016

malisas commented Aug 26, 2016

fitzgen commented Aug 26, 2016

jdm commented Aug 26, 2016

bors-servo commented Aug 27, 2016

malisas commented Aug 29, 2016

malisas commented Aug 29, 2016

malisas commented Aug 30, 2016

malisas commented Aug 30, 2016

Manishearth commented Aug 30, 2016

bors-servo commented Aug 30, 2016

bors-servo commented Aug 30, 2016

Manishearth commented Aug 30, 2016

This comment has been minimized.

This comment has been minimized.

highfive commented Sep 8, 2016

Manishearth commented Sep 8, 2016

bors-servo commented Sep 8, 2016

Ms2ger commented Sep 8, 2016

jdm commented Sep 8, 2016

bors-servo commented Sep 8, 2016

bors-servo commented Sep 8, 2016

bors-servo commented Sep 8, 2016

highfive commented Sep 8, 2016

jdm commented Sep 8, 2016

bors-servo commented Sep 8, 2016

bors-servo commented Sep 9, 2016

highfive commented Sep 9, 2016

KiChjang commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

servo / servo

Join GitHub today

Response API #13058

Response API #13058

Conversation

malisas commented Aug 26, 2016 • edited by larsbergstrom

highfive commented Aug 26, 2016

malisas commented Aug 26, 2016

fitzgen commented Aug 26, 2016

jdm commented Aug 26, 2016

bors-servo commented Aug 27, 2016

malisas commented Aug 29, 2016

malisas commented Aug 29, 2016

malisas commented Aug 30, 2016

malisas commented Aug 30, 2016

Manishearth commented Aug 30, 2016

bors-servo commented Aug 30, 2016

bors-servo commented Aug 30, 2016

Manishearth commented Aug 30, 2016

This comment has been minimized.

Manishearth Aug 30, 2016

This comment has been minimized.

Manishearth Aug 30, 2016

highfive commented Sep 8, 2016

Manishearth commented Sep 8, 2016

bors-servo commented Sep 8, 2016

Ms2ger commented Sep 8, 2016

jdm commented Sep 8, 2016

bors-servo commented Sep 8, 2016

bors-servo commented Sep 8, 2016

bors-servo commented Sep 8, 2016

highfive commented Sep 8, 2016

jdm commented Sep 8, 2016

bors-servo commented Sep 8, 2016

bors-servo commented Sep 9, 2016

highfive commented Sep 9, 2016

KiChjang commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

malisas commented Aug 26, 2016 •

edited by larsbergstrom