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..44d53fb9b7cb4415fd5753ec2d249eb1836bf4cd 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"
@@ -27,19 +28,29 @@
 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;
+class DefaultDelegate: public offline_pages::RecentTabHelper::Delegate {
+ public:
+  DefaultDelegate() {}
+
+  // offline_pages::RecentTabHelper::Delegate
+  std::unique_ptr<offline_pages::OfflinePageArchiver> CreatePageArchiver(
+      content::WebContents* web_contents) override {
+    std::unique_ptr<offline_pages::OfflinePageArchiver> archiver(
+        new offline_pages::OfflinePageMHTMLArchiver(web_contents));
+    return archiver;

[dewittj, 2016/06/24 23:10:33]

nit:
return base::MakeUnique<offline_pages::OffinePageArchiver>(web_contents);

[Dmitry Titov, 2016/06/24 23:58:01]

Done.

+  }
+  scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() override {
+    return base::ThreadTaskRunnerHandle::Get();
+  }
+  bool GetTabId(int* tab_id, content::WebContents* web_contents) override {

[dewittj, 2016/06/24 23:10:33]

nit: outparams usually go after inparams in my experience. 

Also, consider base::Optional :)

[Dmitry Titov, 2016/06/24 23:58:01]

Done. Not Optional...

+    *tab_id = 0;

[dewittj, 2016/06/24 23:10:33]

nit: don't set tab_id on failure.

[Dmitry Titov, 2016/06/24 23:58:01]

Done.

+    TabAndroid* tab_android = TabAndroid::FromWebContents(web_contents);
+    if (!tab_android)
+      return false;
+    *tab_id = tab_android->GetAndroidId();
+    return true;
   }
 };
-
 }  // namespace
 
 namespace offline_pages {
@@ -49,17 +60,31 @@ RecentTabHelper::RecentTabHelper(content::WebContents* web_contents)
       page_model_(nullptr),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  snapshot_controller_.reset(new SnapshotController(
-      base::ThreadTaskRunnerHandle::Get(), this));
+  Initialize(std::unique_ptr<DefaultDelegate>(new DefaultDelegate()));
+}
+
+RecentTabHelper::~RecentTabHelper() {
+}
+
+void RecentTabHelper::Initialize(
+    std::unique_ptr<RecentTabHelper::Delegate> delegate) {
+  DCHECK(delegate.get());

[dewittj, 2016/06/24 23:10:33]

I think you can just use the boolean conversion:
DCHECK(delegate);

[Dmitry Titov, 2016/06/24 23:58:01]

Done.

+  delegate_ = std::move(delegate);
+  snapshot_controller_.reset(new SnapshotController(delegate_->GetTaskRunner(),
+                                                    this));
+  int tab_id_number = 0;
+  if (delegate_->GetTabId(&tab_id_number, web_contents()))
+    tab_id_ = base::IntToString(tab_id_number);
+  else
+    tab_id_.clear();
+
   page_model_ = OfflinePageModelFactory::GetForBrowserContext(
-      web_contents->GetBrowserContext());
+      web_contents()->GetBrowserContext());
 
   // TODO(dimich): When we have BackgroundOffliner, avoid capturing prerenderer
   // WebContents with its origin as well.
-  never_do_snapshots_ = web_contents->GetBrowserContext()->IsOffTheRecord();
-}
-
-RecentTabHelper::~RecentTabHelper() {
+  never_do_snapshots_ = tab_id_.empty() ||
+                        web_contents()->GetBrowserContext()->IsOffTheRecord();
 }
 
 void RecentTabHelper::DidFinishNavigation(
@@ -106,54 +131,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) {
 
-  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(
@@ -169,13 +159,8 @@ void RecentTabHelper::ContinueSnapshotAfterPurge(
     return;
   }
 
-  // Create either test Archiver or a regular one.
-  std::unique_ptr<OfflinePageArchiver> archiver;
-  if (test_archive_factory_.get()) {
-    archiver = test_archive_factory_->CreateArchiver(web_contents());
-  } else {
-    archiver.reset(new OfflinePageMHTMLArchiver(web_contents()));
-  }
+  std::unique_ptr<OfflinePageArchiver> archiver(
+      delegate_->CreatePageArchiver(web_contents()));

[dewittj, 2016/06/24 23:10:33]

nit: move this right into the SavePage call.

[Dmitry Titov, 2016/06/24 23:58:01]

Done.

 
   page_model_->SavePage(
       snapshot_url_, client_id(), std::move(archiver),
@@ -198,17 +183,7 @@ bool RecentTabHelper::IsSamePage() const {
 }
 
 ClientId RecentTabHelper::client_id() const {
-  return ClientId(kLastNNamespace, "");
-}
-
-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));
+  return ClientId(kLastNNamespace, tab_id_);
 }
 
 }  // namespace offline_pages