Implement the Fetch method #13323

jeenalee · 2016-09-19T17:37:37Z

This PR implements the fetch method, which is described in the Fetch Spec. The expected wpt results are updated as well.

A few comments about the current fetch implementation:

The fetch method manually calls JSAutoCompartment in order to prevent SpiderMonkey from crashing. This may have to change in the future.
Not all FetchResponseListener methods are implemented.
net_traits::CoreResourceMsg::Fetch message takes a net_traits::request::RequestInit. However, when the fetch method is called, a RequestBinding::RequestInit is given. The fetch method constructs a dom::request::Request with the given RequestInit, then creates net_traits::request::RequestInit from the dom Request object for the fetch message.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11898 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-09-19T17:37:41Z

Heads up! This PR modifies the following files:

@fitzgen: components/devtools/Cargo.toml, components/devtools/lib.rs, components/devtools/actors/network_event.rs, components/devtools_traits/lib.rs, components/devtools_traits/lib.rs
@KiChjang: components/script/dom/request.rs, components/net_traits/response.rs, components/net_traits/response.rs, components/script/timers.rs, components/net/http_loader.rs, components/script/dom/workerglobalscope.rs, components/script/dom/bindings/error.rs, components/script/fetch.rs, components/script/Cargo.toml, components/net/blob_loader.rs, components/script/script_thread.rs, components/script/dom/promise.rs, components/script/dom/webidls/Fetch.webidl, components/script/dom/mod.rs, components/script/dom/htmllinkelement.rs, components/script/dom/bindings/refcounted.rs, components/script/dom/testbinding.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/webidls/PromiseNativeHandler.webidl, components/script/dom/response.rs, components/script/dom/bindings/codegen/Bindings.conf, components/script/dom/bindings/trace.rs, components/script/dom/window.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webidls/TestBinding.webidl, components/net/fetch/methods.rs, components/script/dom/webidls/Promise.webidl, components/script/dom/bindings/global.rs, components/script/dom/webidls/Response.webidl, components/script/dom/htmlscriptelement.rs, components/script/lib.rs, components/script/dom/bindings/codegen/Configuration.py, components/net/about_loader.rs, components/script/dom/promisenativehandler.rs, components/script/dom/headers.rs, components/script/dom/bindings/js.rs, components/script/script_runtime.rs, components/script/dom/webidls/WindowOrWorkerGlobalScope.webidl

highfive · 2016-09-19T17:37:42Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.
These commits modify unsafe code. Please review it carefully!

jeenalee · 2016-09-19T17:41:34Z

r? @jdm

emilio · 2016-09-21T09:32:26Z

@bors-servo: try

bors-servo · 2016-09-21T09:32:29Z

⌛ Trying commit c135549 with merge 864ba39...


        Auto merge of #13323 - jeenalee:fetch, r=<try>

