Change the RecentTabHelper to only capture the last one page in a tab.

chrome/browser/android/offline_pages/recent_tab_helper.cc

 // Copyright (c) 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 "chrome/browser/android/offline_pages/recent_tab_helper.h"
 
 #include <queue>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/location.h"
-#include "base/logging.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
+#include "chrome/browser/android/tab_android.h"
 #include "components/offline_pages/client_namespace_constants.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/offline_page_model.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::RecentTabHelper);
 
-namespace {
-
-// Max number of pages to keep. The oldest pages that are over this count are
-// deleted before the next one is saved.
-const size_t kMaxPagesToKeep = 50;
-
-// Predicate for priority_queue used to compute the oldest pages.
-struct ComparePagesForPurge {
-  bool operator()(const offline_pages::OfflinePageItem* left,
-                 const offline_pages::OfflinePageItem* right) const {
-    return left->creation_time > right->creation_time;
-  }
-};
-
-}  // namespace
-
 namespace offline_pages {
 
 RecentTabHelper::RecentTabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       page_model_(nullptr),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   snapshot_controller_.reset(new SnapshotController(
       base::ThreadTaskRunnerHandle::Get(), this));
   page_model_ = OfflinePageModelFactory::GetForBrowserContext(
@@ skipping 44 matching lines @@
   snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
 }
 
 // This starts a sequence of async operations chained through callbacks:
 // - compute the set of old 'last_n' pages that have to be purged
 // - delete the pages found in the previous step
 // - snapshot the current web contents
 // Along the chain, the original URL is passed and compared, to detect
 // possible navigation and cancel snapshot in that case.
 void RecentTabHelper::StartSnapshot() {
-  // TODO(dimich): Implement automatic cleanup as per design doc, based on
-  // storage limits and page age.
-  // This algorithm (remove pages before making sure the save was successful)
-  // prefers keeping the storage use low and under control by potentially
-  // sacrificing the current snapshot.
+  // Remove previously captured pages for this tab.
   page_model_->GetOfflineIdsForClientId(
     client_id(),
     base::Bind(&RecentTabHelper::ContinueSnapshotWithIdsToPurge,
                weak_ptr_factory_.GetWeakPtr()));
 }
 
-// Collects following pages from lastN namespace:
-// - the ones with the same online URL
-// - the oldest pages, enough to keep kMaxPagesToKeep limit.
 void RecentTabHelper::ContinueSnapshotWithIdsToPurge(
     const std::vector<int64_t>& page_ids) {
 

[fgorski, 2016/06/24 17:49:20]

nit: remove the empty line, please.

-  std::vector<int64_t> pages_to_purge;
-  // Use priority queue to figure out the set of oldest pages to purge.
-  std::priority_queue<const OfflinePageItem*,
-                      std::vector<const OfflinePageItem*>,
-                      ComparePagesForPurge> pages_queue;
-
-  for (const auto& offline_id : page_ids) {
-    // TODO(dimich): get rid of 'Maybe'. Maybe make it return multiple pages.
-    const OfflinePageItem* page =
-        page_model_->MaybeGetPageByOfflineId(offline_id);
-    // If there is already a snapshot of this page, remove it so we don't
-    // have multiple snapshots of the same page.
-    if (page->url == snapshot_url_) {
-      pages_to_purge.push_back(offline_id);
-    } else {
-      pages_queue.push(page);
-    }
-  }
-
-  // Negative counter means nothing else to purge.
-  int count_to_purge =
-      page_ids.size() - kMaxPagesToKeep - pages_to_purge.size();
-
-  for (int count = 0; count < count_to_purge; ++count) {
-    pages_to_purge.push_back(pages_queue.top()->offline_id);
-    pages_queue.pop();
-  }
-
   page_model_->DeletePagesByOfflineId(
-      pages_to_purge, base::Bind(&RecentTabHelper::ContinueSnapshotAfterPurge,
-                                 weak_ptr_factory_.GetWeakPtr()));
+      page_ids, base::Bind(&RecentTabHelper::ContinueSnapshotAfterPurge,
+                           weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RecentTabHelper::ContinueSnapshotAfterPurge(
     OfflinePageModel::DeletePageResult result) {
   if (result != OfflinePageModel::DeletePageResult::SUCCESS) {
     // If previous pages can't be deleted, don't add new ones.
     ReportSnapshotCompleted();
     return;
   }
 
@@ skipping 23 matching lines @@
 
 void RecentTabHelper::ReportSnapshotCompleted() {
   snapshot_controller_->PendingSnapshotCompleted();
 }
 
 bool RecentTabHelper::IsSamePage() const {
   return web_contents() &&
          (web_contents()->GetLastCommittedURL() == snapshot_url_);
 }
 
 ClientId RecentTabHelper::client_id() const {

[fgorski, 2016/06/24 17:49:20]

could we please add client_id() verification in the unit tests?
It does not need to be exposing the method, but if it could simulate 2 tabs and
make sure that there is at most one copy per tab it would probably be supportive
of the cause.

[dewittj, 2016/06/24 17:55:46]

nit: This should be renamed GetClientId since it is no longer a trivial
function.

-  return ClientId(kLastNNamespace, "");
+  TabAndroid* tab_android = TabAndroid::FromWebContents(web_contents());
+  if (!tab_android) {
+    return ClientId(kLastNNamespace, "");

[fgorski, 2016/06/24 17:49:20]

Please document what behavior you expect here. E.g.

// If we cannot get Tab ID, we are reverting to Last N == 1 globally.

Or something like that.

[dewittj, 2016/06/24 17:55:46]

Is there a case where we can't get tab ID?  If so it might be more prudent to
just disable this feature there.

+  } else {
+    std::string tab_id = base::IntToString(tab_android->GetAndroidId());
+    return ClientId(kLastNNamespace, tab_id);
+  }
 }
 
 void RecentTabHelper::SetArchiveFactoryForTest(
     std::unique_ptr<TestArchiveFactory> test_archive_factory) {
   test_archive_factory_ = std::move(test_archive_factory);
 }
 
 void RecentTabHelper::SetTaskRunnerForTest(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
   snapshot_controller_.reset(new SnapshotController(task_runner, this));
 }
 
 }  // namespace offline_pages