[Offline Pages] Duplicate pages when save/delete bookmarks.

components/offline_pages/offline_page_model_impl.cc

Index: components/offline_pages/offline_page_model_impl.cc
diff --git a/components/offline_pages/offline_page_model_impl.cc b/components/offline_pages/offline_page_model_impl.cc
index 4d6f1d7af3939455a3ab139c1001cfda815afc1a..c0f50078d8eb162fb9d38884642639d2b963f17f 100644
--- a/components/offline_pages/offline_page_model_impl.cc
+++ b/components/offline_pages/offline_page_model_impl.cc
@@ -796,12 +796,65 @@ void OfflinePageModelImpl::InformSavePageDone(const SavePageCallback& callback,
   ReportSavePageResultHistogramAfterSave(client_id, result);
   archive_manager_->GetStorageStats(
       base::Bind(&ReportStorageHistogramsAfterSave));
+  // Remove other user initiated pages with same url if the new save request was

[Dmitry Titov, 2016/06/23 04:23:15]

this comment can be re-phrsed without "user initiated" but with mentioning of
policy.

[romax, 2016/06/23 22:24:03]

Done.

+  // user initiated.
+  size_t pages_allowed =
+      policy_controller_->GetPolicy(client_id.name_space).pages_allowed_per_url;
+  if (result == SavePageResult::SUCCESS && pages_allowed > 0) {

[Dmitry Titov, 2016/06/23 04:23:15]

pages_allowed > 0 looks strange here.

If it can never be <=0 and you guard against invalid initialization of policies,
then it's better to do it in the policy controller, possibly clamping it to a
valid range.

If <=0 can be valid values and mean something (looking at this code, I can start
thinking that it means "no limit") then this should be documented in the policy
class.

[romax, 2016/06/23 22:24:04]

Documented in the client policy class, and since the type is size_t, I don't
think it will be <0. But made some changes to make it looks more reasonable.

+    GetPagesByOnlineURL(
+        offline_pages_[offline_id].url,
+        base::Bind(&OfflinePageModelImpl::OnPagesFoundWithSameURL,
+                   weak_ptr_factory_.GetWeakPtr(), client_id, offline_id,
+                   pages_allowed));
+  } else {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(

[Dmitry Titov, 2016/06/23 04:23:15]

This PostTask call is repeated in OnDeleteOldPagesWithSameURL(). It'd be nice to
extract it into separate method to show clearly that it's the same step. 

[romax, 2016/06/23 22:24:03]

Done.

+        FROM_HERE,
+        base::Bind(&OfflinePageModelImpl::ClearStorageIfNeeded,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   base::Bind(&OfflinePageModelImpl::OnStorageCleared,
+                              weak_ptr_factory_.GetWeakPtr())));
+  }
+  callback.Run(result, offline_id);
+}
+
+void OfflinePageModelImpl::OnPagesFoundWithSameURL(
+    const ClientId& client_id,
+    int64_t offline_id,
+    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) {
+      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;
+         });
+    pages_to_delete.resize(pages_to_delete.size() - pages_allowed + 1);
+  }
+  std::vector<int64_t> page_ids_to_delete;
+  for (const auto& item : pages_to_delete)
+    page_ids_to_delete.push_back(item.offline_id);
+  DeletePagesByOfflineId(
+      page_ids_to_delete,
+      base::Bind(&OfflinePageModelImpl::OnDeleteOldPagesWithSameURL,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void OfflinePageModelImpl::OnDeleteOldPagesWithSameURL(
+    DeletePageResult result) {
+  // TODO(romax) Add UMAs for failure cases.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&OfflinePageModelImpl::ClearStorageIfNeeded,
                             weak_ptr_factory_.GetWeakPtr(),
                             base::Bind(&OfflinePageModelImpl::OnStorageCleared,
                                        weak_ptr_factory_.GetWeakPtr())));
-  callback.Run(result, offline_id);
 }
 
 void OfflinePageModelImpl::DeletePendingArchiver(