[Webkit-unassigned] [Bug 192375] HTTPS Upgrade: Figure out if/how to tell clients that the HTTPS upgrade happened

[bugzilla-daemon at webkit.org]

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

Chris Dumez <cdumez at apple.com> changed:

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

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

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

> Source/WebKit/NetworkProcess/NetworkLoad.cpp:118
> +    if (!redirectCompletionHandler)

Why is this needed?

> Source/WebKit/NetworkProcess/NetworkLoad.h:69
> +    void willPerformHTTPRedirection(WebCore::ResourceResponse&&, WebCore::ResourceRequest&&, RedirectCompletionHandler&&) final;

Why is this needed?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:58
> +    : didUpgradeCallback()

Not needed

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:232
> +    ResourceRequest oldRequest = request;

We normally call this "originalRequest"

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:235
> +    if (didUpgradeCallback && request.requester() == ResourceRequest::Requester::Main) {

Seems like the "request.requester() == ResourceRequest::Requester::Main" check could be inside applyHTTPSUpgradeIfNeeded().

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:258
> +    , oldRequest, request, didUpgradeRequest

Why are you capturing the request here? Also this is wrong since request is WTFMove()'d out on the same line.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:272
> +#if ENABLE(HTTPS_UPGRADE)

I do not think this part needs to be HTTPS_UPGRADE specific. I think we should simulate a redirect for all upgrades so that the Safari URL bar is up-to-date.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:273
> +        if (didUpgradeCallback && didUpgradeRequest) {

didUpgradeRequest can be replaced by checking if originalRequest's URL and result->value()'s request are different.

We may want to rename didUpgradeCallback() to didUpdateRequestURL()

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:274
> +            didUpgradeCallback(oldRequest, request, [this, request = WTFMove(result.value().request), handler = WTFMove(handler)]() mutable {

Why is this using request and not result->request?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:88
> +    WTF::Function<void(WebCore::ResourceRequest, WebCore::ResourceRequest, WTF::Function<void(void)>)> didUpgradeCallback;

Why is this public? This should be a private data member.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:124
> +        m_networkLoadChecker->didUpgradeCallback = [this](ResourceRequest originalRequest, ResourceRequest currentRequest, WTF::Function<void(void)> callback) {

Needs to be a proper setter. Setting a member like this is bad practice. Also parameters should probably not be passed by value:
[this](const ResourceRequest& originalRequest, const ResourceRequest& currentRequest, WTF::Function<void(void)>&& callback) {

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:125
> +            ResourceResponse redirectResponse { };

{ } is not needed.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:128
> +            redirectResponse.setHTTPVersion("HTTP/1.1");

"HTTP/1.1"_s

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:129
> +            redirectResponse.setHTTPHeaderField(String("Location"), currentRequest.url().string());

HTTPHeaderName::Location

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:130
> +            redirectResponse.setHTTPHeaderField(String("Cache-Control"), String("no-store"));

HTTPHeaderName::CacheControl
String("no-store") -> "no-store"_s

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:135
> +            callback();

Seems wrong to call the callback before continueWillSendRequest() has been called. 

Network process sends WillSendRequest IPC to the WebProcess with the proposed request
WebProcess may change the request
WebProcess sends ContinueWillSendRequest IPC back to the network process with the final URL.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:364
> +    m_networkLoadChecker->didUpgradeCallback = nullptr;

Why is this needed?

-- 
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/20181204/6a96086f/attachment-0001.html>