Bug 167474 – Updates to Resource Load Statistics: Get the right website data store and introduce timeout for user interaction

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

Updates to Resource Load Statistics:
1. Get the right website data store. API::WebsiteDataStore::defaultDataStore() does not provide the right data store.
2. Introduce timeout for user interaction. A domain needs interaction every 30 days to stay in that category.
3. Add grandfathered to the statistics model in preparation for grandfathering of existing data records.

[John Wilander]

Created attachment 299881 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/30290270>

[John Wilander]

Created attachment 300260 [details]
Patch

[John Wilander]

Created attachment 300275 [details]
Patch

[John Wilander]

Builds fine on my machine. Made some changes to the lambda captures that seemed to cause EWS build errors.

[John Wilander]

Created attachment 300339 [details]
Patch

[John Wilander]

Fixed a bug in how user interaction timestamps were merged. There are three states – recent user interaction, never seen user interaction in this process, and seen but reset user interaction.

[Alex Christensen]

Comment on attachment 300339 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:43
> +// 30 days in seconds
> +static const auto timeToLiveUserInteraction = 2592000;

We really need a way to test this.  I don't want to have to wait 30 days to reproduce bugs.

[John Wilander]

Created attachment 301025 [details]
Patch

[John Wilander]

Created attachment 301067 [details]
Patch

[John Wilander]

New patch that adds a missing file to the .cmake files for GTK, Efl, and Mac. If all goes green, please review.

[John Wilander]

Created attachment 301073 [details]
Patch

[John Wilander]

Added another file reference to the .cmake files.

[Brent Fulgham]

Comment on attachment 301073 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:62
> +    // May be null

I don't think this comment is necessary. If it's not possible for something to be null, we should return a Ref<>. RefPtr<> implies it can be null.

> Source/WebCore/loader/ResourceLoadObserver.cpp:307
> +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));

If 'm_store' can be null, we should null check here, or at least ASSERT.

> Source/WebCore/loader/ResourceLoadObserver.cpp:316
> +void ResourceLoadObserver::logUserInteraction(const URL url)

This should be passed by reference: const URL& url.

> Source/WebCore/loader/ResourceLoadObserver.cpp:326
> +void ResourceLoadObserver::clearUserInteraction(const URL url)

Ditto URL -> URL&

> Source/WebCore/loader/ResourceLoadObserver.cpp:331
> +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));

If m_store can be null, check. Or, ASSERT if we absolutely know we can't be in that state when these methods get called.

> Source/WebCore/loader/ResourceLoadObserver.cpp:337
> +bool ResourceLoadObserver::hasHadUserInteraction(const URL url)

Ditto URL&

> Source/WebCore/loader/ResourceLoadObserver.cpp:342
> +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));

Ditto comments about null m_store.

> Source/WebCore/loader/ResourceLoadObserver.cpp:347
> +void ResourceLoadObserver::setPrevalentResource(const URL url)

Ditto URL&

> Source/WebCore/loader/ResourceLoadObserver.cpp:352
> +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));

Ditto comments about null m_store.

> Source/WebCore/loader/ResourceLoadObserver.cpp:357
> +bool ResourceLoadObserver::isPrevalentResource(const URL url)

Ditto URL&

> Source/WebCore/loader/ResourceLoadObserver.cpp:362
> +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));

Ditto comments about null m_store.

> Source/WebCore/loader/ResourceLoadObserver.cpp:367
> +void ResourceLoadObserver::clearPrevalentResource(const URL url)

Ditto URL&

> Source/WebCore/loader/ResourceLoadObserver.cpp:372
> +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));

Ditto comments about null m_store.

> Source/WebCore/loader/ResourceLoadObserver.cpp:379
> +    m_store->setTimeToLiveUserInteraction(seconds);

Ditto comments about null m_store.

> Source/WebCore/loader/ResourceLoadObserver.cpp:384
> +    m_store->fireDataModificationHandler();

