Bug 167474 - Updates to Resource Load Statistics: Get the right website data store and introduce timeout for user interaction
Summary: Updates to Resource Load Statistics: Get the right website data store and int...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on: 167960
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-26 16:38 PST by John Wilander
Modified: 2017-04-03 09:43 PDT (History)
13 users (show)

See Also:


Attachments
Patch (16.31 KB, patch)
2017-01-26 16:52 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (23.22 KB, patch)
2017-01-31 14:27 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (21.07 KB, patch)
2017-01-31 15:39 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (23.71 KB, patch)
2017-02-01 11:09 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (118.39 KB, patch)
2017-02-09 01:51 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (120.34 KB, patch)
2017-02-09 12:19 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (121.15 KB, patch)
2017-02-09 12:40 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (121.19 KB, patch)
2017-02-10 22:27 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2017-01-26 16:38:08 PST
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.
Comment 1 John Wilander 2017-01-26 16:52:20 PST
Created attachment 299881 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2017-01-31 11:18:52 PST
<rdar://problem/30290270>
Comment 3 John Wilander 2017-01-31 14:27:25 PST
Created attachment 300260 [details]
Patch
Comment 4 John Wilander 2017-01-31 15:39:27 PST
Created attachment 300275 [details]
Patch
Comment 5 John Wilander 2017-01-31 15:40:32 PST
Builds fine on my machine. Made some changes to the lambda captures that seemed to cause EWS build errors.
Comment 6 John Wilander 2017-02-01 11:09:12 PST
Created attachment 300339 [details]
Patch
Comment 7 John Wilander 2017-02-01 11:10:49 PST
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.
Comment 8 Alex Christensen 2017-02-01 12:58:50 PST
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.
Comment 9 John Wilander 2017-02-09 01:51:11 PST
Created attachment 301025 [details]
Patch
Comment 10 John Wilander 2017-02-09 12:19:48 PST
Created attachment 301067 [details]
Patch
Comment 11 John Wilander 2017-02-09 12:20:52 PST
New patch that adds a missing file to the .cmake files for GTK, Efl, and Mac. If all goes green, please review.
Comment 12 John Wilander 2017-02-09 12:40:34 PST
Created attachment 301073 [details]
Patch
Comment 13 John Wilander 2017-02-09 12:41:02 PST
Added another file reference to the .cmake files.
Comment 14 Brent Fulgham 2017-02-10 11:10:32 PST
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!
Comment 15 Andy Estes 2017-02-10 14:59:33 PST
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.
Comment 16 John Wilander 2017-02-10 22:27:53 PST
Created attachment 301246 [details]
Patch
Comment 17 John Wilander 2017-02-10 22:34:41 PST
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!
Comment 18 John Wilander 2017-02-10 22:46:10 PST
(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.
Comment 19 WebKit Commit Bot 2017-02-10 23:36:16 PST
Comment on attachment 301246 [details]
Patch

Clearing flags on attachment: 301246

Committed r212183: <http://trac.webkit.org/changeset/212183>
Comment 20 WebKit Commit Bot 2017-02-10 23:36:22 PST
All reviewed patches have been landed.  Closing bug.