Anchor element's referrer attribute implementation for Referrer Policy

Anchor element's referrer attribute implementation for Referrer Policy.

Support for the HTML referrer attribute on elements: <a>, <area>, <img>, <iframe> (and possibly others) allowing authors to set a Referrer-Policy for a single request associated with the HTML element referencing a sub-resource. More details are disclosed in the spec.

Design doc: https://goo.gl/aMFGRY
Spec: https://goo.gl/tkLkxN

BUG=490608

[Mike West, 2015-05-20 07:56:40]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[Mike West, 2015-05-20 07:56:42]

This is a good start. I've added some comments. Adding Jochen as well, as he's
done more work on referrers than I have.

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
File Source/core/html/HTMLAnchorElement.cpp (left):

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:362:
frameRequest.setShouldSendReferrer(NeverSendReferrer);
Is `ShouldSendReferrer` actually used after this patch? If not, please remove
it, along with its related methods.

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:366: ReferrerPolicy
referrerPolicyForRequest = ReferrerPolicyNoReferrerWhenDowngrade;
Nit: You can pull this out of the `if`, set `referrerPolicyForRequest` in one of
the two branches, and then call `setReferrerPolicyForRequest` after the whole
thing once, rather than in both branches.

Nit: This should be `ReferrerPolicyDefault` when there's no attribute present,
or when the attribute is invalid.

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:368: // FIXME: This string parsing should
be implemented in one place and
Nit: s/FIXME/TODO(burnik)/

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:369: // not duplicated from
|blink::Document::processReferrerPolicy|.
Perhaps you could extract this out to a method in
`platform/weborigin/ReferrerPolicy.h`?

You could extract the whole thing, actually, as you'll need similar code for
`<a>`, `<area>`, `<img>`, and `<iframe>`.

https://codereview.chromium.org/1148603002/diff/1/Source/core/loader/FrameLoa...
File Source/core/loader/FrameLoadRequest.h (right):

https://codereview.chromium.org/1148603002/diff/1/Source/core/loader/FrameLoa...
Source/core/loader/FrameLoadRequest.h:46: ,
m_referrerPolicyForRequest(ReferrerPolicyNoReferrerWhenDowngrade)
Nit: ReferrerPolicyDefault, here and elsewhere in this file.

https://codereview.chromium.org/1148603002/diff/1/Source/core/loader/FrameLoa...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1148603002/diff/1/Source/core/loader/FrameLoa...
Source/core/loader/FrameLoader.cpp:791:
setReferrerForFrameRequest(request.resourceRequest(),
request.referrerPolicyForRequest(), request.originDocument());
I don't think it's the case that you're consistently setting the referrer policy
on the request, is it? That is, this will work for `<a>`-based navigations, but
will give the wrong result for (for instance) JavaScript-driven navigations,
`<meta refresh>`, etc.

https://codereview.chromium.org/1148603002/diff/1/Source/core/page/CreateWind...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/1148603002/diff/1/Source/core/page/CreateWind...
Source/core/page/CreateWindow.cpp:151: Frame* newFrame =
createWindow(*activeFrame, openerFrame, frameRequest, windowFeatures,
NavigationPolicyIgnore, ReferrerPolicyDefault);
This would previously have read the referrer policy from the active frame's
active document, wouldn't it?

https://codereview.chromium.org/1148603002/diff/1/Source/core/page/CreateWind...
Source/core/page/CreateWindow.cpp:186:
toLocalFrame(newFrame)->document()->setReferrerPolicy(openerFrame.document()->referrerPolicy());
Hrm. What is this code doing? Why are we pushing the referrer policy out to the
newly-created window?

[burnik, 2015-05-20 08:27:56]

Some nits fixed. Open discussion.

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
File Source/core/html/HTMLAnchorElement.cpp (left):

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:362:
frameRequest.setShouldSendReferrer(NeverSendReferrer);
On 2015/05/20 07:56:41, Mike West (holiday in DE) wrote:
> Is `ShouldSendReferrer` actually used after this patch? If not, please remove
> it, along with its related methods.

I think it should be removed completely. Yesterday I was just to lazy to clean
up, but I've added myself a note.

