[Offline Pages] Fix cases where returning NOT_FOUND when deleting.

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..d5d04e554b4499e96a3fee1fde180b458ab608dc 100644
--- a/components/offline_pages/offline_page_model_impl.cc
+++ b/components/offline_pages/offline_page_model_impl.cc
@@ -341,6 +341,12 @@ void OfflinePageModelImpl::DoDeletePagesByOfflineId(
     const DeletePageCallback& callback) {
   DCHECK(is_loaded_);
 
+  // If an empty list of offline id is passed in, consider deletion as success.
+  if (offline_ids.empty()) {
+    InformDeletePageDone(callback, DeletePageResult::SUCCESS);
+    return;
+  }
+
   std::vector<base::FilePath> paths_to_delete;
   for (const auto& offline_id : offline_ids) {
     auto iter = offline_pages_.find(offline_id);
@@ -349,6 +355,10 @@ void OfflinePageModelImpl::DoDeletePagesByOfflineId(
     }
   }
 
+  // Shortcut here, if none of the offline id is found and no other valid

[fgorski, 2016/06/21 18:02:23]

Too long of the comment.

If there are no pages to delete, return early.

[romax, 2016/06/21 19:41:37]

Done.

+  // offline page to delete, just invoke the callback as not found. But if only
+  // some of the pages are not found, will go through the process and delete the
+  // valid ones, and invoke as not found when deletion completed.
   if (paths_to_delete.empty()) {
     InformDeletePageDone(callback, DeletePageResult::NOT_FOUND);
     return;
@@ -395,7 +405,12 @@ void OfflinePageModelImpl::DoDeletePagesByURLPredicate(
     if (predicate.Run(id_page_pair.second.url))
       offline_ids.push_back(id_page_pair.first);
   }
-  DoDeletePagesByOfflineId(offline_ids, callback);
+  // If no pages found by |predicate|, will invoke callback with not found.
+  if (offline_ids.empty()) {
+    InformDeletePageDone(callback, DeletePageResult::NOT_FOUND);
+  } else {
+    DoDeletePagesByOfflineId(offline_ids, callback);
+  }
 }
 
 void OfflinePageModelImpl::HasPages(const std::string& name_space,
@@ -833,15 +848,22 @@ void OfflinePageModelImpl::OnRemoveOfflinePagesDone(
 
   // Delete the offline page from the in memory cache regardless of success in
   // store.
+  bool page_not_found = false;

[fgorski, 2016/06/21 18:02:23]

I don't think we need this logic.
If you identified a page for deletion earlier, you consider it a success when
the page is gone.

[romax, 2016/06/21 19:41:37]

Done.

   for (int64_t offline_id : offline_ids) {
     auto iter = offline_pages_.find(offline_id);
-    if (iter == offline_pages_.end())
+    if (iter == offline_pages_.end()) {
+      page_not_found = true;
       continue;
+    }
     FOR_EACH_OBSERVER(
         Observer, observers_,
         OfflinePageDeleted(iter->second.offline_id, iter->second.client_id));
     offline_pages_.erase(iter);
   }
+
+  if (page_not_found)
+    InformDeletePageDone(callback, DeletePageResult::NOT_FOUND);
+
   // Deleting multiple pages always succeeds when it gets to this point.
   InformDeletePageDone(callback, (success || offline_ids.size() > 1)
                                      ? DeletePageResult::SUCCESS