Created attachment 402209 [details]
Patch

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

Created attachment 402223 [details]
Patch

Created attachment 402275 [details]
Patch

Created attachment 402276 [details]
Patch

Created attachment 402278 [details]
Patch

Created attachment 402282 [details]
Patch

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.

Created attachment 402478 [details]
Patch

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.

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

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

<rdar://problem/64600342>

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.

Reopening to attach new patch.

Created attachment 402584 [details]
Patch

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.

Comment on attachment 402584 [details]
Patch

Could also initialize relAttribute for the same reason.

(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.

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

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

(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.