Ditto comments about null m_store.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:316
> +        if (other.mostRecentUserInteraction == 0.0) {

We usually test against 0 (not 0.0), and usually be using something like:  "if (!other.mostRecentUserInteraction)".

If you are getting very small floating point values in mostRecentUserInteraction, you should use the WTF MathExtras "areEssentiallyEqual" macro.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:213
> +            pendingCallbacks++;

We prefer ++pendingCallbacks

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:237
> +        if (page.value->websiteDataStore().isPersistent()) {

This might be easier to read if you did:

if (!page.value->websiteDataStore().isPersistent())
    continue;

This would allow you to reduce the level of indenting.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsManager.cpp:38
> +void WebResourceLoadStatisticsManager::setPrevalentResource(String hostName, bool value)

All of these String objects should be passed by const&.

> Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsManager.h:47
> +    WK_EXPORT void WKResourceLoadStatisticsManagerResetToConsistentState();

These aren't meant to be API, so we might need to prefix them with underscores or something. Alex will know!

[Andy Estes]

Comment on attachment 301073 [details]
Patch

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

r=me.

I have a bunch of optional comments, but the one thing you do need to address before landing is using WKStringRef instead of WTF::String in the new C API.

> Source/WebCore/loader/ResourceLoadObserver.cpp:52
> +static auto timestampResolution = 86400;

This is fine for now, but I'd prefer to see WTF::Seconds used here.

If you added fromDays() and days() functions to the Seconds class, then you could write this as "static auto timestampResolution = Seconds::fromDays(1);" and your comment would be unnecessary.

>> Source/WebCore/loader/ResourceLoadObserver.cpp:307
>> +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));
> 
> If 'm_store' can be null, we should null check here, or at least ASSERT.

If m_store can't be null here then an assertion wouldn't add much, in my opinion. A crash due to dereferencing a null pointer provides the same information as an assertion failure.

> Source/WebCore/loader/ResourceLoadObserver.cpp:313
> +    if (newTimestamp != statistics.mostRecentUserInteraction) {
> +        statistics.hadUserInteraction = true;
> +        statistics.mostRecentUserInteraction = newTimestamp;
> +        m_store->fireDataModificationHandler();
> +    }

I think this would read better if you instead returned early when newTimestamp == statistics.mostRecentUserInteraction.