The terms ShouldSendReferrer and MaybeSendReferrer sounds so non-deterministic
and clumsy anyway. My first taught was that it was boolean :D

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:366: ReferrerPolicy
referrerPolicyForRequest = ReferrerPolicyNoReferrerWhenDowngrade;
On 2015/05/20 07:56:41, Mike West (holiday in DE) wrote:
> Nit: You can pull this out of the `if`, set `referrerPolicyForRequest` in one
of
> the two branches, and then call `setReferrerPolicyForRequest` after the whole
> thing once, rather than in both branches.
> 
> Nit: This should be `ReferrerPolicyDefault` when there's no attribute present,
> or when the attribute is invalid.

Yes, I've seen it earlier, this initial approach was the worst possible
practice: code duplication, zero refactoring, etc. Just to get a feeling of how
things work together.

Done.

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:368: // FIXME: This string parsing should
be implemented in one place and
On 2015/05/20 07:56:41, Mike West (holiday in DE) wrote:
> Nit: s/FIXME/TODO(burnik)/

Done.

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:369: // not duplicated from
|blink::Document::processReferrerPolicy|.
On 2015/05/20 07:56:41, Mike West (holiday in DE) wrote:
> Perhaps you could extract this out to a method in
> `platform/weborigin/ReferrerPolicy.h`?
> 
> You could extract the whole thing, actually, as you'll need similar code for
> `<a>`, `<area>`, `<img>`, and `<iframe>`.

Yes, the FIXME(TODO) actually instructs me to do so. 
There should be two methods:

ReferrerPolicyFromString and ReferrerPoliyToString.

I believe the RP tests already implement those, so it should be a big win to
refactor this and have it in one place for reuse.

Note: <area> already inherits <a> so that part should already work.

https://codereview.chromium.org/1148603002/diff/1/Source/core/loader/FrameLoa...
File Source/core/loader/FrameLoadRequest.h (right):

https://codereview.chromium.org/1148603002/diff/1/Source/core/loader/FrameLoa...
Source/core/loader/FrameLoadRequest.h:46: ,
m_referrerPolicyForRequest(ReferrerPolicyNoReferrerWhenDowngrade)
On 2015/05/20 07:56:41, Mike West (holiday in DE) wrote:
> Nit: ReferrerPolicyDefault, here and elsewhere in this file.

Done.

https://codereview.chromium.org/1148603002/diff/1/Source/core/loader/FrameLoa...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1148603002/diff/1/Source/core/loader/FrameLoa...
Source/core/loader/FrameLoader.cpp:791:
setReferrerForFrameRequest(request.resourceRequest(),
request.referrerPolicyForRequest(), request.originDocument());
On 2015/05/20 07:56:41, Mike West (holiday in DE) wrote:
> I don't think it's the case that you're consistently setting the referrer
policy
> on the request, is it? That is, this will work for `<a>`-based navigations,
but
> will give the wrong result for (for instance) JavaScript-driven navigations,
> `<meta refresh>`, etc.

If we replace MaybeSendReferrer with ReferrerPolicyDefault and NeverSendReferrer
with ReferrerPolicyNever, shouldn't that be equivalent?

In any case, I'll be adding a bunch of tests before this CL is actually ready
for a review. :-)

Thanks for the note.

https://codereview.chromium.org/1148603002/diff/1/Source/core/page/CreateWind...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/1148603002/diff/1/Source/core/page/CreateWind...
Source/core/page/CreateWindow.cpp:151: Frame* newFrame =
createWindow(*activeFrame, openerFrame, frameRequest, windowFeatures,
NavigationPolicyIgnore, ReferrerPolicyDefault);
On 2015/05/20 07:56:42, Mike West (holiday in DE) wrote:
> This would previously have read the referrer policy from the active frame's
> active document, wouldn't it?

Not quite sure at this point. I just followed this logic for playing around:
s/MaybeSendReferrer/ReferrerPolicyDefault/g
s/NeverSendReferrer/ReferrerPolicyNever/g

https://codereview.chromium.org/1148603002/diff/1/Source/core/page/CreateWind...
Source/core/page/CreateWindow.cpp:186:
toLocalFrame(newFrame)->document()->setReferrerPolicy(openerFrame.document()->referrerPolicy());
On 2015/05/20 07:56:42, Mike West (holiday in DE) wrote:
> Hrm. What is this code doing? Why are we pushing the referrer policy out to
the
> newly-created window?

