[Offline Pages] Filtering expired pages and fix consistency check.

components/offline_pages/offline_page_model_impl.cc

Index: components/offline_pages/offline_page_model_impl.cc
diff --git a/components/offline_pages/offline_page_model_impl.cc b/components/offline_pages/offline_page_model_impl.cc
index b356c6a96834d67f29de9d14459c2c9aa6626c89..5a10c9ed8fef7c645f26f08998c6d4f66d3f3e41 100644
--- a/components/offline_pages/offline_page_model_impl.cc
+++ b/components/offline_pages/offline_page_model_impl.cc
@@ -306,7 +306,7 @@ void OfflinePageModelImpl::SavePage(
 void OfflinePageModelImpl::MarkPageAccessed(int64_t offline_id) {
   DCHECK(is_loaded_);
   auto iter = offline_pages_.find(offline_id);
-  if (iter == offline_pages_.end())
+  if (iter == offline_pages_.end() || iter->second.IsExpired())
     return;
 
   // Make a copy of the cached item and update it. The cached item should only
@@ -414,7 +414,8 @@ void OfflinePageModelImpl::HasPagesAfterLoadDone(
   bool has_pages = false;
 
   for (const auto& id_page_pair : offline_pages_) {
-    if (id_page_pair.second.client_id.name_space == name_space) {
+    if (id_page_pair.second.client_id.name_space == name_space &&
+        !id_page_pair.second.IsExpired()) {
       has_pages = true;
       break;
     }
@@ -437,6 +438,8 @@ void OfflinePageModelImpl::CheckPagesExistOfflineAfterLoadDone(
   DCHECK(is_loaded_);
   CheckPagesExistOfflineResult result;
   for (const auto& id_page_pair : offline_pages_) {
+    if (id_page_pair.second.IsExpired())
+      continue;
     auto iter = urls.find(id_page_pair.second.url);
     if (iter != urls.end())
       result.insert(*iter);
@@ -455,8 +458,10 @@ void OfflinePageModelImpl::GetAllPagesAfterLoadDone(
   DCHECK(is_loaded_);
 
   MultipleOfflinePageItemResult offline_pages;
-  for (const auto& id_page_pair : offline_pages_)
-    offline_pages.push_back(id_page_pair.second);
+  for (const auto& id_page_pair : offline_pages_) {
+    if (!id_page_pair.second.IsExpired())
+      offline_pages.push_back(id_page_pair.second);
+  }
 
   callback.Run(offline_pages);
 }
@@ -483,8 +488,10 @@ const std::vector<int64_t> OfflinePageModelImpl::MaybeGetOfflineIdsForClientId(
   // We want only all pages, including those marked for deletion.
   // TODO(bburns): actually use an index rather than linear scan.
   for (const auto& id_page_pair : offline_pages_) {
-    if (id_page_pair.second.client_id == client_id)
+    if (id_page_pair.second.client_id == client_id &&
+        !id_page_pair.second.IsExpired()) {
       results.push_back(id_page_pair.second.offline_id);
+    }
   }
   return results;
 }
@@ -510,7 +517,9 @@ void OfflinePageModelImpl::GetPageByOfflineIdWhenLoadDone(
 const OfflinePageItem* OfflinePageModelImpl::MaybeGetPageByOfflineId(
     int64_t offline_id) const {
   const auto iter = offline_pages_.find(offline_id);
-  return iter != offline_pages_.end() ? &(iter->second) : nullptr;
+  return iter != offline_pages_.end() && !iter->second.IsExpired()
+             ? &(iter->second)
+             : nullptr;
 }
 
 void OfflinePageModelImpl::GetPageByOfflineURL(
@@ -524,6 +533,9 @@ void OfflinePageModelImpl::GetPageByOfflineURL(
 void OfflinePageModelImpl::GetPageByOfflineURLWhenLoadDone(
     const GURL& offline_url,
     const SingleOfflinePageItemCallback& callback) const {
+  // Getting pages by offline URL does not exclude expired pages, as the caller
+  // already holds the offline URL and simply needs to look up a corresponding
+  // online URL.
   base::Optional<OfflinePageItem> result;
 
   for (const auto& id_page_pair : offline_pages_) {
@@ -538,6 +550,9 @@ void OfflinePageModelImpl::GetPageByOfflineURLWhenLoadDone(
 
 const OfflinePageItem* OfflinePageModelImpl::MaybeGetPageByOfflineURL(
     const GURL& offline_url) const {
+  // Getting pages by offline URL does not exclude expired pages, as the caller
+  // already holds the offline URL and simply needs to look up a corresponding
+  // online URL.
   for (const auto& id_page_pair : offline_pages_) {
     if (id_page_pair.second.GetOfflineURL() == offline_url)
       return &(id_page_pair.second);
@@ -579,8 +594,10 @@ void OfflinePageModelImpl::GetPagesByOnlineURLWhenLoadDone(
   std::vector<OfflinePageItem> result;
 
   for (const auto& id_page_pair : offline_pages_) {
-    if (id_page_pair.second.url == online_url)
+    if (id_page_pair.second.url == online_url &&
+        !id_page_pair.second.IsExpired()) {
       result.push_back(id_page_pair.second);
+    }
   }
 
   callback.Run(result);
@@ -590,7 +607,8 @@ const OfflinePageItem* OfflinePageModelImpl::MaybeGetBestPageForOnlineURL(
     const GURL& online_url) const {
   const OfflinePageItem* result = nullptr;
   for (const auto& id_page_pair : offline_pages_) {
-    if (id_page_pair.second.url == online_url) {
+    if (id_page_pair.second.url == online_url &&
+        !id_page_pair.second.IsExpired()) {
       if (!result || id_page_pair.second.creation_time > result->creation_time)
         result = &(id_page_pair.second);
     }
@@ -856,38 +874,50 @@ void OfflinePageModelImpl::InformDeletePageDone(
 void OfflinePageModelImpl::ScanForMissingArchiveFiles(

[fgorski, 2016/06/07 22:46:15]

I think you added functionality to this method and it no longer matches the
name.

I suggest 2 separate methods instead. I know that you will iterate twice, but I
don't see that as a problem.

[romax, 2016/06/08 01:31:25]

Done.

     const std::set<base::FilePath>& archive_paths) {
   std::vector<int64_t> ids_of_pages_missing_archive_file;
-  std::vector<std::pair<int64_t, ClientId>> offline_client_id_pairs;
+  std::set<base::FilePath> unused_archives(archive_paths);

[fgorski, 2016/06/07 22:46:15]

I like the idea you used here.
I think a comment, such as: Archives are considered unused until proven
otherwise. Will explain this thing nicely.

[romax, 2016/06/08 01:31:25]

Done.

+
   for (const auto& id_page_pair : offline_pages_) {
-    if (archive_paths.count(id_page_pair.second.file_path) == 0UL) {
+    const auto& iter = unused_archives.find(id_page_pair.second.file_path);
+    if (iter == unused_archives.end()) {
       ids_of_pages_missing_archive_file.push_back(id_page_pair.first);
-      offline_client_id_pairs.push_back(
-          std::make_pair(id_page_pair.first, id_page_pair.second.client_id));
+    } else {
+      unused_archives.erase(iter);
     }
   }
 
-  // No offline pages missing archive files, we can bail out.
-  if (ids_of_pages_missing_archive_file.empty())
-    return;
+  if (!ids_of_pages_missing_archive_file.empty()) {
+    ExpirePages(ids_of_pages_missing_archive_file, base::Time::Now(),
+                base::Bind(&OfflinePageModelImpl::OnExpireArchivelessFilesDone,
+                           weak_ptr_factory_.GetWeakPtr(),
+                           ids_of_pages_missing_archive_file));
+  }
 
-  DeletePageCallback remove_pages_done_callback(base::Bind(
-      &OfflinePageModelImpl::OnRemoveOfflinePagesMissingArchiveFileDone,
-      weak_ptr_factory_.GetWeakPtr(), offline_client_id_pairs));
+  if (!unused_archives.empty()) {
+    std::vector<base::FilePath> headless_archives(unused_archives.begin(),

[fgorski, 2016/06/07 22:46:15]

orphaned_archives?

[romax, 2016/06/08 01:31:25]

i explained in previous comments, but I'm willing to change all names (and
Histogram ones) once we settled with a name if that doesn't make sense to you.

+                                                  unused_archives.end());
+    archive_manager_->DeleteMultipleArchives(
+        headless_archives,
+        base::Bind(&OfflinePageModelImpl::OnDeleteHeadlessArchivesDone,
+                   weak_ptr_factory_.GetWeakPtr(), headless_archives));
+  }
+}
 
-  store_->RemoveOfflinePages(
-      ids_of_pages_missing_archive_file,
-      base::Bind(&OfflinePageModelImpl::OnRemoveOfflinePagesDone,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 ids_of_pages_missing_archive_file,
-                 remove_pages_done_callback));
+void OfflinePageModelImpl::OnExpireArchivelessFilesDone(
+    const std::vector<int64_t>& offline_ids,
+    bool success) {
+  UMA_HISTOGRAM_COUNTS("OfflinePages.Consistency.ArchivelessPageCount",

[fgorski, 2016/06/07 22:46:15]

how about .PagesMissingArchiveFiles for the last segment?

[romax, 2016/06/08 01:31:25]

Done.

+                       static_cast<int32_t>(offline_ids.size()));
+  UMA_HISTOGRAM_BOOLEAN("OfflinePages.Consistency.ExpireArchivelessPagesResult",
+                        success);
 }
 
-void OfflinePageModelImpl::OnRemoveOfflinePagesMissingArchiveFileDone(
-    const std::vector<std::pair<int64_t, ClientId>>& offline_client_id_pairs,
-    DeletePageResult /* result */) {
-  for (const auto& id_pair : offline_client_id_pairs) {
-    FOR_EACH_OBSERVER(Observer, observers_,
-                      OfflinePageDeleted(id_pair.first, id_pair.second));
-  }
+void OfflinePageModelImpl::OnDeleteHeadlessArchivesDone(

[fgorski, 2016/06/07 22:46:15]

Again Orphaned feel better here and below.

[romax, 2016/06/08 01:31:25]

Acknowledged.

+    const std::vector<base::FilePath>& archives,
+    bool success) {
+  UMA_HISTOGRAM_COUNTS("OfflinePages.Consistency.HeadlessArchivesCount",
+                       static_cast<int32_t>(archives.size()));
+  UMA_HISTOGRAM_BOOLEAN("OfflinePages.Consistency.DeleteHeadlessArchivesResult",
+                        success);
 }
 
 void OfflinePageModelImpl::OnRemoveAllFilesDoneForClearAll(