Implement the Fetch method  This PR implements the fetch method, which is described in [the Fetch Spec](https://fetch.spec.whatwg.org/#fetch-method). The expected wpt results are updated as well. A few comments about the current fetch implementation: - The fetch method manually calls `JSAutoCompartment` in order to prevent SpiderMonkey from crashing. This may have to change in the future. - Not all `FetchResponseListener` methods are implemented. - `net_traits::CoreResourceMsg::Fetch` message takes a `net_traits::request::RequestInit`. However, when the fetch method is called, a `RequestBinding::RequestInit` is given. The fetch method constructs a `dom::request::Request` with the given `RequestInit`, then creates `net_traits::request::RequestInit` from the dom Request object for the fetch message. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11898 (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/13323)

bors-servo · 2016-09-21T10:24:46Z

💔 Test failed - linux-rel

jdm

Nice work!

jdm · 2016-09-21T13:48:04Z

components/script/dom/headers.rs

@@ -217,6 +217,10 @@ impl Headers {
        self.guard.set(new_guard)
    }

+    pub fn set_guard_immutable(&self) {


We can use set_guard instead.

jdm · 2016-09-21T13:48:04Z

components/script/dom/response.rs

+
+impl Response {
+    pub fn set_headers(&self, option_hyper_headers: Option<Serde<HyperHeaders>>) {
+        self.Headers().set_headers( match option_hyper_headers {


nit: extra space after (

jdm · 2016-09-21T13:48:04Z

components/script/fetch.rs

+use std::sync::{Arc, Mutex};
+use url::Url;
+
+#[allow(unrooted_must_root)]


Do you remember why this was required?

The compiler was saying that that Fallible<Rc<Promise>> should be rooted, and that was the reason it was added. But looking at the code again, I don't think it's required. I'll try removing it.

It was indeed not required. Thank you for pointing it out!

jdm · 2016-09-21T13:48:04Z

components/script/fetch.rs

+}
+
+fn from_referer_to_referer_url(request: &NetTraitsRequest) -> Option<Url> {
+    let referer = request.referrer.borrow();


jdm · 2016-09-21T13:48:04Z

components/script/fetch.rs

+    response_object: Trusted<Response>,
+}
+
+fn from_referer_to_referer_url(request: &NetTraitsRequest) -> Option<Url> {


from_referrer_to_referrer_url

jdm · 2016-09-21T13:48:04Z

components/script/dom/webidls/Fetch.webidl

+[Exposed=(Window,Worker)]
+
+partial interface WindowOrWorkerGlobalScope {
+  [NewObject, Throws] Promise<Response> fetch(RequestInfo input, optional RequestInit init);


No need for Throws anymore.

jdm · 2016-09-21T13:48:04Z

components/script/fetch.rs

+    let (action_sender, action_receiver) = ipc::channel().unwrap();
+    let fetch_context = Arc::new(Mutex::new(FetchContext {
+        fetch_promise: Some(TrustedPromise::new(promise.clone())),
+        response_object: Trusted::new(response.r()),


We can use &* instead of .r() here.

jdm · 2016-09-21T13:48:04Z

components/script/fetch.rs

+    }));
+    let listener = NetworkListener {
+        context: fetch_context.clone(),
+        // TODO: double check how to get script_chan


This comment is meaningless here; let's remove it.

jdm · 2016-09-21T13:48:04Z

components/script/fetch.rs

+        response_object: Trusted::new(response.r()),
+    }));
+    let listener = NetworkListener {
+        context: fetch_context.clone(),


I don't think this clone is necessary.

jdm · 2016-09-21T13:48:04Z

components/script/fetch.rs

+
+    #[allow(unrooted_must_root)]
+    fn process_response(&mut self, metadata: Result<Metadata, NetworkError>) {
+        let promise = match self.fetch_promise.take() {


Let's use let promise = self.fetch_promise.take().expect("fetch promise is missing").root();

jeenalee · 2016-09-21T17:38:28Z

@jdm Review comments addressed!

jdm · 2016-09-21T18:42:42Z

Great! You can squash your commits together, and then we'll merge this once #12830 is merged.

bors-servo · 2016-09-29T12:19:47Z

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


        Implement the Fetch method

jeenalee · 2016-09-29T15:21:03Z

Rebased as #13345 was merged (hooray!)!

jdm · 2016-09-29T15:42:56Z

@jeenalee: Does this include new test results for all the tests? Working body methods will have changed the results from previous runs significantly.

jeenalee · 2016-09-29T16:30:23Z

@jdm I built and ran fetch/api/ tests, and updated the test results. Will that suffice?

jdm · 2016-09-29T16:47:41Z

@bors-servo: r+

bors-servo · 2016-09-29T16:47:42Z

📌 Commit 3216009 has been approved by jdm

bors-servo · 2016-09-29T17:26:11Z

⌛ Testing commit 3216009 with merge f6d5cac...


        Auto merge of #13323 - jeenalee:fetch, r=jdm

Implement the Fetch method  This PR implements the fetch method, which is described in [the Fetch Spec](https://fetch.spec.whatwg.org/#fetch-method). The expected wpt results are updated as well. A few comments about the current fetch implementation: - The fetch method manually calls `JSAutoCompartment` in order to prevent SpiderMonkey from crashing. This may have to change in the future. - Not all `FetchResponseListener` methods are implemented. - `net_traits::CoreResourceMsg::Fetch` message takes a `net_traits::request::RequestInit`. However, when the fetch method is called, a `RequestBinding::RequestInit` is given. The fetch method constructs a `dom::request::Request` with the given `RequestInit`, then creates `net_traits::request::RequestInit` from the dom Request object for the fetch message. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11898 (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/13323)

bors-servo · 2016-09-29T18:16:23Z

💔 Test failed - linux-rel

jdm · 2016-09-29T18:21:51Z

@bors-servo: retry
#12654


        Auto merge of #13323 - jeenalee:fetch, r=jdm

Implement the Fetch method  This PR implements the fetch method, which is described in [the Fetch Spec](https://fetch.spec.whatwg.org/#fetch-method). The expected wpt results are updated as well. A few comments about the current fetch implementation: - The fetch method manually calls `JSAutoCompartment` in order to prevent SpiderMonkey from crashing. This may have to change in the future. - Not all `FetchResponseListener` methods are implemented. - `net_traits::CoreResourceMsg::Fetch` message takes a `net_traits::request::RequestInit`. However, when the fetch method is called, a `RequestBinding::RequestInit` is given. The fetch method constructs a `dom::request::Request` with the given `RequestInit`, then creates `net_traits::request::RequestInit` from the dom Request object for the fetch message. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11898 (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/13323)

bors-servo · 2016-09-29T19:28:55Z

⌛ Testing commit 3216009 with merge 5996e00...

bors-servo · 2016-09-29T20:29:46Z

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

highfive assigned Ms2ger Sep 19, 2016

highfive added the S-awaiting-review label Sep 19, 2016

highfive assigned jdm and unassigned Ms2ger Sep 19, 2016

jeenalee force-pushed the jeenalee:fetch branch 4 times, most recently from c291b8e to c135549 Sep 19, 2016

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

jdm added the S-blocked-on-external label Sep 21, 2016

jdm requested changes Sep 21, 2016

View changes

jdm added S-needs-code-changes S-awaiting-review and removed S-awaiting-review labels Sep 21, 2016

highfive removed S-tests-failed S-needs-code-changes labels Sep 21, 2016

jdm removed the S-awaiting-review label Sep 21, 2016

jdm approved these changes Sep 21, 2016

View changes

jdm added the S-needs-squash label Sep 21, 2016

jeenalee force-pushed the jeenalee:fetch branch from d538c95 to ff043f0 Sep 21, 2016

highfive added the S-awaiting-review label Sep 21, 2016

highfive added the S-needs-rebase label Sep 29, 2016

Implement the Fetch method

Loading status checks…

3216009

jeenalee force-pushed the jeenalee:fetch branch from 0c25840 to 3216009 Sep 29, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Sep 29, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Sep 29, 2016

bors-servo mentioned this pull request Sep 29, 2016

Implement `word-break: keep-all` (#9673) #13414

Merged

3 of 5 tasks complete

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

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

bors-servo merged commit 3216009 into servo:master Sep 29, 2016
1 of 3 checks passed

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

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

bors-servo mentioned this pull request Sep 29, 2016

Implement matchMedia and MediaQueryList #13453

Merged

3 of 5 tasks complete

jdm mentioned this pull request Sep 30, 2016

Remove same-origin-data-url flag from fetch implementation #13467

Merged

4 of 5 tasks complete

servo / servo

Implement the Fetch method #13323

Implement the Fetch method #13323

jeenalee commented Sep 19, 2016 •

edited by larsbergstrom

highfive commented Sep 19, 2016

highfive commented Sep 19, 2016

jeenalee commented Sep 19, 2016

emilio commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jeenalee commented Sep 21, 2016

jdm commented Sep 21, 2016

bors-servo commented Sep 29, 2016

jeenalee commented Sep 29, 2016

jdm commented Sep 29, 2016

jeenalee commented Sep 29, 2016

jdm commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

jdm commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

servo / servo

Join GitHub today

Implement the Fetch method #13323

Implement the Fetch method #13323

Conversation

jeenalee commented Sep 19, 2016 • edited by larsbergstrom

highfive commented Sep 19, 2016

highfive commented Sep 19, 2016

jeenalee commented Sep 19, 2016

emilio commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jeenalee Sep 21, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jeenalee commented Sep 21, 2016

jdm commented Sep 21, 2016

bors-servo commented Sep 29, 2016

jeenalee commented Sep 29, 2016

jdm commented Sep 29, 2016

jeenalee commented Sep 29, 2016

jdm commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

jdm commented Sep 29, 2016

bors-servo commented Sep 29, 2016

bors-servo commented Sep 29, 2016

jeenalee commented Sep 19, 2016 •

edited by larsbergstrom

jeenalee Sep 21, 2016 •

edited