Support undoing offline page deletion

components/offline_pages/offline_page_model.cc

Index: components/offline_pages/offline_page_model.cc
diff --git a/components/offline_pages/offline_page_model.cc b/components/offline_pages/offline_page_model.cc
index a3eac9486398fe786dd25e4fe748fb5d0e330972..f5d993a69b7b0d6267b87d410e59ee1be2e6f23c 100644
--- a/components/offline_pages/offline_page_model.cc
+++ b/components/offline_pages/offline_page_model.cc
@@ -12,6 +12,7 @@
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
@@ -30,6 +31,11 @@ namespace {
 // delete it to free up space.
 const base::TimeDelta kPageCleanUpThreshold = base::TimeDelta::FromDays(30);
 
+// The delay for the final deletion to kick in after the page is marked for
+// deletion. The value set here is a bit longer that the duration of the
+// snackbar that offers undo.
+const base::TimeDelta kFinalDeletionDelay = base::TimeDelta::FromSeconds(6);
+
 SavePageResult ToSavePageResult(ArchiverResult archiver_result) {
   SavePageResult result;
   switch (archiver_result) {
@@ -125,10 +131,30 @@ void OfflinePageModel::MarkPageAccessed(int64 bookmark_id) {
   offline_page_item.access_count++;
   store_->AddOrUpdateOfflinePage(
       offline_page_item,
-      base::Bind(&OfflinePageModel::OnUpdateOfflinePageDone,
+      base::Bind(&OfflinePageModel::OnMarkPageAccesseDone,
                  weak_ptr_factory_.GetWeakPtr(), offline_page_item));
 }
 
+void OfflinePageModel::MarkPageForDeletion(
+    int64 bookmark_id,
+    const DeletePageCallback& callback) {
+  DCHECK(is_loaded_);
+  auto iter = offline_pages_.find(bookmark_id);
+  if (iter == offline_pages_.end()) {
+    InformDeletePageDone(callback, DeletePageResult::NOT_FOUND);
+    return;
+  }
+
+  // Make a copy of the cached item and update it. The cached item should only
+  // be updated upon the successful store operation.
+  OfflinePageItem offline_page_item = iter->second;
+  offline_page_item.MarkForDeletion();
+  store_->AddOrUpdateOfflinePage(
+      offline_page_item,
+      base::Bind(&OfflinePageModel::OnMarkPageForDeletionDone,
+                 weak_ptr_factory_.GetWeakPtr(), offline_page_item, callback));
+}
+
 void OfflinePageModel::DeletePageByBookmarkId(
     int64 bookmark_id,
     const DeletePageCallback& callback) {
@@ -170,8 +196,11 @@ void OfflinePageModel::DeletePagesByBookmarkId(
 const std::vector<OfflinePageItem> OfflinePageModel::GetAllPages() const {
   DCHECK(is_loaded_);
   std::vector<OfflinePageItem> offline_pages;
-  for (const auto& id_page_pair : offline_pages_)
+  for (const auto& id_page_pair : offline_pages_) {
+    if (id_page_pair.second.IsMarkedForDeletion())
+      continue;
     offline_pages.push_back(id_page_pair.second);
+  }
   return offline_pages;
 }
 
@@ -180,8 +209,10 @@ const std::vector<OfflinePageItem> OfflinePageModel::GetPagesToCleanUp() const {
   std::vector<OfflinePageItem> offline_pages;
   base::Time now = base::Time::Now();
   for (const auto& id_page_pair : offline_pages_) {
-    if (now - id_page_pair.second.creation_time > kPageCleanUpThreshold)
+    if (!id_page_pair.second.IsMarkedForDeletion() &&
+        now - id_page_pair.second.creation_time > kPageCleanUpThreshold) {
       offline_pages.push_back(id_page_pair.second);
+    }
   }
   return offline_pages;
 }
@@ -257,13 +288,53 @@ void OfflinePageModel::OnAddOfflinePageDone(OfflinePageArchiver* archiver,
   DeletePendingArchiver(archiver);
 }
 
-void OfflinePageModel::OnUpdateOfflinePageDone(
+void OfflinePageModel::OnMarkPageAccesseDone(
     const OfflinePageItem& offline_page_item, bool success) {
   // Update the item in the cache only upon success.
   if (success)
     offline_pages_[offline_page_item.bookmark_id] = offline_page_item;
 }
 
+void OfflinePageModel::OnMarkPageForDeletionDone(
+    const OfflinePageItem& offline_page_item,
+    const DeletePageCallback& callback,
+    bool success) {
+  // Update the item in the cache only upon success.
+  if (success)
+    offline_pages_[offline_page_item.bookmark_id] = offline_page_item;
+
+  InformDeletePageDone(callback, success ? DeletePageResult::SUCCESS
+                                         : DeletePageResult::STORE_FAILURE);
+
+  if (!success)
+    return;
+
+  // Schedule to do the final deletion.
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageModel::FinalizePageDeletion,
+                 weak_ptr_factory_.GetWeakPtr()),
+      kFinalDeletionDelay);
+}
+
+void OfflinePageModel::OnUndoOfflinePageDone(
+    const OfflinePageItem& offline_page, int64 old_bookmark_id, bool success) {
+  if (!success)
+    return;
+  offline_pages_.erase(old_bookmark_id);
+  offline_pages_[offline_page.bookmark_id] = offline_page;
+}
+
+void OfflinePageModel::FinalizePageDeletion() {
+  std::vector<int64> bookmark_ids_pending_deletion;
+  for (const auto& id_page_pair : offline_pages_) {
+    if (!id_page_pair.second.IsMarkedForDeletion())
+      continue;
+    bookmark_ids_pending_deletion.push_back(id_page_pair.second.bookmark_id);
+  }
+  DeletePagesByBookmarkId(bookmark_ids_pending_deletion, DeletePageCallback());
+}
+
 void OfflinePageModel::BookmarkModelChanged() {
 }
 
@@ -275,13 +346,41 @@ void OfflinePageModel::BookmarkNodeRemoved(
     const std::set<GURL>& removed_urls) {
   if (!is_loaded_) {
     delayed_tasks_.push_back(
-        base::Bind(&OfflinePageModel::DeletePageByBookmarkId,
+        base::Bind(&OfflinePageModel::MarkPageForDeletion,
                    weak_ptr_factory_.GetWeakPtr(),
                    node->id(),
                    base::Bind(&EmptyDeleteCallback)));
     return;
   }
-  DeletePageByBookmarkId(node->id(), base::Bind(&EmptyDeleteCallback));
+  MarkPageForDeletion(node->id(), base::Bind(&EmptyDeleteCallback));
+}
+
+void OfflinePageModel::BookmarkNodeRenumbered(bookmarks::BookmarkModel* model,
+                                              int64 old_bookmark_id,
+                                              int64 new_bookmark_id) {
+  // The old bookmark that is marked for deletion should still exist.
+  auto old_page_item_iter = offline_pages_.find(old_bookmark_id);
+  if (old_page_item_iter == offline_pages_.end() ||
+      !old_page_item_iter->second.IsMarkedForDeletion()) {
+    NOTREACHED();
+    return;
+  }
+
+  // The new bookmark should not exist in our metadata record.
+  if (offline_pages_.find(new_bookmark_id) != offline_pages_.end()) {
+    NOTREACHED();
+    return;
+  }
+
+  // Write the old metadata with the new bookmark ID to the store.
+  OfflinePageItem new_page_item = old_page_item_iter->second;
+  new_page_item.bookmark_id = new_bookmark_id;
+  new_page_item.ClearMarkForDeletion();
+  store_->AddOrUpdateOfflinePage(
+      new_page_item,
+      base::Bind(&OfflinePageModel::OnUndoOfflinePageDone,
+                 weak_ptr_factory_.GetWeakPtr(), new_page_item,
+                 old_bookmark_id));
 }
 
 void OfflinePageModel::OnLoadDone(
@@ -300,6 +399,10 @@ void OfflinePageModel::OnLoadDone(
     delayed_task.Run();
   delayed_tasks_.clear();
 
+  // If there are pages that are marked for deletion, but not yet deleted and
+  // OfflinePageModel gets reloaded. Delete the pages now.

[fgorski, 2015/09/25 21:23:57]

Your first sentence does not conclude. How about:

If there were pages pending deletion when OfflinePageModel was reloaded, they
will be deleted now.

[jianli, 2015/10/09 23:26:26]

Done.

+  FinalizePageDeletion();
+
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelLoaded(this));
 }
 
@@ -365,7 +468,8 @@ void OfflinePageModel::InformDeletePageDone(const DeletePageCallback& callback,
       "OfflinePages.DeletePageResult",
       static_cast<int>(result),
       static_cast<int>(DeletePageResult::RESULT_COUNT));
-  callback.Run(result);
+  if (!callback.is_null())
+    callback.Run(result);
 }
 
 }  // namespace offline_pages