[Webkit-unassigned] [Bug 192094] HTTPS Upgrade: Consult dummy storage for HTTPS Upgrade, Apply If Appropriate

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=192094

Chris Dumez <cdumez at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #355905|review?                     |review-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181128/071f8b6a/attachment.html>