Split SnapshotController into online and offline versions.

To make the Resource Percentage work easier, we are splitting the
snapshot controller into a foreground and a background version.

Bug: 699313
Change-Id: Ideeea7d644a903290d9eeae2d7ab700ab0ff50b6
Reviewed-on: https://chromium-review.googlesource.com/1136023Reviewed-by: Dmitry Titov <dimich@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Commit-Queue: Peter Williamson <petewil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578351}

chrome/browser/offline_pages/background_loader_offliner.cc

@@ -239,7 +239,7 @@ bool BackgroundLoaderOffliner::LoadAndSave(
   // Load page attempt.
   loader_.get()->LoadPage(request.url());
 
-  snapshot_controller_ = SnapshotController::CreateForBackgroundOfflining(
+  snapshot_controller_ = std::make_unique<BackgroundSnapshotController>(
       base::ThreadTaskRunnerHandle::Get(), this, (bool)page_renovator_);
 
   return true;
@@ -327,7 +327,6 @@ void BackgroundLoaderOffliner::MarkLoadStartTime() {
 }
 
 void BackgroundLoaderOffliner::DocumentAvailableInMainFrame() {
-  snapshot_controller_->DocumentAvailableInMainFrame();
   is_low_bar_met_ = true;
 
   // Add this signal to signal_data_.
@@ -413,8 +412,8 @@ void BackgroundLoaderOffliner::DidFinishNavigation(
   RecordOffliningPreviewsUMA(pending_request_->client_id(), navigation_data);
 }
 
-void BackgroundLoaderOffliner::SetSnapshotControllerForTest(
-    std::unique_ptr<SnapshotController> controller) {
+void BackgroundLoaderOffliner::SetBackgroundSnapshotControllerForTest(
+    std::unique_ptr<BackgroundSnapshotController> controller) {
   snapshot_controller_ = std::move(controller);
 }
 

chrome/browser/offline_pages/background_loader_offliner.h

@@ -13,8 +13,8 @@
 #include "components/offline_pages/content/background_loader/background_loader_contents.h"
 #include "components/offline_pages/core/background/load_termination_listener.h"
 #include "components/offline_pages/core/background/offliner.h"
+#include "components/offline_pages/core/background_snapshot_controller.h"
 #include "components/offline_pages/core/offline_page_types.h"
-#include "components/offline_pages/core/snapshot_controller.h"
 #include "content/public/browser/web_contents_observer.h"
 
 namespace content {
@@ -43,7 +43,7 @@ class BackgroundLoaderOffliner
     : public Offliner,
       public background_loader::BackgroundLoaderContents::Delegate,
       public content::WebContentsObserver,
-      public SnapshotController::Client,
+      public BackgroundSnapshotController::Client,
       public ResourceLoadingObserver {
  public:
   BackgroundLoaderOffliner(
@@ -78,12 +78,12 @@ class BackgroundLoaderOffliner
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
 
-  // SnapshotController::Client implementation.
+  // BackgroundSnapshotController::Client implementation.
   void StartSnapshot() override;
   void RunRenovations() override;
 
-  void SetSnapshotControllerForTest(
-      std::unique_ptr<SnapshotController> controller);
+  void SetBackgroundSnapshotControllerForTest(
+      std::unique_ptr<BackgroundSnapshotController> controller);
 
   // ResourceLoadingObserver implemenation
   void ObserveResourceLoading(ResourceLoadingObserver::ResourceDataType type,
@@ -140,7 +140,7 @@ class BackgroundLoaderOffliner
   // Tracks pending request, if any.
   std::unique_ptr<SavePageRequest> pending_request_;
   // Handles determining when a page should be snapshotted.
-  std::unique_ptr<SnapshotController> snapshot_controller_;
+  std::unique_ptr<BackgroundSnapshotController> snapshot_controller_;
   // Callback when pending request completes.
   CompletionCallback completion_callback_;
   // Callback to report progress.

chrome/browser/offline_pages/background_loader_offliner_unittest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/offline_pages/background_loader_offliner.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/run_loop.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
@@ -34,6 +35,11 @@
 #include "net/http/http_response_headers.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+char kShortSnapshotDelayForTest[] =
+    "short-offline-page-snapshot-delay-for-test";
+};  // namespace
+
 namespace offline_pages {
 
 namespace {
@@ -209,15 +215,12 @@ class BackgroundLoaderOfflinerTest : public testing::Test {
 
   void CompleteLoading() {
     // Reset snapshot controller.
-    std::unique_ptr<SnapshotController> snapshot_controller(
-        new SnapshotController(base::ThreadTaskRunnerHandle::Get(),
-                               offliner_.get(),
-                               0L /* DelayAfterDocumentAvailable */,
-                               0L /* DelayAfterDocumentOnLoad */,
-                               0L /* DelayAfterRenovationsCompleted */,
-                               false /* DocumentAvailableTriggersSnapshot */,
-                               false /* RenovationsEnabled */));
-    offliner_->SetSnapshotControllerForTest(std::move(snapshot_controller));
+    std::unique_ptr<BackgroundSnapshotController> snapshot_controller(
+        new BackgroundSnapshotController(base::ThreadTaskRunnerHandle::Get(),
+                                         offliner_.get(),
+                                         false /* RenovationsEnabled */));
+    offliner_->SetBackgroundSnapshotControllerForTest(
+        std::move(snapshot_controller));
     // Call complete loading.
     offliner()->DocumentOnLoadCompletedInMainFrame();
     PumpLoop();
@@ -265,6 +268,10 @@ BackgroundLoaderOfflinerTest::BackgroundLoaderOfflinerTest()
 BackgroundLoaderOfflinerTest::~BackgroundLoaderOfflinerTest() {}
 
 void BackgroundLoaderOfflinerTest::SetUp() {
+  // Set the snapshot controller delay command line switch to short delays.
+  base::CommandLine* cl = base::CommandLine::ForCurrentProcess();
+  cl->AppendSwitch(kShortSnapshotDelayForTest);
+
   std::unique_ptr<TestLoadTerminationListener> listener =
       std::make_unique<TestLoadTerminationListener>();
   load_termination_listener_ = listener.get();

chrome/browser/offline_pages/recent_tab_helper.cc

@@ -167,7 +167,7 @@ bool RecentTabHelper::EnsureInitialized() {
   if (snapshot_controller_)  // Initialized already.
     return snapshots_enabled_;
 
-  snapshot_controller_ = SnapshotController::CreateForForegroundOfflining(
+  snapshot_controller_ = std::make_unique<SnapshotController>(
       base::ThreadTaskRunnerHandle::Get(), this);
   snapshot_controller_->Stop();  // It is reset when navigation commits.
 
@@ -407,10 +407,6 @@ void RecentTabHelper::StartSnapshot() {
   snapshot_controller_->PendingSnapshotCompleted();
 }
 
-void RecentTabHelper::RunRenovations() {
-  snapshot_controller_->RenovationsCompleted();
-}
-
 void RecentTabHelper::SaveSnapshotForDownloads(bool replace_latest) {
   DCHECK_NE(PageQuality::POOR, snapshot_controller_->current_page_quality());
 

chrome/browser/offline_pages/recent_tab_helper.h

@@ -63,7 +63,6 @@ class RecentTabHelper
 
   // SnapshotController::Client
   void StartSnapshot() override;
-  void RunRenovations() override;
 
   // Delegate that is used by RecentTabHelper to get external dependencies.
   // Default implementation lives in .cc file, while tests provide an override.

components/offline_pages/core/BUILD.gn

@@ -12,6 +12,8 @@ static_library("core") {
     "archive_manager.h",
     "archive_validator.cc",
     "archive_validator.h",
+    "background_snapshot_controller.cc",
+    "background_snapshot_controller.h",
     "client_id.cc",
     "client_id.h",
     "client_namespace_constants.cc",
@@ -158,6 +160,7 @@ source_set("unit_tests") {
   sources = [
     "archive_manager_unittest.cc",
     "archive_validator_unittest.cc",
+    "background_snapshot_controller_unittest.cc",
     "client_policy_controller_unittest.cc",
     "model/add_page_task_unittest.cc",
     "model/add_page_to_download_manager_task_unittest.cc",

components/offline_pages/core/background_snapshot_controller.cc

+// Copyright 2018 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/core/background_snapshot_controller.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/time/time.h"
+#include "components/offline_pages/core/offline_page_feature.h"
+
+namespace {
+// Default delay, in milliseconds, between the main document OnLoad event and
+// snapshot.
+const int64_t kDelayAfterDocumentOnLoadCompletedMsBackground = 2000;
+
+// Default delay, in milliseconds, between renovations finishing and
+// taking a snapshot. Allows for page to update in response to the
+// renovations.
+const int64_t kDelayAfterRenovationsCompletedMs = 2000;
+
+// Delay for testing to keep polling times reasonable.
+const int64_t kDelayForTests = 0;
+
+}  // namespace
+
+namespace offline_pages {
+
+BackgroundSnapshotController::BackgroundSnapshotController(
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    BackgroundSnapshotController::Client* client,
+    bool renovations_enabled)
+    : task_runner_(task_runner),
+      client_(client),
+      state_(State::READY),
+      delay_after_document_on_load_completed_ms_(
+          kDelayAfterDocumentOnLoadCompletedMsBackground),
+      delay_after_renovations_completed_ms_(kDelayAfterRenovationsCompletedMs),
+      renovations_enabled_(renovations_enabled),
+      weak_ptr_factory_(this) {
+  if (offline_pages::ShouldUseTestingSnapshotDelay()) {
+    delay_after_document_on_load_completed_ms_ = kDelayForTests;
+    delay_after_renovations_completed_ms_ = kDelayForTests;
+  }
+}
+
+BackgroundSnapshotController::~BackgroundSnapshotController() {}
+
+void BackgroundSnapshotController::Reset() {
+  // Cancel potentially delayed tasks that relate to the previous 'session'.
+  weak_ptr_factory_.InvalidateWeakPtrs();
+  state_ = State::READY;
+}
+
+void BackgroundSnapshotController::Stop() {
+  state_ = State::STOPPED;
+}
+
+void BackgroundSnapshotController::RenovationsCompleted() {
+  if (renovations_enabled_) {
+    task_runner_->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(
+            &BackgroundSnapshotController::MaybeStartSnapshotThenStop,
+            weak_ptr_factory_.GetWeakPtr()),
+        base::TimeDelta::FromMilliseconds(
+            delay_after_renovations_completed_ms_));
+  }
+}
+
+void BackgroundSnapshotController::DocumentOnLoadCompletedInMainFrame() {
+  if (renovations_enabled_) {
+    // Run renovations. After renovations complete, a snapshot will be
+    // triggered after a delay.
+    client_->RunRenovations();
+  } else {
+    // Post a delayed task to snapshot and then stop this controller.
+    task_runner_->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(
+            &BackgroundSnapshotController::MaybeStartSnapshotThenStop,
+            weak_ptr_factory_.GetWeakPtr()),
+        base::TimeDelta::FromMilliseconds(
+            delay_after_document_on_load_completed_ms_));
+  }
+}
+
+void BackgroundSnapshotController::MaybeStartSnapshot() {
+  if (state_ != State::READY)
+    return;
+  state_ = State::SNAPSHOT_PENDING;
+  client_->StartSnapshot();
+}
+
+void BackgroundSnapshotController::MaybeStartSnapshotThenStop() {
+  MaybeStartSnapshot();
+  Stop();
+}
+
+int64_t
+BackgroundSnapshotController::GetDelayAfterDocumentOnLoadCompletedForTest() {
+  return delay_after_document_on_load_completed_ms_;
+}
+
+int64_t
+BackgroundSnapshotController::GetDelayAfterRenovationsCompletedForTest() {
+  return delay_after_renovations_completed_ms_;
+}
+
+}  // namespace offline_pages

components/offline_pages/core/background_snapshot_controller.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_BACKGROUND_SNAPSHOT_CONTROLLER_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_BACKGROUND_SNAPSHOT_CONTROLLER_H_
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
+
+namespace offline_pages {
+
+// Takes various signals and produces StartSnapshot calls following a specific
+// policy.
+// Main invariants:
+// - Some signals prevent more snapshots to be taken.
+//   OnLoad is currently such signal.
+// - Once Reset() is called on the BackgroundSnapshotController, the delayed
+//   tasks are reset so no StartSnapshot calls is made 'cross-session'.
+class BackgroundSnapshotController {
+ public:
+  enum class State {
+    READY,             // Listening to input, will start snapshot when needed.
+    SNAPSHOT_PENDING,  // Snapshot is in progress, don't start another.
+    STOPPED,           // Terminal state, no snapshots until reset.
+  };
+  // The expected quality of a page based on its current loading progress.
+  enum class PageQuality {
+    // Not loaded enough to reach a minimum level of quality. Snapshots taken at
+    // this point are expected to be useless.
+    POOR = 0,
+    // A minimal level of quality has been attained but the page is still
+    // loading so its quality is continuously increasing. One or more snapshots
+    // could be taken at this stage as later ones are expected to have higher
+    // quality.
+    FAIR_AND_IMPROVING,
+    // The page is loaded enough and has attained its peak expected quality.
+    // Snapshots taken at this point are not expected to increase in quality
+    // after the first one.
+    HIGH,
+  };
+
+  // Client of the BackgroundSnapshotController.
+  class Client {
+   public:
+    // Invoked at a good moment to start a snapshot.
+    virtual void StartSnapshot() = 0;
+
+    // Invoked when the page is sufficiently loaded for running
+    // renovations. The client should call the RenovationsCompleted()
+    // when they finish.
+    virtual void RunRenovations() = 0;
+
+   protected:
+    virtual ~Client() {}
+  };
+
+  BackgroundSnapshotController(
+      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+      BackgroundSnapshotController::Client* client,
+      bool renovations_enabled);
+  virtual ~BackgroundSnapshotController();
+
+  // Resets the 'session', returning controller to initial state.
+  void Reset();
+
+  // Stops current session, no more Client::StartSnapshot calls will be
+  // invoked from the BackgroundSnapshotController until current session is
+  // Reset(). Called by Client, for example when it encounters an error loading
+  // the page.
+  void Stop();
+
+  // The Client calls this when renovations have completed.
+  void RenovationsCompleted();
+
+  // Invoked from WebContentObserver::DocumentOnLoadCompletedInMainFrame
+  void DocumentOnLoadCompletedInMainFrame();
+
+  int64_t GetDelayAfterDocumentOnLoadCompletedForTest();
+  int64_t GetDelayAfterRenovationsCompletedForTest();
+
+ private:
+  void MaybeStartSnapshot();
+  void MaybeStartSnapshotThenStop();
+
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  // Client owns this class.
+  BackgroundSnapshotController::Client* client_;
+  BackgroundSnapshotController::State state_;
+  int64_t delay_after_document_on_load_completed_ms_;
+  int64_t delay_after_renovations_completed_ms_;
+  bool renovations_enabled_;
+
+  base::WeakPtrFactory<BackgroundSnapshotController> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(BackgroundSnapshotController);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_BACKGROUND_SNAPSHOT_CONTROLLER_H_

components/offline_pages/core/background_snapshot_controller_unittest.cc

+// Copyright 2018 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/core/background_snapshot_controller.h"
+
+#include "base/bind.h"
+#include "base/run_loop.h"
+#include "base/single_thread_task_runner.h"
+#include "base/test/test_mock_time_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace offline_pages {
+
+class BackgroundSnapshotControllerTest
+    : public testing::Test,
+      public BackgroundSnapshotController::Client {
+ public:
+  BackgroundSnapshotControllerTest();
+  ~BackgroundSnapshotControllerTest() override;
+
+  BackgroundSnapshotController* controller() { return controller_.get(); }
+  int snapshot_count() { return snapshot_count_; }
+
+  // testing::Test
+  void SetUp() override;
+  void TearDown() override;
+
+  // BackgroundSnapshotController::Client
+  void StartSnapshot() override;
+  void RunRenovations() override;
+
+  // Utility methods.
+  // Runs until all of the tasks that are not delayed are gone from the task
+  // queue.
+  void PumpLoop();
+  // Fast-forwards virtual time by |delta|, causing tasks with a remaining
+  // delay less than or equal to |delta| to be executed.
+  void FastForwardBy(base::TimeDelta delta);
+
+ private:
+  std::unique_ptr<BackgroundSnapshotController> controller_;
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
+  bool snapshot_started_;
+  int snapshot_count_;
+};
+
+BackgroundSnapshotControllerTest::BackgroundSnapshotControllerTest()
+    : task_runner_(new base::TestMockTimeTaskRunner),
+      snapshot_started_(true),
+      snapshot_count_(0) {}
+
+BackgroundSnapshotControllerTest::~BackgroundSnapshotControllerTest() {}
+
+void BackgroundSnapshotControllerTest::SetUp() {
+  controller_ =
+      std::make_unique<BackgroundSnapshotController>(task_runner_, this, false);
+  snapshot_started_ = true;
+}
+
+void BackgroundSnapshotControllerTest::TearDown() {
+  controller_.reset();
+}
+
+void BackgroundSnapshotControllerTest::StartSnapshot() {
+  snapshot_count_++;
+}
+
+void BackgroundSnapshotControllerTest::RunRenovations() {
+  controller_->RenovationsCompleted();
+}
+
+void BackgroundSnapshotControllerTest::PumpLoop() {
+  task_runner_->RunUntilIdle();
+}
+
+void BackgroundSnapshotControllerTest::FastForwardBy(base::TimeDelta delta) {
+  task_runner_->FastForwardBy(delta);
+}
+
+TEST_F(BackgroundSnapshotControllerTest, OnLoad) {
+  // Onload should make snapshot after its delay.
+  controller()->DocumentOnLoadCompletedInMainFrame();
+  PumpLoop();
+  EXPECT_EQ(0, snapshot_count());
+  FastForwardBy(base::TimeDelta::FromMilliseconds(
+      controller()->GetDelayAfterDocumentOnLoadCompletedForTest()));
+  EXPECT_EQ(1, snapshot_count());
+}
+
+TEST_F(BackgroundSnapshotControllerTest, Stop) {
+  // OnDOM should make snapshot after a delay.
+  controller()->DocumentOnLoadCompletedInMainFrame();
+  PumpLoop();
+  EXPECT_EQ(0, snapshot_count());
+  controller()->Stop();
+  FastForwardBy(base::TimeDelta::FromMilliseconds(
+      controller()->GetDelayAfterDocumentOnLoadCompletedForTest()));
+  // Should not start snapshots
+  EXPECT_EQ(0, snapshot_count());
+  // Also should not start snapshot.
+  controller()->DocumentOnLoadCompletedInMainFrame();
+  EXPECT_EQ(0, snapshot_count());
+}
+
+// This simulated a Reset while there is ongoing snapshot, which is reported
+// as done later. That reporting should have no effect nor crash.
+TEST_F(BackgroundSnapshotControllerTest, ClientReset) {
+  controller()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardBy(base::TimeDelta::FromMilliseconds(
+      controller()->GetDelayAfterDocumentOnLoadCompletedForTest()));
+  EXPECT_EQ(1, snapshot_count());
+  // This normally happens when navigation starts.
+  controller()->Reset();
+  // Next snapshot should be initiated when new document is loaded.
+  controller()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardBy(base::TimeDelta::FromMilliseconds(
+      controller()->GetDelayAfterDocumentOnLoadCompletedForTest()));
+  // No snapshot since session was reset.
+  EXPECT_EQ(2, snapshot_count());
+}
+
+}  // namespace offline_pages

components/offline_pages/core/snapshot_controller.cc

@@ -11,8 +11,6 @@
 #include "components/offline_pages/core/offline_page_feature.h"
 
 namespace {
-const bool kDocumentAvailableTriggersSnapshot = true;
-
 // Default delay, in milliseconds, between the main document parsed event and
 // snapshot. Note: this snapshot might not occur if the OnLoad event and
 // OnLoad delay elapses first to trigger a final snapshot.
@@ -21,12 +19,6 @@ const int64_t kDefaultDelayAfterDocumentAvailableMs = 7000;
 // Default delay, in milliseconds, between the main document OnLoad event and
 // snapshot.
 const int64_t kDelayAfterDocumentOnLoadCompletedMsForeground = 1000;
-const int64_t kDelayAfterDocumentOnLoadCompletedMsBackground = 2000;
-
-// Default delay, in milliseconds, between renovations finishing and
-// taking a snapshot. Allows for page to update in response to the
-// renovations.
-const int64_t kDelayAfterRenovationsCompletedMs = 2000;
 
 // Delay for testing to keep polling times reasonable.
 const int64_t kDelayForTests = 0;
@@ -35,55 +27,19 @@ const int64_t kDelayForTests = 0;
 
 namespace offline_pages {
 
-// static
-std::unique_ptr<SnapshotController>
-SnapshotController::CreateForForegroundOfflining(
-    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-    SnapshotController::Client* client) {
-  return std::unique_ptr<SnapshotController>(new SnapshotController(
-      task_runner, client, kDefaultDelayAfterDocumentAvailableMs,
-      kDelayAfterDocumentOnLoadCompletedMsForeground,
-      kDelayAfterRenovationsCompletedMs, kDocumentAvailableTriggersSnapshot,
-      false));
-}
-
-// static
-std::unique_ptr<SnapshotController>
-SnapshotController::CreateForBackgroundOfflining(
-    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-    SnapshotController::Client* client,
-    bool renovations_enabled) {
-  return std::unique_ptr<SnapshotController>(new SnapshotController(
-      task_runner, client, kDefaultDelayAfterDocumentAvailableMs,
-      kDelayAfterDocumentOnLoadCompletedMsBackground,
-      kDelayAfterRenovationsCompletedMs, !kDocumentAvailableTriggersSnapshot,
-      renovations_enabled));
-}
-
 SnapshotController::SnapshotController(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-    SnapshotController::Client* client,
-    int64_t delay_after_document_available_ms,
-    int64_t delay_after_document_on_load_completed_ms,
-    int64_t delay_after_renovations_completed_ms,
-    bool document_available_triggers_snapshot,
-    bool renovations_enabled)
+    SnapshotController::Client* client)
     : task_runner_(task_runner),
       client_(client),
       state_(State::READY),
-      delay_after_document_available_ms_(delay_after_document_available_ms),
+      delay_after_document_available_ms_(kDefaultDelayAfterDocumentAvailableMs),
       delay_after_document_on_load_completed_ms_(
-          delay_after_document_on_load_completed_ms),
-      delay_after_renovations_completed_ms_(
-          delay_after_renovations_completed_ms),
-      document_available_triggers_snapshot_(
-          document_available_triggers_snapshot),
-      renovations_enabled_(renovations_enabled),
+          kDelayAfterDocumentOnLoadCompletedMsForeground),
       weak_ptr_factory_(this) {
   if (offline_pages::ShouldUseTestingSnapshotDelay()) {
     delay_after_document_available_ms_ = kDelayForTests;
     delay_after_document_on_load_completed_ms_ = kDelayForTests;
-    delay_after_renovations_completed_ms_ = kDelayForTests;
   }
 }
 
@@ -108,44 +64,25 @@ void SnapshotController::PendingSnapshotCompleted() {
   state_ = State::READY;
 }
 
-void SnapshotController::RenovationsCompleted() {
-  if (renovations_enabled_) {
-    task_runner_->PostDelayedTask(
-        FROM_HERE,
-        base::BindOnce(&SnapshotController::MaybeStartSnapshotThenStop,
-                       weak_ptr_factory_.GetWeakPtr()),
-        base::TimeDelta::FromMilliseconds(
-            delay_after_renovations_completed_ms_));
-  }
-}
-
 void SnapshotController::DocumentAvailableInMainFrame() {
-  if (document_available_triggers_snapshot_) {
-    DCHECK_EQ(PageQuality::POOR, current_page_quality_);
-    // Post a delayed task to snapshot.
-    task_runner_->PostDelayedTask(
-        FROM_HERE,
-        base::BindOnce(&SnapshotController::MaybeStartSnapshot,
-                       weak_ptr_factory_.GetWeakPtr(),
-                       PageQuality::FAIR_AND_IMPROVING),
-        base::TimeDelta::FromMilliseconds(delay_after_document_available_ms_));
-  }
+  DCHECK_EQ(PageQuality::POOR, current_page_quality_);
+  // Post a delayed task to snapshot.
+  task_runner_->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&SnapshotController::MaybeStartSnapshot,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     PageQuality::FAIR_AND_IMPROVING),
+      base::TimeDelta::FromMilliseconds(delay_after_document_available_ms_));
 }
 
 void SnapshotController::DocumentOnLoadCompletedInMainFrame() {
-  if (renovations_enabled_) {
-    // Run renovations. After renovations complete, a snapshot will be
-    // triggered after a delay.
-    client_->RunRenovations();
-  } else {
-    // Post a delayed task to snapshot and then stop this controller.
-    task_runner_->PostDelayedTask(
-        FROM_HERE,
-        base::BindOnce(&SnapshotController::MaybeStartSnapshotThenStop,
-                       weak_ptr_factory_.GetWeakPtr()),
-        base::TimeDelta::FromMilliseconds(
-            delay_after_document_on_load_completed_ms_));
-  }
+  // Post a delayed task to snapshot and then stop this controller.
+  task_runner_->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&SnapshotController::MaybeStartSnapshotThenStop,
+                     weak_ptr_factory_.GetWeakPtr()),
+      base::TimeDelta::FromMilliseconds(
+          delay_after_document_on_load_completed_ms_));
 }
 
 void SnapshotController::MaybeStartSnapshot(PageQuality updated_page_quality) {
@@ -170,8 +107,4 @@ int64_t SnapshotController::GetDelayAfterDocumentOnLoadCompletedForTest() {
   return delay_after_document_on_load_completed_ms_;
 }
 
-int64_t SnapshotController::GetDelayAfterRenovationsCompletedForTest() {
-  return delay_after_renovations_completed_ms_;
-}
-
 }  // namespace offline_pages

components/offline_pages/core/snapshot_controller.h

@@ -56,35 +56,14 @@ class SnapshotController {
     // it is assumed that later snapshots are better then previous.
     virtual void StartSnapshot() = 0;
 
-    // Invoked when the page is sufficiently loaded for running
-    // renovations. The client should call the RenovationsCompleted()
-    // when they finish.
-    virtual void RunRenovations() = 0;
-
    protected:
     virtual ~Client() {}
   };
 
-  // Creates a SnapshotController with document available delay = 7s,
-  // document on load delay = 1s and triggers snapshot on document available.
-  static std::unique_ptr<SnapshotController> CreateForForegroundOfflining(
+  SnapshotController(
       const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
       SnapshotController::Client* client);
-  // Creates a SnapshotController with document on load delay = 2s
-  // and ignores document available signal.
-  static std::unique_ptr<SnapshotController> CreateForBackgroundOfflining(
-      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-      SnapshotController::Client* client,
-      bool renovations_enabled);
 
-  SnapshotController(
-      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-      SnapshotController::Client* client,
-      int64_t delay_after_document_available_ms,
-      int64_t delay_after_document_on_load_completed_ms,
-      int64_t delay_after_renovations_completed_ms,
-      bool document_available_triggers_snapshot,
-      bool renovations_enabled);
   virtual ~SnapshotController();
 
   // Resets the 'session', returning controller to initial state.
@@ -99,9 +78,6 @@ class SnapshotController {
   // now completed (so the next one can be started).
   void PendingSnapshotCompleted();
 
-  // The Client calls this when renovations have completed.
-  void RenovationsCompleted();
-
   // Invoked from WebContentObserver::DocumentAvailableInMainFrame
   void DocumentAvailableInMainFrame();
 
@@ -110,7 +86,6 @@ class SnapshotController {
 
   int64_t GetDelayAfterDocumentAvailableForTest();
   int64_t GetDelayAfterDocumentOnLoadCompletedForTest();
-  int64_t GetDelayAfterRenovationsCompletedForTest();
 
   PageQuality current_page_quality() const { return current_page_quality_; }
 
@@ -124,9 +99,6 @@ class SnapshotController {
   SnapshotController::State state_;
   int64_t delay_after_document_available_ms_;
   int64_t delay_after_document_on_load_completed_ms_;
-  int64_t delay_after_renovations_completed_ms_;
-  bool document_available_triggers_snapshot_;
-  bool renovations_enabled_;
 
   // The expected quality of a snapshot taken at the moment this value is
   // queried.

components/offline_pages/core/snapshot_controller_unittest.cc

@@ -7,9 +7,11 @@
 #include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
+#include "components/offline_pages/core/offline_page_feature.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
@@ -29,7 +31,6 @@ class SnapshotControllerTest : public testing::Test,
 
   // SnapshotController::Client
   void StartSnapshot() override;
-  void RunRenovations() override;
 
   // Utility methods.
   // Runs until all of the tasks that are not delayed are gone from the task
@@ -54,8 +55,7 @@ SnapshotControllerTest::SnapshotControllerTest()
 SnapshotControllerTest::~SnapshotControllerTest() {}
 
 void SnapshotControllerTest::SetUp() {
-  controller_ =
-      SnapshotController::CreateForForegroundOfflining(task_runner_, this);
+  controller_ = std::make_unique<SnapshotController>(task_runner_, this);
   snapshot_started_ = true;
 }
 
@@ -67,10 +67,6 @@ void SnapshotControllerTest::StartSnapshot() {
   snapshot_count_++;
 }
 
-void SnapshotControllerTest::RunRenovations() {
-  controller_->RenovationsCompleted();
-}
-
 void SnapshotControllerTest::PumpLoop() {
   task_runner_->RunUntilIdle();
 }