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 {
+class DefaultDelegate: public offline_pages::RecentTabHelper::Delegate {
+ public:
+  DefaultDelegate() {}
 
-// 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;
+  // 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 {
 
 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(
-      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();
+  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());
+
+  // TODO(dimich): When we have BackgroundOffliner, avoid capturing prerenderer
+  // WebContents with its origin as well.
+  never_do_snapshots_ = tab_id_.empty() ||
+                        web_contents()->GetBrowserContext()->IsOffTheRecord();
+}
+
 void RecentTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (navigation_handle->IsInMainFrame() &&
       navigation_handle->HasCommitted()) {
     // Cancel tasks in flight that relate to the previous page.
     weak_ptr_factory_.InvalidateWeakPtrs();
 
     // New navigation, new snapshot session.
     snapshot_url_ = GURL();
     GURL last_committed_url = web_contents()->GetLastCommittedURL();
@@ skipping 24 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) {
 
-  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;
   }
 
   if (!IsSamePage()) {
     ReportSnapshotCompleted();
     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),
       base::Bind(&RecentTabHelper::SavePageCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RecentTabHelper::SavePageCallback(OfflinePageModel::SavePageResult result,
                                        int64_t offline_id) {
   ReportSnapshotCompleted();
 }
 
 void RecentTabHelper::ReportSnapshotCompleted() {
   snapshot_controller_->PendingSnapshotCompleted();
 }
 
 bool RecentTabHelper::IsSamePage() const {
   return web_contents() &&
          (web_contents()->GetLastCommittedURL() == snapshot_url_);
 }
 
 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