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..b9b3f39ba1f752273e79b3e063630c3ae0e03e7c 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,8 @@ bool PrerenderingLoader::LoadPage(const GURL& url,
     return false;
 
   DCHECK(adapter_->IsActive());
+  // Reset the SnapshotController for this new load request.
+  snapshot_controller_->Reset();

[Dmitry Titov, 2016/06/09 17:41:01]

this is only needed if the LoadPage can be called on the same PrerendererLoader
instance multiple times to load multiple pages and get snapshots from them. If
it is not really possible, it'd be enough to just create an instance here, store
it in unique_ptr and then instead of Stop() just reset that unique_ptr... This
way, you don't have an instance of controller longer then you need and don't
need to worry about stopping/resetting it.

[dougarnett, 2016/06/09 18:25:46]

Ok, creating new instance each LoadPage now

   callback_ = callback;
   state_ = State::PENDING;
   return true;
@@ -88,14 +92,29 @@ 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()) {

[Dmitry Titov, 2016/06/09 17:41:01]

I don't think these checks are really needed, especially repeated several times
in several functions. It woudl be simpler to just directly call methods on the
controller. It is designed to avoid multiple StartSnapshot until you Reset it or
report that previous snapshot is done. And it won't start another if you Stop -
so all the checks here are probably not needed.

[dougarnett, 2016/06/09 18:25:46]

Done.

+    DCHECK(adapter_->GetWebContents());
+    // Inform SnapshotController of OnLoad event so it can determine
+    // when to consider it really LOADED.
+    snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
+  }
 }
 
 void PrerenderingLoader::OnPrerenderDomContentLoaded() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  // TODO(dougarnett): Implement/integrate to delay policy here.
-  HandleLoadEvent();
+  DCHECK(!IsIdle());
+  if (!IsLoaded()) {

[Dmitry Titov, 2016/06/09 17:41:01]

Same as above

[dougarnett, 2016/06/09 18:25:46]

Done.

+    if (!adapter_->GetWebContents()) {
+      // Without a WebContents object at this point, we are done.
+      HandleLoadingStopped();
+    } else {
+      // Inform SnapshotController of DomContentContent event so it can
+      // determine when to consider it really LOADED (e.g., some multiple
+      // second delay from this event).
+      snapshot_controller_->DocumentAvailableInMainFrame();
+    }
+  }
 }
 
 void PrerenderingLoader::OnPrerenderStop() {
@@ -103,6 +122,13 @@ void PrerenderingLoader::OnPrerenderStop() {
   HandleLoadingStopped();
 }
 
+void PrerenderingLoader::StartSnapshot() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(!IsIdle());
+  DCHECK(!IsLoaded());

[Dmitry Titov, 2016/06/09 17:41:01]

This is confusing. Why start snapshot on something that is not loaded? It may be
a naming issue, but this code is hard to parse.

Also, how about renaming HandleLoadEvent into StartSnapshot and not having this
method?

[dougarnett, 2016/06/09 18:25:46]

Precondition check, we are now waiting for StartSnapshot() call to tells us that
we should transition to LOADED state. When we support a second snapshot in the
future, we should refactor the interface with the Offliner (or join the Offliner
and Loader together).

I agree we can collapse StartSnapshot and HandleLoadEvent. This is an
integration step where the SnapshotController and BackgroundLoader were not
designed together so there is impedance and terminology mismatch here. For now I
think I will just directly call with no other code and do more refactoring
later.

+  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 +142,7 @@ void PrerenderingLoader::HandleLoadEvent() {
 
   content::WebContents* web_contents = adapter_->GetWebContents();
   if (web_contents) {
+    snapshot_controller_->Stop();

[Dmitry Titov, 2016/06/09 17:41:01]

No need ot call Stop here, while you didn't report the previous snapshot
actually doen (via SnapshotController::PendingSnapshotCompleted), the new one
won't be started anyways.

[dougarnett, 2016/06/09 18:25:46]

Done.

     state_ = State::LOADED;
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
@@ -148,6 +175,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 +186,7 @@ void PrerenderingLoader::CancelPrerender() {
   if (adapter_->IsActive()) {
     adapter_->DestroyActive();
   }
+  snapshot_controller_->Stop();
   session_contents_.reset(nullptr);
   if (!IsLoaded() && !IsIdle()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(