[Offline pages] Updating the UpdateCallback in OPMStoreSQL

components/offline_pages/offline_page_model_impl.cc

 // Copyright 2016 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_impl.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
@@ skipping 218 matching lines @@
           base::TimeDelta::FromSeconds(1).InSeconds(),
           base::TimeDelta::FromDays(7).InSeconds(), 50);
     }
     UMA_HISTOGRAM_CUSTOM_COUNTS("OfflinePages.DownloadSavedPageDuplicateCount",
                                 matching_url_count, 1, 20, 10);
   }
 }
 
 void ReportPageHistogramsAfterDelete(
     const std::map<int64_t, OfflinePageItem>& offline_pages,
-    const std::vector<int64_t>& deleted_offline_ids,
+    const std::vector<OfflinePageItem>& deleted_pages,
     const base::Time& delete_time) {
   const int max_minutes = base::TimeDelta::FromDays(365).InMinutes();
   int64_t total_size = 0;
-  for (int64_t offline_id : deleted_offline_ids) {
-    auto iter = offline_pages.find(offline_id);
-    if (iter == offline_pages.end())
-      continue;
 
-    total_size += iter->second.file_size;
-    ClientId client_id = iter->second.client_id;
+  for (const auto& page : deleted_pages) {
+    total_size += page.file_size;
+    ClientId client_id = page.client_id;
 
     if (client_id.name_space == kDownloadNamespace) {
       int remaining_pages_with_url;
       GetMatchingURLCountAndMostRecentCreationTime(
-          offline_pages, iter->second.client_id.name_space, iter->second.url,
-          base::Time::Max(), &remaining_pages_with_url, nullptr);
+          offline_pages, page.client_id.name_space, page.url, base::Time::Max(),
+          &remaining_pages_with_url, nullptr);
       UMA_HISTOGRAM_CUSTOM_COUNTS(
           "OfflinePages.DownloadDeletedPageDuplicateCount",
           remaining_pages_with_url, 1, 20, 10);
     }
 
     // The histograms below are an expansion of the UMA_HISTOGRAM_CUSTOM_COUNTS
     // macro adapted to allow for a dynamically suffixed histogram name.
     // Note: The factory creates and owns the histogram.
     base::HistogramBase* histogram = base::Histogram::FactoryGet(
         AddHistogramSuffix(client_id, "OfflinePages.PageLifetime"),
         1, max_minutes, 100, base::HistogramBase::kUmaTargetedHistogramFlag);
-    histogram->Add((delete_time - iter->second.creation_time).InMinutes());
+    histogram->Add((delete_time - page.creation_time).InMinutes());
 
     histogram = base::Histogram::FactoryGet(
         AddHistogramSuffix(
             client_id, "OfflinePages.DeletePage.TimeSinceLastOpen"),
         1, max_minutes, 100, base::HistogramBase::kUmaTargetedHistogramFlag);
-    histogram->Add((delete_time - iter->second.last_access_time).InMinutes());
+    histogram->Add((delete_time - page.last_access_time).InMinutes());
 
     histogram = base::Histogram::FactoryGet(
         AddHistogramSuffix(
             client_id, "OfflinePages.DeletePage.LastOpenToCreated"),
         1, max_minutes, 100, base::HistogramBase::kUmaTargetedHistogramFlag);
-    histogram->Add(
-        (iter->second.last_access_time - iter->second.creation_time).
-            InMinutes());
+    histogram->Add((page.last_access_time - page.creation_time).InMinutes());
 
     // Reported as Kb between 1Kb and 10Mb.
     histogram = base::Histogram::FactoryGet(
         AddHistogramSuffix(client_id, "OfflinePages.DeletePage.PageSize"),
         1, 10000, 50, base::HistogramBase::kUmaTargetedHistogramFlag);
-    histogram->Add(iter->second.file_size / 1024);
+    histogram->Add(page.file_size / 1024);
 
     histogram = base::Histogram::FactoryGet(
         AddHistogramSuffix(client_id, "OfflinePages.DeletePage.AccessCount"),
         1, 1000000, 50, base::HistogramBase::kUmaTargetedHistogramFlag);
-    histogram->Add(iter->second.access_count);
+    histogram->Add(page.access_count);
   }
 
-  if (deleted_offline_ids.size() > 1) {
+  if (deleted_pages.size() > 1) {
     UMA_HISTOGRAM_COUNTS("OfflinePages.BatchDelete.Count",
-                         static_cast<int32_t>(deleted_offline_ids.size()));
+                         static_cast<int32_t>(deleted_pages.size()));
     UMA_HISTOGRAM_MEMORY_KB(
         "OfflinePages.BatchDelete.TotalPageSize", total_size / 1024);
   }
 }
 
 void ReportPageHistogramsAfterAccess(const OfflinePageItem& offline_page_item,
                                      const base::Time& access_time) {
   // The histogram below is an expansion of the UMA_HISTOGRAM_CUSTOM_COUNTS
   // macro adapted to allow for a dynamically suffixed histogram name.
   // Note: The factory creates and owns the histogram.
@@ skipping 374 matching lines @@
     if (iter == offline_pages_.end())
       continue;
 
     OfflinePageItem offline_page = iter->second;
     paths_to_delete.push_back(offline_page.file_path);
     offline_page.expiration_time = expiration_time;
 
     items_to_update.push_back(offline_page);
   }
 
