[Offline pages] Detecting missing offline copy

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..f5ff60330227a435082c9aaa7af02facbeee87f6 100644
--- a/components/offline_pages/offline_page_model.cc
+++ b/components/offline_pages/offline_page_model.cc
@@ -67,6 +67,17 @@ void DeleteArchiveFiles(const std::vector<base::FilePath>& paths_to_delete,
 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(
@@ -203,6 +214,22 @@ const OfflinePageItem* OfflinePageModel::GetPageByOfflineURL(
   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();
 }
@@ -301,6 +328,8 @@ void OfflinePageModel::OnLoadDone(
   delayed_tasks_.clear();
 
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelLoaded(this));
+
+  CheckMetadataConsistency();
 }
 
 void OfflinePageModel::InformSavePageDone(const SavePageCallback& callback,
@@ -357,6 +386,10 @@ void OfflinePageModel::OnRemoveOfflinePagesDone(
       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,
@@ -368,4 +401,17 @@ void OfflinePageModel::InformDeletePageDone(const DeletePageCallback& callback,
   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