[webkit-reviews] review granted: [Bug 210864] Removing website data for a domain should delete corresponding ITP entry : [Attachment 397402] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted katherine_cheney at apple.com's
request for review:
Bug 210864: Removing website data for a domain should delete corresponding ITP
entry
https://bugs.webkit.org/show_bug.cgi?id=210864

Attachment 397402: Patch

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




--- Comment #3 from John Wilander <wilander at apple.com> ---
Comment on attachment 397402
  --> https://bugs.webkit.org/attachment.cgi?id=397402
Patch

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

r=me with comments.

> Source/WebKit/ChangeLog:18
> +	   in the database due to cascading deletions.

Does it also delete for the plist-based store? If so, the terms differ there.
No domainID.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:2252
> +    SQLiteStatement removeAllData(m_database, "DELETE FROM ObservedDomains
WHERE domainID = ?");

Do we typically inline these static strings? Or should it be declared somewhere
else? If here, should we use _s?

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:2909
> +    SQLiteStatement linkDecorationStatement(m_database, "SELECT EXISTS
(SELECT * FROM TopFrameLinkDecorationsFrom WHERE toDomainID = ? OR fromDomainID
= ?)");

Ditto for these strings.

> Source/WebKit/NetworkProcess/NetworkProcess.h:403
> +    void deleteWebsiteDataForOrigins(PAL::SessionID,
OptionSet<WebsiteDataType>, const Vector<WebCore::SecurityOriginData>& origins,
const Vector<String>& cookieHostNames, const Vector<String>&
HSTSCacheHostnames, const Vector<RegistrableDomain>& registrableDomains,
CallbackID);

Doesn't need the parameter name registrableDomains.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:69
> +void WKWebsiteDataStoreRemoveITPDataForDomain(WKWebsiteDataStoreRef
dataStoreRef, WKStringRef host, void* context,
WKWebsiteDataStoreRemoveITPDataForDomainFunction callback)

We should check if this API should be made SPI since clients could call this to
exempt specific domains from ITP treatment.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:110
> +    void deleteWebsiteDataForOrigins(PAL::SessionID,
OptionSet<WebKit::WebsiteDataType>, const Vector<WebCore::SecurityOriginData>&
origins, const Vector<String>& cookieHostNames, const Vector<String>&
HSTSCacheHostNames, const Vector<RegistrableDomain>& registrableDomains,
CompletionHandler<void()>&&);

Doesn't need the parameter name registrableDomains.

> Tools/ChangeLog:9
> +	   Created 2 new SPIs for testing. One to mimic clearing website data

I think they are C APIs.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:296
> +    boolean domainIDExistsInDatabase(unsigned long domainID);

The "statistics" term was added to these function names as a weak namespace
since it's all a flat set of functions under testRunner. We should probably
stick to it but then rename it to ITP once we rename ResourceLoadStatistics in
source code. Or we could figure out a way to have testRunner.itp.* functions.