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 17e499bde753056e803db37f02ff624147ada661..7e99ee6e56108cc100b1320c1b0b02c4f6f76bed 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.last_access_time > kPageCleanUpThreshold)
+    if (!id_page_pair.second.IsMarkedForDeletion() &&
+        now - id_page_pair.second.last_access_time > kPageCleanUpThreshold) {
       offline_pages.push_back(id_page_pair.second);
+    }
   }
   return offline_pages;
 }
@@ -257,16 +288,79 @@ 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, bool success) {
+  if (!success)
+    return;
+  offline_pages_[offline_page.bookmark_id] = offline_page;

[fgorski, 2015/10/12 20:59:52]

you are missing a callback that will tell the caller that the deletion did not
go through DeletePageResult::CANCELED

[jianli, 2015/10/12 23:33:34]

We don't need to fire a callback since it has already been done at line 306.
When undo is done, the observer will kick in to update the UI.

+}
+
+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() {
 }
 
+void OfflinePageModel::BookmarkNodeAdded(bookmarks::BookmarkModel* model,
+                                         const bookmarks::BookmarkNode* parent,
+                                         int index) {
+  const bookmarks::BookmarkNode* node = parent->GetChild(index);
+  DCHECK(node);
+
+  auto iter = offline_pages_.find(node->id());
+  if (iter == offline_pages_.end())
+    return;
+
+  OfflinePageItem offline_page_item = iter->second;
+  if (!offline_page_item.IsMarkedForDeletion())
+    return;
+
+  // Clear the flag to bring it back.
+  // Make a copy of the cached item and update it. The cached item should only
+  // be updated upon the successful store operation.
+  offline_page_item.ClearMarkForDeletion();
+  store_->AddOrUpdateOfflinePage(
+      offline_page_item,
+      base::Bind(&OfflinePageModel::OnUndoOfflinePageDone,
+                 weak_ptr_factory_.GetWeakPtr(), offline_page_item));
+}
+
 void OfflinePageModel::BookmarkNodeRemoved(
     bookmarks::BookmarkModel* model,
     const bookmarks::BookmarkNode* parent,
@@ -275,13 +369,13 @@ 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::OnLoadDone(
@@ -300,6 +394,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.
+  FinalizePageDeletion();
+
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelLoaded(this));
 }
 
@@ -365,7 +463,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