Integrates the SnapshotController into the PrerenderingLoader to provide logic for determining when…

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 7ae17b30408590067ad75d5768409d26e5dcd9cc..a74cd4faab1e53e5cdb1a78a8a08ffff738d2da9 100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.cc
+++ b/chrome/browser/android/offline_pages/prerendering_loader.cc
@@ -18,6 +18,8 @@ namespace offline_pages {
 PrerenderingLoader::PrerenderingLoader(content::BrowserContext* browser_context)
     : state_(State::IDLE), browser_context_(browser_context) {
   adapter_.reset(new PrerenderAdapter(this));
+  snapshot_controller_.reset(
+      new SnapshotController(base::ThreadTaskRunnerHandle::Get(), this));
 }
 
 PrerenderingLoader::~PrerenderingLoader() {
@@ -49,6 +51,7 @@ bool PrerenderingLoader::LoadPage(const GURL& url,
     return false;
 
   DCHECK(adapter_->IsActive());
+  snapshot_controller_->Reset();

[Pete Williamson, 2016/06/09 00:22:51]

Why reset the controller here?  I would guess that this is when we are asking
the prerenderer to load a page, not a callback telling us that the prerenderer
successfully loaded.

[dougarnett, 2016/06/09 00:42:33]

This is to prepare (or arm or clear or Reset()) the state of the controller to
be ready to determine when to snapshot. A subsequent Stop() call on it will
freeze it until it is Reset again.

[Pete Williamson, 2016/06/09 00:49:05]

Ah, this is a bit confusing, since 'reset'-ing a smart pointer is how you clear
it!  At the very least we should comment here, better still  might be to rename
the reset method.

[dougarnett, 2016/06/09 15:12:27]

Added comment. 

I wasn't part of the SnapshotController review so not sure what API naming
discussion might have occurred but dimich@ is reviewer here and so he might
comment on its naming. I agree it is a bit confusing when contained in
unique_ptr. 

   callback_ = callback;
   state_ = State::PENDING;
   return true;
@@ -88,14 +91,25 @@ void PrerenderingLoader::OnPrerenderStart() {
 
 void PrerenderingLoader::OnPrerenderStopLoading() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  // TODO(dougarnett): Implement/integrate to delay policy here.
-  HandleLoadEvent();
+  DCHECK(!IsIdle());
+  if (!IsLoaded()) {
+    DCHECK(adapter_->GetWebContents());
+    snapshot_controller_->DocumentOnLoadCompletedInMainFrame();

[Pete Williamson, 2016/06/09 00:22:51]

(For my education as a reviewer of this code):
What is the difference between OnPrerendererStopLoading() and
OnPrerendererDomContentLoaded()?  Do we get both in some cases?

Also does the snapshot_controller have some kind of built in Start + N seconds
functionality where it takes a snapshot in case we never get the page loaded
event, or are we relying on the prerender stop loading event for our timeout?

It looks like this is set up for single snapshot, right?  Do we anticipate being
able to move to several snapshots someday, or does the implementation of the
prerenderer make that not feasible?

[dougarnett, 2016/06/09 00:42:33]

This code is single shot at the moment however the SnapshotController has
handshaking to support multiple ones. We can decide whether to utilize that here
down the road (let's get single shot working first!).

The SnapshotController currently sets a 7 second delay on the DocumentAvailable
(or DomContentLoaded) event and no delay for the OnLoadCompleted (or
OnPrerenderStopLoading) event. So we expect to get the HandleLoadEvent() call
once for whichever happens first: OnPrerenderStopLoading or
OnPrerenderDomContentLoaded+7secs

[Pete Williamson, 2016/06/09 00:49:05]

OK, code is good as is, then, but let's document this.

[dougarnett, 2016/06/09 15:12:27]

Done - let me know if not clear enough.

+  }
 }
 
 void PrerenderingLoader::OnPrerenderDomContentLoaded() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  // TODO(dougarnett): Implement/integrate to delay policy here.
-  HandleLoadEvent();
+  DCHECK(!IsIdle());
+  if (!IsLoaded()) {
+    if (!adapter_->GetWebContents()) {
+      // Without a WebContents object at this point, we are done.
+      HandleLoadingStopped();
+    } else {
+      // Engage the logic to detect when to snapshot.
+      snapshot_controller_->DocumentAvailableInMainFrame();
+    }
+  }
 }
 
 void PrerenderingLoader::OnPrerenderStop() {
@@ -103,6 +117,13 @@ void PrerenderingLoader::OnPrerenderStop() {
   HandleLoadingStopped();
 }
 
+void PrerenderingLoader::StartSnapshot() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(!IsIdle());
+  DCHECK(!IsLoaded());
+  HandleLoadEvent();
+}
+
 void PrerenderingLoader::HandleLoadEvent() {
   // If still loading, check if the load succeeded or not, then update
   // the internal state (LOADED for success or IDLE for failure) and post
@@ -116,6 +137,7 @@ void PrerenderingLoader::HandleLoadEvent() {
 
   content::WebContents* web_contents = adapter_->GetWebContents();
   if (web_contents) {
+    snapshot_controller_->Stop();
     state_ = State::LOADED;
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
@@ -148,6 +170,7 @@ void PrerenderingLoader::HandleLoadingStopped() {
                  : Offliner::RequestStatus::FAILED;
   // TODO(dougarnett): For failure, determine from final status if retry-able
   // and report different failure statuses if retry-able or not.
+  snapshot_controller_->Stop();
   session_contents_.reset(nullptr);
   state_ = State::IDLE;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -158,6 +181,7 @@ void PrerenderingLoader::CancelPrerender() {
   if (adapter_->IsActive()) {
     adapter_->DestroyActive();
   }
+  snapshot_controller_->Stop();
   session_contents_.reset(nullptr);
   if (!IsLoaded() && !IsIdle()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(