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

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/threading/thread_task_runner_handle.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 "ui/gfx/geometry/size.h"
 
 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() {
   CancelPrerender();
 }
 
 bool PrerenderingLoader::LoadPage(const GURL& url,
                                   const LoadPageCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!IsIdle()) {
@@ skipping 11 matching lines @@
       content::WebContents::CreateParams(browser_context_)));
   content::SessionStorageNamespace* sessionStorageNamespace =
       session_contents_->GetController().GetDefaultSessionStorageNamespace();
   gfx::Size renderWindowSize = session_contents_->GetContainerBounds().size();
   bool accepted = adapter_->StartPrerender(
       browser_context_, url, sessionStorageNamespace, renderWindowSize);
   if (!accepted)
     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;
 }
 
 void PrerenderingLoader::StopLoading() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   CancelPrerender();
 }
 
@@ skipping 19 matching lines @@
 }
 
 void PrerenderingLoader::OnPrerenderStart() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(state_ == State::PENDING);
   state_ = State::LOADING;
 }
 
 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() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   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
   // callback.
   // Note: it is possible to receive a load event (e.g., if timeout-based)
   // after the request has completed via another path (e.g., canceled) so
   // the Loader may be idle at this point.
 
   if (IsIdle() || IsLoaded())
     return;
 
   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,
         base::Bind(callback_, Offliner::RequestStatus::LOADED, web_contents));
   } else {
     // No WebContents means that the load failed (and it stopped).
     HandleLoadingStopped();
   }
 }
 
@@ skipping 12 matching lines @@
     DVLOG(1) << "Load failed: " << adapter_->GetFinalStatus();
     adapter_->DestroyActive();
   }
   // Request status depends on whether we are still loading (failed) or
   // did load and then loading was stopped (cancel - from prerender stack).
   Offliner::RequestStatus request_status =
       IsLoaded() ? Offliner::RequestStatus::CANCELED
                  : 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(
       FROM_HERE, base::Bind(callback_, request_status, nullptr));
 }
 
 void PrerenderingLoader::CancelPrerender() {
   if (adapter_->IsActive()) {
     adapter_->DestroyActive();
   }
+  snapshot_controller_->Stop();
   session_contents_.reset(nullptr);
   if (!IsLoaded() && !IsIdle()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(callback_, Offliner::RequestStatus::CANCELED, nullptr));
   }
   state_ = State::IDLE;
 }
 
 }  // namespace offline_pages