PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle

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 08576826a92db9984c60bbbfe2baf32711a173b8..74a4ffa9b4503256af361c8828f8743e8f457a96 100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.cc
+++ b/chrome/browser/android/offline_pages/prerendering_loader.cc
@@ -4,26 +4,143 @@
 
 #include "chrome/browser/android/offline_pages/prerendering_loader.h"
 
+#include "base/location.h"
+#include "base/logging.h"
+#include "chrome/browser/prerender/prerender_manager.h"
+#include "chrome/browser/prerender/prerender_manager_factory.h"
+#include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace offline_pages {
 
-PrerenderingLoader::PrerenderingLoader(
-    content::BrowserContext* browser_context) {}
+PrerenderingLoader::PrerenderingLoader(content::BrowserContext* browser_context)
+    : state_(State::kIdle),
+      browser_context_(browser_context),
+      adapter_(new PrerenderAdapter()) {}
 
-PrerenderingLoader::~PrerenderingLoader() {}
+PrerenderingLoader::~PrerenderingLoader() {
+  CancelPrerender();
+}
+
+bool PrerenderingLoader::LoadPage(const GURL& url,
+                                  const LoadPageCallback& callback) {

[pasko, 2016/05/12 19:10:07]

assert that runs on the correct thread for every method please

[dougarnett, 2016/05/13 00:00:24]

Done.

+  if (state_ != State::kIdle) {
+    DVLOG(1) << "WARNING: Existing request pending";
+    return false;
+  }
+  if (!CanPrerender())
+    return false;
+
+  // Create a WebContents instance to define and hold a SessionStorageNamespace
+  // for this load request.
+  DCHECK(!session_contents_.get());
+  session_contents_.reset(content::WebContents::Create(
+      content::WebContents::CreateParams(browser_context_)));
+
+  adapter_->AddPrerenderForOffline(

[pasko, 2016/05/12 19:10:07]

does not check return value, which can be false in case Prerender gave up early
(due to many concurrent prerenders or other reasons).

[dougarnett, 2016/05/13 00:00:24]

Done.

+      browser_context_, url,
+      GetSessionStorageNamespace(session_contents_.get()),
+      GetSize(session_contents_.get()));
+  if (!adapter_->IsActive())
+    return false;
 
-bool PrerenderingLoader::LoadPage(
-    const GURL& url,
-    const LoadPageCallback& callback) {
-  // TODO(dougarnett): implement.
-  return false;
+  callback_ = callback;
+  adapter_->SetObserver(this);
+  state_ = State::kLoading;

[pasko, 2016/05/12 19:10:07]

Though the Google C++ Style Guide now says to use kConstantNaming for enums,
Chromium was written using MACRO_STYLE naming. In enums that are actually
enumerations (i.e. have multiple values), continue to use this style for
consistency. Use kConstantNaming when using the "enum hack" to define a single
constant, as you would for a const int or the like.

from: http://www.chromium.org/developers/coding-style#Naming

Also, see SHOUTY_CASE here: https://chromium-cpp.appspot.com/

[dougarnett, 2016/05/13 00:00:24]

Done.

+  return true;
 }
 
 void PrerenderingLoader::StopLoading() {
-  // TODO(dougarnett): implement.
+  CancelPrerender();
+  state_ = State::kIdle;
+}
+
+bool PrerenderingLoader::CanPrerender() {
+  // First check if prerendering is enabled.
+  return adapter_->CanPrerender();
+}
+
+bool PrerenderingLoader::IsIdle() {
+  return state_ == State::kIdle;
+}
+
+content::SessionStorageNamespace*
+PrerenderingLoader::GetSessionStorageNamespace(content::WebContents* contents) {
+  return contents->GetController().GetDefaultSessionStorageNamespace();
+}
+
+const gfx::Size PrerenderingLoader::GetSize(content::WebContents* contents) {
+  return contents->GetContainerBounds().size();

[pasko, 2016/05/12 19:10:07]

I suspect the size would be not what we want here, we'll probably need to set
the size from outsize the loader.

[dougarnett, 2016/05/13 00:00:24]

Any idea on how to determine it if no Tab context available?

[dougarnett, 2016/05/16 23:51:38]

Egor, still open question for alternative.

+}
+
+void PrerenderingLoader::ReportLoaded() {
+  if (state_ == State::kLoading) {
+    content::WebContents* contents = adapter_->GetWebContents();
+    if (contents) {
+      state_ = State::kLoaded;
+      base::ThreadTaskRunnerHandle::Get()->PostTask(

[pasko, 2016/05/12 19:10:07]

why ThreadTaskRunnerHandle? Can this just call the callback?

[dougarnett, 2016/05/13 00:00:24]

This one probably could. The PostTask was added per feedback from fgorski as a
safer pattern (eg, so that callback's associated object can be checked if
weakptr still good). He recently had a nasty crash by calling where the object
was deleted.

+          FROM_HERE,
+          base::Bind(callback_, Offliner::CompletionStatus::LOADED, contents));
+    } else {
+      ReportLoadFailed();
+    }
+  }
+}
+
+void PrerenderingLoader::ReportLoadFailed() {
+  if (state_ != State::kLoaded && state_ != State::kIdle) {
+    if (adapter_->IsActive())
+      DVLOG(1) << "Load failed: " << adapter_->GetFinalStatus();
+    // TODO(dougarnett): Determine from final status if retry-able.
+    state_ = State::kIdle;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(callback_, Offliner::CompletionStatus::FAILED_CONSIDER_RETRY,

[pasko, 2016/05/12 19:10:07]

should we assume non-retriable by default and retry based on a whitelist of
final statuses? Feels safer against wasted retries this way.

[dougarnett, 2016/05/13 00:00:24]

Done.

+                   nullptr));
+  }
+}
+
+void PrerenderingLoader::CancelPrerender() {
+  if (adapter_->IsActive()) {
+    adapter_->OnCancel();
+    adapter_->DestroyActive();
+  }
+  session_contents_.reset(nullptr);

[pasko, 2016/05/12 19:10:07]

we should run the callback here

[dougarnett, 2016/05/13 00:00:24]

Done.

+}
+
+void PrerenderingLoader::OnPrerenderStart(prerender::PrerenderHandle* handle) {
+  if (adapter_->IsActive(handle) && state_ == State::kPending) {
+    state_ = State::kLoading;
+  }
+}
+
+void PrerenderingLoader::OnPrerenderStopLoading(
+    prerender::PrerenderHandle* handle) {
+  if (adapter_->IsActive(handle)) {
+    // TODO(dougarnett): Implement/integrate to delay policy here.
+    ReportLoaded();
+  }
+}
+
+void PrerenderingLoader::OnPrerenderDomContentLoaded(
+    prerender::PrerenderHandle* handle) {
+  if (adapter_->IsActive(handle)) {
+    // TODO(dougarnett): Implement/integrate to delay policy here.
+    ReportLoaded();
+  }
+}
+
+void PrerenderingLoader::OnPrerenderStop(prerender::PrerenderHandle* handle) {
+  if (adapter_->IsActive(handle)) {
+    ReportLoadFailed();
+  }
+}
+
+void PrerenderingLoader::SetAdapterForTesting(

[pasko, 2016/05/12 19:10:07]

the order of methods in the .cc file should match the order in the .h file

[dougarnett, 2016/05/13 00:00:24]

Done.

+    PrerenderAdapter* prerender_adapter) {
+  adapter_.reset(prerender_adapter);
 }
 
 }  // namespace offline_pages