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..43941ec8b95f6ff367c3c6c1b8db0d064c46f4fb 100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.cc
+++ b/chrome/browser/android/offline_pages/prerendering_loader.cc
@@ -4,26 +4,158 @@
 
 #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/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) {}
+PrerenderingLoader::PrerenderingLoader(content::BrowserContext* browser_context)
+    : state_(State::IDLE),
+      browser_context_(browser_context),
+      adapter_(new PrerenderAdapter()) {}
 
-PrerenderingLoader::~PrerenderingLoader() {}
+PrerenderingLoader::~PrerenderingLoader() {
+  CancelPrerender();
+}
+
+bool PrerenderingLoader::LoadPage(const GURL& url,
+                                  const LoadPageCallback& callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (state_ != State::IDLE) {
+    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_)));
+
+  bool accepted = adapter_->AddPrerenderForOffline(
+      browser_context_, url,
+      GetSessionStorageNamespace(session_contents_.get()),
+      GetSize(session_contents_.get()));
+  if (!accepted)
+    return false;
 
-bool PrerenderingLoader::LoadPage(
-    const GURL& url,
-    const LoadPageCallback& callback) {
-  // TODO(dougarnett): implement.
-  return false;
+  DCHECK(adapter_->IsActive());
+  callback_ = callback;
+  adapter_->SetObserver(this);
+  state_ = State::LOADING;
+  return true;
 }
 
 void PrerenderingLoader::StopLoading() {
-  // TODO(dougarnett): implement.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  CancelPrerender();
+}
+
+bool PrerenderingLoader::CanPrerender() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // First check if prerendering is enabled.
+  return adapter_->CanPrerender();
+}
+
+bool PrerenderingLoader::IsIdle() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return state_ == State::IDLE;
+}
+
+void PrerenderingLoader::SetAdapterForTesting(
+    PrerenderAdapter* prerender_adapter) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  adapter_.reset(prerender_adapter);
+}
+
+void PrerenderingLoader::OnPrerenderStart(prerender::PrerenderHandle* handle) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (adapter_->IsActive(handle) && state_ == State::PENDING) {
+    state_ = State::LOADING;
+  }
+}
+
+void PrerenderingLoader::OnPrerenderStopLoading(
+    prerender::PrerenderHandle* handle) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (adapter_->IsActive(handle)) {

[gabadie, 2016/05/13 07:55:37]

I think asserts here would be more appropriate that a call that does nothing.
Unless I would be wrong and the Prerender API sends some different handles here?

[dougarnett, 2016/05/13 16:49:45]

Yes, sounds good

+    // TODO(dougarnett): Implement/integrate to delay policy here.
+    ReportLoaded();
+  }
+}
+
+void PrerenderingLoader::OnPrerenderDomContentLoaded(
+    prerender::PrerenderHandle* handle) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (adapter_->IsActive(handle)) {

[gabadie, 2016/05/13 07:55:37]

Please assert.

[dougarnett, 2016/05/13 16:49:45]

Done.

+    // TODO(dougarnett): Implement/integrate to delay policy here.
+    ReportLoaded();

[gabadie, 2016/05/13 07:55:38]

To me it looks weird that there is 2 ways we ReportLoaded(). Is there any
particular reason? Yeah, the second call of ReportLoaded() would simply early
return...

[dougarnett, 2016/05/13 16:49:45]

Not complete yet. Expect to have two tunable timeout values for the two events -
whichever event plus its timeout occurs first will trigger the load callback.
Eg, DomContentLoaded + 10 seconds and OnLoad/StopLoading + 2 seconds. This would
be just an initial approach to decide when to snapshot.

+  }
+}
+
+void PrerenderingLoader::OnPrerenderStop(prerender::PrerenderHandle* handle) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (adapter_->IsActive(handle)) {

[gabadie, 2016/05/13 07:55:37]

Please assert.

[dougarnett, 2016/05/13 16:49:45]

Done.

+    ReportLoadFailed();
+  }
+}
+
+content::SessionStorageNamespace*
+PrerenderingLoader::GetSessionStorageNamespace(content::WebContents* contents) {

[gabadie, 2016/05/13 07:55:37]

Must be in an anonymous namespace

[dougarnett, 2016/05/13 16:49:45]

Are you saying the function must be in C++ anonymous namespace or the
WebContents must be in anonymous/incognito namespace?

[pasko, 2016/05/17 15:34:20]

I think gabadie@ means that this function does not need the PrerenderingLoader
state, and does not need to be exported, so it is better to limit its visibility
further by putting it into the anonymous namespace (as in c++) in this .cc file.

Same with GetSize() below.

[dougarnett, 2016/05/18 00:37:46]

Thx, makes sense. I have since modified to use member variable per other
feedback so now not sure which feedback should win. 

+  return contents->GetController().GetDefaultSessionStorageNamespace();
+}
+
+const gfx::Size PrerenderingLoader::GetSize(content::WebContents* contents) {

[gabadie, 2016/05/13 07:55:37]

Must be in an anonymous namespace

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

Guillaume, can you explain further? I don't understand what you mean (C++
namespace scope?).

[dougarnett, 2016/05/18 00:37:46]

Egor clarified.

+  return contents->GetContainerBounds().size();
+}
+
+void PrerenderingLoader::ReportLoaded() {
+  if (state_ == State::LOADING) {

[gabadie, 2016/05/13 07:55:37]

Why not asserting?

[dougarnett, 2016/05/13 16:49:45]

Took stab at a comment. If/when we have timeout triggered load events, either
one could happen before the other or a StopLoading might have happened. This
method is meant to be the one copy of logic checking on whether to issue the
load callback or not.

+    content::WebContents* contents = adapter_->GetWebContents();
+    if (contents) {
+      state_ = State::LOADED;
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE,
+          base::Bind(callback_, Offliner::RequestStatus::LOADED, contents));
+    } else {
+      ReportLoadFailed();
+    }
+  }
+}
+
+void PrerenderingLoader::ReportLoadFailed() {
+  if (state_ != State::LOADED && state_ != State::IDLE) {

[gabadie, 2016/05/13 07:55:37]

DCHECK(state_ == State::LOADING || state_ == State::PENDING) instead of the IF?

[dougarnett, 2016/05/13 16:49:45]

In my prototype I recall getting an OnPrerenderStop() callback after a
successful load which is why this ended up here. I was trying to collect cases
and callback management into these couple methods. Let me try revising their
names anyway.

+    if (adapter_->IsActive())
+      DVLOG(1) << "Load failed: " << adapter_->GetFinalStatus();
+    // TODO(dougarnett): Determine from final status if retry-able.
+    state_ = State::IDLE;

[gabadie, 2016/05/13 07:55:37]

Looks like here your state tracking is bugged because you are missing a
session_contents_.reset(nullptr); and a adapter_->DestroyActive(); as you do in
CancelPrerender(). Or may be do a LOAD_FAILED state?

[dougarnett, 2016/05/13 16:49:45]

Done.

+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(callback_, Offliner::RequestStatus::FAILED_DO_NOT_RETRY,
+                   nullptr));
+  }
+}
+
+void PrerenderingLoader::CancelPrerender() {
+  if (adapter_->IsActive()) {
+    adapter_->DestroyActive();
+  }
+  session_contents_.reset(nullptr);
+  if (state_ != State::LOADED && state_ != State::IDLE) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(callback_, Offliner::RequestStatus::CANCELED, nullptr));
+  }
+  state_ = State::IDLE;
 }
 
 }  // namespace offline_pages