>> Source/WebCore/loader/ResourceLoadStatistics.cpp:316
>> +        if (other.mostRecentUserInteraction == 0.0) {
> 
> We usually test against 0 (not 0.0), and usually be using something like:  "if (!other.mostRecentUserInteraction)".
> 
> If you are getting very small floating point values in mostRecentUserInteraction, you should use the WTF MathExtras "areEssentiallyEqual" macro.

I'd also combine this with the if statement above to reduce indentation: if (!other.hadUserInteraction && !other.mostRecentInteraction) {}

> Source/WebCore/loader/ResourceLoadStatistics.h:57
> +    double mostRecentUserInteraction { -1.0 };

This should be -1 instead of -1.0.

Having a special default values is fine for now, but I'd prefer to see you use WTF::Optional.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:43
> +// 30 days in seconds
> +static auto timeToLiveUserInteraction = 2592000;

See comment about using WTF::Seconds.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:167
> +    if (currentTime() > (resourceStatistic.mostRecentUserInteraction + timeToLiveUserInteraction)) {

No need for parentheses around the right operand of >.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:171
> +        resourceStatistic.mostRecentUserInteraction = 0.0;

0 instead of 0.0.

> Source/WebKit2/PlatformEfl.cmake:71
> +    UIProcess/API/C/WKResourceLoadStatisticsManager.cpp
> +

This should be inserted in sorted order.

> Source/WebKit2/PlatformGTK.cmake:97
> +    UIProcess/API/C/WKResourceLoadStatisticsManager.cpp
> +

Ditto.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:203
> +void WebProcessProxy::deleteWebsiteDataForTopPrivatelyOwnedDomainsInAllPersistentDataStores(OptionSet<WebsiteDataType> dataTypes, Vector<String>& topPrivatelyOwnedDomains, bool shouldNotifyPage, std::function<void()> completionHandler)

completionHandler's type should be WTF::Function<void()>&&.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:205
> +    struct CallbackAggregator : ThreadSafeRefCounted<CallbackAggregator> {

This is fine for now, but {add,remove}PendingCallback() is really just an explicit duplication of the existing ref-counting. You could instead have a destructor that calls completionHandler, since callbackAggregator will be destroyed when the last removeDataForTopPrivatelyOwnedDomains() completion handler finishes executing.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:206
> +        explicit CallbackAggregator(std::function<void()> completionHandler)

Ditto.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:227
> +                RunLoop::main().dispatch(WTFMove(completionHandler));

removePendingCallback() is already called on the main thread, so is this necessary?

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:231
> +        std::function<void()> completionHandler;

Let's use WTF::Function instead of std::function.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:49
> +static auto minimumTimeBetweeenDataRecordsRemoval = 60;

Another good use for WTF::Seconds.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:52
> +static bool notifyPages = false;
> +static bool shouldClassifyResourcesBeforeDataRecordsRemoval = true;

auto works here too.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:133
> +        && now < (m_lastTimeDataRecordsWereRemoved + minimumTimeBetweeenDataRecordsRemoval))

No need for parentheses around the right operand of <.

> Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsManager.h:40
> +    WK_EXPORT void WKResourceLoadStatisticsManagerSetPrevalentResource(String hostName, bool value);
> +    WK_EXPORT bool WKResourceLoadStatisticsManagerIsPrevalentResource(String hostName);
> +    WK_EXPORT void WKResourceLoadStatisticsManagerSetHasHadUserInteraction(String hostName, bool value);
> +    WK_EXPORT bool WKResourceLoadStatisticsManagerIsHasHadUserInteraction(String hostName);

We need to use WKStringRef in the API, not WTF::String.

>> Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsManager.h:47
>> +    WK_EXPORT void WKResourceLoadStatisticsManagerResetToConsistentState();
> 
> These aren't meant to be API, so we might need to prefix them with underscores or something. Alex will know!

None of the C API is public, so no need for underscores.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:476
> +void WebsiteDataStore::fetchDataForTopPrivatelyOwnedDomains(OptionSet<WebsiteDataType> dataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, const Vector<String>& topPrivatelyOwnedDomains, std::function<void(Vector<WebsiteDataRecord>)> completionHandler)

completionHandler should be a Function<...>&&.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:478
> +    fetchData(dataTypes, fetchOptions, [topPrivatelyOwnedDomains, completionHandler, this](auto existingDataRecords) {

you should WTFMove completionHandler in the capture list (completionHandler = WTFMove(completionHandler).

It'd be a little better if existingDataRecords was passed as an rvalue reference, but this is fine for now.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:489
> +                    if (dataRecordHost.endsWithIgnoringASCIICase(topPrivatelyOwnedDomain)) {
> +                        auto suffixStart = dataRecordHost.length() - topPrivatelyOwnedDomain.length();
> +                        if (!suffixStart || dataRecordHost[suffixStart - 1] == '.') {

I believe this check is done in a few places now. It'd be nice if we had a helper function for this.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1030
> +void WebsiteDataStore::removeDataForTopPrivatelyOwnedDomains(OptionSet<WebsiteDataType> dataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, const Vector<String>& topPrivatelyOwnedDomains, std::function<void()> completionHandler)

Ditto about using WTF::Function<...>&& instead.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1033
> +        this->removeData(dataTypes, websiteDataRecords, [completionHandler]() {

Ditto about WTFMoving completionHandler in the capture list.

[John Wilander]

Created attachment 301246 [details]
Patch

[John Wilander]

Thanks, Brent! See details below.

(In reply to comment #14)
> Comment on attachment 301073 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301073&action=review
> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:62
> > +    // May be null
> 
> I don't think this comment is necessary. If it's not possible for something
> to be null, we should return a Ref<>. RefPtr<> implies it can be null.

Agreed. I added an ASSERT here to catch any access to this when it's null.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:307
> > +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));
> 
> If 'm_store' can be null, we should null check here, or at least ASSERT.

I agree with Andy's point that a null pointer crash will suffice on debug builds. The reason I put the one ASSERT on the getter is that the class in not in control of how that pointer is used so any crash might happen much later.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:316
> > +void ResourceLoadObserver::logUserInteraction(const URL url)
> 
> This should be passed by reference: const URL& url.

Fixed.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:326
> > +void ResourceLoadObserver::clearUserInteraction(const URL url)
> 
> Ditto URL -> URL&

Fixed.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:331
> > +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));
> 
> If m_store can be null, check. Or, ASSERT if we absolutely know we can't be
> in that state when these methods get called.

See above.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:337
> > +bool ResourceLoadObserver::hasHadUserInteraction(const URL url)
> 
> Ditto URL&

Fixed.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:342
> > +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));
> 
> Ditto comments about null m_store.

See above.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:347
> > +void ResourceLoadObserver::setPrevalentResource(const URL url)
> 
> Ditto URL&

Fixed.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:352
> > +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));
> 
> Ditto comments about null m_store.

See above.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:357
> > +bool ResourceLoadObserver::isPrevalentResource(const URL url)
> 
> Ditto URL&

Fixed.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:362
> > +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));
> 
> Ditto comments about null m_store.

