[Offline Pages] Refactoring deletion of pages with same urls.

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 722 matching lines @@
     result = SavePageResult::SUCCESS;
     ReportPageHistogramAfterSave(offline_page);
     offline_event_logger_.RecordPageSaved(
         offline_page.client_id.name_space, offline_page.url.spec(),
         std::to_string(offline_page.offline_id));
   } 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) {
   // Update the item in the cache only upon success.
   if (success)
     offline_pages_[offline_page_item.offline_id] = offline_page_item;
 
@@ skipping 46 matching lines @@
       kStorageManagerStartingDelay);
 }
 
 void OfflinePageModelImpl::InformSavePageDone(const SavePageCallback& callback,
                                               SavePageResult result,
                                               const ClientId& client_id,
                                               int64_t offline_id) {
   ReportSavePageResultHistogramAfterSave(client_id, result);
   archive_manager_->GetStorageStats(
       base::Bind(&ReportStorageHistogramsAfterSave));
-  // Remove existing pages generated by the same policy and with same url.
-  size_t pages_allowed =
-      policy_controller_->GetPolicy(client_id.name_space).pages_allowed_per_url;
-  if (result == SavePageResult::SUCCESS && pages_allowed != kUnlimitedPages) {
-    GetPagesByOnlineURL(
-        offline_pages_[offline_id].url,
-        base::Bind(&OfflinePageModelImpl::OnPagesFoundWithSameURL,
-                   weak_ptr_factory_.GetWeakPtr(), client_id, offline_id,
-                   pages_allowed));
-  } else {
-    PostClearStorageIfNeededTask();
-  }
   callback.Run(result, offline_id);
 }
 
+void OfflinePageModelImpl::DeleteExistingPagesWithSameURL(
+    const OfflinePageItem& offline_page) {
+  // Remove existing pages generated by the same policy and with same url.
+  size_t pages_allowed =
+      policy_controller_->GetPolicy(offline_page.client_id.name_space)
+          .pages_allowed_per_url;
+  if (pages_allowed != kUnlimitedPages) {

[fgorski, 2016/07/07 17:03:46]

nit:

if (pages_allowed == kUnlimited...)
  return;

+    GetPagesByOnlineURL(
+        offline_page.url,
+        base::Bind(&OfflinePageModelImpl::OnPagesFoundWithSameURL,
+                   weak_ptr_factory_.GetWeakPtr(), offline_page,
+                   pages_allowed));
+  }
+}
+
 void OfflinePageModelImpl::OnPagesFoundWithSameURL(
-    const ClientId& client_id,
-    int64_t offline_id,
+    const OfflinePageItem& offline_page,
     size_t pages_allowed,
     const MultipleOfflinePageItemResult& items) {
   std::vector<OfflinePageItem> pages_to_delete;
   for (const auto& item : items) {
-    if (item.offline_id != offline_id &&
-        item.client_id.name_space == client_id.name_space) {
+    if (item.offline_id != offline_page.offline_id &&
+        item.client_id.name_space == offline_page.client_id.name_space) {
       pages_to_delete.push_back(item);
     }
   }
   // Only keep |pages_allowed|-1 of most fresh pages and delete others, by
   // sorting pages with the oldest  ones first and resize the vector.
   if (pages_to_delete.size() >= pages_allowed) {
     sort(pages_to_delete.begin(), pages_to_delete.end(),
          [](const OfflinePageItem& a, const OfflinePageItem& b) -> bool {
            return a.last_access_time < b.last_access_time;
          });
@@ skipping 208 matching lines @@
 void OfflinePageModelImpl::RunWhenLoaded(const base::Closure& task) {
   if (!is_loaded_) {
     delayed_tasks_.push_back(task);
     return;
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, task);
 }
 
 }  // namespace offline_pages