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

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 323 matching lines @@
     return;
   }
   DoDeletePagesByOfflineId(offline_ids, callback);
 }
 
 void OfflinePageModelImpl::DoDeletePagesByOfflineId(
     const std::vector<int64_t>& offline_ids,
     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);
     if (iter != offline_pages_.end() && !iter->second.IsExpired()) {
       paths_to_delete.push_back(iter->second.file_path);
     }
   }
 
+  // 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;
   }
 
   archive_manager_->DeleteMultipleArchives(
       paths_to_delete,
       base::Bind(&OfflinePageModelImpl::OnDeleteArchiveFilesDone,
                  weak_ptr_factory_.GetWeakPtr(), offline_ids, callback));
 }
@@ skipping 26 matching lines @@
 void OfflinePageModelImpl::DoDeletePagesByURLPredicate(
     const UrlPredicate& predicate,
     const DeletePageCallback& callback) {
   DCHECK(is_loaded_);
 
   std::vector<int64_t> offline_ids;
   for (const auto& id_page_pair : offline_pages_) {
     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,
                                     const HasPagesCallback& callback) {
   RunWhenLoaded(base::Bind(&OfflinePageModelImpl::HasPagesAfterLoadDone,
                            weak_ptr_factory_.GetWeakPtr(), name_space,
                            callback));
 }
 
 void OfflinePageModelImpl::HasPagesAfterLoadDone(
@@ skipping 417 matching lines @@
 }
 
 void OfflinePageModelImpl::OnRemoveOfflinePagesDone(
     const std::vector<int64_t>& offline_ids,
     const DeletePageCallback& callback,
     bool success) {
   ReportPageHistogramsAfterDelete(offline_pages_, offline_ids);
 
   // 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
                                      : DeletePageResult::STORE_FAILURE);
 }
 
 void OfflinePageModelImpl::InformDeletePageDone(
     const DeletePageCallback& callback,
     DeletePageResult result) {
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.DeletePageResult",
@@ skipping 128 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