[webkit-reviews] review granted: [Bug 188623] [WebAuthN] Support CTAP HID authenticators on macOS : [Attachment 354607] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 188623: [WebAuthN] Support CTAP HID authenticators on macOS
https://bugs.webkit.org/show_bug.cgi?id=188623

Attachment 354607: Patch

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




--- Comment #9 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 354607
  --> https://bugs.webkit.org/attachment.cgi?id=354607
Patch

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

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:598
> +    if (hidRef) {

I think this would be better as:

if (auto hidRef = .... stuff...) {

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:613
> +

Should probably just be one blank line here.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:629
> +	   if (payloadBase64)

if (auto payloadBase64 = ... stuff ... ) {

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:633
> +	   if (keepAlive)

Ditto one-line format

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:637
> +	   if (fastDataArrival)

Ditto

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:641
> +	   if (continueAfterErrorData)

Ditto

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:58
> +	   ASSERT_UNUSED(addResult, addResult.isNewEntry);

Is there any reason this is macOS only? Seems like gtk or Windows might be able
to use this code if they had relevant HID code.

Maybe we should have a "#if USE(HID)" or something to represent this, rather
than OS-specific checks?

This would be fine to do as a separate patch.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:65
>      void clearState();

Should clearState ever be called directly? Maybe it should be private.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:75
> +    m_initialized = true;

Seems like we could have a PAL layer for HID handling, then this code could be
shared across ports.

Note: Not for the current patch though -- just an idea for a future cleanup.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:89
> +    auto task = BlockPtr<void()>::fromCallable([device = m_device, data =
WTFMove(data), callback = WTFMove(callback)]() mutable {

Do we want device to retain m_device? Or should it be a weakptr?

>
LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-hid-silent.h
ttps-expected.txt:5
> +PASS PublicKeyCredential's [[create]] with mixed options in a mock hid
authenticator. 

Nice. Web Platform Tests for this!