[webkit-reviews] review denied: [Bug 182644] The referer header is not set after redirect : [Attachment 336743] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 182644: The referer header is not set after redirect
https://bugs.webkit.org/show_bug.cgi?id=182644

Attachment 336743: Patch

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




--- Comment #31 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 336743
  --> https://bugs.webkit.org/attachment.cgi?id=336743
Patch

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

> Source/WebCore/ChangeLog:9
> +	   Add Referer head back in redirection check.

I do not understand this sentence.

> Source/WebCore/dom/Document.cpp:3464
> +    ReferrerPolicy referrerPolicy= toReferrerPolicy(policy,
ReferrerPolicyDeliveryMethod::Meta);

Missing space before =.

Also, we could use:
auto referrerPolicy = ..

> Source/WebCore/loader/CrossOriginAccessControl.cpp:37
> +#include "SecurityPolicy.h"

Unnecessary include?

> Source/WebCore/loader/ResourceLoader.h:164
> +    ReferrerPolicy getReferrerPolicy() { return m_options.referrerPolicy; }

We do not use "get" prefix for getters in WebKit. This should be named
referrerPolicy(). Also, this method should be const.

> Source/WebCore/loader/SubresourceLoader.cpp:579
> +    String outgoingReferrer = previousRequest.hasHTTPReferrer() ?
previousRequest.httpReferrer() : emptyString();

Why emptyString() and not String()?

> Source/WebCore/loader/SubresourceLoader.cpp:592
> +    Vector<String> referrerTokens;

This is unused.

> Source/WebCore/loader/SubresourceLoader.cpp:593
> +    referrerPolicyValue.split(",", false, referrerTokens);

Seems unnecessary since referrerTokens is unused.

> Source/WebCore/loader/SubresourceLoader.cpp:595
> +	   const String token =
stripLeadingAndTrailingHTTPSpaces(tokenView).toString();

We do not need toString() here, this is inefficient. a StringView is sufficient
to convert to a ReferrerPolicy.
auto token = stripLeadingAndTrailingHTTPSpaces(tokenView);

> Source/WebCore/loader/SubresourceLoader.h:32
> +#include "ReferrerPolicy.h"

Unnecessary include?

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:236
> +    if (!m_resourceRequest.httpReferrer().isNull()) {

if (m_resourceRequest.hasHTTPReferrer())

> Source/WebCore/platform/ReferrerPolicy.cpp:34
> +

This is not building on the bots. Looks like you're missing some includes here.

> Source/WebCore/platform/ReferrerPolicy.cpp:37
> +ReferrerPolicy toReferrerPolicy(const String& policy,
ReferrerPolicyDeliveryMethod method)

policy should be a StringView here for performance. This way, calls site that
have a StringView (by value) do not need to construct a String:
ReferrerPolicy toReferrerPolicy(StringView policy, ReferrerPolicyDeliveryMethod
method)

Although I would have preferred parseReferrerPolicy as naming.

> Source/WebCore/platform/ReferrerPolicy.cpp:39
> +    // "never" / "default" / "always" are legacy keywords that we will
support. They were defined in:

s/we will support/we support/

> Source/WebCore/platform/ReferrerPolicy.cpp:41
> +    ReferrerPolicy referrerPolicy = ReferrerPolicy::EmptyString;

I do not think we need this local variable. We can just use early returns.

> Source/WebCore/platform/ReferrerPolicy.cpp:45
> +	       referrerPolicy = ReferrerPolicy::NoReferrer;

return ReferrerPolicy::NoReferrer;

> Source/WebCore/platform/ReferrerPolicy.cpp:53
> +	   referrerPolicy = ReferrerPolicy::NoReferrer;

ReferrerPolicy::NoReferrer;

> Source/WebCore/platform/ReferrerPolicy.cpp:54
> +    else if (equalLettersIgnoringASCIICase(policy, "unsafe-url"))

then "if" instead of "else if" here.

> Source/WebCore/platform/ReferrerPolicy.cpp:68
> +    

if (policy == "")
    return ReferrerPolicy::EmptyString;

> Source/WebCore/platform/ReferrerPolicy.cpp:69
> +    return referrerPolicy;

return std::nullopt;

> Source/WebCore/platform/ReferrerPolicy.h:51
> +enum class ReferrerPolicyDeliveryMethod {

How about ShouldParseLegacyKeywords { No, Yes }; ?

> Source/WebCore/platform/ReferrerPolicy.h:56
> +ReferrerPolicy toReferrerPolicy(const String&,
ReferrerPolicyDeliveryMethod);

I think this should return a std::optional<ReferrerPolicy> and return
std::nullopt when the keyword cannot be parsed. Note that the empty string is
valid referrerPolicy and the specification distinguishes the empty string and
no valid token.