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

components/offline_pages/offline_page_model.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/offline_pages/offline_page_model.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/optional.h"
 #include "base/rand_util.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "components/offline_pages/archive_manager.h"
 #include "components/offline_pages/client_policy_controller.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/offline_page_storage_manager.h"
 #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 {
 
 namespace {
 
 // This enum is used in an UMA histogram. Hence the entries here shouldn't
 // be deleted or re-ordered and new ones should be added to the end.
 enum ClearAllStatus {
   CLEAR_ALL_SUCCEEDED,
   STORE_RESET_FAILED,
@@ skipping 490 matching lines @@
   if (paths_to_delete.empty()) {
     callback.Run(true);
     return;
   }
   archive_manager_->DeleteMultipleArchives(paths_to_delete, callback);
 }
 
 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.

   }
 }
 
 ClientPolicyController* OfflinePageModel::GetPolicyController() {
   return policy_controller_.get();
 }
 
 OfflinePageMetadataStore* OfflinePageModel::GetStoreForTesting() {
   return store_.get();
 }
@@ skipping 103 matching lines @@
 
   // Create Storage Manager.
   storage_manager_.reset(new OfflinePageStorageManager(
       this, GetPolicyController(), archive_manager_.get()));
 
   // Run all the delayed tasks.
   for (const auto& delayed_task : delayed_tasks_)
     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.

 }
 
 void OfflinePageModel::InformSavePageDone(const SavePageCallback& callback,
                                           SavePageResult result,
                                           const ClientId& client_id,
                                           int64_t offline_id) {
   UMA_HISTOGRAM_ENUMERATION(
       AddHistogramSuffix(client_id, "OfflinePages.SavePageResult").c_str(),
       static_cast<int>(result),
       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);
 }
 
 void OfflinePageModel::DeletePendingArchiver(OfflinePageArchiver* archiver) {
   pending_archivers_.erase(std::find(
       pending_archivers_.begin(), pending_archivers_.end(), archiver));
 }
 
 void OfflinePageModel::OnDeleteArchiveFilesDone(
     const std::vector<int64_t>& offline_ids,
@@ skipping 157 matching lines @@
   callback.Run();
 }
 
 void OfflinePageModel::CacheLoadedData(
     const std::vector<OfflinePageItem>& offline_pages) {
   offline_pages_.clear();
   for (const auto& offline_page : offline_pages)
     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;
 }
 
 void OfflinePageModel::RunWhenLoaded(const base::Closure& task) {
   if (!is_loaded_) {
     delayed_tasks_.push_back(task);
     return;
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, task);
 }
 
 }  // namespace offline_pages