[webkit-reviews] review denied: [Bug 181082] Update Credential Management API for WebAuthentication : [Attachment 330072] Part 2/2

[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 181082: Update Credential Management API for WebAuthentication
https://bugs.webkit.org/show_bug.cgi?id=181082

Attachment 330072: Part 2/2

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




--- Comment #11 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 330072
  --> https://bugs.webkit.org/attachment.cgi?id=330072
Part 2/2

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

I think this looks good, but I had some wording changes and a method name
change I'd like you to make. Also, can you please explain the std::optional
change I asked about in the notes below?

> Source/WebCore/ChangeLog:12
> +	   which is required by WebAuthN. Besides, it also sets the
CredentialManagement runtime flag to enable testing.

You don't need 'Besides' here.

> Source/WebCore/ChangeLog:13
> +	   Noted, it introduces a dummy PublicKeyCredential interface for
testing functionalities of the Credential interface,

'Noted, it introduces' should be 'Note that it introduces'

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:48
> +bool CredentialsContainer::isSameOriginWithItsAncestors()

I think this should be called 'doesHaveSameOriginAsItsAncestors()'

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
> +    auto task = [promiseIndex, weakThis, isSameOriginWithItsAncestors =
isSameOriginWithItsAncestors(), operation = WTFMove(operation)] () {

Should we be using WTFMove() on the weakThis?

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:85
> +    // FIXME: Optional options are passed with no contents. It should be
std::optional.

I don't understand this comment. You seem to be changing this code to pass
CredentialRequestOptions directly, from a std::optional argument.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:-41
> -    void get(std::optional<CredentialRequestOptions>,
DOMPromiseDeferred<IDLInterface<BasicCredential>>&&);

I don't get why you are making this change, if you want to go back to
std::optional. Can you clarify?

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.h:58
> +    bool isSameOriginWithItsAncestors();

I think this should be called 'doesHaveSameOriginAsItsAncestors()'

> Source/WebCore/Modules/credentialmanagement/NavigatorCredentials.cpp:46
> +	   m_credentialsContainer = CredentialsContainer::create(frame);

Can we have a CredentialsContainer with a nullptr frame? Is returning a
null-constructed CredentialsContainer appropriate?

> Source/WebCore/page/RuntimeEnabledFeatures.h:251
> +    bool m_isCredentialManagementEnabled { true };

I don't think we want to turn this on yet. Test cases should be able to flip
this switch on in the test case.

>
LayoutTests/imported/w3c/web-platform-tests/credential-management/idl.https-exp
ected.txt:19
> +PASS CredentialsContainer interface: navigator.credentials must inherit
property "preventSilentAccess()" with the proper type 

Yay!