[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 3e93bf9b86b157b8db4373d286061ebf12a76b80..353166c530f94855ee5787c0c150979a6a4cc4a4 100644
--- a/components/offline_pages/offline_page_model.cc
+++ b/components/offline_pages/offline_page_model.cc
@@ -5,6 +5,7 @@
 #include "components/offline_pages/offline_page_model.h"
 
 #include <algorithm>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
@@ -73,6 +74,19 @@ void DeleteArchiveFiles(const std::vector<base::FilePath>& paths_to_delete,
 void EmptyDeleteCallback(OfflinePageModel::DeletePageResult /* result */) {
 }
 
+void ListPagesMissingArchiveFile(

[jianli, 2015/10/20 21:56:49]

I think FindPagesMissingArchiveFile sounds easier to understand than
ListPagesMissingArchiveFile.

[fgorski, 2015/10/21 20:01:33]

Done.

+    const std::vector<std::pair<int64, base::FilePath>>& id_path_pairs,
+    std::vector<int64>* ids_of_pages_missing_archive_file) {
+  DCHECK(ids_of_pages_missing_archive_file);
+
+  for (const auto& id_path : id_path_pairs) {
+    if (!base::PathExists(id_path.second) ||
+        !base::DirectoryExists(id_path.second)) {

[jianli, 2015/10/20 21:56:49]

The not operator should be removed for the 2nd test since if the path denotes a
directory, it should be treated as an error.

[fgorski, 2015/10/21 20:01:33]

Done.

+      ids_of_pages_missing_archive_file->push_back(id_path.first);
+    }
+  }
+}
+
 }  // namespace
 
 OfflinePageModel::OfflinePageModel(
@@ -233,6 +247,25 @@ const OfflinePageItem* OfflinePageModel::GetPageByOfflineURL(
   return nullptr;
 }
 
+void OfflinePageModel::CheckForExternalFileDeletion() {
+  DCHECK(is_loaded_);
+
+  std::vector<std::pair<int64, base::FilePath>> id_path_pairs;
+  for (const auto& id_page_pair : offline_pages_) {
+    id_path_pairs.push_back(
+        std::make_pair(id_page_pair.first, id_page_pair.second.file_path));
+  }
+
+  std::vector<int64>* ids_of_pages_missing_archive_file =
+      new std::vector<int64>();
+  task_runner_->PostTaskAndReply(
+      FROM_HERE, base::Bind(&ListPagesMissingArchiveFile, id_path_pairs,
+                            ids_of_pages_missing_archive_file),
+      base::Bind(&OfflinePageModel::OnListPagesMissingArchiveFileDone,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(ids_of_pages_missing_archive_file)));
+}
+
 OfflinePageMetadataStore* OfflinePageModel::GetStoreForTesting() {
   return store_.get();
 }
@@ -320,7 +353,8 @@ void OfflinePageModel::OnMarkPageForDeletionDone(
                  weak_ptr_factory_.GetWeakPtr()),
       kFinalDeletionDelay);
 
-  FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelChanged(this));
+  FOR_EACH_OBSERVER(
+      Observer, observers_, OfflinePageDeleted(offline_page_item.bookmark_id));

[jianli, 2015/10/20 21:56:48]

I think we should be firing both OfflinePageModelChanged and OfflinePageDeleted
events.

[fgorski, 2015/10/21 20:01:33]

I think triggering OfflinePageDeleted is enough and to the point. Plus it
carries specific information which helps observers react, while the other is
very generic and does not.

If we fire both events the UI listening to these might make a bookmark specific
update, followed by complete update, because you don't even know what just
happened.

[jianli, 2015/10/21 21:21:11]

Yes, we should avoid this. I now understand how bookmark model code handles
these events. I will fix this in another patch after you land this patch.

 }
 
 void OfflinePageModel::OnUndoOfflinePageDone(
@@ -410,6 +444,8 @@ void OfflinePageModel::OnLoadDone(
   FinalizePageDeletion();
 
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelLoaded(this));
+
+  CheckForExternalFileDeletion();
 }
 
 void OfflinePageModel::InformSavePageDone(const SavePageCallback& callback,
@@ -478,4 +514,32 @@ void OfflinePageModel::InformDeletePageDone(const DeletePageCallback& callback,
     callback.Run(result);
 }
 
+void OfflinePageModel::OnListPagesMissingArchiveFileDone(
+    const std::vector<int64>* ids_of_pages_missing_archive_file) {
+  DCHECK(ids_of_pages_missing_archive_file);
+  if (ids_of_pages_missing_archive_file->empty())
+    return;
+
+  DeletePageCallback done_callback(
+      base::Bind(&OfflinePageModel::OnRemovalOfPagesMissingArchiveFileDone,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 *ids_of_pages_missing_archive_file));
+
+  store_->RemoveOfflinePages(
+      *ids_of_pages_missing_archive_file,
+      base::Bind(&OfflinePageModel::OnRemoveOfflinePagesDone,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 *ids_of_pages_missing_archive_file,
+                 done_callback));
+}
+
+void OfflinePageModel::OnRemovalOfPagesMissingArchiveFileDone(

[jianli, 2015/10/20 21:56:49]

The name OnRemovalOfPagesMissingArchiveFileDone seems to be confusing. How about
OnRemoveMetadataOfPagesMissingArchiveFileDone?

[fgorski, 2015/10/21 20:01:33]

How about: OnRemoveOfflinePagesMissingArchiveFileDone?

I disagree with Metadata part, because this is a callback to RemoveOfflinePages.

+    const std::vector<int64>& bookmark_ids,
+    OfflinePageModel::DeletePageResult /* result */) {
+  for (int64 bookmark_id : bookmark_ids) {

[jianli, 2015/10/20 21:56:49]

nit: remove unneeded brackets

[fgorski, 2015/10/21 20:01:33]

No, because it won't compile on Windows

+    FOR_EACH_OBSERVER(Observer, observers_, OfflinePageDeleted(bookmark_id));
+  }
+}
+

[jianli, 2015/10/20 21:56:48]

nit: remove extra blank line

[fgorski, 2015/10/21 20:01:33]

Done.

+
 }  // namespace offline_pages