Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 714813003: Referrer Policy: Add new policies to URLRequest. (Closed)

Created:
5 years ago by Mike West
Modified:
4 years, 12 months ago
CC:
cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, mcasas+watch_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Referrer Policy: Add new policies to URLRequest. This patch introduces two new referrer policy at the network level in order to support stripping detail from the referrer header when a redirect transitions across origins. These policies support both the "reduce referer granularity" command-line flag, as well as the yet-to-be-implemented "OriginWhenCrossOrigin" referrer policy specified at [1]. [1]: http://www.w3.org/TR/referrer-policy/#referrer-policy-state-origin-when-cross-origin BUG=431711 Committed: https://crrev.com/0c5eab87cc0777e33407d2980622fa1cec4e3dfe Cr-Commit-Position: refs/heads/master@{#305211}

Patch Set 1 #

Total comments: 8

Patch Set 2 : Tests. #

Total comments: 6

Patch Set 3 : Feedback. #

Total comments: 1

Patch Set 4 : Helper. #

Total comments: 25

Patch Set 5 : Rework. #

Patch Set 6 : I hate compilers. #

Total comments: 18

Patch Set 7 : Feedback #

Patch Set 8 : Android. #

Patch Set 9 : Tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+385 lines, -157 lines) Patch
M build/android/pylib/gtest/filter/net_unittests_disabled View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -1 line 0 comments Download
M chrome/browser/referrer_policy_browsertest.cc View 1 11 chunks +75 lines, -95 lines 0 comments Download
M chrome/test/data/referrer_policy/referrer-policy-start.html View 1 2 3 4 1 chunk +15 lines, -9 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 2 chunks +11 lines, -3 lines 0 comments Download
M net/url_request/url_request.h View 1 2 3 4 1 chunk +14 lines, -6 lines 0 comments Download
M net/url_request/url_request.cc View 1 2 3 4 5 6 1 chunk +2 lines, -6 lines 0 comments Download
M net/url_request/url_request_job.h View 1 2 3 4 5 6 3 chunks +7 lines, -1 line 0 comments Download
M net/url_request/url_request_job.cc View 1 2 3 4 5 6 3 chunks +41 lines, -10 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 1 2 3 4 5 6 2 chunks +213 lines, -26 lines 0 comments Download

Messages