-  store_->UpdateOfflinePages(items_to_update,
-                             base::Bind(&OfflinePageModelImpl::OnExpirePageDone,
-                                        weak_ptr_factory_.GetWeakPtr(),
-                                        items_to_update, expiration_time));
+  store_->UpdateOfflinePages(
+      items_to_update,
+      base::Bind(&OfflinePageModelImpl::OnExpirePageDone,
+                 weak_ptr_factory_.GetWeakPtr(), expiration_time));
 
   if (paths_to_delete.empty()) {
     callback.Run(true);
     return;
   }
   archive_manager_->DeleteMultipleArchives(paths_to_delete, callback);
 }
 
 void OfflinePageModelImpl::OnExpirePageDone(
-    const std::vector<OfflinePageItem>& expired_pages,
     const base::Time& expiration_time,
-    bool success) {
-  UMA_HISTOGRAM_BOOLEAN("OfflinePages.ExpirePage.StoreUpdateResult", success);
-  if (!success)
-    return;
-  for (auto& expired_page : expired_pages) {
+    std::unique_ptr<StoreUpdateResult> result) {
+  UMA_HISTOGRAM_BOOLEAN("OfflinePages.ExpirePage.StoreUpdateResult",
+                        result->updated_items.size() > 0);
+  for (const auto& expired_page : result->updated_items) {
     const auto& iter = offline_pages_.find(expired_page.offline_id);
     if (iter == offline_pages_.end())
       continue;
 
     iter->second.expiration_time = expiration_time;
     ClientId client_id = iter->second.client_id;
     offline_event_logger_.RecordPageExpired(
         std::to_string(expired_page.offline_id));
     base::HistogramBase* histogram = base::Histogram::FactoryGet(
         AddHistogramSuffix(client_id, "OfflinePages.ExpirePage.PageLifetime"),
@@ skipping 63 matching lines @@
   store_->AddOfflinePage(offline_page_item,
                          base::Bind(&OfflinePageModelImpl::OnAddOfflinePageDone,
                                     weak_ptr_factory_.GetWeakPtr(), archiver,
                                     callback, offline_page_item));
 }
 
 void OfflinePageModelImpl::OnAddOfflinePageDone(
     OfflinePageArchiver* archiver,
     const SavePageCallback& callback,
     const OfflinePageItem& offline_page,
-    OfflinePageMetadataStore::ItemActionStatus status) {
+    ItemActionStatus status) {
   SavePageResult result;
-  if (status == OfflinePageMetadataStore::SUCCESS) {
+  if (status == ItemActionStatus::SUCCESS) {
     offline_pages_[offline_page.offline_id] = offline_page;
     result = SavePageResult::SUCCESS;
     ReportPageHistogramAfterSave(offline_pages_, offline_page,
                                  GetCurrentTime());
     offline_event_logger_.RecordPageSaved(
         offline_page.client_id.name_space, offline_page.url.spec(),
         std::to_string(offline_page.offline_id));
-  } else if (status == OfflinePageMetadataStore::ALREADY_EXISTS) {
+  } else if (status == ItemActionStatus::ALREADY_EXISTS) {
     result = SavePageResult::ALREADY_EXISTS;
   } else {
     result = SavePageResult::STORE_FAILURE;
   }
   InformSavePageDone(callback, result, offline_page.client_id,
                      offline_page.offline_id);
   if (result == SavePageResult::SUCCESS) {
     DeleteExistingPagesWithSameURL(offline_page);
   } else {
     PostClearStorageIfNeededTask();
   }
 
   DeletePendingArchiver(archiver);
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelChanged(this));
 }
 
 void OfflinePageModelImpl::OnMarkPageAccesseDone(
     const OfflinePageItem& offline_page_item,
-    bool success) {
+    std::unique_ptr<StoreUpdateResult> result) {
   // Update the item in the cache only upon success.
-  if (success)
+  if (result->updated_items.size() > 0)
     offline_pages_[offline_page_item.offline_id] = offline_page_item;
 
   // No need to fire OfflinePageModelChanged event since updating access info
   // should not have any impact to the UI.
 }
 
 void OfflinePageModelImpl::OnEnsureArchivesDirCreatedDone(
     const base::TimeTicks& start_time) {
   UMA_HISTOGRAM_TIMES("OfflinePages.Model.ArchiveDirCreationTime",
                       base::TimeTicks::Now() - start_time);
@@ skipping 107 matching lines @@
 void OfflinePageModelImpl::OnDeleteArchiveFilesDone(
     const std::vector<int64_t>& offline_ids,
     const DeletePageCallback& callback,
     bool success) {
   if (!success) {
     InformDeletePageDone(callback, DeletePageResult::DEVICE_FAILURE);
     return;
   }
 
   store_->RemoveOfflinePages(
-      offline_ids,
-      base::Bind(&OfflinePageModelImpl::OnRemoveOfflinePagesDone,
-                 weak_ptr_factory_.GetWeakPtr(), offline_ids, callback));
+      offline_ids, base::Bind(&OfflinePageModelImpl::OnRemoveOfflinePagesDone,
+                              weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void OfflinePageModelImpl::OnRemoveOfflinePagesDone(
-    const std::vector<int64_t>& offline_ids,
     const DeletePageCallback& callback,
-    bool success) {
-  ReportPageHistogramsAfterDelete(
-      offline_pages_, offline_ids, GetCurrentTime());
+    std::unique_ptr<StoreUpdateResult> result) {
+  ReportPageHistogramsAfterDelete(offline_pages_, result->updated_items,
+                                  GetCurrentTime());
 
-  for (int64_t offline_id : offline_ids) {
+  // This pat of the loop is explicitly broken out, as it should be gone in

[dougarnett, 2016/09/20 16:12:44]

pat => part ?

[fgorski, 2016/09/20 18:43:55]

Done.

+  // fully asynchronous code.
+  for (const auto& page : result->updated_items) {
+    int64_t offline_id = page.offline_id;
     offline_event_logger_.RecordPageDeleted(std::to_string(offline_id));
     auto iter = offline_pages_.find(offline_id);
     if (iter == offline_pages_.end())
       continue;
-    FOR_EACH_OBSERVER(
-        Observer, observers_,
-        OfflinePageDeleted(iter->second.offline_id, iter->second.client_id));
     offline_pages_.erase(iter);
   }
 
-  // Deleting multiple pages always succeeds when it gets to this point.
-  InformDeletePageDone(callback, (success || offline_ids.size() > 1)
-                                     ? DeletePageResult::SUCCESS
-                                     : DeletePageResult::STORE_FAILURE);
+  for (const auto& page : result->updated_items) {
+    FOR_EACH_OBSERVER(Observer, observers_,
+                      OfflinePageDeleted(page.offline_id, page.client_id));
+  }
+
+  // TODO(fgorski): React the FAILED_INITIALIZATION, FAILED_RESET here.
+  DeletePageResult delete_result;
+  if (result->store_state == StoreState::LOADED)
+    delete_result = DeletePageResult::SUCCESS;

[dougarnett, 2016/09/20 16:12:44]

Is this mapping accurate? Does the DeletePageResult here just mean whether the
request was able to be successfully attempted against the store vs. whether
items were actually found and deleted (vs. DOESNT_EXIST)? If so, might be good
to briefly comment that intent.

[fgorski, 2016/09/20 18:43:55]

1. I agree we have some work to do here. For now we are not changing the
semantics of this call, but I'll add the TODO for fixing the offline page model
interface and pass more reasonable result.

2. The behavior you ask for is already documented with DeletePageResult  in
offline_page_types.h. The reason we don't treat DOESNT_EXIST as failure is that
we want to remove the items, and they are gone, so no issue there, from the
caller perspective. Result is the same. As per point 1 this behavior definitely
requires a refinement.

[dougarnett, 2016/09/20 20:22:10]

Ah, ok, I see the deprecated comment on NOT_FOUND now (was just looking at the
top comment and SUCCESS).

+  else
+    delete_result = DeletePageResult::STORE_FAILURE;
+
+  InformDeletePageDone(callback, delete_result);
 }
 
 void OfflinePageModelImpl::InformDeletePageDone(
     const DeletePageCallback& callback,
     DeletePageResult result) {
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.DeletePageResult",
                             static_cast<int>(result),
                             static_cast<int>(DeletePageResult::RESULT_COUNT));
   archive_manager_->GetStorageStats(
       base::Bind(&ReportStorageHistogramsAfterDelete));
@@ skipping 150 matching lines @@
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, task);
 }
 
 base::Time OfflinePageModelImpl::GetCurrentTime() const {
   return testing_clock_ ? testing_clock_->Now() : base::Time::Now();
 }
 
 }  // namespace offline_pages