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

JanZerebecki · 2016-09-27T20:56:44Z

The spec removed it. Check the scheme instead, data is always same origin now,
except for workers.
Closes #13362

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

There are tests for these changes OR
These changes do not require tests because they only remove code.

This change is

highfive · 2016-09-27T20:56:49Z

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

highfive · 2016-09-27T20:56:50Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/request.rs, components/net/fetch/methods.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/htmlscriptelement.rs, components/net_traits/request.rs, components/net_traits/request.rs

KiChjang · 2016-09-27T21:19:27Z

components/net/fetch/methods.rs

@@ -673,8 +673,6 @@ fn http_redirect_fetch(request: Rc<Request>,
    request.redirect_count.set(request.redirect_count.get() + 1);

    // Step 9


Step annotations for this fetch function needs to be updated to match the spec, I'm afraid.

KiChjang

Almost done! If you can squash your commits after stylistic nits, I can land this :).

KiChjang · 2016-09-27T22:12:21Z

components/net/fetch/methods.rs

    let same_origin = if let Origin::Origin(ref origin) = *request.origin.borrow() {
        *origin == request.current_url().origin()
    } else {
        false
    };
    let has_credentials = has_credentials(&location_url);
-
-    // Step 10
+    // Step 7


nit: add a newline before step 7.

The newline is missing intentionally because the previous lines are preparation for Step 7 and not used in Step 6.

Then the step annotation should be above the previous line.

KiChjang · 2016-09-27T22:12:21Z

components/net/fetch/methods.rs

    let location_url = match location_url {
        Ok(url) => url,
        _ => return Response::network_error()
    };

-    // Step 7
+    // Step 4
+    //TODO implement return network_error if not HTTP(S)


nit: space after //.

KiChjang · 2016-09-27T22:12:21Z

components/net/fetch/methods.rs

    request.url_list.borrow_mut().push(location_url);

-    // Step 15
+    // Step 12
+    //TODO implement referrer policy


nit: space after //.

KiChjang · 2016-09-28T23:03:08Z

@bors-servo r+

Thanks!

bors-servo · 2016-09-28T23:03:09Z

📌 Commit beaf91d has been approved by KiChjang

bors-servo · 2016-09-30T11:18:30Z

⌛ Testing commit beaf91d with merge a014e4b...


        Auto merge of #13467 - JanZerebecki:rm-same-origin-data-url, r=KiChjang

Remove same-origin-data-url flag from fetch implementation  The spec removed it. Check the scheme instead, data is always same origin now, except for workers. Closes #13362 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13362 .  - [ ] There are tests for these changes OR - [x] These changes do not require tests because they only remove code.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13467)

bors-servo · 2016-09-30T11:22:26Z

💔 Test failed - linux-dev

jdm · 2016-09-30T14:41:16Z

error[E0560]: struct `net_traits::request::RequestInit` has no field named `same_origin_data`
  --> /home/servo/buildbot/slave/linux-dev/build/components/script/fetch.rs:47:9
   |
47 |         same_origin_data: request.same_origin_data.get(),
   |         ^^^^^^^^^^^^^^^^

error: attempted access of field `same_origin_data` on type `net_traits::request::Request`, but no field with that name was found
  --> /home/servo/buildbot/slave/linux-dev/build/components/script/fetch.rs:47:27
   |
47 |         same_origin_data: request.same_origin_data.get(),
   |                           ^^^^^^^^^^^^^^^^^^^^^^^^
   |
help: did you mean `omit_origin_header`?
  --> /home/servo/buildbot/slave/linux-dev/build/components/script/fetch.rs:47:35
   |
47 |         same_origin_data: request.same_origin_data.get(),
   |                                   ^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

jdm · 2016-09-30T14:41:53Z

That code was added in #13323 which only merged yesterday.


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

The spec removed it. Check the scheme instead, data is always same origin now, except for workers. This also updates the comments to make step numbers match the spec. Closes #13362

KiChjang · 2016-10-01T07:27:56Z

@bors-servo r+

bors-servo · 2016-10-01T07:27:57Z

📌 Commit 95a7482 has been approved by KiChjang

bors-servo · 2016-10-01T07:52:29Z

