We probably should combine AuthenticatorResponse with PublicKeyCredentialData, and customize AuthenticatorResponse more to include fields like numberOfCredentials from FIDO CTAP authenticators. Thus, AuthenticatorResponse will not serve merely for IDL interfaces.
The difficulty is that IPC can't serialize ref counted objects. Therefore, we might still need a separate AuthenticatorResponseData struct that can be passed around different processes.
<rdar://problem/57781183>
Created attachment 385239 [details] Patch
The layout test failure doesn't seem to be related: http/tests/cache-storage/page-cache-domcache-pending-promise.html.
Comment on attachment 385239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385239&action=review Looks good! r=me > Source/WebCore/ChangeLog:12 > + all different type of responses from authenticators anymore. For example, authenticatorGetNextAssertion all THE different > Source/WebCore/ChangeLog:13 > + depends on numberOfCredentials member from authenticatorGetAssertion response to function, but depends on THE numberOfCredentials ... > Source/WebCore/ChangeLog:14 > + numberOfCredentials is not used anywhere else. Therefore, a polymorphism type is needed to Therefore, a POLYMORPHIC type is needed to ... > Source/WebCore/ChangeLog:19 > + IPC. To solve this, AuthenticatorResponseData (PublicKeyCredentialData) is kept to be an intermediate type is kept AS an intermediate type ... > Source/WebCore/Modules/webauthn/AuthenticationExtensionsClientOutputs.h:30 > +#include <wtf/text/WTFString.h> Is WTFString actually needed here? No string appears in the header. Seems more likely we would need <wtf/Optional.h> > Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.cpp:45 > + RefPtr<ArrayBuffer> userhandle; I don't think it's wise to rely on case difference to distinguish between 'userhandle' and 'userHandle'. I suggest 'userHandleData' and 'userHandle'. > Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:55 > +// FIXME: Find a way to reduce the unnecessary copies. You should file a bugzilla or we will never remember to do this! > Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:55 > + void setClientDataJSON(Ref<ArrayBuffer>&&); Why isn't this inlined like 'setExtensions'? Seems like we should be consistent (all in the implementation file, or all in the header). > Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:2 > + * Copyright (C) 2018 Apple Inc. All rights reserved. 2019 maybe?
Comment on attachment 385239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385239&action=review Thanks Brent for r+ this patch. >> Source/WebCore/ChangeLog:12 >> + all different type of responses from authenticators anymore. For example, authenticatorGetNextAssertion > > all THE different Fixed. >> Source/WebCore/ChangeLog:13 >> + depends on numberOfCredentials member from authenticatorGetAssertion response to function, but > > depends on THE numberOfCredentials ... Fixed. >> Source/WebCore/ChangeLog:14 >> + numberOfCredentials is not used anywhere else. Therefore, a polymorphism type is needed to > > Therefore, a POLYMORPHIC type is needed to ... Fixed. >> Source/WebCore/ChangeLog:19 >> + IPC. To solve this, AuthenticatorResponseData (PublicKeyCredentialData) is kept to be an intermediate type > > is kept AS an intermediate type ... Fixed. >> Source/WebCore/Modules/webauthn/AuthenticationExtensionsClientOutputs.h:30 >> +#include <wtf/text/WTFString.h> > > Is WTFString actually needed here? No string appears in the header. > > Seems more likely we would need <wtf/Optional.h> Removed. >> Source/WebCore/Modules/webauthn/AuthenticatorAssertionResponse.cpp:45 >> + RefPtr<ArrayBuffer> userhandle; > > I don't think it's wise to rely on case difference to distinguish between 'userhandle' and 'userHandle'. > > I suggest 'userHandleData' and 'userHandle'. Right, I was somehow lazy. > Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:42 > +struct AuthenticatorResponseData; Moved to the top. >> Source/WebCore/Modules/webauthn/AuthenticatorResponse.cpp:55 >> +// FIXME: Find a way to reduce the unnecessary copies. > > You should file a bugzilla or we will never remember to do this! I just removed the FIXME actually as I don't think it worths optimizing. >> Source/WebCore/Modules/webauthn/AuthenticatorResponse.h:55 >> + void setClientDataJSON(Ref<ArrayBuffer>&&); > > Why isn't this inlined like 'setExtensions'? Seems like we should be consistent (all in the implementation file, or all in the header). The considerations here is to limit the access to certain methods given this is a private header. I moved all to the implementations and use WEBCORE_EXPORT for rawId and setExtensions. >> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:2 >> + * Copyright (C) 2018 Apple Inc. All rights reserved. > > 2019 maybe? Yes!
Created attachment 385441 [details] Patch for landing
Comment on attachment 385441 [details] Patch for landing Clearing flags on attachment: 385441 Committed r253398: <https://trac.webkit.org/changeset/253398>