[webkit-reviews] review denied: [Bug 171217] Support removal of authentication data through the Website data store API. : [Attachment 311997] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Per Arne Vollan
<pvollan at apple.com>'s request for review:
Bug 171217: Support removal of authentication data through the Website data
store API.
https://bugs.webkit.org/show_bug.cgi?id=171217

Attachment 311997: Patch

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




--- Comment #32 from Brady Eidson <beidson at apple.com> ---
Comment on attachment 311997
  --> https://bugs.webkit.org/attachment.cgi?id=311997
Patch

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

> Source/WebCore/platform/network/CredentialStorage.h:60
> +    HashSet<String> originsWithCredentials() const { return
m_originsWithCredentials; }

This makes a copy of the set on every call.

const HashSet<String>&

> Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataRecord.h:60
> +/*! @constant WKWebsiteDataTypeCredentials Credentials, such as Basic
Authentication. */
> +WK_EXTERN NSString * const WKWebsiteDataTypeCredentials
WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> +

I don't think we intend to actually go through API review for this... do we?

If you're planning to, then fine... If not, this has to be moved to the private
header.

I do not think it's important to make this API (and, in fact, discussion in
this bug has been centered around *not* making it API for now)

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:208
> +void TestController::removeAllSessionCredentials()
> +{
> +#if WK_API_ENABLED
> +    [globalWebViewConfiguration.websiteDataStore
removeDataOfTypes:[WKWebsiteDataStore allWebsiteDataTypes]
modifiedSince:[NSDate distantPast] completionHandler:^() { }];
> +#endif
> +}

I'm very concerned about inherent flakiness in this test.

Since the UI process doesn't wait on the completion handler for this call, the
iframe that is loaded can definitely end up having the credentials still
applied.