[webkit-reviews] review granted: [Bug 201353] Resource Load Statistics: Downgrade all third-party referrer headers : [Attachment 379646] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Katherine_cheney at apple.com's
request for review:
Bug 201353: Resource Load Statistics: Downgrade all third-party referrer
headers
https://bugs.webkit.org/show_bug.cgi?id=201353

Attachment 379646: Patch

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




--- Comment #34 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 379646
  --> https://bugs.webkit.org/attachment.cgi?id=379646
Patch

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

Looks good. I think the tests could be a bit better with the creation of a
common utility function, but that doesn't have to be done in this patch.

> Tools/ChangeLog:52
> +	   * BuildSlaveSupport/ews-build/steps_unittest.py: Updated unit-tests
accordingly.

Looks like we have accidentally included extra ChangeLog stuff here.

>
LayoutTests/http/tests/referrer-policy-script/no-referrer-when-downgrade/cross-
origin-http-http.html:25
> +		   document.body.appendChild(scriptElement);

It seems like this block of code could be a function provided by one of our
test.js files (if there is one used by all of these tests). Otherwise we may
need to update 'referrerPolicy' or 'src' in a number of places if we change our
test infrastructure.

Perhaps something like 

function referrerCompletionHandler(policy) { ... }

Then this becomes:

testRunner.setStatisticsShouldDowngradeReferrer(false,
referrerCompletionHandler('no-referrer-when-downgrade'));

>
LayoutTests/http/tests/referrer-policy-script/no-referrer/cross-origin-http-htt
p.html:24
> +    });

... and this could become:

testRunner.setStatisticsShouldDowngradeReferrer(false,
referrerCompletionHandler('no-referrer'));

>
LayoutTests/http/tests/referrer-policy-script/origin-when-cross-origin/cross-or
igin-http-http.html:23
> +	   document.body.appendChild(scriptElement);

... and this could become:

testRunner.setStatisticsShouldDowngradeReferrer(false,
referrerCompletionHandler('origin-when-cross-origin'));

>
LayoutTests/http/tests/referrer-policy-script/origin/cross-origin-http.https.ht
ml:27
> +    });

... and this could become:

testRunner.setStatisticsShouldDowngradeReferrer(false,
referrerCompletionHandler('origin'));