[webkit-reviews] review granted: [Bug 172155] Resource Load Statistics: Grandfather domains for existing data records : [Attachment 310312] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 172155: Resource Load Statistics: Grandfather domains for existing data
records
https://bugs.webkit.org/show_bug.cgi?id=172155

Attachment 310312: Patch

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




--- Comment #13 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 310312
  --> https://bugs.webkit.org/attachment.cgi?id=310312
Patch

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

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:47
> +// 3 days in seconds
> +static auto grandfatheringTime = 259200;

static const auto secondsPerDay = 24 * 3600;
static auto grandfatheringTime = 3 * secondsPerDay;
etc.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:103
> +    if (version > 3) {

static const auto minimumVersionWithGrandfathering = 3;

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:346
> +    double now = currentTime();

This local variable seems excessive.

> Source/WebCore/loader/ResourceLoadStatisticsStore.h:94
> +    double m_endOfGrandfatheringTimestamp { 0 };
> +    double m_lastTimeDataRecordsWereRemoved { 0 };

double -> Seconds?

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:268
> +void
WebProcessProxy::topPrivatelyControlledDomainsWithWebiteData(OptionSet<WebsiteD
ataType> dataTypes, bool shouldNotifyPage,
std::function<void(HashSet<String>&&)> completionHandler)

I know we use std::function elsewhere in this file, but I don't think we should
keep adding std::function where we can use Function, even if it differs in
style from nearby code, unless it would require huge refactoring to not.

> Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsManager.h:-50
> -    WK_EXPORT void
WKResourceLoadStatisticsManagerSetMinimumTimeBetweeenDataRecordsRemoval(double
seconds);

I assume this has never been adopted so we don't need to keep it in for binary
compatibility with anything.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:526
> +	   for (auto&& dataRecord : existingDataRecords)

auto&&
cool!