Avoid duplicating default parameter values in [WKWebsiteDataStore _resourceLoadStatis...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Jul 2017 00:23:16 +0000 (00:23 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Jul 2017 00:23:16 +0000 (00:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174402

Reviewed by Brent Fulgham.

* UIProcess/API/Cocoa/WKWebsiteDataStore.mm:
(-[WKWebsiteDataStore _resourceLoadStatisticsResetToConsistentState]):
* UIProcess/WebResourceLoadStatisticsStore.cpp:
(WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
(WebKit::WebResourceLoadStatisticsStore::removeDataRecords):
(WebKit::WebResourceLoadStatisticsStore::processStatisticsAndDataRecords):
(WebKit::WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData):
(WebKit::WebResourceLoadStatisticsStore::performDailyTasks):
(WebKit::WebResourceLoadStatisticsStore::setTimeToLiveUserInteraction):
(WebKit::WebResourceLoadStatisticsStore::setTimeToLiveCookiePartitionFree):
(WebKit::WebResourceLoadStatisticsStore::setMinimumTimeBetweenDataRecordsRemoval):
(WebKit::WebResourceLoadStatisticsStore::setGrandfatheringTime):
(WebKit::WebResourceLoadStatisticsStore::shouldRemoveDataRecords):
(WebKit::WebResourceLoadStatisticsStore::shouldPartitionCookies):
(WebKit::WebResourceLoadStatisticsStore::hasStatisticsExpired):
(WebKit::WebResourceLoadStatisticsStore::setMaxStatisticsEntries):
(WebKit::WebResourceLoadStatisticsStore::setPruneEntriesDownTo):
(WebKit::WebResourceLoadStatisticsStore::pruneStatisticsIfNeeded):
(WebKit::WebResourceLoadStatisticsStore::resetParametersToDefaultValues):
* UIProcess/WebResourceLoadStatisticsStore.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219374 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit2/ChangeLog
Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataStore.mm
Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp
Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h

index c2da8a6..62b73e8 100644 (file)
@@ -1,3 +1,31 @@
+2017-07-11  Chris Dumez  <cdumez@apple.com>
+
+        Avoid duplicating default parameter values in [WKWebsiteDataStore _resourceLoadStatisticsResetToConsistentState]
+        https://bugs.webkit.org/show_bug.cgi?id=174402
+
+        Reviewed by Brent Fulgham.
+
+        * UIProcess/API/Cocoa/WKWebsiteDataStore.mm:
+        (-[WKWebsiteDataStore _resourceLoadStatisticsResetToConsistentState]):
+        * UIProcess/WebResourceLoadStatisticsStore.cpp:
+        (WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
+        (WebKit::WebResourceLoadStatisticsStore::removeDataRecords):
+        (WebKit::WebResourceLoadStatisticsStore::processStatisticsAndDataRecords):
+        (WebKit::WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData):
+        (WebKit::WebResourceLoadStatisticsStore::performDailyTasks):
+        (WebKit::WebResourceLoadStatisticsStore::setTimeToLiveUserInteraction):
+        (WebKit::WebResourceLoadStatisticsStore::setTimeToLiveCookiePartitionFree):
+        (WebKit::WebResourceLoadStatisticsStore::setMinimumTimeBetweenDataRecordsRemoval):
+        (WebKit::WebResourceLoadStatisticsStore::setGrandfatheringTime):
+        (WebKit::WebResourceLoadStatisticsStore::shouldRemoveDataRecords):
+        (WebKit::WebResourceLoadStatisticsStore::shouldPartitionCookies):
+        (WebKit::WebResourceLoadStatisticsStore::hasStatisticsExpired):
+        (WebKit::WebResourceLoadStatisticsStore::setMaxStatisticsEntries):
+        (WebKit::WebResourceLoadStatisticsStore::setPruneEntriesDownTo):
+        (WebKit::WebResourceLoadStatisticsStore::pruneStatisticsIfNeeded):
+        (WebKit::WebResourceLoadStatisticsStore::resetParametersToDefaultValues):
+        * UIProcess/WebResourceLoadStatisticsStore.h:
+
 2017-07-11  Dean Jackson  <dino@apple.com>
 
         Rolling out r219372.
index c0e31d5..1a59c75 100644 (file)
@@ -463,20 +463,13 @@ static Vector<WebKit::WebsiteDataRecord> toWebsiteDataRecords(NSArray *dataRecor
 
 - (void)_resourceLoadStatisticsResetToConsistentState
 {
+    WebKit::WebResourceLoadStatisticsTelemetry::setNotifyPagesWhenTelemetryWasCaptured(false);
+
     auto* store = _websiteDataStore->websiteDataStore().resourceLoadStatistics();
     if (!store)
         return;
 
-    // FIXME: These needs to match the default data member values in ResourceLoadStatistics, which is fragile.
-    store->setMaxStatisticsEntries(1000);
-    store->setPruneEntriesDownTo(800);
-    store->setTimeToLiveUserInteraction(std::nullopt);
-    store->setTimeToLiveCookiePartitionFree(24_h);
-    store->setMinimumTimeBetweenDataRecordsRemoval(1_h);
-    store->setGrandfatheringTime(1_h);
-    store->setNotifyPagesWhenDataRecordsWereScanned(false);
-    WebKit::WebResourceLoadStatisticsTelemetry::setNotifyPagesWhenTelemetryWasCaptured(false);
-    store->setShouldClassifyResourcesBeforeDataRecordsRemoval(true);
+    store->resetParametersToDefaultValues();
     store->scheduleClearInMemory();
 }
 
index d196671..185a18b 100644 (file)
@@ -99,7 +99,7 @@ WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore(const String& res
         startMonitoringStatisticsStorage();
     });
     m_statisticsQueue->dispatchAfter(5_s, [this, protectedThis = makeRef(*this)] {
-        if (m_shouldSubmitTelemetry)
+        if (m_parameters.shouldSubmitTelemetry)
             WebResourceLoadStatisticsTelemetry::calculateAndSubmit(*this);
     });
 
@@ -124,7 +124,7 @@ void WebResourceLoadStatisticsStore::removeDataRecords()
     setDataRecordsBeingRemoved(true);
 
     RunLoop::main().dispatch([prevalentResourceDomains = CrossThreadCopier<Vector<String>>::copy(prevalentResourceDomains), this, protectedThis = makeRef(*this)] () mutable {
-        WebProcessProxy::deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(dataTypesToRemove(), WTFMove(prevalentResourceDomains), m_shouldNotifyPagesWhenDataRecordsWereScanned, [this, protectedThis = WTFMove(protectedThis)](const HashSet<String>& domainsWithDeletedWebsiteData) mutable {
+        WebProcessProxy::deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(dataTypesToRemove(), WTFMove(prevalentResourceDomains), m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, [this, protectedThis = WTFMove(protectedThis)](const HashSet<String>& domainsWithDeletedWebsiteData) mutable {
             m_statisticsQueue->dispatch([this, protectedThis = WTFMove(protectedThis), topDomains = CrossThreadCopier<HashSet<String>>::copy(domainsWithDeletedWebsiteData)] () mutable {
                 for (auto& prevalentResourceDomain : topDomains) {
                     auto& statistic = ensureResourceStatisticsForPrimaryDomain(prevalentResourceDomain);
@@ -139,7 +139,7 @@ void WebResourceLoadStatisticsStore::removeDataRecords()
 void WebResourceLoadStatisticsStore::processStatisticsAndDataRecords()
 {
     m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this)] () {
-        if (m_shouldClassifyResourcesBeforeDataRecordsRemoval) {
+        if (m_parameters.shouldClassifyResourcesBeforeDataRecordsRemoval) {
             for (auto& resourceStatistic : m_resourceStatisticsMap.values()) {
                 if (!resourceStatistic.isPrevalentResource && m_resourceLoadStatisticsClassifier.hasPrevalentResourceCharacteristics(resourceStatistic))
                     resourceStatistic.isPrevalentResource = true;
@@ -149,7 +149,7 @@ void WebResourceLoadStatisticsStore::processStatisticsAndDataRecords()
         
         pruneStatisticsIfNeeded();
 
-        if (m_shouldNotifyPagesWhenDataRecordsWereScanned) {
+        if (m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned) {
             RunLoop::main().dispatch([] {
                 WebProcessProxy::notifyPageStatisticsAndDataRecordsProcessed();
             });
@@ -174,13 +174,13 @@ void WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData()
     ASSERT(!RunLoop::isMain());
 
     RunLoop::main().dispatch([this, protectedThis = makeRef(*this)] () mutable {
-        WebProcessProxy::topPrivatelyControlledDomainsWithWebsiteData(dataTypesToRemove(), m_shouldNotifyPagesWhenDataRecordsWereScanned, [this, protectedThis = WTFMove(protectedThis)] (HashSet<String>&& topPrivatelyControlledDomainsWithWebsiteData) mutable {
+        WebProcessProxy::topPrivatelyControlledDomainsWithWebsiteData(dataTypesToRemove(), m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, [this, protectedThis = WTFMove(protectedThis)] (HashSet<String>&& topPrivatelyControlledDomainsWithWebsiteData) mutable {
             m_statisticsQueue->dispatch([this, protectedThis = WTFMove(protectedThis), topDomains = CrossThreadCopier<HashSet<String>>::copy(topPrivatelyControlledDomainsWithWebsiteData)] () mutable {
                 for (auto& topPrivatelyControlledDomain : topDomains) {
                     auto& statistic = ensureResourceStatisticsForPrimaryDomain(topPrivatelyControlledDomain);
                     statistic.grandfathered = true;
                 }
-                m_endOfGrandfatheringTimestamp = WallTime::now() + m_grandfatheringTime;
+                m_endOfGrandfatheringTimestamp = WallTime::now() + m_parameters.grandfatheringTime;
             });
         });
     });
@@ -449,7 +449,7 @@ void WebResourceLoadStatisticsStore::performDailyTasks()
     ASSERT(RunLoop::isMain());
 
     includeTodayAsOperatingDateIfNecessary();
-    if (m_shouldSubmitTelemetry)
+    if (m_parameters.shouldSubmitTelemetry)
         submitTelemetry();
 }
 
@@ -665,28 +665,28 @@ void WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent(std::chr
     scheduleClearInMemoryAndPersistent();
 }
 
-void WebResourceLoadStatisticsStore::setTimeToLiveUserInteraction(std::optional<Seconds> seconds)
+void WebResourceLoadStatisticsStore::setTimeToLiveUserInteraction(Seconds seconds)
 {
-    ASSERT(!seconds || seconds.value() >= 0_s);
-    m_timeToLiveUserInteraction = seconds;
+    ASSERT(seconds >= 0_s);
+    m_parameters.timeToLiveUserInteraction = seconds;
 }
 
 void WebResourceLoadStatisticsStore::setTimeToLiveCookiePartitionFree(Seconds seconds)
 {
     ASSERT(seconds >= 0_s);
-    m_timeToLiveCookiePartitionFree = seconds;
+    m_parameters.timeToLiveCookiePartitionFree = seconds;
 }
 
 void WebResourceLoadStatisticsStore::setMinimumTimeBetweenDataRecordsRemoval(Seconds seconds)
 {
     ASSERT(seconds >= 0_s);
-    m_minimumTimeBetweenDataRecordsRemoval = seconds;
+    m_parameters.minimumTimeBetweenDataRecordsRemoval = seconds;
 }
 
 void WebResourceLoadStatisticsStore::setGrandfatheringTime(Seconds seconds)
 {
     ASSERT(seconds >= 0_s);
-    m_grandfatheringTime = seconds;
+    m_parameters.grandfatheringTime = seconds;
 }
 
 bool WebResourceLoadStatisticsStore::shouldRemoveDataRecords() const
@@ -695,7 +695,7 @@ bool WebResourceLoadStatisticsStore::shouldRemoveDataRecords() const
     if (m_dataRecordsBeingRemoved)
         return false;
 
-    return !m_lastTimeDataRecordsWereRemoved || MonotonicTime::now() >= (m_lastTimeDataRecordsWereRemoved + m_minimumTimeBetweenDataRecordsRemoval);
+    return !m_lastTimeDataRecordsWereRemoved || MonotonicTime::now() >= (m_lastTimeDataRecordsWereRemoved + m_parameters.minimumTimeBetweenDataRecordsRemoval);
 }
 
 void WebResourceLoadStatisticsStore::setDataRecordsBeingRemoved(bool value)
@@ -807,7 +807,7 @@ void WebResourceLoadStatisticsStore::mergeStatistics(Vector<ResourceLoadStatisti
 
 inline bool WebResourceLoadStatisticsStore::shouldPartitionCookies(const ResourceLoadStatistics& statistic) const
 {
-    return statistic.isPrevalentResource && (!statistic.hadUserInteraction || WallTime::now() > statistic.mostRecentUserInteractionTime + m_timeToLiveCookiePartitionFree);
+    return statistic.isPrevalentResource && (!statistic.hadUserInteraction || WallTime::now() > statistic.mostRecentUserInteractionTime + m_parameters.timeToLiveCookiePartitionFree);
 }
 
 void WebResourceLoadStatisticsStore::updateCookiePartitioning()
@@ -923,10 +923,9 @@ bool WebResourceLoadStatisticsStore::hasStatisticsExpired(const ResourceLoadStat
             return true;
     }
 
-    // If we don't meet the real criteria for an expired statistic, check the user
-    // setting for a tighter restriction (mainly for testing).
-    if (m_timeToLiveUserInteraction) {
-        if (WallTime::now() > resourceStatistic.mostRecentUserInteractionTime + m_timeToLiveUserInteraction.value())
+    // If we don't meet the real criteria for an expired statistic, check the user setting for a tighter restriction (mainly for testing).
+    if (m_parameters.timeToLiveUserInteraction) {
+        if (WallTime::now() > resourceStatistic.mostRecentUserInteractionTime + m_parameters.timeToLiveUserInteraction.value())
             return true;
     }
 
@@ -935,12 +934,12 @@ bool WebResourceLoadStatisticsStore::hasStatisticsExpired(const ResourceLoadStat
     
 void WebResourceLoadStatisticsStore::setMaxStatisticsEntries(size_t maximumEntryCount)
 {
-    m_maxStatisticsEntries = maximumEntryCount;
+    m_parameters.maxStatisticsEntries = maximumEntryCount;
 }
     
 void WebResourceLoadStatisticsStore::setPruneEntriesDownTo(size_t pruneTargetCount)
 {
-    m_pruneEntriesDownTo = pruneTargetCount;
+    m_parameters.pruneEntriesDownTo = pruneTargetCount;
 }
     
 struct StatisticsLastSeen {
@@ -973,12 +972,12 @@ static unsigned computeImportance(const ResourceLoadStatistics& resourceStatisti
 void WebResourceLoadStatisticsStore::pruneStatisticsIfNeeded()
 {
     ASSERT(!RunLoop::isMain());
-    if (m_resourceStatisticsMap.size() <= m_maxStatisticsEntries)
+    if (m_resourceStatisticsMap.size() <= m_parameters.maxStatisticsEntries)
         return;
 
-    ASSERT(m_pruneEntriesDownTo <= m_maxStatisticsEntries);
+    ASSERT(m_parameters.pruneEntriesDownTo <= m_parameters.maxStatisticsEntries);
 
-    size_t numberOfEntriesLeftToPrune = m_resourceStatisticsMap.size() - m_pruneEntriesDownTo;
+    size_t numberOfEntriesLeftToPrune = m_resourceStatisticsMap.size() - m_parameters.pruneEntriesDownTo;
     ASSERT(numberOfEntriesLeftToPrune);
     
     Vector<StatisticsLastSeen> resourcesToPrunePerImportance[maxImportance + 1];
@@ -990,5 +989,10 @@ void WebResourceLoadStatisticsStore::pruneStatisticsIfNeeded()
 
     ASSERT(!numberOfEntriesLeftToPrune);
 }
+
+void WebResourceLoadStatisticsStore::resetParametersToDefaultValues()
+{
+    m_parameters = { };
+}
     
 } // namespace WebKit
index 0f2613c..aad9e6e 100644 (file)
@@ -66,9 +66,9 @@ public:
 
     ~WebResourceLoadStatisticsStore();
 
-    void setNotifyPagesWhenDataRecordsWereScanned(bool value) { m_shouldNotifyPagesWhenDataRecordsWereScanned = value; }
-    void setShouldClassifyResourcesBeforeDataRecordsRemoval(bool value) { m_shouldClassifyResourcesBeforeDataRecordsRemoval = value; }
-    void setShouldSubmitTelemetry(bool value) { m_shouldSubmitTelemetry = value; }
+    void setNotifyPagesWhenDataRecordsWereScanned(bool value) { m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned = value; }
+    void setShouldClassifyResourcesBeforeDataRecordsRemoval(bool value) { m_parameters.shouldClassifyResourcesBeforeDataRecordsRemoval = value; }
+    void setShouldSubmitTelemetry(bool value) { m_parameters.shouldSubmitTelemetry = value; }
 
     void resourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics>&& origins);
 
@@ -98,7 +98,7 @@ public:
     void scheduleClearInMemoryAndPersistent();
     void scheduleClearInMemoryAndPersistent(std::chrono::system_clock::time_point modifiedSince);
 
-    void setTimeToLiveUserInteraction(std::optional<Seconds>);
+    void setTimeToLiveUserInteraction(Seconds);
     void setTimeToLiveCookiePartitionFree(Seconds);
     void setMinimumTimeBetweenDataRecordsRemoval(Seconds);
     void setGrandfatheringTime(Seconds);
@@ -107,6 +107,8 @@ public:
     
     void processStatistics(const WTF::Function<void (const WebCore::ResourceLoadStatistics&)>&) const;
     void pruneStatisticsIfNeeded();
+
+    void resetParametersToDefaultValues();
     
 private:
     WebResourceLoadStatisticsStore(const String&, UpdateCookiePartitioningForDomainsHandler&&);
@@ -157,6 +159,18 @@ private:
     void registerUserDefaultsIfNeeded();
 #endif
 
+    struct Parameters {
+        size_t pruneEntriesDownTo { 800 };
+        size_t maxStatisticsEntries { 1000 };
+        std::optional<Seconds> timeToLiveUserInteraction;
+        Seconds timeToLiveCookiePartitionFree { 24_h };
+        Seconds minimumTimeBetweenDataRecordsRemoval { 1_h };
+        Seconds grandfatheringTime { 1_h };
+        bool shouldNotifyPagesWhenDataRecordsWereScanned { false };
+        bool shouldClassifyResourcesBeforeDataRecordsRemoval { true };
+        bool shouldSubmitTelemetry { true };
+    };
+
     HashMap<String, WebCore::ResourceLoadStatistics> m_resourceStatisticsMap;
 #if HAVE(CORE_PREDICTION)
     ResourceLoadStatisticsClassifierCocoa m_resourceLoadStatisticsClassifier;
@@ -175,17 +189,11 @@ private:
     MonotonicTime m_lastStatisticsWriteTime;
     RunLoop::Timer<WebResourceLoadStatisticsStore> m_dailyTasksTimer;
     MonotonicTime m_lastTimeDataRecordsWereRemoved;
-    Seconds m_minimumTimeBetweenDataRecordsRemoval { 1_h };
-    std::optional<Seconds> m_timeToLiveUserInteraction;
-    Seconds m_timeToLiveCookiePartitionFree { 24_h };
-    Seconds m_grandfatheringTime { 1_h };
-    size_t m_maxStatisticsEntries { 1000 };
-    size_t m_pruneEntriesDownTo { 800 };
+
+    Parameters m_parameters;
+
     bool m_dataRecordsBeingRemoved { false };
     bool m_didScheduleWrite { false };
-    bool m_shouldNotifyPagesWhenDataRecordsWereScanned { false };
-    bool m_shouldClassifyResourcesBeforeDataRecordsRemoval { true };
-    bool m_shouldSubmitTelemetry { true };
 };
 
 } // namespace WebKit