Apply Referrer-Policy header when following redirects

Apply Referrer-Policy header when following redirects

When a Referrer-Policy header is received during a redirect,
URLRequestJob parses it and updates the referrer and referrer policy on
the request, if necessary.

The Referrer-Policy header is being implemented as an experimental web
platform feature. The experimental web platform feature flag is plumbed
to URLRequestJob via a boolean on URLRequestContext. This flag should be
temporary and only live until the Referrer-Policy feature ships.

Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Umj9iVRJM70
Implementation plan: https://docs.google.com/document/d/1SyQhP6Y7BHIQXWL8S1saWqMuar4hoWLGigQuHmizg3g/edit

BUG=619228
Committed: https://crrev.com/550ba7f9533922cfeac9709d99815cec9b2ad52a
Cr-Commit-Position: refs/heads/master@{#403017}

[estark, 2016-06-25 00:59:33]

estark@chromium.org changed reviewers:
+ jochen@chromium.org, mmenke@chromium.org

[estark, 2016-06-25 00:59:33]

mmenke, jochen, PTAL?

(mkwst optional but welcome, as usual)

Thanks!

[estark, 2016-06-25 14:42:15]

https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
net/url_request/url_request_job.cc:91:
request->set_referrer_policy(URLRequest::NEVER_CLEAR_REFERRER);
Hrm. These set_referrer_policy() calls run afoul of set_referrer_policy()'s
DCHECK that enforces that it is only called before Start().

To my reading, it doesn't look like anything will explode by removing that
DCHECK and making it allowed to call set_referrer_policy() after Start(). After
all, changing the referrer policy during the request is exactly what the
Referrer-Policy header is designed to support, so it seems like calling
set_referrer_policy() after Start() ought to be allowed now.

jochen or mmenke, let me know if you have opinions.

[mmenke, 2016-06-25 14:52:09]

On 2016/06/25 14:42:15, estark wrote:
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> File net/url_request/url_request_job.cc (right):
> 
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> net/url_request/url_request_job.cc:91:
> request->set_referrer_policy(URLRequest::NEVER_CLEAR_REFERRER);
> Hrm. These set_referrer_policy() calls run afoul of set_referrer_policy()'s
> DCHECK that enforces that it is only called before Start().
> 
> To my reading, it doesn't look like anything will explode by removing that
> DCHECK and making it allowed to call set_referrer_policy() after Start().
After
> all, changing the referrer policy during the request is exactly what the
> Referrer-Policy header is designed to support, so it seems like calling
> set_referrer_policy() after Start() ought to be allowed now.
> 
> jochen or mmenke, let me know if you have opinions.

That DCHECK is for external callers - if they call it other than during start,
we can make no guarantees of what referrer we actually send, so I'd rather try
and figure out a way to keep it.

[estark, 2016-06-25 15:11:12]

On 2016/06/25 14:52:09, mmenke wrote:
> On 2016/06/25 14:42:15, estark wrote:
> >
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> > File net/url_request/url_request_job.cc (right):
> > 
> >
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> > net/url_request/url_request_job.cc:91:
> > request->set_referrer_policy(URLRequest::NEVER_CLEAR_REFERRER);
> > Hrm. These set_referrer_policy() calls run afoul of set_referrer_policy()'s
> > DCHECK that enforces that it is only called before Start().
> > 
> > To my reading, it doesn't look like anything will explode by removing that
> > DCHECK and making it allowed to call set_referrer_policy() after Start().
> After
> > all, changing the referrer policy during the request is exactly what the
> > Referrer-Policy header is designed to support, so it seems like calling
> > set_referrer_policy() after Start() ought to be allowed now.
> > 
> > jochen or mmenke, let me know if you have opinions.
> 
> That DCHECK is for external callers - if they call it other than during start,
> we can make no guarantees of what referrer we actually send, so I'd rather try
> and figure out a way to keep it.

Ah, ok, that makes sense. One option might be adding a |new_referrer_policy|
field to RedirectInfo, and then URLRequest can set its own referrer_policy_ from
the RedirectInfo in URLRequest::Redirect(), similar to how it sets |referrer_|
from redirect_info.new_referrer.

[mmenke, 2016-06-25 18:23:51]

On 2016/06/25 15:11:12, estark wrote:
> On 2016/06/25 14:52:09, mmenke wrote:
> > On 2016/06/25 14:42:15, estark wrote:
> > >
> >
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> > > File net/url_request/url_request_job.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> > > net/url_request/url_request_job.cc:91:
> > > request->set_referrer_policy(URLRequest::NEVER_CLEAR_REFERRER);
> > > Hrm. These set_referrer_policy() calls run afoul of
set_referrer_policy()'s
> > > DCHECK that enforces that it is only called before Start().
> > > 
> > > To my reading, it doesn't look like anything will explode by removing that
> > > DCHECK and making it allowed to call set_referrer_policy() after Start().
> > After
> > > all, changing the referrer policy during the request is exactly what the
> > > Referrer-Policy header is designed to support, so it seems like calling
> > > set_referrer_policy() after Start() ought to be allowed now.
> > > 
> > > jochen or mmenke, let me know if you have opinions.
> > 
> > That DCHECK is for external callers - if they call it other than during
start,
> > we can make no guarantees of what referrer we actually send, so I'd rather
try
> > and figure out a way to keep it.
> 
> Ah, ok, that makes sense. One option might be adding a |new_referrer_policy|
> field to RedirectInfo, and then URLRequest can set its own referrer_policy_
from
> the RedirectInfo in URLRequest::Redirect(), similar to how it sets |referrer_|
> from redirect_info.new_referrer.

Maybe move the new referrer policy into RedirectInfo, and calculate (Or make the
call to calculate it) it in URLRequestJob::GetRedirectInfo?  Update the policy
in URLRequest::Redirect.

That also makes sense because this is a property of the redirect (And is
meaningless for non-redirected requests).

[estark, 2016-06-26 16:07:38]

On 2016/06/25 18:23:51, mmenke wrote:
> On 2016/06/25 15:11:12, estark wrote:
> > On 2016/06/25 14:52:09, mmenke wrote:
> > > On 2016/06/25 14:42:15, estark wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> > > > File net/url_request/url_request_job.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> > > > net/url_request/url_request_job.cc:91:
> > > > request->set_referrer_policy(URLRequest::NEVER_CLEAR_REFERRER);
> > > > Hrm. These set_referrer_policy() calls run afoul of
> set_referrer_policy()'s
> > > > DCHECK that enforces that it is only called before Start().
> > > > 
> > > > To my reading, it doesn't look like anything will explode by removing
that
> > > > DCHECK and making it allowed to call set_referrer_policy() after
Start().
> > > After
> > > > all, changing the referrer policy during the request is exactly what the
> > > > Referrer-Policy header is designed to support, so it seems like calling
> > > > set_referrer_policy() after Start() ought to be allowed now.
> > > > 
> > > > jochen or mmenke, let me know if you have opinions.
> > > 
> > > That DCHECK is for external callers - if they call it other than during
> start,
> > > we can make no guarantees of what referrer we actually send, so I'd rather
> try
> > > and figure out a way to keep it.
> > 
> > Ah, ok, that makes sense. One option might be adding a |new_referrer_policy|
> > field to RedirectInfo, and then URLRequest can set its own referrer_policy_
> from
> > the RedirectInfo in URLRequest::Redirect(), similar to how it sets
|referrer_|
> > from redirect_info.new_referrer.
> 
> Maybe move the new referrer policy into RedirectInfo, and calculate (Or make
the
> call to calculate it) it in URLRequestJob::GetRedirectInfo?  Update the policy
> in URLRequest::Redirect.
> 
> That also makes sense because this is a property of the redirect (And is
> meaningless for non-redirected requests).

Yeah, I think that's exactly what I was suggesting.

Updated the CL to do that.

[mmenke, 2016-06-26 16:18:51]

On 2016/06/26 16:07:38, estark wrote:
> On 2016/06/25 18:23:51, mmenke wrote:
> > On 2016/06/25 15:11:12, estark wrote:
> > > On 2016/06/25 14:52:09, mmenke wrote:
> > > > On 2016/06/25 14:42:15, estark wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> > > > > File net/url_request/url_request_job.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2100583002/diff/100001/net/url_request/url_re...
> > > > > net/url_request/url_request_job.cc:91:
> > > > > request->set_referrer_policy(URLRequest::NEVER_CLEAR_REFERRER);
> > > > > Hrm. These set_referrer_policy() calls run afoul of
> > set_referrer_policy()'s
> > > > > DCHECK that enforces that it is only called before Start().
> > > > > 
> > > > > To my reading, it doesn't look like anything will explode by removing
> that
> > > > > DCHECK and making it allowed to call set_referrer_policy() after
> Start().
> > > > After
> > > > > all, changing the referrer policy during the request is exactly what
the
> > > > > Referrer-Policy header is designed to support, so it seems like
calling
> > > > > set_referrer_policy() after Start() ought to be allowed now.
> > > > > 
> > > > > jochen or mmenke, let me know if you have opinions.
> > > > 
> > > > That DCHECK is for external callers - if they call it other than during
> > start,
> > > > we can make no guarantees of what referrer we actually send, so I'd
rather
> > try
> > > > and figure out a way to keep it.
> > > 
> > > Ah, ok, that makes sense. One option might be adding a
|new_referrer_policy|
> > > field to RedirectInfo, and then URLRequest can set its own
referrer_policy_
> > from
> > > the RedirectInfo in URLRequest::Redirect(), similar to how it sets
> |referrer_|
> > > from redirect_info.new_referrer.
> > 
> > Maybe move the new referrer policy into RedirectInfo, and calculate (Or make
> the
> > call to calculate it) it in URLRequestJob::GetRedirectInfo?  Update the
policy
> > in URLRequest::Redirect.
> > 
> > That also makes sense because this is a property of the redirect (And is
> > meaningless for non-redirected requests).
> 
> Yeah, I think that's exactly what I was suggesting.
> 
> Updated the CL to do that.

Oops - yea, it's exactly what you were suggesting.  Not sure if I missed your
post or what.

[jochen (gone - plz use gerrit), 2016-06-27 08:33:40]

https://codereview.chromium.org/2100583002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1036:
command_line.HasSwitch(switches::kEnableExperimentalWebPlatformFeatures));
how will this work in content shell?

