[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

--- Comment #20 from Vivek Seth <v_seth at apple.com> ---
Comment on attachment 356844
  --> https://bugs.webkit.org/attachment.cgi?id=356844
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:226
>>> +    auto continueCheckingRequestOrAllowSimulatedRedirect = [this](ResourceRequest&& originalRequest, ResourceRequest&& currentRequest, ValidationHandler&& handler) {
>> 
>> continueCheckingRequestOrAllowSimulatedRedirect -> continueCheckingRequestOrDoSimulatedRedirect ?
> 
> How about making the lambda a method of NetworkLoadChecker.
> This remove the need to capture this (as who knows if 'this' is valid).

OK

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:228
>> +        if ((m_requestLoadType != LoadType::MainFrame) && (currentRequest.url() != originalRequest.url())) {
> 
> Isn't this supposed to be m_requestLoadType == LoadType::MainFrame ?
> 
> Also, we prefer mot to avoid unnecessary parentheses.

OK

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:238
>> +        RELEASE_LOG_IF_ALLOWED("checkRequest - Upgrade URL from HTTP to HTTPS");
> 
> I would tend to make applyHTTPSUpgradeIfNeeded return void.
> And put the RELEASE_LOG_IF_ALLOWED in applyHTTPSUpgradeIfNeeded.
> Depending on applyHTTPSUpgradeIfNeeded implementation, the ASSERT might be useful or not.

Yeah I suppose the assert doesn't add much value. I'll make the change you suggest.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:88
>> +    void setRequestLoadType(LoadType requestLoadType) { m_requestLoadType = requestLoadType; }
> 
> As I mentioned before, I think it may look better without a setter and passing it as a parameter with a default value.

Not sure I understand. Lets talk about it on Monday.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:148
>> +    bool applyHTTPSUpgradeIfNeeded(WebCore::ResourceRequest&) const;
> 
> Methods should come *before* data members for clarity.

ok

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:126
>> +            m_networkLoadChecker->setRequestLoadType(NetworkLoadChecker::LoadType::MainFrame);
> 
> I think we should set request load type in NetworkLoadChecker constructor since it stays the same for the lifetime of m_networkLoadChecker .
> This will remove the need for setRequestLoadType.

Lets talk about this on Monday.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:212
>> +            if (isMainFrameLoad() && (originalRequest().url() != currentRequest.url())) {
> 
> It is kind of sad that this check is duplicated from NetworkLoadChecker. I think it would look better it the check was done only in NetworkLoadChecker, and if we'd pass an extra "ResourceResponse redirect" to this lambda. We'd then have a if (!redirect.isNull()) check here instead.

What makes this tricky is that ValidationHandler is called in 20 separate places in NetworkLoadChecker and we probably don't won't to modify all of the call sites to add an extra param to the lambda for a special case. I'll experiment with a few ideas, but I might need to speak to you about this on Monday.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:216
>> +                m_isWaitingContinueWillSendRequestForCachedRedirect = true;
> 
> I think this is ok to keep the name m_isWaitingContinueWillSendRequestForCachedRedirect even though it is not exactly correct anymore.

OK sounds good.

-- 
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/20181209/7d407e53/attachment-0001.html>