Bug 177105 – [Curl] Move Authentication related tasks into AuthenticationChallenge class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Basuke Suzuki]

Currently those codes are in ResourceHandle or related companion. It will be reused with NetworkLoadTask so that it should be separated from them.

[Basuke Suzuki]

Created attachment 321147 [details]
PATCH

[Alex Christensen]

Comment on attachment 321147 [details]
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.

[Basuke Suzuki]

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

Fine for us right now. We understand the direction and that further changes are waiting.


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

my bad.


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

Okay for our code base, but eventually we will create a ProtectionSpace object, which is the world where `port` is `int`. We don't want to change them with this patch.

[Basuke Suzuki]

Created attachment 321233 [details]
fix

[Basuke Suzuki]

I cannot handle this test failure. https://webkit-queues.webkit.org/patch/321233/commit-queue

[WebKit Commit Bot]

Comment on attachment 321233 [details]
fix

Clearing flags on attachment: 321233

Committed r222223: <http://trac.webkit.org/changeset/222223>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693240>