[Offline pages] Detecting missing offline copy

components/offline_pages/offline_page_model.cc

 // Copyright 2015 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.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
@@ skipping 49 matching lines @@
   DCHECK(success);
   for (const auto& file_path : paths_to_delete) {
     // Make sure delete happens on the left of || so that it is always executed.
     *success = base::DeleteFile(file_path, false) || *success;
   }
 }
 
 void EmptyDeleteCallback(OfflinePageModel::DeletePageResult /* result */) {
 }
 
+void ListPagesMissingOfflineCopy(

[jianli, 2015/10/13 00:17:53]

ValidateArchiveFiles?

[fgorski, 2015/10/13 17:39:58]

Changed to ListPagesMissingArchiveFile since that is the only thing we are doing
here.

+    const std::vector<OfflinePageItem>& offline_pages,
+    std::vector<int64>* pages_missing_offline_copy) {
+  DCHECK(pages_missing_offline_copy);
+
+  for (const auto& offline_page : offline_pages) {
+    if (!base::PathExists(offline_page.file_path))
+      pages_missing_offline_copy->push_back(offline_page.bookmark_id);
+  }
+}
+
 }  // namespace
 
 OfflinePageModel::OfflinePageModel(
     scoped_ptr<OfflinePageMetadataStore> store,
     const scoped_refptr<base::SequencedTaskRunner>& task_runner)
     : store_(store.Pass()),
       is_loaded_(false),
       task_runner_(task_runner),
       scoped_observer_(this),
       weak_ptr_factory_(this) {
@@ skipping 116 matching lines @@
     const GURL& offline_url) const {
   for (auto iter = offline_pages_.begin();
        iter != offline_pages_.end();
        ++iter) {
     if (iter->second.GetOfflineURL() == offline_url)
       return &(iter->second);
   }
   return nullptr;
 }
 
+void OfflinePageModel::CheckMetadataConsistency() {
+  DCHECK(is_loaded_);
+
+  std::vector<OfflinePageItem> offline_pages;
+  for (const auto& id_page_pair : offline_pages_)

[jianli, 2015/10/13 00:17:54]

I think it might be better to combine this logic with the logic in line 157-163.
We can add a helper function to produce a list of pair of file path and
bookmark_id, like:
  void GetArchiveFiles(std::vector<std::pair<base::FilePath, int64>> files)

Change DeleteArchiveFiles to take this list and go through all files and return
those that are deleted successfully. Change OnDeleteArchiveFilesDone to update
the store accordingly.

Change ListPagesMissingOfflineCopy (better called as ValidateArchiveFiles) to
take this list.

[fgorski, 2015/10/13 17:39:58]

I don't understand what exactly is your expectation here. We can discuss this
when you are available.

+    offline_pages.push_back(id_page_pair.second);
+
+  std::vector<int64>* pages_missing_offline_copy = new std::vector<int64>();
+  task_runner_->PostTaskAndReply(
+      FROM_HERE, base::Bind(&ListPagesMissingOfflineCopy, offline_pages,
+                            pages_missing_offline_copy),
+      base::Bind(&OfflinePageModel::OnListPagesMissingOfflineCopy,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(pages_missing_offline_copy)));
+}
+
 OfflinePageMetadataStore* OfflinePageModel::GetStoreForTesting() {
   return store_.get();
 }
 
 void OfflinePageModel::OnCreateArchiveDone(const GURL& requested_url,
                                            int64 bookmark_id,
                                            const SavePageCallback& callback,
                                            OfflinePageArchiver* archiver,
                                            ArchiverResult archiver_result,
                                            const GURL& url,
@@ skipping 78 matching lines @@
     for (const auto& offline_page : offline_pages)
       offline_pages_[offline_page.bookmark_id] = offline_page;
   }
 
   // Run all the delayed tasks.
   for (const auto& delayed_task : delayed_tasks_)
     delayed_task.Run();
   delayed_tasks_.clear();
 
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelLoaded(this));
+
+  CheckMetadataConsistency();
 }
 
 void OfflinePageModel::InformSavePageDone(const SavePageCallback& callback,
                                           SavePageResult result) {
   UMA_HISTOGRAM_ENUMERATION(
       "OfflinePages.SavePageResult",
       static_cast<int>(result),
       static_cast<int>(SavePageResult::RESULT_COUNT));
   callback.Run(result);
 }
@@ skipping 36 matching lines @@
                                 1,
                                 base::TimeDelta::FromDays(365).InMinutes(),
                                 100);
     offline_pages_.erase(iter);
   }
   // Deleting multiple pages always succeeds when it gets to this point.
   InformDeletePageDone(
       callback,
       (success || bookmark_ids.size() > 1) ? DeletePageResult::SUCCESS
                                            : DeletePageResult::STORE_FAILURE);
+
+  for (int64 bookmark_id : bookmark_ids) {

[jianli, 2015/10/13 00:17:53]

nit: brackets are not needed

[fgorski, 2015/10/13 17:39:58]

Not true:
a) FOR_EACH_OBSERVER is in fact multi-line after macro is expanded.
b) compilation fails on windows without {}

+    FOR_EACH_OBSERVER(Observer, observers_, OfflinePageDeleted(bookmark_id));
+  }
 }
 
 void OfflinePageModel::InformDeletePageDone(const DeletePageCallback& callback,
                                             DeletePageResult result) {
   UMA_HISTOGRAM_ENUMERATION(
       "OfflinePages.DeletePageResult",
       static_cast<int>(result),
       static_cast<int>(DeletePageResult::RESULT_COUNT));
   callback.Run(result);
 }
 
+void OfflinePageModel::OnListPagesMissingOfflineCopy(
+    const std::vector<int64>* pages_missing_offline_copy) {
+  DCHECK(pages_missing_offline_copy);
+  if (pages_missing_offline_copy->empty())
+    return;
+
+  store_->RemoveOfflinePages(
+      *pages_missing_offline_copy,
+      base::Bind(&OfflinePageModel::OnRemoveOfflinePagesDone,
+                 weak_ptr_factory_.GetWeakPtr(), *pages_missing_offline_copy,
+                 base::Bind(&EmptyDeleteCallback)));
+}
+
 }  // namespace offline_pages