[mmenke, 2016-06-27 17:18:04]

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job.cc:1057: if
(request_->context()->enable_referrer_policy_header()) {
Question...I may have asked this before, but it seems really weird to me...

Suppose we have:

Site A -> Site B1 -> Site B2 -> Site C.

Assume site B1/B2 are secure, and on the same site and site C is not (I don't
care about site A).

Site A sets NEVER_CLEAR_REFERRER on the redirect.  Site B is an old site, and
expects us to clear referrers on secure to insecure, but instead, inherits site
A's referrer policy, so site B assumes B2's URL is private, but since Site A set
NEVER_CLEAR_REFERRER, it inherits Site A's referrer policy.  Is this a problem? 
It seems really weird to me.

Note that this is the case for both subframes and main frames (Which don't even
get CORS protection).  I'm not sure if this is an issue for main frames
currently - I'm not sure how referrer_policy is set by Blink.

[mmenke, 2016-06-27 17:20:04]

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job.cc:1057: if
(request_->context()->enable_referrer_policy_header()) {
On 2016/06/27 17:18:04, mmenke wrote:
> Question...I may have asked this before, but it seems really weird to me...
> 
> Suppose we have:
> 
> Site A -> Site B1 -> Site B2 -> Site C.
> 
> Assume site B1/B2 are secure, and on the same site and site C is not (I don't
> care about site A).

That is, Site C is not secure, and not on the same site as B1/B2.

> 
> Site A sets NEVER_CLEAR_REFERRER on the redirect.  Site B is an old site, and
> expects us to clear referrers on secure to insecure, but instead, inherits
site
> A's referrer policy, so site B assumes B2's URL is private, but since Site A
set
> NEVER_CLEAR_REFERRER, it inherits Site A's referrer policy.  Is this a
problem? 
> It seems really weird to me.
> 
> Note that this is the case for both subframes and main frames (Which don't
even
> get CORS protection).  I'm not sure if this is an issue for main frames
> currently - I'm not sure how referrer_policy is set by Blink.

[estark, 2016-06-27 17:27:52]

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job.cc:1057: if
(request_->context()->enable_referrer_policy_header()) {
On 2016/06/27 17:18:04, mmenke wrote:
> Question...I may have asked this before, but it seems really weird to me...
> 
> Suppose we have:
> 
> Site A -> Site B1 -> Site B2 -> Site C.
> 
> Assume site B1/B2 are secure, and on the same site and site C is not (I don't
> care about site A).
> 
> Site A sets NEVER_CLEAR_REFERRER on the redirect.  Site B is an old site, and
> expects us to clear referrers on secure to insecure, but instead, inherits
site
> A's referrer policy, so site B assumes B2's URL is private, but since Site A
set
> NEVER_CLEAR_REFERRER, it inherits Site A's referrer policy.  Is this a
problem? 
> It seems really weird to me.
> 
> Note that this is the case for both subframes and main frames (Which don't
even
> get CORS protection).  I'm not sure if this is an issue for main frames
> currently - I'm not sure how referrer_policy is set by Blink.

That does seem weird, but I'm pretty sure that's been the case for several
years, for as long as referrer policy has been implemented, right? And that
weirdness isn't really affected by this CL... if anything, what this CL
implements slightly improves the situation. I know your point is that B might be
an old site, but now at least B would have the option to set a Referrer-Policy
header to change the policy back to the default despite A having set unsafe-url.
Before this CL, it doesn't really matter if B is an old site or not, because B
would have no means of changing the referrer policy in the middle of the request
even if it wanted to.

[estark, 2016-06-27 17:29:51]

On 2016/06/27 17:27:52, estark wrote:
>
https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
> File net/url_request/url_request_job.cc (right):
> 
>
https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
> net/url_request/url_request_job.cc:1057: if
> (request_->context()->enable_referrer_policy_header()) {
> On 2016/06/27 17:18:04, mmenke wrote:
> > Question...I may have asked this before, but it seems really weird to me...
> > 
> > Suppose we have:
> > 
> > Site A -> Site B1 -> Site B2 -> Site C.
> > 
> > Assume site B1/B2 are secure, and on the same site and site C is not (I
don't
> > care about site A).
> > 
> > Site A sets NEVER_CLEAR_REFERRER on the redirect.  Site B is an old site,
and
> > expects us to clear referrers on secure to insecure, but instead, inherits
> site
> > A's referrer policy, so site B assumes B2's URL is private, but since Site A
> set
> > NEVER_CLEAR_REFERRER, it inherits Site A's referrer policy.  Is this a
> problem? 
> > It seems really weird to me.
> > 
> > Note that this is the case for both subframes and main frames (Which don't
> even
> > get CORS protection).  I'm not sure if this is an issue for main frames
> > currently - I'm not sure how referrer_policy is set by Blink.
> 
> That does seem weird, but I'm pretty sure that's been the case for several
> years, for as long as referrer policy has been implemented, right? And that
> weirdness isn't really affected by this CL... if anything, what this CL
> implements slightly improves the situation. I know your point is that B might
be
> an old site, but now at least B would have the option to set a Referrer-Policy
> header to change the policy back to the default despite A having set
unsafe-url.
> Before this CL, it doesn't really matter if B is an old site or not, because B
> would have no means of changing the referrer policy in the middle of the
request
> even if it wanted to.

Oh, but, by the way, it's not quite as weird as you made it sound, because the
referrer is Site A across the entire request. So B2's URL doesn't get leaked --
the referrer URL is Site A all the way throughout the request.

[mmenke, 2016-06-27 17:52:26]

On 2016/06/27 17:29:51, estark wrote:
> On 2016/06/27 17:27:52, estark wrote:
> >
>
https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
> > File net/url_request/url_request_job.cc (right):
> > 
> >
>
https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
> > net/url_request/url_request_job.cc:1057: if
> > (request_->context()->enable_referrer_policy_header()) {
> > On 2016/06/27 17:18:04, mmenke wrote:
> > > Question...I may have asked this before, but it seems really weird to
me...
> > > 
> > > Suppose we have:
> > > 
> > > Site A -> Site B1 -> Site B2 -> Site C.
> > > 
> > > Assume site B1/B2 are secure, and on the same site and site C is not (I
> don't
> > > care about site A).
> > > 
> > > Site A sets NEVER_CLEAR_REFERRER on the redirect.  Site B is an old site,
> and
> > > expects us to clear referrers on secure to insecure, but instead, inherits
> > site
> > > A's referrer policy, so site B assumes B2's URL is private, but since Site
A
> > set
> > > NEVER_CLEAR_REFERRER, it inherits Site A's referrer policy.  Is this a
> > problem? 
> > > It seems really weird to me.
> > > 
> > > Note that this is the case for both subframes and main frames (Which don't
> > even
> > > get CORS protection).  I'm not sure if this is an issue for main frames
> > > currently - I'm not sure how referrer_policy is set by Blink.
> > 
> > That does seem weird, but I'm pretty sure that's been the case for several
> > years, for as long as referrer policy has been implemented, right? And that
> > weirdness isn't really affected by this CL... if anything, what this CL
> > implements slightly improves the situation. I know your point is that B
might
> be
> > an old site, but now at least B would have the option to set a
Referrer-Policy
> > header to change the policy back to the default despite A having set
> unsafe-url.
> > Before this CL, it doesn't really matter if B is an old site or not, because
B
> > would have no means of changing the referrer policy in the middle of the
> request
> > even if it wanted to.
> 
> Oh, but, by the way, it's not quite as weird as you made it sound, because the
> referrer is Site A across the entire request. So B2's URL doesn't get leaked
--
> the referrer URL is Site A all the way throughout the request.

Ah, right, good point.  That seems much more sane.

[mmenke, 2016-06-27 18:38:05]

https://codereview.chromium.org/2100583002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1036:
command_line.HasSwitch(switches::kEnableExperimentalWebPlatformFeatures));
On 2016/06/27 08:33:40, jochen wrote:
> how will this work in content shell?

I don't think we normally enable experimental features on content shell, do we? 
When we finally enable it unconditionally, we're remove all the code to
conditionally enable it.  i.e., the bool to enable / disable it will be removed,
so content shell won't need to be updated.

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job.cc:84: std::string* new_referrer_source) {
"source" is a really confusing name.  It's actually the original referrer,
right?

Also, the original referrer is updated based on referrer policy in
ComputeReferrerForRedirect, which is called after this method, so why are we
updating referrer here, too?  Just so we can clear the referrer "no-referer",
since we don't have a URLRequest::NO_REFERRER?  I think if that's the case, it
would be much simpler to add that policy, and respect it in
ComputeReferrerForRedirect.

When there are multiple tokens, repeatedly computing, and then ignoring,
new_referrer_source is also weird.  Only calculating it once we have the policy
at the end also more closely adhere to how the spec is written, though the
result is the same.

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:267: const char* original_referrer;
These two are always the same in this test, so I think we can remove
original_referrer.  Or we should have tests where they're not the same.

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:277: {"http://foo.test/one",
"http://foo.test/one",
A bit of a pain, but I suggest comments labelling these arguments, or at least
original_referrer and expected_final_referrer.  With 4 URLs per test, can be
hard to keep them straight.

[estark, 2016-06-27 22:18:34]

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job.cc:84: std::string* new_referrer_source) {
On 2016/06/27 18:38:05, mmenke wrote:
> "source" is a really confusing name.  It's actually the original referrer,
> right?

Heh, I called it "referrer source" because I found "original referrer" to be a
really confusing name. :) Well, that and because the spec calls it
referrerSource (step 3 of
https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer). I
find "original referrer" confusing because the request's referrer might have
several different values as it follows several redirects, and I was getting
confused as to which value was the "original" one.

I don't feel too strongly though and am happy to change it, just want to resolve
the below points first.

> 
> Also, the original referrer is updated based on referrer policy in
> ComputeReferrerForRedirect, which is called after this method, so why are we
> updating referrer here, too?  Just so we can clear the referrer "no-referer",
> since we don't have a URLRequest::NO_REFERRER?  I think if that's the case, it
> would be much simpler to add that policy, and respect it in
> ComputeReferrerForRedirect.

We'd need to add two policies, I think -- NO_REFERRER and ORIGIN.

Adding those policies feels a little weird to me. URLRequest::ReferrerPolicy is
just about behavior on redirects, and to an external caller, there'd be no
difference between NO_REFERRER and NEVER_CLEAR_REFERRER, or between ORIGIN and
NEVER_CLEAR_REFERRER. In other words we'd be adding values to the public enum
just to facilitate an internal implementation detail of URLRequestJob.

I'll defer to your judgement if you still think that's the right way to do it,
just wanted to point out that IMO it confuses the public API. I can also play
with restructuring the code a bit, maybe there's a way to make it less confusing
without adding new URLRequest::ReferrerPolicy values.


> 
> When there are multiple tokens, repeatedly computing, and then ignoring,
> new_referrer_source is also weird.  Only calculating it once we have the
policy
> at the end also more closely adhere to how the spec is written, though the
> result is the same.

Acknowledged. Will address this once above question is resolved.

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:267: const char* original_referrer;
On 2016/06/27 18:38:05, mmenke wrote:
> These two are always the same in this test, so I think we can remove
> original_referrer.  Or we should have tests where they're not the same.

Done. (Changed a test so they're different.)

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:277: {"http://foo.test/one",
"http://foo.test/one",
On 2016/06/27 18:38:05, mmenke wrote:
> A bit of a pain, but I suggest comments labelling these arguments, or at least
> original_referrer and expected_final_referrer.  With 4 URLs per test, can be
> hard to keep them straight.

Done. Though the formatting's a little weird :(

[jochen (gone - plz use gerrit), 2016-06-28 13:03:46]

https://codereview.chromium.org/2100583002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1036:
command_line.HasSwitch(switches::kEnableExperimentalWebPlatformFeatures));
On 2016/06/27 at 18:38:05, mmenke wrote:
> On 2016/06/27 08:33:40, jochen wrote:
> > how will this work in content shell?
> 
> I don't think we normally enable experimental features on content shell, do
we?  When we finally enable it unconditionally, we're remove all the code to
conditionally enable it.  i.e., the bool to enable / disable it will be removed,
so content shell won't need to be updated.

content_shell is run with both test-only and experimental features enabled for
layout tests

[mmenke, 2016-06-28 14:29:23]

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
net/url_request/url_request_job.cc:84: std::string* new_referrer_source) {
On 2016/06/27 22:18:34, estark wrote:
> On 2016/06/27 18:38:05, mmenke wrote:
> > "source" is a really confusing name.  It's actually the original referrer,
> > right?
> 
> Heh, I called it "referrer source" because I found "original referrer" to be a
> really confusing name. :) Well, that and because the spec calls it
> referrerSource (step 3 of
> https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer).
I
> find "original referrer" confusing because the request's referrer might have
> several different values as it follows several redirects, and I was getting
> confused as to which value was the "original" one.
> 
> I don't feel too strongly though and am happy to change it, just want to
resolve
> the below points first.

"referrer_source" sounds like the source of the referrer to me.  I'm fine with
anything clearly indicating it is the old referrer (old_referrer? 
previous_referrer?)

> > Also, the original referrer is updated based on referrer policy in
> > ComputeReferrerForRedirect, which is called after this method, so why are we
> > updating referrer here, too?  Just so we can clear the referrer
"no-referer",
> > since we don't have a URLRequest::NO_REFERRER?  I think if that's the case,
it
> > would be much simpler to add that policy, and respect it in
> > ComputeReferrerForRedirect.
> 
> We'd need to add two policies, I think -- NO_REFERRER and ORIGIN.
> 
> Adding those policies feels a little weird to me. URLRequest::ReferrerPolicy
is
> just about behavior on redirects, and to an external caller, there'd be no
> difference between NO_REFERRER and NEVER_CLEAR_REFERRER, or between ORIGIN and
> NEVER_CLEAR_REFERRER. In other words we'd be adding values to the public enum
> just to facilitate an internal implementation detail of URLRequestJob.
> 
> I'll defer to your judgement if you still think that's the right way to do it,
> just wanted to point out that IMO it confuses the public API. I can also play
> with restructuring the code a bit, maybe there's a way to make it less
confusing
> without adding new URLRequest::ReferrerPolicy values.

I think having a 1-to-1 mapping between URLRequest::ReferrerPolicy and the
specced out "referrer policies" in that doc makes sense, though that's not
really my main concern here.  What bugs me about this is that
"ComputeReferrerForRedirect" no longer computes the referrer for the redirect. 
It's a combination of ComputeReferrerForRedirect and the referrer changes made
here.  And repeatedly updating new_referrer_source when there are multiple
headers also seems really weird.

Also note that if we switch to having ComputeReferrerForRedirect actually
compute the referrer, there will be a difference.  It would clear the referrer
on NO_REFERRER, and do nothing on NEVER_CLEAR_REFERRER.  And on ORIGIN, it would
make the referrer the origin.  Since it would do this on every redirect, there
would be a bit of wasted work, with multiple redirect, of course.

One ugliness with that approach is that we wouldn't adhere to those policies, if
set, for the initial request, and blink wouldn't be setting those policies. 
(Well beyond the scope of this CL, but if we switched to having the renderer
just ass along teh policy and the original referrer, and net actually enforce
those policies, that might reduce the amount of code to manage referrers. 
Haven't thought about the practicality of that, not familiar with all the stuff,
renderer side, to deal with it)

Another option I could live with would be to make one method compute both the
policy and the referrer, and to stick with the reduced set of policies.  I
suspect that would result in much harder to read code, but if we can get
something reasonable with that approach...

> > When there are multiple tokens, repeatedly computing, and then ignoring,
> > new_referrer_source is also weird.  Only calculating it once we have the
> policy
> > at the end also more closely adhere to how the spec is written, though the
> > result is the same.
> 
> Acknowledged. Will address this once above question is resolved.

[estark, 2016-06-28 20:10:31]

On 2016/06/28 14:29:23, mmenke wrote:
>
https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
> File net/url_request/url_request_job.cc (right):
> 
>
https://codereview.chromium.org/2100583002/diff/140001/net/url_request/url_re...
> net/url_request/url_request_job.cc:84: std::string* new_referrer_source) {
> On 2016/06/27 22:18:34, estark wrote:
> > On 2016/06/27 18:38:05, mmenke wrote:
> > > "source" is a really confusing name.  It's actually the original referrer,
> > > right?
> > 
> > Heh, I called it "referrer source" because I found "original referrer" to be
a
> > really confusing name. :) Well, that and because the spec calls it
> > referrerSource (step 3 of
> >
https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer).
> I
> > find "original referrer" confusing because the request's referrer might have
> > several different values as it follows several redirects, and I was getting
> > confused as to which value was the "original" one.
> > 
> > I don't feel too strongly though and am happy to change it, just want to
> resolve
> > the below points first.
> 
> "referrer_source" sounds like the source of the referrer to me.  I'm fine with
> anything clearly indicating it is the old referrer (old_referrer? 
> previous_referrer?)
> 
> > > Also, the original referrer is updated based on referrer policy in
> > > ComputeReferrerForRedirect, which is called after this method, so why are
we
> > > updating referrer here, too?  Just so we can clear the referrer
> "no-referer",
> > > since we don't have a URLRequest::NO_REFERRER?  I think if that's the
case,
> it
> > > would be much simpler to add that policy, and respect it in
> > > ComputeReferrerForRedirect.
> > 
> > We'd need to add two policies, I think -- NO_REFERRER and ORIGIN.
> > 
> > Adding those policies feels a little weird to me. URLRequest::ReferrerPolicy
> is
> > just about behavior on redirects, and to an external caller, there'd be no
> > difference between NO_REFERRER and NEVER_CLEAR_REFERRER, or between ORIGIN
and
> > NEVER_CLEAR_REFERRER. In other words we'd be adding values to the public
enum
> > just to facilitate an internal implementation detail of URLRequestJob.
> > 
> > I'll defer to your judgement if you still think that's the right way to do
it,
> > just wanted to point out that IMO it confuses the public API. I can also
play
> > with restructuring the code a bit, maybe there's a way to make it less
> confusing
> > without adding new URLRequest::ReferrerPolicy values.
> 
> I think having a 1-to-1 mapping between URLRequest::ReferrerPolicy and the
> specced out "referrer policies" in that doc makes sense, though that's not
> really my main concern here.  What bugs me about this is that
> "ComputeReferrerForRedirect" no longer computes the referrer for the redirect.

> It's a combination of ComputeReferrerForRedirect and the referrer changes made
> here.  And repeatedly updating new_referrer_source when there are multiple
> headers also seems really weird.
> 
> Also note that if we switch to having ComputeReferrerForRedirect actually
> compute the referrer, there will be a difference.  It would clear the referrer
> on NO_REFERRER, and do nothing on NEVER_CLEAR_REFERRER.  And on ORIGIN, it
would
> make the referrer the origin.  Since it would do this on every redirect, there
> would be a bit of wasted work, with multiple redirect, of course.
> 
> One ugliness with that approach is that we wouldn't adhere to those policies,
if
> set, for the initial request, and blink wouldn't be setting those policies. 
> (Well beyond the scope of this CL, but if we switched to having the renderer
> just ass along teh policy and the original referrer, and net actually enforce
> those policies, that might reduce the amount of code to manage referrers. 
> Haven't thought about the practicality of that, not familiar with all the
stuff,
> renderer side, to deal with it)

Yeah that's what I meant by "confuses the public API". I think Blink will always
need to compute the referrer itself because it's exposed in the Fetch API, but I
suppose net could recompute it independently on initial requests.

Anyway it does make the code a lot simpler so I went ahead and did this, PTAL.

> 
> Another option I could live with would be to make one method compute both the
> policy and the referrer, and to stick with the reduced set of policies.  I
> suspect that would result in much harder to read code, but if we can get
> something reasonable with that approach...
> 
> > > When there are multiple tokens, repeatedly computing, and then ignoring,
> > > new_referrer_source is also weird.  Only calculating it once we have the
> > policy
> > > at the end also more closely adhere to how the spec is written, though the
> > > result is the same.
> > 
> > Acknowledged. Will address this once above question is resolved.

[mmenke, 2016-06-28 21:32:09]

A couple suggestions.  I haven't quite finished thinking about extra tests or
looking at code outside of net/, expect to finish that tomorrow.

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request.h:95: //   (NO_REFERRER).
optional:  Could move each of these to just before the constant it describes, so
you don't have to list the constants twice.  Would need to modify the old
comments similarly, though.  Up to you.

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request.h:330: void set_referrer_policy(ReferrerPolicy
referrer_policy);
Optional:  Could DCHECK that the policy is not set to ORIGIN or NO_REFFER here,
as it doesn't quite work as expected.  Or just add a comment about that
weirdness, DCHECK seems more reliable, though.

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request_job.cc:84: new_policy = URLRequest::NO_REFERRER;
Should probably have continues after all of these, just to make it clearer
they're mutually exclusive.  (else if's also work, but unless the code is likely
to change in a way that anything after all these ifs should be shared by them,
think the continue is generally preferred).

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request_job.cc:397: case URLRequest::MAX_REFERRER_POLICY:
I'm fine with adding this, if it's useful, but why did you need to add this?

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request_job.cc:399: return original_referrer;
This should probably be GURL(), to match the fallthrough case.

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:378: "https://foo.test/" /* expected
final referrer */},
Maybe a test like this, but it makes sure we actually clear the referrer, if
we're being redirected to an insecure URL?

[estark, 2016-06-28 22:38:43]

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request.h:95: //   (NO_REFERRER).
On 2016/06/28 21:32:08, mmenke wrote:
> optional:  Could move each of these to just before the constant it describes,
so
> you don't have to list the constants twice.  Would need to modify the old
> comments similarly, though.  Up to you.

Done.

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request.h:330: void set_referrer_policy(ReferrerPolicy
referrer_policy);
On 2016/06/28 21:32:08, mmenke wrote:
> Optional:  Could DCHECK that the policy is not set to ORIGIN or NO_REFFER
here,
> as it doesn't quite work as expected.  Or just add a comment about that
> weirdness, DCHECK seems more reliable, though.

Done.

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request_job.cc:84: new_policy = URLRequest::NO_REFERRER;
On 2016/06/28 21:32:08, mmenke wrote:
> Should probably have continues after all of these, just to make it clearer
> they're mutually exclusive.  (else if's also work, but unless the code is
likely
> to change in a way that anything after all these ifs should be shared by them,
> think the continue is generally preferred).

