[webkit-reviews] review granted: [Bug 208424] Add referrerpolicy attribute support for anchors : [Attachment 392100] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 208424: Add referrerpolicy attribute support for anchors
https://bugs.webkit.org/show_bug.cgi?id=208424

Attachment 392100: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 392100
  --> https://bugs.webkit.org/attachment.cgi?id=392100
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::EmptyStr
ing);
> +    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.