[Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the …

components/offline_pages/core/snapshot_controller.cc

 // Copyright 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 "components/offline_pages/core/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 {
+const bool kDocumentAvailableTriggersSnapshotForeground = true;
+const bool kDocumentAvailableTriggersSnapshotBackground = false;

[carlosk, 2017/04/21 01:10:58]

nit: it seems to me these constants are now unneeded verbosity. Using true/false
in the new Create* methods below is still clear enough.

[fgorski, 2017/04/21 16:05:10]

Alternative (also nit):
kDocumentAvailableTriggersSnapshot = true;

use it directly for Foreground
use it with negation for Background: !kDocumentAvailableTriggersSnapshot

[chili, 2017/04/24 20:34:14]

Done.

+
 // 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.
 const int64_t kDefaultDelayAfterDocumentAvailableMs = 7000;

[fgorski, 2017/04/21 16:05:10]

Wasn't this set to 25 seconds for Background?
I see you are making a change to 7s now. Was that intended?

Also documentation would probably need to be updated for respective
CreateForBackgroundOfflining if you decide to update.

[chili, 2017/04/24 20:34:14]

Yes, but the DocumentAvailable signal is ignored anyway, so the 25s is
essentially void...

[fgorski, 2017/04/24 20:56:54]

Acknowledged.
Thanks for looking into it. I am glad we have it explained here. It may be worth
putting it in bug description in case anyone drills down from source code and
wonders why we did that.

 
 // Default delay, in milliseconds, between the main document OnLoad event and
 // snapshot.
-const int64_t kDelayAfterDocumentOnLoadCompletedMs = 1000;
+const int64_t kDelayAfterDocumentOnLoadCompletedMsForeground = 1000;
+const int64_t kDelayAfterDocumentOnLoadCompletedMsBackground = 2000;
 
 // Delay for testing to keep polling times reasonable.
 const int64_t kDelayForTests = 0;
 
 }  // namespace
 
 namespace offline_pages {
 
-SnapshotController::SnapshotController(
+// static
+SnapshotController* SnapshotController::CreateForForegroundOfflining(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-    SnapshotController::Client* client)
-    : SnapshotController(task_runner,
-                         client,
-                         kDefaultDelayAfterDocumentAvailableMs,
-                         kDelayAfterDocumentOnLoadCompletedMs) {}
+    SnapshotController::Client* client) {
+  return new SnapshotController(task_runner, client,
+                                kDefaultDelayAfterDocumentAvailableMs,
+                                kDelayAfterDocumentOnLoadCompletedMsForeground,
+                                kDocumentAvailableTriggersSnapshotForeground);
+}
+
+// static
+SnapshotController* SnapshotController::CreateForBackgroundOfflining(
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    SnapshotController::Client* client) {
+  return new SnapshotController(task_runner, client,
+                                kDefaultDelayAfterDocumentAvailableMs,
+                                kDelayAfterDocumentOnLoadCompletedMsBackground,
+                                kDocumentAvailableTriggersSnapshotBackground);
+}
 
 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_document_on_load_completed_ms,
+    bool document_available_triggers_snapshot)
     : task_runner_(task_runner),
       client_(client),
       state_(State::READY),
       delay_after_document_available_ms_(delay_after_document_available_ms),
       delay_after_document_on_load_completed_ms_(
           delay_after_document_on_load_completed_ms),
+      document_available_triggers_snapshot_(
+          document_available_triggers_snapshot),
       weak_ptr_factory_(this) {
   if (offline_pages::ShouldUseTestingSnapshotDelay()) {
     delay_after_document_available_ms_ = kDelayForTests;
     delay_after_document_on_load_completed_ms_ = kDelayForTests;
   }
 }
 
 SnapshotController::~SnapshotController() {}
 
 void SnapshotController::Reset() {
   // Cancel potentially delayed tasks that relate to the previous 'session'.
   weak_ptr_factory_.InvalidateWeakPtrs();
   state_ = State::READY;
   current_page_quality_ = PageQuality::POOR;
 }
 
 void SnapshotController::Stop() {
   state_ = State::STOPPED;
 }
 
 void SnapshotController::PendingSnapshotCompleted() {
   // Unless the controller is "stopped", enable the subsequent snapshots.
   // Stopped state prevents any further snapshots form being started.
   if (state_ == State::STOPPED)
     return;
   state_ = State::READY;
 }
 
 void SnapshotController::DocumentAvailableInMainFrame() {
-  DCHECK_EQ(PageQuality::POOR, current_page_quality_);
-  // Post a delayed task to snapshot.
-  task_runner_->PostDelayedTask(
-      FROM_HERE, base::Bind(&SnapshotController::MaybeStartSnapshot,
-                            weak_ptr_factory_.GetWeakPtr(),
-                            PageQuality::FAIR_AND_IMPROVING),
-      base::TimeDelta::FromMilliseconds(delay_after_document_available_ms_));
+  if (document_available_triggers_snapshot_) {
+    DCHECK_EQ(PageQuality::POOR, current_page_quality_);
+    // Post a delayed task to snapshot.
+    task_runner_->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&SnapshotController::MaybeStartSnapshot,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   PageQuality::FAIR_AND_IMPROVING),
+        base::TimeDelta::FromMilliseconds(delay_after_document_available_ms_));
+  }
 }
 
 void SnapshotController::DocumentOnLoadCompletedInMainFrame() {
   // Post a delayed task to snapshot and then stop this controller.
   task_runner_->PostDelayedTask(
       FROM_HERE, base::Bind(&SnapshotController::MaybeStartSnapshotThenStop,
                             weak_ptr_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(
           delay_after_document_on_load_completed_ms_));
 }
@@ skipping 14 matching lines @@
 
 int64_t SnapshotController::GetDelayAfterDocumentAvailableForTest() {
   return delay_after_document_available_ms_;
 }
 
 int64_t SnapshotController::GetDelayAfterDocumentOnLoadCompletedForTest() {
   return delay_after_document_on_load_completed_ms_;
 }
 
 }  // namespace offline_pages