Done.

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request_job.cc:397: case URLRequest::MAX_REFERRER_POLICY:
On 2016/06/28 21:32:08, mmenke wrote:
> I'm fine with adding this, if it's useful, but why did you need to add this?

RedirectInfo is sent in IPCs, and adding a ReferrerPolicy to the RedirectInfo
IPC struct requires a max value for the enum... I think. (See
resource_messages.h)

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request_job.cc:399: return original_referrer;
On 2016/06/28 21:32:08, mmenke wrote:
> This should probably be GURL(), to match the fallthrough case.

Done.

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/2100583002/diff/200001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:378: "https://foo.test/" /* expected
final referrer */},
On 2016/06/28 21:32:08, mmenke wrote:
> Maybe a test like this, but it makes sure we actually clear the referrer, if
> we're being redirected to an insecure URL?

Done.

[mmenke, 2016-06-29 15:05:09]

A couple nits, but otherwise, LGTM

https://codereview.chromium.org/2100583002/diff/220001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/2100583002/diff/220001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:66: void
MakeMockReferrerPolicyTransaction(const char* original_location,
Could you rename original_location to original_url or something - harder to
confuse with location header.

https://codereview.chromium.org/2100583002/diff/220001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:266: const char* original_location;
location -> original_url

https://codereview.chromium.org/2100583002/diff/220001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:277: {"http://foo.test/one" /*
location */,
Similar to above comment, location -> original_url on these.  Too easy to
confuse with the location header.  It's happened to me every time I've looked at
these.

[jochen (gone - plz use gerrit), 2016-06-29 15:26:39]

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1035:
main_request_context_->set_enable_referrer_policy_header(
can we just turn this unconditionally on in content_shell's
ShellURLRequestContextGetter::GetURLRequestContext ?

[estark, 2016-06-29 17:36:14]

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1035:
main_request_context_->set_enable_referrer_policy_header(
On 2016/06/29 15:26:39, jochen wrote:
> can we just turn this unconditionally on in content_shell's
> ShellURLRequestContextGetter::GetURLRequestContext ?

Why unconditionally? Would you be okay if I did it only when
kEnableExperimentalWebPlatformFeatures is on?

[estark, 2016-06-29 17:44:25]

https://codereview.chromium.org/2100583002/diff/220001/net/url_request/url_re...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/2100583002/diff/220001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:66: void
MakeMockReferrerPolicyTransaction(const char* original_location,
On 2016/06/29 15:05:09, mmenke wrote:
> Could you rename original_location to original_url or something - harder to
> confuse with location header.

Done.

https://codereview.chromium.org/2100583002/diff/220001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:266: const char* original_location;
On 2016/06/29 15:05:09, mmenke wrote:
> location -> original_url

Done.

https://codereview.chromium.org/2100583002/diff/220001/net/url_request/url_re...
net/url_request/url_request_job_unittest.cc:277: {"http://foo.test/one" /*
location */,
On 2016/06/29 15:05:09, mmenke wrote:
> Similar to above comment, location -> original_url on these.  Too easy to
> confuse with the location header.  It's happened to me every time I've looked
at
> these.

Done.

[estark, 2016-06-29 17:45:11]

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1035:
main_request_context_->set_enable_referrer_policy_header(
On 2016/06/29 17:36:14, estark wrote:
> On 2016/06/29 15:26:39, jochen wrote:
> > can we just turn this unconditionally on in content_shell's
> > ShellURLRequestContextGetter::GetURLRequestContext ?
> 
> Why unconditionally? Would you be okay if I did it only when
> kEnableExperimentalWebPlatformFeatures is on?

(I went ahead and did that, let me know if there's a reason we should do it
unconditionally instead.)

[estark, 2016-06-29 17:46:27]

estark@chromium.org changed reviewers:
+ avi@chromium.org, eugenebut@chromium.org

[estark, 2016-06-29 17:46:28]

avi: can you please review resource_messages.h?
eugenebut: can you please review ios?

Thanks!

[Eugene But (OOO till 7-30), 2016-06-29 17:47:38]

ios lgtm

[Avi (use Gerrit), 2016-06-29 19:25:27]

On 2016/06/29 17:46:28, estark wrote:
> avi: can you please review resource_messages.h?

I can't help you there. It requires an IPC reviewer which I am not.

[estark, 2016-06-29 19:35:54]

estark@chromium.org changed reviewers:
+ palmer@chromium.org
- avi@chromium.org

[estark, 2016-06-29 19:35:55]

palmer can you please review resource_messages.h? Thanks!

[jochen (gone - plz use gerrit), 2016-06-29 19:36:15]

jochen@chromium.org changed reviewers:
+ avi@chromium.org
- palmer@chromium.org

[jochen (gone - plz use gerrit), 2016-06-29 19:36:17]

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1035:
main_request_context_->set_enable_referrer_policy_header(
On 2016/06/29 at 17:45:10, estark wrote:
> On 2016/06/29 17:36:14, estark wrote:
> > On 2016/06/29 15:26:39, jochen wrote:
> > > can we just turn this unconditionally on in content_shell's
> > > ShellURLRequestContextGetter::GetURLRequestContext ?
> > 
> > Why unconditionally? Would you be okay if I did it only when
> > kEnableExperimentalWebPlatformFeatures is on?
> 
> (I went ahead and did that, let me know if there's a reason we should do it
unconditionally instead.)

It should be enabled for layout tests as they should run with all experimental
features enabled, however, layout tests don't specify that command line flag.

You could add a helper method to ShellURLRequestContextGetter and override it in
LayoutTestURLRequestContextGetter

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1035:
main_request_context_->set_enable_referrer_policy_header(
On 2016/06/29 at 17:45:10, estark wrote:
> On 2016/06/29 17:36:14, estark wrote:
> > On 2016/06/29 15:26:39, jochen wrote:
> > > can we just turn this unconditionally on in content_shell's
> > > ShellURLRequestContextGetter::GetURLRequestContext ?
> > 
> > Why unconditionally? Would you be okay if I did it only when
> > kEnableExperimentalWebPlatformFeatures is on?
> 
> (I went ahead and did that, let me know if there's a reason we should do it
unconditionally instead.)

It should be enabled for layout tests as they should run with all experimental
features enabled, however, layout tests don't specify that command line flag.

You could add a helper method to ShellURLRequestContextGetter and override it in
LayoutTestURLRequestContextGetter

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1035:
main_request_context_->set_enable_referrer_policy_header(
On 2016/06/29 at 17:45:10, estark wrote:
> On 2016/06/29 17:36:14, estark wrote:
> > On 2016/06/29 15:26:39, jochen wrote:
> > > can we just turn this unconditionally on in content_shell's
> > > ShellURLRequestContextGetter::GetURLRequestContext ?
> > 
> > Why unconditionally? Would you be okay if I did it only when
> > kEnableExperimentalWebPlatformFeatures is on?
> 
> (I went ahead and did that, let me know if there's a reason we should do it
unconditionally instead.)

It should be enabled for layout tests as they should run with all experimental
features enabled, however, layout tests don't specify that command line flag.

You could add a helper method to ShellURLRequestContextGetter and override it in
LayoutTestURLRequestContextGetter

[estark, 2016-06-29 20:09:07]

estark@chromium.org changed reviewers:
+ palmer@chromium.org
- avi@chromium.org

[estark, 2016-06-29 20:09:08]

Also I apparently forgot to add palmer... palmer can you please review
resource_messages.h?

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2100583002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1035:
main_request_context_->set_enable_referrer_policy_header(
On 2016/06/29 19:36:16, jochen wrote:
> On 2016/06/29 at 17:45:10, estark wrote:
> > On 2016/06/29 17:36:14, estark wrote:
> > > On 2016/06/29 15:26:39, jochen wrote:
> > > > can we just turn this unconditionally on in content_shell's
> > > > ShellURLRequestContextGetter::GetURLRequestContext ?
> > > 
> > > Why unconditionally? Would you be okay if I did it only when
> > > kEnableExperimentalWebPlatformFeatures is on?
> > 
> > (I went ahead and did that, let me know if there's a reason we should do it
> unconditionally instead.)
> 
> It should be enabled for layout tests as they should run with all experimental
> features enabled, however, layout tests don't specify that command line flag.
> 
> You could add a helper method to ShellURLRequestContextGetter and override it
in
> LayoutTestURLRequestContextGetter

Ah ok I see, I did that and added a layout test so we know it's working.

[jochen (gone - plz use gerrit), 2016-06-29 20:44:05]

lgtm, thanks!

https://codereview.chromium.org/2100583002/diff/300001/content/shell/browser/...
File content/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/2100583002/diff/300001/content/shell/browser/...
content/shell/browser/shell_url_request_context_getter.cc:118: return false;
nit. could be return
base::CommandLine::ForCurrentProcess()->HasSwitch(swtiches::kEnableExperimentalWebPlatformFeatures)

[palmer, 2016-06-29 20:56:46]

IPC security LGTM.

[estark, 2016-06-29 21:16:48]

Thanks all!

https://codereview.chromium.org/2100583002/diff/300001/content/shell/browser/...
File content/shell/browser/shell_url_request_context_getter.cc (right):

https://codereview.chromium.org/2100583002/diff/300001/content/shell/browser/...
content/shell/browser/shell_url_request_context_getter.cc:118: return false;
On 2016/06/29 20:44:05, jochen wrote:
> nit. could be return
>
base::CommandLine::ForCurrentProcess()->HasSwitch(swtiches::kEnableExperimentalWebPlatformFeatures)

Done.

[estark, 2016-06-29 21:17:06]

The CQ bit was checked by estark@chromium.org

[estark, 2016-06-29 21:17:07]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
eugenebut@chromium.org, palmer@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2100583002/#ps320001
(title: "jochen suggestion")

[commit-bot: I haz the power, 2016-06-29 21:18:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 00:20:34]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-06-30 00:23:15]

Description was changed from

==========
Apply Referrer-Policy header when following redirects

When a Referrer-Policy header is received during a redirect,
URLRequestJob parses it and updates the referrer and referrer policy on
the request, if necessary.

The Referrer-Policy header is being implemented as an experimental web
platform feature. The experimental web platform feature flag is plumbed
to URLRequestJob via a boolean on URLRequestContext. This flag should be
temporary and only live until the Referrer-Policy feature ships.

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Umj9iVRJM70
Implementation plan:
https://docs.google.com/document/d/1SyQhP6Y7BHIQXWL8S1saWqMuar4hoWLGigQuHmizg...

BUG=619228
==========

to

==========
Apply Referrer-Policy header when following redirects

When a Referrer-Policy header is received during a redirect,
URLRequestJob parses it and updates the referrer and referrer policy on
the request, if necessary.

The Referrer-Policy header is being implemented as an experimental web
platform feature. The experimental web platform feature flag is plumbed
to URLRequestJob via a boolean on URLRequestContext. This flag should be
temporary and only live until the Referrer-Policy feature ships.

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Umj9iVRJM70
Implementation plan:
https://docs.google.com/document/d/1SyQhP6Y7BHIQXWL8S1saWqMuar4hoWLGigQuHmizg...

BUG=619228

Committed: https://crrev.com/550ba7f9533922cfeac9709d99815cec9b2ad52a
Cr-Commit-Position: refs/heads/master@{#403017}
==========

[commit-bot: I haz the power, 2016-06-30 00:23:17]

Patchset 17 (id:??) landed as
https://crrev.com/550ba7f9533922cfeac9709d99815cec9b2ad52a
Cr-Commit-Position: refs/heads/master@{#403017}

[estark, 2016-06-30 02:57:58]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/2108423002/ by estark@chromium.org.

The reason for reverting is: Broke msan tests
https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Test....