[webkit-reviews] review denied: [Bug 192094] HTTPS Upgrade: Consult dummy storage for HTTPS Upgrade, Apply If Appropriate : [Attachment 355905] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Vivek Seth <v_seth at apple.com>'s
request for review:
Bug 192094: HTTPS Upgrade: Consult dummy storage for HTTPS Upgrade, Apply If
Appropriate
https://bugs.webkit.org/show_bug.cgi?id=192094

Attachment 355905: Patch

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




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

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:197
> +    URL url = request.url();

We do not technically need to copy the URL here, only later if we upgrade.
Therefore I would use
auto& url = .. here.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:199
> +    // Only upgrade http urls

WebKit comments need to end with a period.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:200
> +    if (url.protocol() == "http") {

url.protocolIs("http")

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:202
> +	   Vector<String> upgradableHosts = {

You do not want to construct this vector every time this method is called, this
would be expensive. This should be static:

static NeverDestroyed<HashSet<String>> upgradableHosts =
std::initializer_list<String> {
    "www.bbc.com"_s, // (source: https://whynohttps.com)
    "www.speedtest.net"_s, // (source: https://whynohttps.com)
    "www.bea.gov"_s // (source: https://pulse.cio.gov/data/domains/https.csv)
};

Also, please move the static outside the if scope.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:207
> +	   auto requestHost = url.host();

We do not really use this local variable, just use url.host() below.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:210
> +	       url.setProtocol("https");

"https"_s is slightly more efficient.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:226
> +	       ASSERT(request.url().protocol() == "https");

protocolIs("https")

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:143
> +

I do not think we need this blank line.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:145
> +

I do not think we need this blank line.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:198
> +#if ENABLE(HTTPS_UPGRADE)

I do not think this should be specific to HTTPS_UPGRADE. CSP / content
extensions can also upgrade the URL. I think we should just use result.value()
instead of originalRequest() in this block.