[webkit-reviews] review denied: [Bug 182771] [WebAuthN] Implement PublicKeyCredential.isUserVerifyingPlatformAuthenticatorAvailable() : [Attachment 333769] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 182771: [WebAuthN] Implement
PublicKeyCredential.isUserVerifyingPlatformAuthenticatorAvailable()
https://bugs.webkit.org/show_bug.cgi?id=182771

Attachment 333769: Patch

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




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

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

This looks good, but I think you should try to implement on macOS as well. The
SDKs exist! r- because of lack of macOS support.

> Source/WebCore/ChangeLog:13
> +	   Besides, it changes DeferredPromise to DOMPromiseDeferred<> for all
CredentialsManagement and

I would say, "In addition, it changes ..."

> Source/WebKit/ChangeLog:9
> +	   This patch uitlizes LocalAuthentication Framework to determine if
biometrics

utilizes

> Source/WebKit/ChangeLog:14
> +	   Corresponding macOS implementations are marked as unimplemented as a
result.

I don't think this is true.
<https://developer.apple.com/documentation/localauthentication?language=objc>
claims that it is present in iOS 8+ and macOS 10.10+

>
Source/WebKit/UIProcess/CredentialManagement/ios/WebCredentialsMessengerProxyIO
S.mm:44
> +	   LOG_ERROR("Couldn't evaluate policy: %@", error);

This might be better as "Couldn't evaluate authentication with biometrics
policy: %@"

>
Source/WebKit/UIProcess/CredentialManagement/mac/WebCredentialsMessengerProxyMa
c.mm:37
> +    notImplemented();

I think this can be implemented, based on
<https://developer.apple.com/documentation/localauthentication?language=objc>.