This was a bit confusing to me too. I'll ponder on it.

[burnik, 2015-05-20 13:45:18]

One major bug fixed. A bit of clean up and refactoring. 

I now also have WP tests for <a> and <area>:
https://github.com/w3c/web-platform-tests/pull/1814

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
File Source/core/html/HTMLAnchorElement.cpp (left):

https://codereview.chromium.org/1148603002/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:362:
frameRequest.setShouldSendReferrer(NeverSendReferrer);
On 2015/05/20 08:27:56, burnik wrote:
> On 2015/05/20 07:56:41, Mike West (holiday in DE) wrote:
> > Is `ShouldSendReferrer` actually used after this patch? If not, please
remove
> > it, along with its related methods.
> 
> I think it should be removed completely. Yesterday I was just to lazy to clean
> up, but I've added myself a note.
> 
> The terms ShouldSendReferrer and MaybeSendReferrer sounds so non-deterministic
> and clumsy anyway. My first taught was that it was boolean :D
>  

Done.

[jochen (gone - plz use gerrit), 2015-05-21 07:48:58]

what about layout tests?

https://codereview.chromium.org/1148603002/diff/60001/Source/core/html/HTMLAn...
File Source/core/html/HTMLAnchorElement.idl (right):

https://codereview.chromium.org/1148603002/diff/60001/Source/core/html/HTMLAn...
Source/core/html/HTMLAnchorElement.idl:29: [Reflect] attribute DOMString
referrer;
this should be a runtime enabled feature, no?

https://codereview.chromium.org/1148603002/diff/60001/Source/platform/weborig...
File Source/platform/weborigin/ReferrerPolicy.cpp (right):

https://codereview.chromium.org/1148603002/diff/60001/Source/platform/weborig...
Source/platform/weborigin/ReferrerPolicy.cpp:64: case ReferrerPolicyDefault:
what is no-meta?

[burnik, 2015-05-21 10:44:37]

jochen@: Thanks for the notes. I left some questions for you and mkwst@.

I'll be adding the layout tests as well in a patchset or so.

https://codereview.chromium.org/1148603002/diff/60001/Source/core/html/HTMLAn...
File Source/core/html/HTMLAnchorElement.idl (right):

https://codereview.chromium.org/1148603002/diff/60001/Source/core/html/HTMLAn...
Source/core/html/HTMLAnchorElement.idl:29: [Reflect] attribute DOMString
referrer;
On 2015/05/21 07:48:58, jochen wrote:
> this should be a runtime enabled feature, no?

Yes, I think that's the idea, I would wrap this behind a flag before sending an
intent to ship.

@mkwst: Is this a good approach for this feature?

https://codereview.chromium.org/1148603002/diff/60001/Source/platform/weborig...
File Source/platform/weborigin/ReferrerPolicy.cpp (right):

https://codereview.chromium.org/1148603002/diff/60001/Source/platform/weborig...
Source/platform/weborigin/ReferrerPolicy.cpp:64: case ReferrerPolicyDefault:
On 2015/05/21 07:48:58, jochen wrote:
> what is no-meta?

This is copied from a test. See above. Mike added this, I suppose for testing
purposes only. If I'm not mistaken there should be a fall-through here, right?

[Mike West, 2015-05-21 11:10:27]

On 2015/05/21 at 10:44:37, burnik wrote:
> @mkwst: Is this a good approach for this feature?

It is! It would be worth sending an "Intent to Implement" as well to blink-dev@,
and adding the attribute to chromestatus.com.

>
https://codereview.chromium.org/1148603002/diff/60001/Source/platform/weborig...
> File Source/platform/weborigin/ReferrerPolicy.cpp (right):
> 
>
https://codereview.chromium.org/1148603002/diff/60001/Source/platform/weborig...
> Source/platform/weborigin/ReferrerPolicy.cpp:64: case ReferrerPolicyDefault:
> On 2015/05/21 07:48:58, jochen wrote:
> > what is no-meta?
> 
> This is copied from a test. See above. Mike added this, I suppose for testing
purposes only. If I'm not mistaken there should be a fall-through here, right?

If we're going to expose this to the web, we certainly shouldn't use "no-meta".
Are we exposing it to the web anywhere? Looking at the diff, I don't actually
see where this method is used at all. Do we need it?