Refactor cache counting into a separate helper class

chrome/browser/browsing_data/cache_counter.cc

Index: chrome/browser/browsing_data/cache_counter.cc
diff --git a/chrome/browser/browsing_data/cache_counter.cc b/chrome/browser/browsing_data/cache_counter.cc
index 5a5890c84e1a63762c628f60274c7900425b7777..e0b023a9a7eb7f3f2a7aa778e8a537377e4addcf 100644
--- a/chrome/browser/browsing_data/cache_counter.cc
+++ b/chrome/browser/browsing_data/cache_counter.cc
@@ -4,10 +4,23 @@
 
 #include "chrome/browser/browsing_data/cache_counter.h"
 #include "chrome/browser/profiles/profile.h"
-#include "components/browsing_data/content/storage_partition_http_cache_data_remover.h"
+#include "components/browsing_data/content/conditional_cache_counting_helper.h"
 #include "components/browsing_data/core/pref_names.h"
+#include "content/public/browser/browser_thread.h"
 #include "net/base/net_errors.h"
 
+using content::BrowserThread;

[msramek, 2016/12/15 15:58:50]

style nit: Please use namespaces explicitly in non-test files when possible.

[dullweber, 2016/12/16 16:41:10]

Done.

+using browsing_data::ConditionalCacheCountingHelper;
+
+CacheCounter::CacheResult::CacheResult(const CacheCounter* source,

[msramek, 2016/12/15 15:58:50]

style nit: please keep the ":" from initializers under the first letter of the
first parameter; that is, break the parameters on the next line

[dullweber, 2016/12/16 16:41:10]

clangformat doesn't let me do this.

+                                       int64_t cache_size,
+                                       bool is_upper_limit)
+    : FinishedResult(source, cache_size),
+      cache_size_(cache_size),
+      is_upper_limit_(is_upper_limit) {}
+
+CacheCounter::CacheResult::~CacheResult() {}
+
 CacheCounter::CacheCounter(Profile* profile)
     : profile_(profile),
       pending_(false),
@@ -21,27 +34,53 @@ const char* CacheCounter::GetPrefName() const {
 }
 
 void CacheCounter::Count() {
-  // TODO(msramek): StoragePartitionHttpCacheDataRemover currently does not
-  // implement counting for subsets of cache, only for the entire cache. Thus,
-  // we ignore the time period setting and always request counting for
-  // the unbounded time interval. It is up to the UI to interpret the results
-  // for finite time intervals as upper estimates.
-  browsing_data::StoragePartitionHttpCacheDataRemover::CreateForRange(
+  base::WeakPtr<ConditionalCacheCountingHelper> counter =
+      ConditionalCacheCountingHelper::CreateForRange(
+          content::BrowserContext::GetDefaultStoragePartition(profile_),
+          GetPeriodStart(), base::Time::Max())
+          ->CountAndDestroySelfWhenFinished(
+              base::Bind(&CacheCounter::OnCacheSizeCalculated,
+                         weak_ptr_factory_.GetWeakPtr(), false));
+  // Counting a subset of the cache can take a long time. If it doesn't succeed

[msramek, 2016/12/15 15:58:50]

nit: The following block is wrapped in GetPeriodStart().is_null(); please
explain that the counting doesn't take long time in that case.

[dullweber, 2016/12/16 16:41:10]

Done.

+  // before |cache_counter_timeout| runs out, get the total cache size as an
+  // estimate instead.
+  if (!GetPeriodStart().is_null()) {
+    base::TimeDelta cache_counter_timeout = base::TimeDelta::FromSeconds(3);

[msramek, 2016/12/15 15:58:50]

nit: Please pull this up as kCacheCounterTimeout constant in an anonymous
namespace.

[dullweber, 2016/12/16 16:41:10]

Done.

+    BrowserThread::PostDelayedTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&CacheCounter::FetchEstimate, weak_ptr_factory_.GetWeakPtr(),
+                   counter),
+        cache_counter_timeout);
+  }
+  pending_ = true;
+}
+
+void CacheCounter::FetchEstimate(
+    base::WeakPtr<ConditionalCacheCountingHelper> counter) {
+  if (!counter || counter->IsFinished())
+    return;
+  counter->CancelCounting();
+  // Try again for the whole cache.
+  ConditionalCacheCountingHelper::CreateForRange(
       content::BrowserContext::GetDefaultStoragePartition(profile_),
       base::Time(), base::Time::Max())
-      ->Count(base::Bind(&CacheCounter::OnCacheSizeCalculated,
-                         weak_ptr_factory_.GetWeakPtr()));
-  pending_ = true;
+      ->CountAndDestroySelfWhenFinished(
+          base::Bind(&CacheCounter::OnCacheSizeCalculated,
+                     weak_ptr_factory_.GetWeakPtr(), true));

[msramek, 2016/12/15 15:58:50]

|is_upper_limit| is not always true; it depends on whether the time range is
bounded.

[dullweber, 2016/12/16 16:41:10]

This code is only executed if the timer finishes before the counter is responds
for a restricted time range, so this result is always an estimate.

[msramek, 2016/12/20 01:02:59]

Acknowledged.

 }
 
-void CacheCounter::OnCacheSizeCalculated(int64_t result_bytes) {
+void CacheCounter::OnCacheSizeCalculated(bool is_upper_limit,
+                                         int64_t result_bytes) {
+  if (result_bytes == net::ERR_ABORTED) {
+    return;
+  }
   pending_ = false;

[msramek, 2016/12/15 15:58:50]

nit: Please re-add the empty line for readability.

[dullweber, 2016/12/16 16:41:10]

Done.

-
   // A value less than 0 means a net error code.
   if (result_bytes < 0)
     return;
-
-  ReportResult(result_bytes);
+  auto result =
+      base::MakeUnique<CacheResult>(this, result_bytes, is_upper_limit);
+  ReportResult(std::move(result));
 }
 
 bool CacheCounter::Pending() {