See above.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:367
> > +void ResourceLoadObserver::clearPrevalentResource(const URL url)
> 
> Ditto URL&

Fixed.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:372
> > +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));
> 
> Ditto comments about null m_store.

See above.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:379
> > +    m_store->setTimeToLiveUserInteraction(seconds);
> 
> Ditto comments about null m_store.

See above.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:384
> > +    m_store->fireDataModificationHandler();
> 
> Ditto comments about null m_store.

See above.

> > Source/WebCore/loader/ResourceLoadStatistics.cpp:316
> > +        if (other.mostRecentUserInteraction == 0.0) {
> 
> We usually test against 0 (not 0.0), and usually be using something like: 
> "if (!other.mostRecentUserInteraction)".

Fixed.

> If you are getting very small floating point values in
> mostRecentUserInteraction, you should use the WTF MathExtras
> "areEssentiallyEqual" macro.

I don't think it applies here but it may do in another comparison. I will revisit.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:213
> > +            pendingCallbacks++;
> 
> We prefer ++pendingCallbacks

Fixed.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:237
> > +        if (page.value->websiteDataStore().isPersistent()) {
> 
> This might be easier to read if you did:
> 
> if (!page.value->websiteDataStore().isPersistent())
>     continue;

Fixed.

> This would allow you to reduce the level of indenting.
> 
> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsManager.cpp:38
> > +void WebResourceLoadStatisticsManager::setPrevalentResource(String hostName, bool value)
> 
> All of these String objects should be passed by const&.

Fixed.

> > Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsManager.h:47
> > +    WK_EXPORT void WKResourceLoadStatisticsManagerResetToConsistentState();
> 
> These aren't meant to be API, so we might need to prefix them with
> underscores or something. Alex will know!

As per Andy's comment I left it as is.

Again, thanks for the review!

[John Wilander]

(In reply to comment #15)
> Comment on attachment 301073 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301073&action=review
> 
> r=me.
> 
> I have a bunch of optional comments, but the one thing you do need to
> address before landing is using WKStringRef instead of WTF::String in the
> new C API.

Got it! I was looking at the old mechanism for Basic Authentication in TestRunner where WTF Strings are used IIRC.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:52
> > +static auto timestampResolution = 86400;
> 
> This is fine for now, but I'd prefer to see WTF::Seconds used here.

I will revisit.

> If you added fromDays() and days() functions to the Seconds class, then you
> could write this as "static auto timestampResolution =
> Seconds::fromDays(1);" and your comment would be unnecessary.
> 
> >> Source/WebCore/loader/ResourceLoadObserver.cpp:307
> >> +    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));
> > 
> > If 'm_store' can be null, we should null check here, or at least ASSERT.
> 
> If m_store can't be null here then an assertion wouldn't add much, in my
> opinion. A crash due to dereferencing a null pointer provides the same
> information as an assertion failure.

I agree. I put one ASSERT on the getter since the class in not in control of how the receiver uses it. The intention is to find null uses early if we have any.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:313
> > +    if (newTimestamp != statistics.mostRecentUserInteraction) {
> > +        statistics.hadUserInteraction = true;
> > +        statistics.mostRecentUserInteraction = newTimestamp;
> > +        m_store->fireDataModificationHandler();
> > +    }
> 
> I think this would read better if you instead returned early when
> newTimestamp == statistics.mostRecentUserInteraction.

Fixed.

> >> Source/WebCore/loader/ResourceLoadStatistics.cpp:316
> >> +        if (other.mostRecentUserInteraction == 0.0) {
> > 
> > We usually test against 0 (not 0.0), and usually be using something like:  "if (!other.mostRecentUserInteraction)".

Fixed.

> > If you are getting very small floating point values in mostRecentUserInteraction, you should use the WTF MathExtras "areEssentiallyEqual" macro.
> 
> I'd also combine this with the if statement above to reduce indentation: if
> (!other.hadUserInteraction && !other.mostRecentInteraction) {}

That actually changes the semantics. I bent this one in several ways before I went with my solution. Leaving as is except for the change from a comparison with 0.0 to a not operator.

> > Source/WebCore/loader/ResourceLoadStatistics.h:57
> > +    double mostRecentUserInteraction { -1.0 };
> 
> This should be -1 instead of -1.0.

Fixed.

> Having a special default values is fine for now, but I'd prefer to see you
> use WTF::Optional.

Got it. I felt a bit uncomfortable with this solution myself. Will revisit.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:43
> > +// 30 days in seconds
> > +static auto timeToLiveUserInteraction = 2592000;
> 
> See comment about using WTF::Seconds.

Will revisit.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:167
> > +    if (currentTime() > (resourceStatistic.mostRecentUserInteraction + timeToLiveUserInteraction)) {
> 
> No need for parentheses around the right operand of >.

Fixed.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:171
> > +        resourceStatistic.mostRecentUserInteraction = 0.0;
> 
> 0 instead of 0.0.

Fixed.

> > Source/WebKit2/PlatformEfl.cmake:71
> > +    UIProcess/API/C/WKResourceLoadStatisticsManager.cpp
> > +
> 
> This should be inserted in sorted order.

It was, but the style script complained and wanted it where it is. I should probably file a bug. Leaving as is.

> > Source/WebKit2/PlatformGTK.cmake:97
> > +    UIProcess/API/C/WKResourceLoadStatisticsManager.cpp
> > +
> 
> Ditto.

Same thing here – the style script said the order was wrong.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:203
> > +void WebProcessProxy::deleteWebsiteDataForTopPrivatelyOwnedDomainsInAllPersistentDataStores(OptionSet<WebsiteDataType> dataTypes, Vector<String>& topPrivatelyOwnedDomains, bool shouldNotifyPage, std::function<void()> completionHandler)
> 
> completionHandler's type should be WTF::Function<void()>&&.

I tried this but ended up with a crasher. I probably need some guidance. Will revisit.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:205
> > +    struct CallbackAggregator : ThreadSafeRefCounted<CallbackAggregator> {
> 
> This is fine for now, but {add,remove}PendingCallback() is really just an
> explicit duplication of the existing ref-counting. You could instead have a
> destructor that calls completionHandler, since callbackAggregator will be
> destroyed when the last removeDataForTopPrivatelyOwnedDomains() completion
> handler finishes executing.
> 
> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:206
> > +        explicit CallbackAggregator(std::function<void()> completionHandler)
> 
> Ditto.

See above.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:227
> > +                RunLoop::main().dispatch(WTFMove(completionHandler));
> 
> removePendingCallback() is already called on the main thread, so is this
> necessary?

No, you're right. Fixed.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:231
> > +        std::function<void()> completionHandler;
> 
> Let's use WTF::Function instead of std::function.

See above.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:49
> > +static auto minimumTimeBetweeenDataRecordsRemoval = 60;
> 
> Another good use for WTF::Seconds.

Will revisit.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:52
> > +static bool notifyPages = false;
> > +static bool shouldClassifyResourcesBeforeDataRecordsRemoval = true;
> 
> auto works here too.

Fixed.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:133
> > +        && now < (m_lastTimeDataRecordsWereRemoved + minimumTimeBetweeenDataRecordsRemoval))
> 
> No need for parentheses around the right operand of <.

Fixed.

> > Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsManager.h:40
> > +    WK_EXPORT void WKResourceLoadStatisticsManagerSetPrevalentResource(String hostName, bool value);
> > +    WK_EXPORT bool WKResourceLoadStatisticsManagerIsPrevalentResource(String hostName);
> > +    WK_EXPORT void WKResourceLoadStatisticsManagerSetHasHadUserInteraction(String hostName, bool value);
> > +    WK_EXPORT bool WKResourceLoadStatisticsManagerIsHasHadUserInteraction(String hostName);
> 
> We need to use WKStringRef in the API, not WTF::String.

Fixed.

> >> Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsManager.h:47
> >> +    WK_EXPORT void WKResourceLoadStatisticsManagerResetToConsistentState();
> > 
> > These aren't meant to be API, so we might need to prefix them with underscores or something. Alex will know!
> 
> None of the C API is public, so no need for underscores.

Got it. Leaving as is.

> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:476
> > +void WebsiteDataStore::fetchDataForTopPrivatelyOwnedDomains(OptionSet<WebsiteDataType> dataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, const Vector<String>& topPrivatelyOwnedDomains, std::function<void(Vector<WebsiteDataRecord>)> completionHandler)
> 
> completionHandler should be a Function<...>&&.

See above.

> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:478
> > +    fetchData(dataTypes, fetchOptions, [topPrivatelyOwnedDomains, completionHandler, this](auto existingDataRecords) {
> 
> you should WTFMove completionHandler in the capture list (completionHandler
> = WTFMove(completionHandler).
> 
> It'd be a little better if existingDataRecords was passed as an rvalue
> reference, but this is fine for now.

These changes were also part of what ended up crashing. Will revisit after I fully grasp Function<...>&&.

> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:489
> > +                    if (dataRecordHost.endsWithIgnoringASCIICase(topPrivatelyOwnedDomain)) {
> > +                        auto suffixStart = dataRecordHost.length() - topPrivatelyOwnedDomain.length();
> > +                        if (!suffixStart || dataRecordHost[suffixStart - 1] == '.') {
> 
> I believe this check is done in a few places now. It'd be nice if we had a
> helper function for this.

Agree.

> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1030
> > +void WebsiteDataStore::removeDataForTopPrivatelyOwnedDomains(OptionSet<WebsiteDataType> dataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, const Vector<String>& topPrivatelyOwnedDomains, std::function<void()> completionHandler)
> 
> Ditto about using WTF::Function<...>&& instead.
> 
> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1033
> > +        this->removeData(dataTypes, websiteDataRecords, [completionHandler]() {
> 
> Ditto about WTFMoving completionHandler in the capture list.

See above.

Thanks a lot for going through all this, Andy! I appreciate it.

[WebKit Commit Bot]

Comment on attachment 301246 [details]
Patch

Clearing flags on attachment: 301246

Committed r212183: <http://trac.webkit.org/changeset/212183>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.