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

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 778 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 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(),
                             base::Bind(&OfflinePageModelImpl::OnStorageCleared,
                                        weak_ptr_factory_.GetWeakPtr())));
   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(),
                                      pending_archivers_.end(), archiver));
 }
 
 void OfflinePageModelImpl::OnDeleteArchiveFilesDone(
     const std::vector<int64_t>& offline_ids,
     const DeletePageCallback& callback,
     bool success) {
@@ skipping 166 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