[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..585adf684c3112444aecabc6388137b205d71659 100644
--- a/components/offline_pages/offline_page_model_impl.cc
+++ b/components/offline_pages/offline_page_model_impl.cc
@@ -796,6 +796,15 @@ 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
+  // user initiated.
+  if (result == SavePageResult::SUCCESS &&
+      OfflinePageModel::IsUserInitiated(client_id)) {

[Dmitry Titov, 2016/06/16 04:07:15]

So far, we seem to gravitate to a model where each namespace maintains one page
per online URL.
However, this is not clearly the only solution we need - for example, when using
download action, user may download the same page multiple times - for example,
various snapshots of his Facebook timeline or today's job openings etc. When
saving other downloads, traditional UX concept is to add a new entry rather then
override. We might end up with similar approach in Download Home.

On the other hand, 'cache'-like namespaces like bookmarking or lastN might want
to keep only the latest snapshot of the online URL. 

I would not link user-initiated with this policy because apparently it can be
different case by case. Also, the method like this that adds some
client-specific classification to the OPM shoudl not be on OPM... Perhaps better
variant would be to pass additional parameter to SavePage, a boolean
'overwriteExistingPages' or something like that which will modify behavior of
SavePage - then specific callers could invoke SavePage differently. At the place
of call it is always known what we want in terms of overwrite behavior.

A agree the purge should be confined to a namspace.

[fgorski, 2016/06/16 15:05:13]

I think we already have the policy object which should be used for the purpose
of making the call:
Keep one or allow multiple.
I agree that this call can be orthogonal from whether the snapshot is user
initiated, especially because bookmarked page is technically cache, that was
triggered by the user behavior, which makes it something in between the two.

Please use OfflinePageClientPolicy here.

+    GetPagesByOnlineURL(
+        offline_pages_[offline_id].url,
+        base::Bind(&OfflinePageModelImpl::OnPagesFoundWithSameURL,
+                   weak_ptr_factory_.GetWeakPtr(), client_id, offline_id));
+  }
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&OfflinePageModelImpl::ClearStorageIfNeeded,
                             weak_ptr_factory_.GetWeakPtr(),
@@ -804,6 +813,28 @@ void OfflinePageModelImpl::InformSavePageDone(const SavePageCallback& callback,
   callback.Run(result, offline_id);
 }
 
+void OfflinePageModelImpl::OnPagesFoundWithSameURL(
+    const ClientId& client_id,
+    int64_t offline_id,
+    const MultipleOfflinePageItemResult& items) {
+  std::vector<int64_t> 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.offline_id);
+    }
+  }
+  DeletePagesByOfflineId(
+      pages_to_delete,
+      base::Bind(&OfflinePageModelImpl::OnDeleteOldPagesWithSameURL,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void OfflinePageModelImpl::OnDeleteOldPagesWithSameURL(
+    DeletePageResult result) {
+  // TODO(romax) Seems like nothing to do here.

[Dmitry Titov, 2016/06/16 04:07:15]

UMA on failures would be interesting.

+}
+
 void OfflinePageModelImpl::DeletePendingArchiver(
     OfflinePageArchiver* archiver) {
   pending_archivers_.erase(std::find(pending_archivers_.begin(),