[webkit-reviews] review granted: [Bug 189668] Clean up AuthenticationChallengeProxy : [Attachment 349934] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 189668: Clean up AuthenticationChallengeProxy
https://bugs.webkit.org/show_bug.cgi?id=189668

Attachment 349934: Patch

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




--- Comment #10 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 349934
  --> https://bugs.webkit.org/attachment.cgi?id=349934
Patch

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

> Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp:101
> +	  
challenge->listener().useCredential(WebCredential::create(credential).ptr());

Preexisting code but it seems a bit weird to wrap the WebCore credential into a
WebCredential one.
Could the listener directly take a WebCore::Credential*?
If it exposes this API, I guess WebCredential makes sense. Maybe it could also
directly take a WebCore::credential overload.

> Source/WebKit/UIProcess/Authentication/AuthenticationChallengeProxy.cpp:45
>
+AuthenticationChallengeProxy::AuthenticationChallengeProxy(WebCore::Authentica
tionChallenge&& authenticationChallenge, uint64_t challengeID, IPC::Connection*
connection, WeakPtr<SecKeyProxyStore>&& secKeyProxyStore)

A null connection does not make a lot of sense probably, could be a Connection&
probably.
Since it is refed by the lambda, it could be made a Ref<>&&.

> Source/WebKit/UIProcess/Authentication/AuthenticationChallengeProxy.cpp:47
> +    , m_listener(AuthenticationDecisionListener::create([challengeID,
connection = makeRefPtr(connection), secKeyProxyStore =
WTFMove(secKeyProxyStore)](AuthenticationChallengeDisposition disposition,
WebCredential* credential) {

I am not sure how readable that is to make a big lambda in the constructor.
Here it seems to make sense to take a WebCore credential.

> Source/WebKit/UIProcess/Authentication/AuthenticationChallengeProxy.cpp:69
> +	       }

Previously in the code, if m_secKeyProxyStore is null, we were sending
ContinueWithoutCredentialForChallenge.
Now we are sending UseCredentialForChallenge even though we might not send the
client certificate.
Is there no change of behavior? Should we assert that secKeyProxyStore is not
null if having ProtectionSpaceAuthenticationSchemeClientCertificateRequested?

> Source/WebKit/UIProcess/Authentication/AuthenticationDecisionListener.cpp:52
> +    if (m_completionHandler)

Can we have normal cases where we would call useCredential() and
m_completionHandler be null?
Should we assert?

> Source/WebKit/UIProcess/Authentication/AuthenticationDecisionListener.cpp:53
> +	  
m_completionHandler(AuthenticationChallengeDisposition::UseCredential,
credential);

By going with the completion handler, we end-up with an unneeded switch in the
completion handler, slight loss of efficiency here.

It might be slightly clearer to have a ContinueWithoutCredential in case
credential is nullptr, or maybe surface a continueWithoutCredential so that we
would pass a Credential& here.

> Source/WebKit/UIProcess/Authentication/AuthenticationDecisionListener.cpp:59
> +	   m_completionHandler(AuthenticationChallengeDisposition::Cancel,
nullptr);

With Expected<Credential*, Action>, we would not need this nullptr, here and
below.
But we probably more often end up without credential than with credentials.

> Source/WebKit/UIProcess/Authentication/AuthenticationDecisionListener.h:31
>  #include <wtf/RefPtr.h>

Is RefPtr.h include needed?

>
Source/WebKit/UIProcess/Authentication/cocoa/AuthenticationChallengeProxyCocoa.
mm:39
> +void
AuthenticationChallengeProxy::sendClientCertificateCredentialOverXpc(IPC::Conne
ction* connection, SecKeyProxyStore& secKeyProxyStore, uint64_t challengeID,
const WebCore::Credential& credential)

Connection& probably.