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

chrome/browser/android/offline_pages/prerendering_loader.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 "chrome/browser/android/offline_pages/prerendering_loader.h"
 
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/network_change_notifier.h"
 #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;
 }  // namespace
 
 
 namespace offline_pages {
 
 
 // Classifies the appropriate RequestStatus for for the given prerender
 // FinalStatus.
@@ skipping 88 matching lines @@
           content::WebContents::CreateParams(browser_context_)));
   content::SessionStorageNamespace* sessionStorageNamespace =
       new_web_contents->GetController().GetDefaultSessionStorageNamespace();
   gfx::Size renderWindowSize = new_web_contents->GetContainerBounds().size();
   bool accepted = adapter_->StartPrerender(
       browser_context_, url, sessionStorageNamespace, renderWindowSize);
   if (!accepted)
     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);
   state_ = State::LOADING;
   return true;
 }
 
 void PrerenderingLoader::StopLoading() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   CancelPrerender();
@@ skipping 28 matching lines @@
 }
 
 void PrerenderingLoader::OnPrerenderDomContentLoaded() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!IsIdle());
   if (!adapter_->GetWebContents()) {
     // Without a WebContents object at this point, we are done.
     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");
   }
 }
 
 void PrerenderingLoader::OnPrerenderStop() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   HandleLoadingStopped();
 }
@@ skipping 94 matching lines @@
   if (adapter_->IsActive()) {
     adapter_->DestroyActive();
   }
   snapshot_controller_.reset(nullptr);
   session_contents_.reset(nullptr);
   state_ = State::IDLE;
   is_lowbar_met_ = false;
 }
 
 }  // namespace offline_pages