[webkit-reviews] review granted: [Bug 203432] Expose basic ITP data from the database for future API/SPI use : [Attachment 384665] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted katherine_cheney at apple.com's
request for review:
Bug 203432: Expose basic ITP data from the database for future API/SPI use
https://bugs.webkit.org/show_bug.cgi?id=203432

Attachment 384665: Patch

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




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

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

Please address comments before landing.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:418
> +    return interactionTimeSeconds > 0 &&
WallTime::now().secondsSinceEpoch().value() - interactionTimeSeconds <
Seconds(24_h).value();

24_h is a Seconds object. Do we need to wrap it this way and call value()? If
not, please change before landing.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:789
> +Vector<ThirdPartyDataForSpecificFirstParty>
ResourceLoadStatisticsDatabaseStore::getFirstPartyDataForDomain(unsigned
domainID, const RegistrableDomain& domainString) const

I would prefer this function name to be aligned with the return type. Is the
domainString the third-party? Maybe we could make that clearer too? Anyway, I
trust you'll come up with a new function name parameter name that make sense
given what the function does and what it returns.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:2423
> +    return interactionTimeSeconds > 0 &&
WallTime::now().secondsSinceEpoch().value() - interactionTimeSeconds <
Seconds(24_h).value();

Same thing here with the Seconds(24_h) wrapping. Needed?

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1
12
> +static Vector<ThirdPartyDataForSpecificFirstParty>
getFirstPartyDataForDomain(const ResourceLoadStatistics& statistic)

Same thing here for the function name and parameter name.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:80
> +struct ThirdPartyDataForSpecificFirstParty {

Much better.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:86
> +	   return makeString(firstPartyDomain.string(), ", Has Granted Storage
Access: ", storageAccessGranted);

Hmm. Now I'm confused. The user grants storage access, neither the first or
third party. And the third-party requests storage access, not the first party.
So the first party is never involved in anything around requesting or granting
storage access. The data point should say whether or not a third party has been
granted storage access under a first party. So maybe a generated string saying
"Has been granted storage access under { first-party domain string } : {
boolean }." Unfortunately, this means regenerating expect files for your tests.