[webkit-reviews] review granted: [Bug 181946] [WebAuthN] Revisit the whole async model of task dispatching, timeout and aborting : [Attachment 333547] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Jiewen Tan <jiewen_tan at apple.com>'s
request for review:
Bug 181946: [WebAuthN] Revisit the whole async model of task dispatching,
timeout and aborting
https://bugs.webkit.org/show_bug.cgi?id=181946

Attachment 333547: Patch

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




--- Comment #14 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 333547
  --> https://bugs.webkit.org/attachment.cgi?id=333547
Patch

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

r=me with comments.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:35
> +static constexpr uint64_t MaxMessageId = 0xFFFFFFFFFFFFFF; // 56 bits

const maxMessageId = 0xFFFFFFFFFFFFFF; // 56 bits

No need for static since this is a global const variable. Also, I do not
believe we usually start global variable names with a capital letter.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:36
> +static constexpr size_t CallBackClassifierOffset = 56;

ditto.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:41
> +static constexpr size_t CredentialIdLengthOffset = 43;

ditto

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:42
> +static constexpr size_t CredentialIdLengthLength = 2;

ditto.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:51
> +	   auto handler = m_pendingCreationCompletionHandlers.take(messageId);

takeCreationCompletionHandler()?

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:56
> +	   auto handler = m_pendingRequestCompletionHandlers.take(messageId);

takeCreationCompletionHandler()?

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:72
> +class CredentialsMessenger {

WTF_MAKE_FAST_ALLOCATED;

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:86
> +static std::unique_ptr<Timer> initTimer(std::optional<unsigned long>
timeOutInMs, const Ref<DeferredPromise>& promise)

initTimeoutTimer()?

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:92
> +	   promise->reject(Exception { NotAllowedError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:94
> +    timer->startOneShot(Seconds(timeOutInMs.value() / 1000.0));

Seconds::fromMilliseconds(*timeOutInMs);

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:98
> +static bool didTimerFire(Timer* timer)

didTimeoutTimerFire()?

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:131
> +	   promise->reject(Exception { NotAllowedError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:136
> +    std::unique_ptr<Timer> timer = initTimer(options.timeout, promise);

timeoutTimer?

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:143
> +	   promise->reject(Exception { SecurityError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:153
> +	   promise->reject(Exception { NotSupportedError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:166
> +	   promise->reject(Exception { UnknownError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:174
> +	       promise->reject(Exception { AbortError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:198
> +	   promise->reject(Exception { NotAllowedError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:210
> +	   promise->reject(Exception { SecurityError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:225
> +	   promise->reject(Exception { UnknownError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:233
> +	       promise->reject(Exception { AbortError });

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/testing/MockCredentialsMessenger.cpp:60
> +	   exceptionReply(messageId, ExceptionData { NotAllowedError,
emptyString()});

Please provide a useful exception message instead of the empty string.

> Source/WebCore/testing/MockCredentialsMessenger.cpp:79
> +	   exceptionReply(messageId, ExceptionData { NotAllowedError,
emptyString()});

Please provide a useful exception message instead of the empty string.

> Source/WebCore/testing/MockCredentialsMessenger.h:40
> +    void timeOut() { m_didTimeOut = true; }

Confusing naming. I would suggest: markAsTimedOut(), setDidTimeOut().

> Source/WebCore/testing/MockCredentialsMessenger.h:41
> +    void userCancel() { m_didUserCancel = true; }

Confusing naming. I would suggest markAsCancelledByUser() or
setDidUserCancel().

> Source/WebCore/testing/MockCredentialsMessenger.h:45
> +    void ref() const { }

We usually handle this by ref'ing / derefing the owner, so:
1. Pass a Internals& to the constructor
2. Store it in a Internals m_internals; data member
3. Have ref() / deref() forward the calls to m_internals (i.e.
m_internals.ref() / m_internals.deref())

Note that you need to keep MockCredentialsMessenger as a unique_ptr inside
Internals.