[webkit-reviews] review granted: [Bug 189279] [WebAuthN] Make AuthenticatorManager : [Attachment 350759] 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 189279: [WebAuthN] Make AuthenticatorManager
https://bugs.webkit.org/show_bug.cgi?id=189279

Attachment 350759: Patch

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




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

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

r=me

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:57
> +    // FIXME: Maybe we should use safe casting.

Safe casting macros would still require this virtual function as it is so I am
not sure we need a FIXME comment here.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:83
> +    HashMap<Authenticator*, Ref<Authenticator>> m_authenticators;

Why isn't this a HashSet<Ref<Authenticator>> ?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:96
> +static inline bool emptyTransportsOrContain(const
Vector<AuthenticatorTransport>& transports, AuthenticatorTransport target)

emptyTransportsOrContains with an 's' would be better I think

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2387
> +    localValues.append(WKBooleanCreate(acceptAuthentication));

You need to adopt the value returned by WKBooleanCreate, otherwise you ref one
time too many.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2389
> +    localValues.append(WKBooleanCreate(acceptAttestation));

ditto.

> Tools/WebKitTestRunner/TestController.cpp:3179
> +bool TestController::keyExistedInKeychain(const String&, const String&)

Why keyExisted and not keyExists ? Isn't it checking if the key is present in
the keychain *now*?