Updating http_network_or_cache_fetch to better match the fetch API spec #14784

mattnenterprise · 2016-12-30T01:41:28Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14742

There are tests for these changes

This change is

highfive · 2016-12-30T01:41:31Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/http_loader.rs

highfive · 2016-12-30T01:41:32Z

Warning

These commits modify net code, but no tests are modified. Please consider adding a test!

KiChjang · 2016-12-30T01:58:04Z

@bors-servo try

bors-servo · 2016-12-30T01:58:08Z

⌛ Trying commit 563c9eb with merge 5944e85...


        Auto merge of #14784 - mattnenterprise:fetch-api-updates, r=<try>

Updating http_network_or_cache_fetch to better match the fetch API spec  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #14742  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14784)

bors-servo · 2016-12-30T02:35:58Z

💔 Test failed - linux-rel-wpt

KiChjang · 2016-12-30T02:42:09Z


  ▶ Unexpected subtest result in /XMLHttpRequest/send-entity-body-empty.htm:
  └ PASS [expected FAIL] XMLHttpRequest: send("") - empty entity body (HEAD)

  ▶ Unexpected subtest result in /XMLHttpRequest/send-entity-body-get-head.htm:
  └ PASS [expected FAIL] XMLHttpRequest: send() - non-empty data argument and GET/HEAD (HEAD)

  ▶ Unexpected subtest result in /XMLHttpRequest/send-entity-body-none.htm:
  └ PASS [expected FAIL] No content type (HEAD)

  ▶ Unexpected subtest result in /XMLHttpRequest/send-entity-body-none.htm:
  └ PASS [expected FAIL] Explicit content type (HEAD)

  ▶ Unexpected subtest result in /eventsource/request-cache-control.htm:
  └ PASS [expected TIMEOUT] EventSource: Cache-Control

  ▶ Unexpected subtest result in /html/webappapis/scripting/event-loops/microtask_after_script.html:
  └ PASS [expected FAIL] Microtask immediately after script

Looking good! You'll need to update the WPT test expectations.

jdm · 2016-12-30T04:35:09Z

tests/wpt/metadata/html/webappapis/scripting/event-loops/microtask_after_script.html.ini

-  type: testharness
-  [Microtask immediately after script]
-    expected: FAIL
-


Unfortunately this isn't actually a result caused by these changes (see #14221), so we should revert this.

mattnenterprise · 2016-12-30T17:01:01Z

I have made the updates.

jdm

This looks fine with some indentation adjustments.

jdm · 2016-12-30T18:46:46Z

