[webkit-reviews] review denied: [Bug 192375] HTTPS Upgrade: Figure out if/how to tell clients that the HTTPS upgrade happened : [Attachment 356527] 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 192375: HTTPS Upgrade: Figure out if/how to tell clients that the HTTPS
upgrade happened
https://bugs.webkit.org/show_bug.cgi?id=192375

Attachment 356527: Patch

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




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