[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 <utility>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "components/bookmarks/browser/bookmark_model.h"
@@ skipping 48 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 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(
     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 140 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::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();
 }
 
 void OfflinePageModel::OnCreateArchiveDone(const GURL& requested_url,
                                            int64 bookmark_id,
                                            const SavePageCallback& callback,
                                            OfflinePageArchiver* archiver,
                                            ArchiverResult archiver_result,
                                            const GURL& url,
@@ skipping 67 matching lines @@
   if (!success)
     return;
 
   // Schedule to do the final deletion.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&OfflinePageModel::FinalizePageDeletion,
                  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(
     const OfflinePageItem& offline_page, bool success) {
   if (!success)
     return;
   offline_pages_[offline_page.bookmark_id] = offline_page;
 
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelChanged(this));
 }
@@ skipping 69 matching lines @@
   // Run all the delayed tasks.
   for (const auto& delayed_task : delayed_tasks_)
     delayed_task.Run();
   delayed_tasks_.clear();
 
   // If there are pages that are marked for deletion, but not yet deleted and
   // OfflinePageModel gets reloaded. Delete the pages now.
   FinalizePageDeletion();
 
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelLoaded(this));
+
+  CheckForExternalFileDeletion();
 }
 
 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 48 matching lines @@
 void OfflinePageModel::InformDeletePageDone(const DeletePageCallback& callback,
                                             DeletePageResult result) {
   UMA_HISTOGRAM_ENUMERATION(
       "OfflinePages.DeletePageResult",
       static_cast<int>(result),
       static_cast<int>(DeletePageResult::RESULT_COUNT));
   if (!callback.is_null())
     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