Attachment 343880 Details for Bug 187165 – Patch

[patch] Patch

bug-187165-20180628183929.patch (text/plain), 15.85 KB, created by Chris Dumez on 2018-06-28 18:39:05 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-28 18:39:05 PDT

Size: 15.85 KB

patch

obsolete

>Subversion Revision: 233345
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 5cbf62fbc10d6ba24328436aa79dd1d77cb572ee..59092ed2d007c459cd9ac2b2fcadbc2fa68b1662 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,25 @@
>+2018-06-28  Chris Dumez  <cdumez@apple.com>
>+
>+        Stop using lambdas for WebResourceLoadStatisticsStore to interact with its WebsiteDataStore
>+        https://bugs.webkit.org/show_bug.cgi?id=187165
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Stop using lambdas for WebResourceLoadStatisticsStore to interact with its WebsiteDataStore. Instead,
>+        WebResourceLoadStatisticsStore now holds a weak pointer to its WebsiteDataStore and is able to call
>+        methods on it directly. Reducing the indirection makes the code less complex and more understandable.
>+
>+        * UIProcess/WebResourceLoadStatisticsStore.cpp:
>+        (WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
>+        (WebKit::WebResourceLoadStatisticsStore::callHasStorageAccessForFrameHandler):
>+        (WebKit::WebResourceLoadStatisticsStore::callGrantStorageAccessHandler):
>+        (WebKit::WebResourceLoadStatisticsStore::removeAllStorageAccess):
>+        (WebKit::WebResourceLoadStatisticsStore::callUpdatePrevalentDomainsToPartitionOrBlockCookiesHandler):
>+        (WebKit::WebResourceLoadStatisticsStore::callRemoveDomainsHandler):
>+        * UIProcess/WebResourceLoadStatisticsStore.h:
>+        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
>+        (WebKit::WebsiteDataStore::enableResourceLoadStatisticsAndSetTestingCallback):
>+
> 2018-06-28  Chris Dumez  <cdumez@apple.com>
> 
>         Make sure the WebResourceLoadStatisticsStore gets destroyed on the main thread
>diff --git a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp
>index 6f98e751e59431c04e99987118b19c005696c70e..6a25a0d96f7489ce4c55e1174ed76afdef5b28a8 100644
>--- a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp
>+++ b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp
>@@ -111,21 +111,16 @@ void WebResourceLoadStatisticsStore::setShouldSubmitTelemetry(bool value)
>     });
> }
> 
>-WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore(const String& resourceLoadStatisticsDirectory, Function<void(const String&)>&& testingCallback, bool isEphemeral, UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler&& updatePrevalentDomainsToPartitionOrBlockCookiesHandler, HasStorageAccessForFrameHandler&& hasStorageAccessForFrameHandler, GrantStorageAccessHandler&& grantStorageAccessHandler, RemoveAllStorageAccessHandler&& removeAllStorageAccessHandler, RemovePrevalentDomainsHandler&& removeDomainsHandler)
>-    : m_statisticsQueue(WorkQueue::create("WebResourceLoadStatisticsStore Process Data Queue", WorkQueue::Type::Serial, WorkQueue::QOS::Utility))
>-    , m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler(WTFMove(updatePrevalentDomainsToPartitionOrBlockCookiesHandler))
>-    , m_hasStorageAccessForFrameHandler(WTFMove(hasStorageAccessForFrameHandler))
>-    , m_grantStorageAccessHandler(WTFMove(grantStorageAccessHandler))
>-    , m_removeAllStorageAccessHandler(WTFMove(removeAllStorageAccessHandler))
>-    , m_removeDomainsHandler(WTFMove(removeDomainsHandler))
>+WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore(WebsiteDataStore& websiteDataStore)
>+    : m_websiteDataStore(makeWeakPtr(websiteDataStore))
>+    , m_statisticsQueue(WorkQueue::create("WebResourceLoadStatisticsStore Process Data Queue", WorkQueue::Type::Serial, WorkQueue::QOS::Utility))
>     , m_dailyTasksTimer(RunLoop::main(), this, &WebResourceLoadStatisticsStore::performDailyTasks)
>-    , m_statisticsTestingCallback(WTFMove(testingCallback))
> {
>     ASSERT(RunLoop::isMain());
> 
>-    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), isEphemeral, resourceLoadStatisticsDirectory = resourceLoadStatisticsDirectory.isolatedCopy()] {
>+    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), isPersistent = websiteDataStore.isPersistent(), resourceLoadStatisticsDirectory = websiteDataStore.resolvedResourceLoadStatisticsDirectory().isolatedCopy()] {
>         m_memoryStore = std::make_unique<ResourceLoadStatisticsMemoryStore>(*this, m_statisticsQueue);
>-        m_persistentStorage = std::make_unique<ResourceLoadStatisticsPersistentStorage>(*m_memoryStore, m_statisticsQueue, resourceLoadStatisticsDirectory, isEphemeral ? ResourceLoadStatisticsPersistentStorage::IsReadOnly::Yes : ResourceLoadStatisticsPersistentStorage::IsReadOnly::No);
>+        m_persistentStorage = std::make_unique<ResourceLoadStatisticsPersistentStorage>(*m_memoryStore, m_statisticsQueue, resourceLoadStatisticsDirectory, isPersistent ? ResourceLoadStatisticsPersistentStorage::IsReadOnly::No : ResourceLoadStatisticsPersistentStorage::IsReadOnly::Yes);
>     });
> 
>     m_statisticsQueue->dispatchAfter(5_s, [this, protectedThis = makeRef(*this)] {
>@@ -227,7 +222,10 @@ void WebResourceLoadStatisticsStore::callHasStorageAccessForFrameHandler(const S
> {
>     ASSERT(RunLoop::isMain());
> 
>-    m_hasStorageAccessForFrameHandler(resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(callback));
>+    if (m_websiteDataStore)
>+        m_websiteDataStore->hasStorageAccessForFrameHandler(resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(callback));
>+    else
>+        callback(false);
> }
> 
> void WebResourceLoadStatisticsStore::requestStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, bool promptEnabled, CompletionHandler<void(StorageAccessStatus)>&& completionHandler)
>@@ -294,14 +292,18 @@ void WebResourceLoadStatisticsStore::callGrantStorageAccessHandler(const String&
> {
>     ASSERT(RunLoop::isMain());
> 
>-    m_grantStorageAccessHandler(subFramePrimaryDomain, topFramePrimaryDomain, frameID, pageID, WTFMove(callback));
>+    if (m_websiteDataStore)
>+        m_websiteDataStore->grantStorageAccessHandler(subFramePrimaryDomain, topFramePrimaryDomain, frameID, pageID, WTFMove(callback));
>+    else
>+        callback(false);
> }
> 
> void WebResourceLoadStatisticsStore::removeAllStorageAccess()
> {
>     ASSERT(RunLoop::isMain());
> 
>-    m_removeAllStorageAccessHandler();
>+    if (m_websiteDataStore)
>+        m_websiteDataStore->removeAllStorageAccessHandler();
> }
> 
> 
>@@ -804,14 +806,18 @@ void WebResourceLoadStatisticsStore::setGrandfatheringTime(Seconds seconds)
> void WebResourceLoadStatisticsStore::callUpdatePrevalentDomainsToPartitionOrBlockCookiesHandler(const Vector<String>& domainsToPartition, const Vector<String>& domainsToBlock, const Vector<String>& domainsToNeitherPartitionNorBlock, ShouldClearFirst shouldClearFirst, CompletionHandler<void()>&& completionHandler)
> {
>     ASSERT(RunLoop::isMain());
>-    m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler(domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, shouldClearFirst, WTFMove(completionHandler));
>+    if (m_websiteDataStore)
>+        m_websiteDataStore->updatePrevalentDomainsToPartitionOrBlockCookies(domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, shouldClearFirst, WTFMove(completionHandler));
>+    else
>+        completionHandler();
> }
> 
> void WebResourceLoadStatisticsStore::callRemoveDomainsHandler(const Vector<String>& domains)
> {
>     ASSERT(RunLoop::isMain());
> 
>-    m_removeDomainsHandler(domains);
>+    if (m_websiteDataStore)
>+        m_websiteDataStore->removePrevalentDomains(domains);
> }
>     
> void WebResourceLoadStatisticsStore::setMaxStatisticsEntries(size_t maximumEntryCount)
>diff --git a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h
>index a4c02cb37ee56bf02fc9bceb7a745c1343dca510..147c4669b1e837e90404c8d9df335ad7aa494612 100644
>--- a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h
>+++ b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h
>@@ -32,6 +32,7 @@
> #include <wtf/ThreadSafeRefCounted.h>
> #include <wtf/Vector.h>
> #include <wtf/WallTime.h>
>+#include <wtf/WeakPtr.h>
> #include <wtf/text/WTFString.h>
> 
> namespace WTF {
>@@ -50,6 +51,7 @@ class ResourceLoadStatisticsMemoryStore;
> class ResourceLoadStatisticsPersistentStorage;
> class WebFrameProxy;
> class WebProcessProxy;
>+class WebsiteDataStore;
> 
> enum class ShouldClearFirst;
> enum class StorageAccessStatus {
>@@ -60,14 +62,9 @@ enum class StorageAccessStatus {
> 
> class WebResourceLoadStatisticsStore final : public ThreadSafeRefCounted<WebResourceLoadStatisticsStore, WTF::DestructionThread::Main>, private IPC::MessageReceiver {
> public:
>-    using UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler = WTF::Function<void(const WTF::Vector<String>& domainsToPartition, const WTF::Vector<String>& domainsToBlock, const WTF::Vector<String>& domainsToNeitherPartitionNorBlock, ShouldClearFirst, CompletionHandler<void()>&&)>;
>-    using HasStorageAccessForFrameHandler = WTF::Function<void(const String& resourceDomain, const String& firstPartyDomain, uint64_t frameID, uint64_t pageID, WTF::Function<void(bool hasAccess)>&& callback)>;
>-    using GrantStorageAccessHandler = WTF::Function<void(const String& resourceDomain, const String& firstPartyDomain, std::optional<uint64_t> frameID, uint64_t pageID, WTF::Function<void(bool wasGranted)>&& callback)>;
>-    using RemoveAllStorageAccessHandler = WTF::Function<void()>;
>-    using RemovePrevalentDomainsHandler = WTF::Function<void(const WTF::Vector<String>&)>;
>-    static Ref<WebResourceLoadStatisticsStore> create(const String& resourceLoadStatisticsDirectory, WTF::Function<void(const String&)>&& testingCallback, bool isEphemeral, UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler&& updatePrevalentDomainsToPartitionOrBlockCookiesHandler = [](const WTF::Vector<String>&, const WTF::Vector<String>&, const WTF::Vector<String>&, ShouldClearFirst, CompletionHandler<void()>&& callback) { callback(); }, HasStorageAccessForFrameHandler&& hasStorageAccessForFrameHandler = [](const String&, const String&, uint64_t, uint64_t, WTF::Function<void(bool)>&&) { }, GrantStorageAccessHandler&& grantStorageAccessHandler = [](const String&, const String&, std::optional<uint64_t>, uint64_t, WTF::Function<void(bool)>&&) { }, RemoveAllStorageAccessHandler&& removeAllStorageAccessHandler = []() { }, RemovePrevalentDomainsHandler&& removeDomainsHandler = [] (const WTF::Vector<String>&) { })
>+    static Ref<WebResourceLoadStatisticsStore> create(WebsiteDataStore& websiteDataStore)
>     {
>-        return adoptRef(*new WebResourceLoadStatisticsStore(resourceLoadStatisticsDirectory, WTFMove(testingCallback), isEphemeral, WTFMove(updatePrevalentDomainsToPartitionOrBlockCookiesHandler), WTFMove(hasStorageAccessForFrameHandler), WTFMove(grantStorageAccessHandler), WTFMove(removeAllStorageAccessHandler), WTFMove(removeDomainsHandler)));
>+        return adoptRef(*new WebResourceLoadStatisticsStore(websiteDataStore));
>     }
> 
>     ~WebResourceLoadStatisticsStore();
>@@ -146,7 +143,7 @@ public:
>     void callHasStorageAccessForFrameHandler(const String& resourceDomain, const String& firstPartyDomain, uint64_t frameID, uint64_t pageID, WTF::Function<void(bool)>&&);
> 
> private:
>-    WebResourceLoadStatisticsStore(const String&, WTF::Function<void(const String&)>&& testingCallback, bool isEphemeral, UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler&&, HasStorageAccessForFrameHandler&&, GrantStorageAccessHandler&&, RemoveAllStorageAccessHandler&&, RemovePrevalentDomainsHandler&&);
>+    explicit WebResourceLoadStatisticsStore(WebsiteDataStore&);
> 
>     // IPC::MessageReceiver.
>     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
>@@ -161,16 +158,11 @@ private:
> 
>     void flushAndDestroyPersistentStore();
> 
>+    WeakPtr<WebsiteDataStore> m_websiteDataStore;
>     Ref<WorkQueue> m_statisticsQueue;
>     std::unique_ptr<ResourceLoadStatisticsMemoryStore> m_memoryStore;
>     std::unique_ptr<ResourceLoadStatisticsPersistentStorage> m_persistentStorage;
> 
>-    UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler;
>-    HasStorageAccessForFrameHandler m_hasStorageAccessForFrameHandler;
>-    GrantStorageAccessHandler m_grantStorageAccessHandler;
>-    RemoveAllStorageAccessHandler m_removeAllStorageAccessHandler;
>-    RemovePrevalentDomainsHandler m_removeDomainsHandler;
>-
>     RunLoop::Timer<WebResourceLoadStatisticsStore> m_dailyTasksTimer;
> 
>     bool m_hasScheduledProcessStats { false };
>diff --git a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>index 7d181b9ce692638f356b8e1fab90ea955da99027..108260a0ae7ad10ce738c0d6f4fc1f965099e2d4 100644
>--- a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>+++ b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>@@ -1476,26 +1476,8 @@ void WebsiteDataStore::enableResourceLoadStatisticsAndSetTestingCallback(Functio
>         return;
>     }
> 
>-#if HAVE(CFNETWORK_STORAGE_PARTITIONING)
>-    m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(m_configuration.resourceLoadStatisticsDirectory, WTFMove(callback), m_sessionID.isEphemeral(), [weakThis = makeWeakPtr(*this)] (const Vector<String>& domainsToPartition, const Vector<String>& domainsToBlock, const Vector<String>& domainsToNeitherPartitionNorBlock, ShouldClearFirst shouldClearFirst, CompletionHandler<void()>&& completionHandler) {
>-        if (weakThis)
>-            weakThis->updatePrevalentDomainsToPartitionOrBlockCookies(domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, shouldClearFirst, WTFMove(completionHandler));
>-    }, [weakThis = makeWeakPtr(*this)] (const String& resourceDomain, const String& firstPartyDomain, uint64_t frameID, uint64_t pageID, CompletionHandler<void(bool hasAccess)>&& completionHandler) {
>-        if (weakThis)
>-            weakThis->hasStorageAccessForFrameHandler(resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(completionHandler));
>-    }, [weakThis = makeWeakPtr(*this)] (const String& resourceDomain, const String& firstPartyDomain, std::optional<uint64_t> frameID, uint64_t pageID, WTF::CompletionHandler<void(bool wasGranted)>&& completionHandler) {
>-        if (weakThis)
>-            weakThis->grantStorageAccessHandler(resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(completionHandler));
>-    }, [weakThis = makeWeakPtr(*this)] () {
>-        if (weakThis)
>-            weakThis->removeAllStorageAccessHandler();
>-    }, [weakThis = makeWeakPtr(*this)] (const Vector<String>& domainsToRemove) {
>-        if (weakThis)
>-            weakThis->removePrevalentDomains(domainsToRemove);
>-    });
>-#else
>-    m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(m_configuration.resourceLoadStatisticsDirectory, WTFMove(callback), m_sessionID.isEphemeral());
>-#endif
>+    m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(*this, m_configuration.resourceLoadStatisticsDirectory);
>+    m_resourceLoadStatistics->setStatisticsTestingCallback(WTFMove(callback));
> 
>     for (auto& processPool : processPools(std::numeric_limits<size_t>::max(), false))
>         processPool->setResourceLoadStatisticsEnabled(true);
>diff --git a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
>index 1a56a018698551eec3a608d46c46822ce4652b9b..b9066ae4e1b08eb4566819d60975ce1a8aadc18a 100644
>--- a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
>+++ b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
>@@ -150,6 +150,7 @@ public:
>     const String& resolvedCookieStorageFile() const { return m_resolvedConfiguration.cookieStorageFile; }
>     const String& resolvedIndexedDatabaseDirectory() const { return m_resolvedConfiguration.indexedDBDatabaseDirectory; }
>     const String& resolvedServiceWorkerRegistrationDirectory() const { return m_resolvedConfiguration.serviceWorkerRegistrationDirectory; }
>+    const String& resolvedResourceLoadStatisticsDirectory() const { return m_resolvedConfiguration.resourceLoadStatisticsDirectory; }
> 
>     StorageManager* storageManager() { return m_storageManager.get(); }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 187165: 343880 | 343881 | 343882