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.

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

Response API #13058

Merged
merged 1 commit into from Sep 9, 2016
Merged

Response API #13058

merged 1 commit into from Sep 9, 2016

Conversation

malisas
Copy link
Contributor

@malisas malisas commented Aug 26, 2016

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 Reviewable

@highfive
Copy link

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

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 26, 2016
@malisas
Copy link
Contributor Author

malisas commented Aug 26, 2016

r? @jdm

@highfive highfive assigned jdm and unassigned nox Aug 26, 2016
@fitzgen
Copy link
Contributor

fitzgen commented Aug 26, 2016

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 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 Aug 26, 2016
@jdm
Copy link
Member

jdm commented Aug 26, 2016

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

@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 Aug 26, 2016
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Aug 27, 2016
@malisas
Copy link
Contributor Author

malisas commented Aug 29, 2016

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
Copy link
Contributor Author

malisas commented Aug 29, 2016

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
Copy link
Contributor Author

malisas commented Aug 30, 2016

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
Copy link
Contributor Author

malisas commented Aug 30, 2016

r? @Manishearth
Thanks!

@highfive highfive assigned Manishearth and unassigned jdm Aug 30, 2016
@Manishearth
Copy link
Member

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit dd0b5b3 with merge dff4a7d...

bors-servo pushed a commit that referenced this pull request Aug 30, 2016
Response API

<!-- Please describe your changes on the following line: -->
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).

---
<!-- 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 _____

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13058)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-dev

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 30, 2016
@Manishearth
Copy link
Member

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

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 30, 2016
@@ -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())),
Copy link
Member

Choose a reason for hiding this comment

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

b"OK".to_vec() will work

@highfive
Copy link

highfive commented Sep 8, 2016

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

@bors-servo
Copy link
Contributor

⚡ 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
Copy link
Contributor

Ms2ger commented Sep 8, 2016

@bors-servo r- force

(testing)

@Ms2ger Ms2ger closed this Sep 8, 2016
@Ms2ger Ms2ger reopened this Sep 8, 2016
@jdm
Copy link
Member

jdm commented Sep 8, 2016

@bors-servo: r=Manishearth,jdm

@bors-servo
Copy link
Contributor

📌 Commit faf32a7 has been approved by Manishearth,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-tests-failed The changes caused existing tests to fail. labels Sep 8, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit faf32a7 with merge c5be617...

bors-servo pushed a commit that referenced this pull request Sep 8, 2016
Response API

<!-- Please describe your changes on the following line: -->
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).

---
<!-- 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 _____

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13058)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt

@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 8, 2016
@highfive
Copy link

highfive commented Sep 8, 2016

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

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 8, 2016
bors-servo pushed a commit that referenced this pull request Sep 8, 2016
Response API

<!-- Please describe your changes on the following line: -->
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).

---
<!-- 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 _____

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13058)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

⌛ Testing commit faf32a7 with merge 5a5a76c...

@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 Sep 8, 2016
@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 Sep 9, 2016
@highfive
Copy link

highfive commented Sep 9, 2016

  ▶ 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
Copy link
Contributor

KiChjang commented Sep 9, 2016

@bors-servo
Copy link
Contributor

⚡ 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
Copy link
Contributor

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

@bors-servo bors-servo merged commit faf32a7 into servo:master Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-tests-failed The changes caused existing tests to fail.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants