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

chrome/browser/android/offline_pages/recent_tab_helper.cc

Index: chrome/browser/android/offline_pages/recent_tab_helper.cc
diff --git a/chrome/browser/android/offline_pages/recent_tab_helper.cc b/chrome/browser/android/offline_pages/recent_tab_helper.cc
index 94b2c50df795d55b46a9046c62d201d56b4add2a..e636fac212ff18a728e6d68d37041900abc28527 100644
--- a/chrome/browser/android/offline_pages/recent_tab_helper.cc
+++ b/chrome/browser/android/offline_pages/recent_tab_helper.cc
@@ -9,13 +9,14 @@
 
 #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"
@@ -26,22 +27,6 @@
 
 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)
@@ -106,54 +91,19 @@ void RecentTabHelper::DocumentOnLoadCompletedInMainFrame() {
 // 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(
@@ -198,7 +148,13 @@ bool RecentTabHelper::IsSamePage() const {
 }
 
 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(