[webkit-reviews] review denied: [Bug 177105] [Curl] Move Authentication related tasks into AuthenticationChallenge class : [Attachment 321147] PATCH

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 177105: [Curl] Move Authentication related tasks into
AuthenticationChallenge class
https://bugs.webkit.org/show_bug.cgi?id=177105

Attachment 321147: PATCH

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




--- Comment #2 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 321147
  --> https://bugs.webkit.org/attachment.cgi?id=321147
PATCH

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

> Source/WebCore/platform/network/curl/AuthenticationChallenge.h:53
>      RefPtr<AuthenticationClient> m_authenticationClient;

FYI, we're kind of moving away from each challenging having a client in favor
of a challenge being received with a completion handler so the challenge itself
doesn't know what is to be done with it.  So in a hopefully-not-too-distant
future once everyone has adopted the NetworkSession abstraction which is
similar to the async ResourceHandleClient callbacks and we've removed
ResourceHandle in favor of everything being asynchronous, we will organize it
more like that.  This is fine for now, though.

> Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:69
> +    int realmPos = authHeader.find(realmString);
> +    if (realmPos > 0) {

find returns a size_t, and we should check it against notFound.

> Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.h:77
> +    void didReceiveAllHeaders(long httpCode, long long contentLength, long
connectPort, long availableHttpAuth);

ports should be uint16_t or std::optional<uint16_t> if the underlying logic
knows to use the default port for the protocol if there is none.