[webkit-reviews] review denied: [Bug 171217] Support removal of authentication data through the WebKit API. : [Attachment 308381] 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 WebKit API.
https://bugs.webkit.org/show_bug.cgi?id=171217

Attachment 308381: Patch

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




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

The patch doesn't message the WebProcesses. While most credential storage
operations are in the networking process, sadly there's still a bit left in
WebProcesses as well.

It also can't be correct because it operates on the global session ID. Note,
this came up in earlier comments but was never addressed.

The testing is also problematic. The API tests are designed to work offline,
yet the added test hits a public URL. Additionally it doesn't attempt to do any
validation on credentials actually being sent to the server. The test doesn't
actually verify the behavior that is important; That the credentials were
cleared.

I think:
- "Session Credentials" can be a new type on WebsiteDataStore, and that's where
it should be exposed in SPI
- As long as there's any doubt that WebsiteDataStore is the wrong place, I
still think that's fine as the C-API is SPI, and we'll learn whether it was
right or not before adding anything to the Cocoa API (but I think it's right)
- This needs to go to the WebProcess(es) in addition to Networking
- To that effect, the "Web" prefix should be dropped from the CredentialManager
objects and the receiver-side should be moved to Shared.