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

chrome/browser/android/offline_pages/prerendering_loader.cc

Index: chrome/browser/android/offline_pages/prerendering_loader.cc
diff --git a/chrome/browser/android/offline_pages/prerendering_loader.cc b/chrome/browser/android/offline_pages/prerendering_loader.cc
index 680d7bee7e0c57c7ec6c610e82b8b3e7b19134f1..89cbffaaeb22e22a87a332cbfdb0a75dd538f8f4 100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.cc
+++ b/chrome/browser/android/offline_pages/prerendering_loader.cc
@@ -17,10 +17,7 @@
 #include "ui/gfx/geometry/size.h"
 
 namespace {
-// Whether to report DomContentLoaded event to the snapshot controller.

[romax, 2017/04/19 17:40:29]

seems like the delay can also be used as the boolean, like 0 means false? i'm
fine with either, but is there a preference on higher-level?

[chili, 2017/04/19 21:10:14]

I would rather not use delay as a boolean. Delay is answering the question "how
many seconds after the event should snapshot happen?".  A delay of 0 would
therefore answer it as "immediately". The only number that would make sense
would be something like MAX_LONG, for "please wait forever". It will also be
harder to read than a boolean would, in my opinion.

-bool kConsiderDclForSnapshot = false;
-// The delay to wait for snapshotting after DomContentLoaded event if
-// kConsiderDclForSnapshot is true.
+// The delay to wait for snapshotting after DomContentLoaded event.
 long kOfflinePageDclDelayMs = 25000;

[fgorski, 2017/04/20 16:39:05]

Since you are already changing this file, I recommend getting moving this to
snapshot controller as well.

[chili, 2017/04/21 00:12:54]

Done.

 // The delay to wait for snapshotting after OnLoad event.
 long kOfflinePageOnloadDelayMs = 2000;
@@ -130,10 +127,10 @@ bool PrerenderingLoader::LoadPage(const GURL& url,
     return false;
 
   DCHECK(adapter_->IsActive());
-  snapshot_controller_.reset(
-      new SnapshotController(base::ThreadTaskRunnerHandle::Get(), this,
-                             kOfflinePageDclDelayMs,
-                             kOfflinePageOnloadDelayMs));
+  snapshot_controller_.reset(new SnapshotController(
+      base::ThreadTaskRunnerHandle::Get(), this, kOfflinePageDclDelayMs,
+      kOfflinePageOnloadDelayMs,
+      false /* consider_document_available_for_snapshot */));

[fgorski, 2017/04/20 16:39:05]

The fact that for both background cases we are passing false tells me that:
* we should not expose that as a constructor parameter for the background case.
* Perhaps should wrap a single private constructor into 2 factory methods:
SnapshotController::CreateForForegroundSnapshot(...)
SnapshotController::CreateForBackgroundSnapshot(...)

We don't vary parameters now, which does not worry me. When we get to varying
parameters I think we should have that well planned.

[chili, 2017/04/21 00:12:54]

Done.

   load_done_callback_ = load_done_callback;
   progress_callback_ = progress_callback;
   session_contents_.swap(new_web_contents);
@@ -182,12 +179,10 @@ void PrerenderingLoader::OnPrerenderDomContentLoaded() {
     HandleLoadingStopped();
   } else {
     is_lowbar_met_ = true;
-    if (kConsiderDclForSnapshot) {
-      // Inform SnapshotController of DomContentLoaded event so it can
-      // determine when to consider it really LOADED (e.g., some multiple
-      // second delay from this event).
-      snapshot_controller_->DocumentAvailableInMainFrame();
-    }
+    // Inform SnapshotController of DomContentLoaded event so it can
+    // determine when to consider it really LOADED (e.g., some multiple
+    // second delay from this event).
+    snapshot_controller_->DocumentAvailableInMainFrame();
 
     // Add this signal to signal_data_.
     AddLoadingSignal("OnDomContentLoaded");