⌛ Testing commit 95a7482 with merge 389a798...


        Auto merge of #13467 - JanZerebecki:rm-same-origin-data-url, r=KiChjang

Remove same-origin-data-url flag from fetch implementation  The spec removed it. Check the scheme instead, data is always same origin now, except for workers. Closes #13362 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13362 .  - [ ] There are tests for these changes OR - [x] These changes do not require tests because they only remove code.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13467)

bors-servo · 2016-10-01T08:35:44Z

💔 Test failed - mac-rel-wpt2

KiChjang · 2016-10-01T10:16:13Z

@bors-servo retry

#13133

bors-servo · 2016-10-01T10:19:06Z

⌛ Testing commit 95a7482 with merge e494ded...


        Auto merge of #13467 - JanZerebecki:rm-same-origin-data-url, r=KiChjang

Remove same-origin-data-url flag from fetch implementation  The spec removed it. Check the scheme instead, data is always same origin now, except for workers. Closes #13362 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13362 .  - [ ] There are tests for these changes OR - [x] These changes do not require tests because they only remove code.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13467)

bors-servo · 2016-10-01T11:18:57Z

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

highfive assigned pcwalton Sep 27, 2016

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

KiChjang reviewed Sep 27, 2016

View changes

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Sep 27, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 27, 2016

KiChjang requested changes Sep 27, 2016

View changes

KiChjang assigned KiChjang and unassigned pcwalton Sep 27, 2016

JanZerebecki force-pushed the JanZerebecki:rm-same-origin-data-url branch from 5fc9a72 to 44517e0 Sep 27, 2016

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Sep 27, 2016

JanZerebecki force-pushed the JanZerebecki:rm-same-origin-data-url branch from 44517e0 to beaf91d Sep 28, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 28, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 28, 2016

KiChjang approved these changes Sep 28, 2016

View changes

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

KiChjang added S-needs-code-changes S-needs-rebase and removed S-tests-failed labels Sep 30, 2016

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

Loading status checks…

95a7482

The spec removed it. Check the scheme instead, data is always same origin now, except for workers. This also updates the comments to make step numbers match the spec. Closes #13362

KiChjang force-pushed the JanZerebecki:rm-same-origin-data-url branch from beaf91d to 95a7482 Oct 1, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 1, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Oct 1, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 1, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 1, 2016

bors-servo merged commit 95a7482 into servo:master Oct 1, 2016
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

highfive removed the S-awaiting-merge label Oct 1, 2016

JanZerebecki deleted the JanZerebecki:rm-same-origin-data-url branch Oct 1, 2016

servo / servo

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

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

JanZerebecki commented Sep 27, 2016 •

edited by larsbergstrom

highfive commented Sep 27, 2016

highfive commented Sep 27, 2016

This comment has been minimized.

This comment has been minimized.

KiChjang 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.

KiChjang commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 30, 2016

bors-servo commented Sep 30, 2016

jdm commented Sep 30, 2016

jdm commented Sep 30, 2016

KiChjang commented Oct 1, 2016

bors-servo commented Oct 1, 2016

bors-servo commented Oct 1, 2016

bors-servo commented Oct 1, 2016

KiChjang commented Oct 1, 2016

bors-servo commented Oct 1, 2016

bors-servo commented Oct 1, 2016

		@@ -673,8 +673,6 @@ fn http_redirect_fetch(request: Rc<Request>,
		request.redirect_count.set(request.redirect_count.get() + 1);

		// Step 9

servo / servo

Join GitHub today

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

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

Conversation

JanZerebecki commented Sep 27, 2016 • edited by larsbergstrom

highfive commented Sep 27, 2016

highfive commented Sep 27, 2016

This comment has been minimized.

This comment has been minimized.

KiChjang 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.

KiChjang commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 30, 2016

bors-servo commented Sep 30, 2016

jdm commented Sep 30, 2016

jdm commented Sep 30, 2016

KiChjang commented Oct 1, 2016

bors-servo commented Oct 1, 2016

bors-servo commented Oct 1, 2016

bors-servo commented Oct 1, 2016

KiChjang commented Oct 1, 2016

bors-servo commented Oct 1, 2016

bors-servo commented Oct 1, 2016

JanZerebecki commented Sep 27, 2016 •

edited by larsbergstrom