<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body><span class="vcard"><a class="email" href="mailto:cdumez@apple.com" title="Chris Dumez <cdumez@apple.com>"> <span class="fn">Chris Dumez</span></a>
</span> changed
          <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - HTTPS Upgrade: Figure out if/how to tell clients that the HTTPS upgrade happened"
   href="https://bugs.webkit.org/show_bug.cgi?id=192375">bug 192375</a>
          <br>
             <table border="1" cellspacing="0" cellpadding="8">
          <tr>
            <th>What</th>
            <th>Removed</th>
            <th>Added</th>
          </tr>

         <tr>
           <td style="text-align:right;">Attachment #356527 Flags</td>
           <td>review?
           </td>
           <td>review-
           </td>
         </tr></table>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - HTTPS Upgrade: Figure out if/how to tell clients that the HTTPS upgrade happened"
   href="https://bugs.webkit.org/show_bug.cgi?id=192375#c4">Comment # 4</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - HTTPS Upgrade: Figure out if/how to tell clients that the HTTPS upgrade happened"
   href="https://bugs.webkit.org/show_bug.cgi?id=192375">bug 192375</a>
              from <span class="vcard"><a class="email" href="mailto:cdumez@apple.com" title="Chris Dumez <cdumez@apple.com>"> <span class="fn">Chris Dumez</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=356527&action=diff" name="attach_356527" title="Patch">attachment 356527</a> <a href="attachment.cgi?id=356527&action=edit" title="Patch">[details]</a></span>
Patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=356527&action=review">https://bugs.webkit.org/attachment.cgi?id=356527&action=review</a>

<span class="quote">> Source/WebKit/NetworkProcess/NetworkLoad.cpp:118
> +    if (!redirectCompletionHandler)</span >

Why is this needed?

<span class="quote">> Source/WebKit/NetworkProcess/NetworkLoad.h:69
> +    void willPerformHTTPRedirection(WebCore::ResourceResponse&&, WebCore::ResourceRequest&&, RedirectCompletionHandler&&) final;</span >

Why is this needed?

<span class="quote">> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:58
> +    : didUpgradeCallback()</span >

Not needed

<span class="quote">> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:232
> +    ResourceRequest oldRequest = request;</span >

We normally call this "originalRequest"

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

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

<span class="quote">> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:258
> +    , oldRequest, request, didUpgradeRequest</span >

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

<span class="quote">> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:272
> +#if ENABLE(HTTPS_UPGRADE)</span >

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.

<span class="quote">> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:273
> +        if (didUpgradeCallback && didUpgradeRequest) {</span >

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()

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

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

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

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

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

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) {

<span class="quote">> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:125
> +            ResourceResponse redirectResponse { };</span >

{ } is not needed.

<span class="quote">> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:128
> +            redirectResponse.setHTTPVersion("HTTP/1.1");</span >

"HTTP/1.1"_s

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

HTTPHeaderName::Location

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

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

<span class="quote">> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:135
> +            callback();</span >

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.

<span class="quote">> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:364
> +    m_networkLoadChecker->didUpgradeCallback = nullptr;</span >

Why is this needed?</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>