Total messages: 38 (7 generated)
Mike West
Does this approach make sense? It's not clear to me how much the network stack ...
5 years ago (2014-11-11 11:26:51 UTC) #2
Mike West
Adding sleevi@ as a friendly net/ OWNER, as there's no way I'm going to have ...
5 years ago (2014-11-11 14:19:05 UTC) #4
Mike West
Adding the right Sleevi (which is what I assume the "r" in "rsleevi" stands for), ...
5 years ago (2014-11-12 11:25:40 UTC) #6
jochen (gone - plz use gerrit)
plz also update URLRequest::StartJob to verify that the referrer with the given policy matches the ...
5 years ago (2014-11-12 14:54:06 UTC) #7
jochen (gone - plz use gerrit)
and can we have tests plz? c/b/referrer_policy_browsertest.cc
5 years ago (2014-11-12 14:54:36 UTC) #8
mmenke
What are the plans for actually using this? The bug doesn't make it clear. If ...
5 years ago (2014-11-12 15:03:29 UTC) #9
mmenke
+1 to tests. Should have net unit tests for this (I defer to jochen on ...
5 years ago (2014-11-12 15:11:13 UTC) #10
Mike West
On 2014/11/12 15:03:29, mmenke wrote: > What are the plans for actually using this? The ...
5 years ago (2014-11-12 15:17:35 UTC) #11
Mike West
On 2014/11/12 15:11:13, mmenke wrote: > +1 to tests. Should have net unit tests for ...
5 years ago (2014-11-13 18:24:26 UTC) #12
mmenke
On 2014/11/13 18:24:26, Mike West wrote: > On 2014/11/12 15:11:13, mmenke wrote: > > +1 ...
5 years ago (2014-11-13 18:36:24 UTC) #13
jochen (gone - plz use gerrit)
there's URLRequestTest.InvalidReferrerTest and URLRequestTestHTTP.HTTPSToHTTPRedirectNoRefererTest
5 years ago (2014-11-13 18:40:46 UTC) #14
mmenke
Oops, I missed that one! That should be server-redirect, not client-redirect (That, the spelling of ...
5 years ago (2014-11-13 18:46:34 UTC) #15
Ryan Sleevi
Looks like Matt already pointed you to the tests. Just a chime-in on behaviour change. ...
5 years ago (2014-11-13 20:00:33 UTC) #16
mmenke
https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_job.cc File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_job.cc#newcode855 net/url_request/url_request_job.cc:855: break; On 2014/11/13 20:00:33, Ryan Sleevi (OOO until 11-18) ...
5 years ago (2014-11-13 20:06:38 UTC) #17
Mike West
Mind taking another look? Now there are tests! https://codereview.chromium.org/714813003/diff/20001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/714813003/diff/20001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode258 content/browser/loader/resource_dispatcher_host_impl.cc:258: case ...
5 years ago (2014-11-19 09:57:37 UTC) #18
jochen (gone - plz use gerrit)
https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_request.cc File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_request.cc#newcode659 net/url_request/url_request.cc:659: REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN) && you could also check that for the ...
5 years ago (2014-11-19 10:03:48 UTC) #19
Mike West
https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_request.cc File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_request.cc#newcode659 net/url_request/url_request.cc:659: REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN) && On 2014/11/19 10:03:48, jochen (slow) wrote: > ...
5 years ago (2014-11-19 10:23:08 UTC) #20
jochen (gone - plz use gerrit)
https://codereview.chromium.org/714813003/diff/40001/net/url_request/url_request.cc File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/40001/net/url_request/url_request.cc#newcode656 net/url_request/url_request.cc:656: GURL referrer(referrer_); what about moving this to a helper ...
5 years ago (2014-11-19 10:25:13 UTC) #21
mmenke
I haven't reviewed the browser tests (Which seem to be failing on the ng bots) ...
5 years ago (2014-11-19 16:29:04 UTC) #22
Mike West
Would you mind taking another look, Matt? I think this pass is a significant improvement ...
5 years ago (2014-11-20 10:45:30 UTC) #23
jochen (gone - plz use gerrit)
chrome/ and content/ lgtm
5 years ago (2014-11-20 15:28:06 UTC) #24
mmenke
This looks really good, just some minor suggestions. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_request_unittest.cc File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_request_unittest.cc#newcode2840 net/url_request/url_request_unittest.cc:2840: ASSERT_TRUE(test_server_.Start()); ...
5 years ago (2014-11-20 16:23:46 UTC) #25
Mike West
Thanks for the feedback. I'll clean these up in the morning. Given the timezone difference, ...
5 years ago (2014-11-20 16:32:55 UTC) #26
mmenke
On 2014/11/20 16:32:55, Mike West wrote: > Thanks for the feedback. I'll clean these up ...
5 years ago (2014-11-20 16:36:04 UTC) #27
Mike West
Thanks Matt! -mike https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_request.cc File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_request.cc#newcode654 net/url_request/url_request.cc:654: URLRequestJob::ComputeReferrerForRedirect(*this, url())) { On 2014/11/20 16:23:45, ...
5 years ago (2014-11-21 09:29:29 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/714813003/120001
5 years ago (2014-11-21 09:30:33 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tests_recipe/builds/30674)
4 years, 12 months ago (2014-11-21 11:30:00 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/714813003/120001
4 years, 12 months ago (2014-11-21 11:41:30 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/714813003/160001
4 years, 12 months ago (2014-11-21 13:18:59 UTC) #36
commit-bot: I haz the power
Committed patchset #9 (id:160001)
4 years, 12 months ago (2014-11-21 14:19:06 UTC) #37
commit-bot: I haz the power
4 years, 12 months ago (2014-11-21 14:19:56 UTC) #38
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/0c5eab87cc0777e33407d2980622fa1cec4e3dfe
Cr-Commit-Position: refs/heads/master@{#305211}

Powered by Google App Engine
This is Rietveld 408576698