[webkit-reviews] review denied: [Bug 171217] Support removal of authentication data through the Website data store API. : [Attachment 310654] 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 310654: Patch

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




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

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

Thinking it was very odd that this patch adds a deleter without a completion
handler, I dug in to the history of WKWebsiteDataStoreRef.

It was added in r181707 and its sole purpose was to provide a toll-free bridge
from WKContextRef to WKWebsiteDataStore (the Cocoa API object) It never
should've had any API/SPI added to it. In fact some of the methods there
(rightfully) have zero clients.

In my 5-3 comment I suggested adding it to the C-SPI was fine, but I
misunderstood the history of WebsiteDataStoreRef - It never predated
WKWebsiteDataStore and never had any API.

The rest of my 5-3 comment applies. You'll need to add this to
WKWebsiteDataStore.

Look at WKWebsiteDataRecord.h and WKWebsiteDataRecordPrivate.h to see how the
types are exposed - They're just strings, and you'll need to add a new string
type.

Additionally, it's not okay to *only* expose a deleter. WKWebsiteDataStore
supports Safari's privacy UI, and that UI needs to accurately reflect when a
site has data stored against it. In other words, you'll need to add the getters
as well.

> Source/WebKit2/UIProcess/API/C/WKCredential.cpp:-55
>  {
>      return toCopiedAPI(toImpl(credentialRef)->credential().user());
>  }
> -

This does not need to be in this patch.