components/net/http_loader.rs

    if response.is_none() {
+        if http_request.cache_mode.get() == CacheMode::OnlyIfCached {
+          return Response::network_error(NetworkError::Internal("Couldn't find response in cache".into()))


nit: four space indents, please.

jdm · 2016-12-30T18:46:46Z

components/net/http_loader.rs

-            http_request.cache_mode.get() == CacheMode::NoCache) {
+      match status {
+        StatusCode::NotModified => {
+          // Step 21


nit: four space indents on this block and the previous line, please.

jdm · 2016-12-30T18:46:46Z

components/net/http_loader.rs

+        StatusCode::NotModified => {
+          // Step 21
+          if http_request.cache_mode.get() == CacheMode::Default ||
+          http_request.cache_mode.get() == CacheMode::NoCache {
            // Substep 1


nit: this block should be indented four spaces past the previous line.

jdm · 2016-12-30T18:46:46Z

components/net/http_loader.rs

+          }
+        },
+        StatusCode::Unauthorized => {
+          // Step 22


nit: indent this block by another two spaces.

jdm · 2016-12-30T18:46:46Z

components/net/http_loader.rs

+          return http_network_or_cache_fetch(http_request, true, cors_flag, done_chan, context);
+        },
+        StatusCode::ProxyAuthenticationRequired => {
+          // Step 23


nit: indentation.

jdm · 2016-12-30T18:46:46Z

components/net/http_loader.rs

-    // Step 20
+    // Step 24
+    if authentication_fetch_flag {
+      // TODO Create the authentication entry for request and the given realm


nit: indentation.

mattnenterprise · 2016-12-31T17:43:26Z

I have made the updates.

KiChjang

Almost there! Still needs some indentation changes, and after that it can be r=me.

KiChjang · 2017-01-01T06:03:39Z

components/net/http_loader.rs

-        let fetch_result = http_network_or_cache_fetch(request.clone(), credentials, authentication_fetch_flag,
-                                                       done_chan, context);
+        let fetch_result = http_network_or_cache_fetch(request.clone(), authentication_fetch_flag,
+          cors_flag, done_chan, context);


nit: Indentation.

KiChjang · 2017-01-01T06:03:39Z

components/net/http_loader.rs

    // TODO have a HTTP cache to check for a completed response
    let complete_http_response_from_cache: Option<Response> = None;
    if http_request.cache_mode.get() != CacheMode::NoStore &&
        http_request.cache_mode.get() != CacheMode::Reload &&
        complete_http_response_from_cache.is_some() {
        // Substep 1
-        if http_request.cache_mode.get() == CacheMode::ForceCache {
+        if http_request.cache_mode.get() == CacheMode::ForceCache ||
+          http_request.cache_mode.get() == CacheMode::OnlyIfCached {


nit: Indentation, this should match the previous line's http_request.

mattnenterprise · 2017-01-01T18:28:00Z

I have made the updates.

jdm · 2017-01-01T23:33:19Z

@bors-servo r=KiChjang,jdm
Thanks!

bors-servo · 2017-01-01T23:33:20Z

📌 Commit 6caa5a2 has been approved by KiChjang,jdm

bors-servo · 2017-01-01T23:33:23Z

⌛ Testing commit 6caa5a2 with merge b337e92...


        Auto merge of #14784 - mattnenterprise:fetch-api-updates, r=KiChjang,jdm

Updating http_network_or_cache_fetch to better match the fetch API spec  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14742  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14784)

bors-servo · 2017-01-01T23:52:23Z

💔 Test failed - linux-dev

jdm · 2017-01-02T16:09:37Z

Checking files for tidiness...
./components/net/http_loader.rs:1058: Line is longer than 120 characters


        Updating http_network_or_cache_fetch to better match the fetch API spec

mattnenterprise · 2017-01-02T17:17:51Z

I have fixed that line.

jdm · 2017-01-02T18:35:59Z

@bors-servo: r=jdm,KiChjang

bors-servo · 2017-01-02T18:35:59Z

📌 Commit 09194a1 has been approved by jdm,KiChjang

bors-servo · 2017-01-02T19:30:53Z

⌛ Testing commit 09194a1 with merge 987b640...


        Auto merge of #14784 - mattnenterprise:fetch-api-updates, r=jdm,KiChjang

Updating http_network_or_cache_fetch to better match the fetch API spec  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14742  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14784)

bors-servo · 2017-01-02T20:06:11Z

💔 Test failed - mac-rel-wpt1

jdm · 2017-01-02T21:36:22Z

@bors-servo: retry

#13400

bors-servo · 2017-01-02T21:36:25Z

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

bors-servo · 2017-01-02T22:17:09Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned asajeffrey Dec 30, 2016

highfive added the S-awaiting-review label Dec 30, 2016

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

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

jdm reviewed Dec 30, 2016

View changes

jdm requested changes Dec 30, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 30, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 30, 2016

KiChjang requested changes Jan 1, 2017

View changes

highfive assigned KiChjang and unassigned asajeffrey Jan 1, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 1, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 1, 2017

jdm added S-fails-tidy and removed S-tests-failed labels Jan 2, 2017

Updating http_network_or_cache_fetch to better match the fetch API spec

Loading status checks…

09194a1

highfive added the S-awaiting-review label Jan 2, 2017

highfive assigned jdm and unassigned KiChjang Jan 2, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 2, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 2, 2017

bors-servo merged commit 09194a1 into servo:master Jan 2, 2017
2 of 3 checks passed

2 of 3 checks passed

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

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

bors-servo mentioned this pull request Jan 2, 2017

Fixes #14787: Set Origin header in http_network_or_cache_fetch #14811

Closed

4 of 4 tasks complete

jonathanmcelroy mentioned this pull request Jan 6, 2017

Broken link servo/blog.servo.org#130

Closed

servo / servo

Updating http_network_or_cache_fetch to better match the fetch API spec #14784

Updating http_network_or_cache_fetch to better match the fetch API spec #14784

mattnenterprise commented Dec 30, 2016 •

edited

highfive commented Dec 30, 2016

highfive commented Dec 30, 2016

KiChjang commented Dec 30, 2016

bors-servo commented Dec 30, 2016

bors-servo commented Dec 30, 2016

KiChjang commented Dec 30, 2016

This comment has been minimized.

mattnenterprise commented Dec 30, 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.

mattnenterprise commented Dec 31, 2016

KiChjang left a comment

This comment has been minimized.

This comment has been minimized.

mattnenterprise commented Jan 1, 2017

jdm commented Jan 1, 2017

bors-servo commented Jan 1, 2017

bors-servo commented Jan 1, 2017

bors-servo commented Jan 1, 2017

jdm commented Jan 2, 2017

mattnenterprise commented Jan 2, 2017

jdm commented Jan 2, 2017

bors-servo commented Jan 2, 2017

bors-servo commented Jan 2, 2017

bors-servo commented Jan 2, 2017

jdm commented Jan 2, 2017

bors-servo commented Jan 2, 2017

bors-servo commented Jan 2, 2017

servo / servo

Join GitHub today

Updating http_network_or_cache_fetch to better match the fetch API spec #14784

Updating http_network_or_cache_fetch to better match the fetch API spec #14784

Conversation

mattnenterprise commented Dec 30, 2016 • edited

highfive commented Dec 30, 2016

highfive commented Dec 30, 2016

KiChjang commented Dec 30, 2016

bors-servo commented Dec 30, 2016

bors-servo commented Dec 30, 2016

KiChjang commented Dec 30, 2016

This comment has been minimized.

mattnenterprise commented Dec 30, 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.

mattnenterprise commented Dec 31, 2016

KiChjang left a comment

This comment has been minimized.

This comment has been minimized.

mattnenterprise commented Jan 1, 2017

jdm commented Jan 1, 2017

bors-servo commented Jan 1, 2017

bors-servo commented Jan 1, 2017

bors-servo commented Jan 1, 2017

jdm commented Jan 2, 2017

mattnenterprise commented Jan 2, 2017

jdm commented Jan 2, 2017

bors-servo commented Jan 2, 2017

bors-servo commented Jan 2, 2017

bors-servo commented Jan 2, 2017

jdm commented Jan 2, 2017

bors-servo commented Jan 2, 2017

bors-servo commented Jan 2, 2017

mattnenterprise commented Dec 30, 2016 •

edited