[Offline Pages] Linking storage manager and model with UMAs.

components/offline_pages/offline_page_model.cc

Index: components/offline_pages/offline_page_model.cc
diff --git a/components/offline_pages/offline_page_model.cc b/components/offline_pages/offline_page_model.cc
index af39f37a8fd0d318eb0679a3ab940a786be1617e..66354dc3378cb13431da0e4f6d72b1b7839c60d3 100644
--- a/components/offline_pages/offline_page_model.cc
+++ b/components/offline_pages/offline_page_model.cc
@@ -25,6 +25,10 @@
 #include "url/gurl.h"
 
 using ArchiverResult = offline_pages::OfflinePageArchiver::ArchiverResult;
+using ClearStorageCallback =
+    offline_pages::OfflinePageStorageManager::ClearStorageCallback;
+using ClearStorageResult =
+    offline_pages::OfflinePageStorageManager::ClearStorageResult;
 
 namespace offline_pages {
 
@@ -535,11 +539,25 @@ void OfflinePageModel::ExpirePages(const std::vector<int64_t>& offline_ids,
 void OfflinePageModel::OnExpirePageDone(int64_t offline_id,
                                         const base::Time& expiration_time,
                                         bool success) {
-  // TODO(romax): Report UMA about successful expiration.
-  if (success) {
-    auto iter = offline_pages_.find(offline_id);
-    if (iter != offline_pages_.end())
-      iter->second.expiration_time = expiration_time;
+  UMA_HISTOGRAM_COUNTS("OfflinePages.ExpirePage.StoreUpdateResult",

[jianli, 2016/05/27 19:16:12]

You should use UMA_HISTOGRAM_BOOLEAN.

[romax, 2016/05/27 20:36:02]

Done.

+                       success ? 0 : 1);
+  if (!success)
+    return;
+  auto iter = offline_pages_.find(offline_id);

[jianli, 2016/05/27 19:16:12]

nit: add const&

[romax, 2016/05/27 20:36:02]

Done.

+  if (iter != offline_pages_.end()) {
+    iter->second.expiration_time = expiration_time;
+    ClientId client_id = iter->second.client_id;
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        AddHistogramSuffix(client_id, "OfflinePages.ExpirePage.PageLifetime")
+            .c_str(),
+        (expiration_time - iter->second.creation_time).InMinutes(), 1,
+        base::TimeDelta::FromDays(30).InMinutes(), 120);

[jianli, 2016/05/27 19:16:12]

I think 50 buckets should be enough.

[romax, 2016/05/27 20:36:01]

I don't have a strong opinion here, but can you elaborate how to estimate the
number of buckets?

[jianli, 2016/05/27 20:46:21]

We should only try to use as many buckets as needed, in order not to incur
unnecessary storage cost in the server. Probably our UMA reviewer could better
explain this.

+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        AddHistogramSuffix(client_id,
+                           "OfflinePages.ExpirePage.TimeSinceLastAccess")
+            .c_str(),
+        (expiration_time - iter->second.last_access_time).InMinutes(), 1,
+        base::TimeDelta::FromDays(30).InMinutes(), 120);

[jianli, 2016/05/27 19:16:12]

ditto

[romax, 2016/05/27 20:36:01]

Done.

   }
 }
 
@@ -663,6 +681,13 @@ void OfflinePageModel::OnLoadDone(
     delayed_task.Run();
   delayed_tasks_.clear();
 
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

[jianli, 2016/05/27 19:16:12]

It just seems to be more natural to put this at the end of function unless the
ordering does matter.

[romax, 2016/05/27 20:36:02]

Done.

+      FROM_HERE, base::Bind(&OfflinePageModel::ClearStorageIfNeeded,
+                            weak_ptr_factory_.GetWeakPtr(),
+                            base::Bind(&OfflinePageModel::OnStorageCleared,
+                                       weak_ptr_factory_.GetWeakPtr())),
+      base::TimeDelta::FromSeconds(20));

[jianli, 2016/05/27 19:16:12]

Better define this as constant.

[romax, 2016/05/27 20:36:02]

Done.

+
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelLoaded(this));
 
   CheckForExternalFileDeletion();

[jianli, 2016/05/27 19:16:13]

Do we have the plan to move this to StorageManager?

[romax, 2016/05/27 20:36:02]

I think that would be the plan... I'll do that in a future patch.

@@ -678,6 +703,11 @@ void OfflinePageModel::InformSavePageDone(const SavePageCallback& callback,
       static_cast<int>(SavePageResult::RESULT_COUNT));
   archive_manager_->GetStorageStats(
       base::Bind(&ReportStorageHistogramsAfterSave));
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&OfflinePageModel::ClearStorageIfNeeded,
+                            weak_ptr_factory_.GetWeakPtr(),
+                            base::Bind(&OfflinePageModel::OnStorageCleared,
+                                       weak_ptr_factory_.GetWeakPtr())));
   callback.Run(result, offline_id);
 }
 
@@ -855,6 +885,23 @@ void OfflinePageModel::CacheLoadedData(
     offline_pages_[offline_page.offline_id] = offline_page;
 }
 
+void OfflinePageModel::ClearStorageIfNeeded(
+    const ClearStorageCallback& callback) {
+  storage_manager_->ClearPagesIfNeeded(callback);
+}
+
+void OfflinePageModel::OnStorageCleared(const size_t expired_page_count,

[jianli, 2016/05/27 19:16:12]

nit: add const before size_t is not needed

[romax, 2016/05/27 20:36:02]

Done.

+                                        ClearStorageResult result) {
+  UMA_HISTOGRAM_ENUMERATION("OfflinePages.ExpirePage.Result",
+                            static_cast<int>(result),
+                            static_cast<int>(ClearStorageResult::RESULT_COUNT));
+  if (expired_page_count > 0) {
+    UMA_HISTOGRAM_COUNTS("OfflinePages.ExpirePage.BatchSize",
+                         static_cast<int32_t>(expired_page_count));
+  }
+}
+
+// static
 int64_t OfflinePageModel::GenerateOfflineId() {
   return base::RandGenerator(std::numeric_limits<int64_t>::max()) + 1;
 }