Bug 213342 – Add referrerpolicy attribute support for <link>

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rob Buis]

Add referrerpolicy attribute support for <link>

[Rob Buis]

Created attachment 402209 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 402223 [details]
Patch

[Rob Buis]

Created attachment 402275 [details]
Patch

[Rob Buis]

Created attachment 402276 [details]
Patch

[Rob Buis]

Created attachment 402278 [details]
Patch

[Rob Buis]

Created attachment 402282 [details]
Patch

[Darin Adler]

Comment on attachment 402282 [details]
Patch

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

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:290
> +            else if (match(attributeName, referrerpolicyAttr)) {
> +                m_referrerPolicy = parseReferrerPolicy(attributeValue, ReferrerPolicySource::ReferrerPolicyAttribute).valueOr(ReferrerPolicy::EmptyString);
> +                break;
> +            }

Doesn't seem necessary to include "break" here. The ones above don’t, and there is a break after the chained if/else statements. That would also allow us to omit the braces and this would fit in better with the lines above.

> Source/WebCore/loader/LinkLoader.h:57
> +    ReferrerPolicy referrerPolicy;

Should we set a default here of ReferrerPolicy::EmptyString? The other members have defaults, except for relAttribute. Generally seems good to not risk this ever being uninitialized. And maybe we can even omit this in some places if it has a default?

> LayoutTests/TestExpectations:244
> -imported/w3c/web-platform-tests/referrer-policy [ Skip ]
> +imported/w3c/web-platform-tests/referrer-policy/origin [ Skip ]

Sure could use a comment.

[Rob Buis]

Created attachment 402478 [details]
Patch

[Rob Buis]

Comment on attachment 402282 [details]
Patch

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

>> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:290
>> +            }
> 
> Doesn't seem necessary to include "break" here. The ones above don’t, and there is a break after the chained if/else statements. That would also allow us to omit the braces and this would fit in better with the lines above.

You are right, I probably had debug statement there while developing the patch and forgot to remove the break and braces. Fixed.

>> Source/WebCore/loader/LinkLoader.h:57
>> +    ReferrerPolicy referrerPolicy;
> 
> Should we set a default here of ReferrerPolicy::EmptyString? The other members have defaults, except for relAttribute. Generally seems good to not risk this ever being uninitialized. And maybe we can even omit this in some places if it has a default?

I think it gets the EmptyString value by default.

>> LayoutTests/TestExpectations:244
>> +imported/w3c/web-platform-tests/referrer-policy/origin [ Skip ]
> 
> Sure could use a comment.

Yeah I am very familiar with this problem but indeed others may not be. Fixed.

[EWS]

Committed r263356: <https://trac.webkit.org/changeset/263356>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402478 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64600342>

[Darin Adler]

Comment on attachment 402282 [details]
Patch

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

>>> Source/WebCore/loader/LinkLoader.h:57
>>> +    ReferrerPolicy referrerPolicy;
>> 
>> Should we set a default here of ReferrerPolicy::EmptyString? The other members have defaults, except for relAttribute. Generally seems good to not risk this ever being uninitialized. And maybe we can even omit this in some places if it has a default?
> 
> I think it gets the EmptyString value by default.

I don’t think it does. As I understand it, if it’s a scalar, it won’t have a default value unless we specify one.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 402584 [details]
Patch

[Rob Buis]

Comment on attachment 402282 [details]
Patch

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

>>> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:290
>>> +            }
>> 
>> Doesn't seem necessary to include "break" here. The ones above don’t, and there is a break after the chained if/else statements. That would also allow us to omit the braces and this would fit in better with the lines above.
> 
> You are right, I probably had debug statement there while developing the patch and forgot to remove the break and braces. Fixed.

I am still not 100% sure, but initializing can do no harm, so I made the patch.

[Darin Adler]

Comment on attachment 402584 [details]
Patch

Could also initialize relAttribute for the same reason.

[Rob Buis]

(In reply to Darin Adler from comment #17)
> Comment on attachment 402584 [details]
> Patch
> 
> Could also initialize relAttribute for the same reason.

I believe in the relAttribute the default ctor will be called since LinkRelAttribute is non POD.

[EWS]

Committed r263442: <https://trac.webkit.org/changeset/263442>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402584 [details].

[Darin Adler]

(In reply to Rob Buis from comment #18)
> I believe in the relAttribute the default ctor will be called since
> LinkRelAttribute is non POD.

Oh, OK, great.