Bug 208424 – Add referrerpolicy attribute support for anchors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Rob Buis]

Add referrerpolicy attribute support for anchors

[Rob Buis]

Created attachment 392095 [details]
Patch

[Rob Buis]

Created attachment 392096 [details]
Patch

[Rob Buis]

Created attachment 392098 [details]
Patch

[Rob Buis]

Created attachment 392100 [details]
Patch

[Darin Adler]

Comment on attachment 392100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392100&action=review

All these frame load request with all these default policies listed over and over again point to poor design patterns; we should figure out a way to construct these things where we don’t write so much boilerplate code. I’d say we should use a struct so we can set named properties, but FrameLoadRequest already is a bit like that. Seriously the number of places with these ridiculously long lists of arguments and policy settings is out of hand.

> Source/WebCore/html/HTMLAnchorElement.cpp:497
> +    ReferrerPolicy referrerPolicy = hasRel(Relation::NoReferrer) ? ReferrerPolicy::NoReferrer : this->referrerPolicy();

I’d use auto here for the variable type. No reason to repeat the name ReferrerPolicy extra times.

> Source/WebCore/html/HTMLAnchorElement.cpp:622
> +void HTMLAnchorElement::setReferrerPolicyForBindings(const AtomString& value)
> +{
> +    setAttributeWithoutSynchronization(referrerpolicyAttr, value);
> +}
> +
> +String HTMLAnchorElement::referrerPolicyForBindings() const
> +{
> +    return referrerPolicyToString(referrerPolicy());
> +}
> +
> +ReferrerPolicy HTMLAnchorElement::referrerPolicy() const
> +{
> +    if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled())
> +        return parseReferrerPolicy(attributeWithoutSynchronization(referrerpolicyAttr), ReferrerPolicySource::ReferrerPolicyAttribute).valueOr(ReferrerPolicy::EmptyString);
> +    return ReferrerPolicy::EmptyString;
> +}

Would be nicer to share a little bit more code with image element, but not sure I know how to do that.

> Source/WebCore/loader/FrameLoadRequest.cpp:65
> +    , m_referrerPolicy { ReferrerPolicy::EmptyString }

These defaults should be in the class definition instead of here. Unless there’s some reason not to do that.

> Source/WebCore/loader/FrameLoadRequest.h:113
> +    ReferrerPolicy m_referrerPolicy;

Like here:

    ReferrerPolicy m_referrerPolicy { ReferrerPolicy::EmptyString };

> LayoutTests/http/tests/referrer-policy-anchor/no-referrer-when-downgrade/cross-origin-http-http-expected.txt:9
> +Tests the behavior of no-referrer-when-downgrade referrer policy when same origin.
> +
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> +
> +
> +PASS referrer is expected
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE

This is a frustrating style of test output. The entire text amounts to a single "PASS"; nothing about the test is visible. Even the title of the test has been copied and pasted and so does not make clear what is being tested and how this differs from other tests.

The standard form writes out a value and what the expected value was. That gives the person running the test more insight into what is going on.

We should put some thought into the design of the test output. Seems like the tests have pretty good coverage.

Maybe a single test that covers multiple cases would be practical, too.

These tests are OK, much better than not having test coverage, but the design here is not the intent of the test framework.

[Rob Buis]

Created attachment 392117 [details]
Patch

[Rob Buis]

Comment on attachment 392100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392100&action=review

>> Source/WebCore/html/HTMLAnchorElement.cpp:497
>> +    ReferrerPolicy referrerPolicy = hasRel(Relation::NoReferrer) ? ReferrerPolicy::NoReferrer : this->referrerPolicy();
> 
> I’d use auto here for the variable type. No reason to repeat the name ReferrerPolicy extra times.

Done.

>> Source/WebCore/html/HTMLAnchorElement.cpp:622
>> +}
> 
> Would be nicer to share a little bit more code with image element, but not sure I know how to do that.

There are multiple places now with this code, but indeed hard to share.

>> Source/WebCore/loader/FrameLoadRequest.cpp:65
>> +    , m_referrerPolicy { ReferrerPolicy::EmptyString }
> 
> These defaults should be in the class definition instead of here. Unless there’s some reason not to do that.

Done.

>> Source/WebCore/loader/FrameLoadRequest.h:113
>> +    ReferrerPolicy m_referrerPolicy;
> 
> Like here:
> 
>     ReferrerPolicy m_referrerPolicy { ReferrerPolicy::EmptyString };

Done.

>> LayoutTests/http/tests/referrer-policy-anchor/no-referrer-when-downgrade/cross-origin-http-http-expected.txt:9
>> +TEST COMPLETE
> 
> This is a frustrating style of test output. The entire text amounts to a single "PASS"; nothing about the test is visible. Even the title of the test has been copied and pasted and so does not make clear what is being tested and how this differs from other tests.
> 
> The standard form writes out a value and what the expected value was. That gives the person running the test more insight into what is going on.
> 
> We should put some thought into the design of the test output. Seems like the tests have pretty good coverage.
> 
> Maybe a single test that covers multiple cases would be practical, too.
> 
> These tests are OK, much better than not having test coverage, but the design here is not the intent of the test framework.

Agreed, the output is not great, but at least it covers everything. Note that the real problem is that we can't import the WPT tests because of the problem illustrated here https://github.com/web-platform-tests/wpt/pull/19381.

[Rob Buis]

(In reply to Darin Adler from comment #5)
> Comment on attachment 392100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392100&action=review
> 
> All these frame load request with all these default policies listed over and
> over again point to poor design patterns; we should figure out a way to
> construct these things where we don’t write so much boilerplate code. I’d
> say we should use a struct so we can set named properties, but
> FrameLoadRequest already is a bit like that. Seriously the number of places
> with these ridiculously long lists of arguments and policy settings is out
> of hand.

Agreed, it is not pretty. I am likely to touch FrameLoadRequest in the future so I will keep it in the back of my mind.

[WebKit Commit Bot]

Comment on attachment 392117 [details]
Patch

Clearing flags on attachment: 392117

Committed r257707: <https://trac.webkit.org/changeset/257707>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59945018>

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 392132 [details]
Patch

[Rob Buis]

Wrong bug?

[youenn fablet]

(In reply to Rob Buis from comment #14)
> Wrong bug?

Definitely....