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

components/offline_pages/offline_page_model_impl.cc

 // Copyright 2016 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_impl.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
@@ skipping 288 matching lines @@
       archives_dir_, offline_id,
       base::Bind(&OfflinePageModelImpl::OnCreateArchiveDone,
                  weak_ptr_factory_.GetWeakPtr(), url, offline_id, client_id,
                  base::Time::Now(), callback));
   pending_archivers_.push_back(std::move(archiver));
 }
 
 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
   // be updated upon the successful store operation.
   OfflinePageItem offline_page_item = iter->second;
 
   ReportPageHistogramsAfterAccess(offline_page_item);
 
   offline_page_item.last_access_time = base::Time::Now();
   offline_page_item.access_count++;
@@ skipping 87 matching lines @@
 }
 
 void OfflinePageModelImpl::HasPagesAfterLoadDone(
     const std::string& name_space,
     const HasPagesCallback& callback) const {
   DCHECK(is_loaded_);
 
   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;
     }
   }
 
   callback.Run(has_pages);
 }
 
 void OfflinePageModelImpl::CheckPagesExistOffline(
     const std::set<GURL>& urls,
     const CheckPagesExistOfflineCallback& callback) {
   RunWhenLoaded(
       base::Bind(&OfflinePageModelImpl::CheckPagesExistOfflineAfterLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), urls, callback));
 }
 
 void OfflinePageModelImpl::CheckPagesExistOfflineAfterLoadDone(
     const std::set<GURL>& urls,
     const CheckPagesExistOfflineCallback& callback) {
   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);
   }
   callback.Run(result);
 }
 
 void OfflinePageModelImpl::GetAllPages(
     const MultipleOfflinePageItemCallback& callback) {
   RunWhenLoaded(base::Bind(&OfflinePageModelImpl::GetAllPagesAfterLoadDone,
                            weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void OfflinePageModelImpl::GetAllPagesAfterLoadDone(
     const MultipleOfflinePageItemCallback& callback) const {
   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);
 }
 
 void OfflinePageModelImpl::GetOfflineIdsForClientId(
     const ClientId& client_id,
     const MultipleOfflineIdCallback& callback) {
   RunWhenLoaded(
       base::Bind(&OfflinePageModelImpl::GetOfflineIdsForClientIdWhenLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), client_id, callback));
 }
 
 void OfflinePageModelImpl::GetOfflineIdsForClientIdWhenLoadDone(
     const ClientId& client_id,
     const MultipleOfflineIdCallback& callback) const {
   callback.Run(MaybeGetOfflineIdsForClientId(client_id));
 }
 
 const std::vector<int64_t> OfflinePageModelImpl::MaybeGetOfflineIdsForClientId(
     const ClientId& client_id) const {
   DCHECK(is_loaded_);
   std::vector<int64_t> results;
 
   // 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;
 }
 
 void OfflinePageModelImpl::GetPageByOfflineId(
     int64_t offline_id,
     const SingleOfflinePageItemCallback& callback) {
   RunWhenLoaded(
       base::Bind(&OfflinePageModelImpl::GetPageByOfflineIdWhenLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), offline_id, callback));
 }
 
 void OfflinePageModelImpl::GetPageByOfflineIdWhenLoadDone(
     int64_t offline_id,
     const SingleOfflinePageItemCallback& callback) const {
   SingleOfflinePageItemResult result;
   const OfflinePageItem* match = MaybeGetPageByOfflineId(offline_id);
   if (match != nullptr)
     result = *match;
   callback.Run(result);
 }
 
 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(
     const GURL& offline_url,
     const SingleOfflinePageItemCallback& callback) {
   RunWhenLoaded(
       base::Bind(&OfflinePageModelImpl::GetPageByOfflineURLWhenLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), offline_url, callback));
 }
 
 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_) {
     if (id_page_pair.second.GetOfflineURL() == offline_url) {
       callback.Run(base::make_optional(id_page_pair.second));
       return;
     }
   }
 
   callback.Run(base::nullopt);
 }
 
 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);
   }
   return nullptr;
 }
 
 void OfflinePageModelImpl::GetBestPageForOnlineURL(
     const GURL& online_url,
     const SingleOfflinePageItemCallback callback) {
@@ skipping 21 matching lines @@
       base::Bind(&OfflinePageModelImpl::GetPagesByOnlineURLWhenLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), online_url, callback));
 }
 
 void OfflinePageModelImpl::GetPagesByOnlineURLWhenLoadDone(
     const GURL& online_url,
     const MultipleOfflinePageItemCallback& callback) const {
   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);
 }
 
 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);
     }
   }
   return result;
 }
 
 void OfflinePageModelImpl::CheckForExternalFileDeletion() {
   DCHECK(is_loaded_);
 
@@ skipping 242 matching lines @@
     DeletePageResult result) {
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.DeletePageResult",
                             static_cast<int>(result),
                             static_cast<int>(DeletePageResult::RESULT_COUNT));
   archive_manager_->GetStorageStats(
       base::Bind(&ReportStorageHistogramsAfterDelete));
   if (!callback.is_null())
     callback.Run(result);
 }
 
 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));
-
-  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::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));
+  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));
   }
 }
 
+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::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(
     const base::Closure& callback,
     DeletePageResult result) {
   store_->Reset(base::Bind(&OfflinePageModelImpl::OnResetStoreDoneForClearAll,
                            weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void OfflinePageModelImpl::OnResetStoreDoneForClearAll(
     const base::Closure& callback,
     bool success) {
@@ skipping 50 matching lines @@
 void OfflinePageModelImpl::RunWhenLoaded(const base::Closure& task) {
   if (!is_loaded_) {
     delayed_tasks_.push_back(task);
     return;
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, task);
 }
 
 }  // namespace offline_pages