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 c29e1fd56fcd486c3fd34a38b2a6a02cb17bd696..3b384a48b61645d22ad7f5d0e7f52093d824d78a 100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.cc
+++ b/chrome/browser/android/offline_pages/prerendering_loader.cc
@@ -4,26 +4,198 @@
 
 #include "chrome/browser/android/offline_pages/prerendering_loader.h"
 
+#include "base/location.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(PrerenderManager* prerender_manager) {}
+using PrerenderingAdapter = PrerenderingLoader::PrerenderingAdapter;
 
-PrerenderingLoader::~PrerenderingLoader() {}
+PrerenderingLoader::PrerenderingLoader(content::BrowserContext* browser_context)
+    : browser_context_(browser_context) {
+  adapter_.reset(new PrerenderingAdapter());

[Pete Williamson, 2016/05/10 20:00:27]

If we pass in the adapter as an argument, we won't need the override for testing
method.

[dougarnett, 2016/05/10 21:33:11]

Maybe use factory methods then to inject appropriate adapter?

[fgorski, 2016/05/10 21:36:27]

also, if you are initializing in constructor, please use the initializer list.

[dougarnett, 2016/05/10 23:08:21]

Revised to use initializer list for now - but haven't yet addressed passing in. 

+}
+
+PrerenderingLoader::~PrerenderingLoader() {
+  ReleaseCurrentLoad();
+}
+
+bool PrerenderingLoader::LoadPage(const GURL& url,
+                                  const LoadPageCallback& callback) {
+  if (state_ != State::kIdle) {
+    LOG(WARNING) << "Existing request pending";

[fgorski, 2016/05/10 21:36:27]

use DVLOG

LOG is too heavy.

[dougarnett, 2016/05/10 23:08:22]

Done.

+    return false;
+  }
+  if (!CanPrerender())
+    return false;
+
+  adapter_->AddPrerenderForOffline(browser_context_, url,
+                                   GetSessionStorageNamespace(), GetSize());
+  if (!adapter_->IsActive())
+    return false;
+
+  callback_ = callback;
+  adapter_->SetObserver(this);
+  state_ = State::kLoading;
+  return true;
+}
+
+void PrerenderingLoader::StopLoading() {
+  ReleaseCurrentLoad();
+  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() {
+  // TODO(dougarnett): Create separate namespace from default.

[Pete Williamson, 2016/05/10 20:00:27]

A comment should say *why* we use a separate session storage namespace. (Avoid
contaminating user history, etc).

[dougarnett, 2016/05/10 21:33:11]

Done.

+  return content::WebContents::Create(
+             content::WebContents::CreateParams(browser_context_))
+      ->GetController()

[Pete Williamson, 2016/05/10 20:00:27]

I don't think we need a newline here

[dougarnett, 2016/05/10 21:33:11]

not my original edit - brought to you by "git cl format"

[fgorski, 2016/05/10 21:36:27]

Make sure to run git cl format

+      .GetDefaultSessionStorageNamespace();
+}
+
+const gfx::Size PrerenderingLoader::GetSize() {
+  return content::WebContents::Create(
+             content::WebContents::CreateParams(browser_context_))
+      ->GetContainerBounds()

[Pete Williamson, 2016/05/10 20:00:27]

I think I normally see -> at the end of the line above instead of on the
continuation line.

[dougarnett, 2016/05/10 21:33:11]

ditto

+      .size();

[Pete Williamson, 2016/05/10 20:00:27]

I don't think we need a newline here (before .size();)

[dougarnett, 2016/05/10 21:33:11]

ditto

+}
+
+void PrerenderingLoader::ReportLoaded() {
+  if (state_ == State::kLoading) {
+    content::WebContents* contents = adapter_->GetPrerenderContents();
+    if (contents) {
+      state_ = State::kLoaded;
+      callback_.Run(true, contents);
+    } else {
+      ReportLoadFailed();
+    }
+  }
+}
+
+void PrerenderingLoader::ReportLoadFailed() {
+  if (state_ != State::kLoaded && state_ != State::kIdle) {
+    if (adapter_->IsActive())
+      DVLOG(1) << "Load failed: " << adapter_->GetFinalStatus();
+    state_ = State::kIdle;
+    callback_.Run(false, nullptr);

[fgorski, 2016/05/10 21:36:27]

add a todo, that you want to post it to the thread instead.
This prevents a retry when you have this method still on stack.

[dougarnett, 2016/05/10 23:08:22]

Done.

+  }
+}
+
+void PrerenderingLoader::ReleaseCurrentLoad() {

[Pete Williamson, 2016/05/10 20:00:27]

Naming suggestion:  CancelPrerender()

[dougarnett, 2016/05/10 21:33:11]

Done.

+  if (adapter_->IsPrerendering())
+    adapter_->OnCancel();
+  adapter_->DestroyActive();
+}
+
+void PrerenderingLoader::OnPrerenderStart(prerender::PrerenderHandle* handle) {
+  if (adapter_->IsActive(handle) && state_ == State::kPending) {

[fgorski, 2016/05/10 21:36:27]

nit: skip {}

Also consider if this should be DCHECK() of some conditions in addition to if().
What if the criteria are not met? Is it normal?

[dougarnett, 2016/05/10 23:08:21]

I've been wondering if there is some different project style guideline for
braces? Since this is what I found in the style guide: "In general, curly braces
are not required for single-line statements, but they are allowed if you like
them"

I generally anticipate more code could be added within these clauses in this
family of callbacks - vs. simple one-liner return on the condition.

<Java guy struggling with no-brace-guilt>

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

[Pete Williamson, 2016/05/10 20:00:27]

So we seem to return "loaded" for both stop loading and com content loaded, does
that mean ReportLoaded is designed to be called twice?

[dougarnett, 2016/05/10 21:33:11]

Not initially, the state change from kLoading to kLoaded is meant to limit to
once (need/todo to add test for this). When timeouts are implemented, then it
will be combination of the event plus its timeout after it as to which will
report loaded.

We may revisit this with some experience - that is, if worth trying two
snapshots based from the two events (if second one happens before overall time
limit).

+  }
+}
+
+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::OnPrerenderCreatedMatchCompleteReplacement(
+    prerender::PrerenderHandle* handle) {
+  LOG(WARNING) << "Offlining prerender should never do replacement";

[fgorski, 2016/05/10 21:36:27]

dvlog

[dougarnett, 2016/05/10 23:08:22]

Done

+}
 
-bool PrerenderingLoader::LoadPage(
+void PrerenderingLoader::SetAdapterForTesting(
+    PrerenderingAdapter* prerendering_adapter) {
+  adapter_.reset(prerendering_adapter);
+}
+
+// Adapter implementation to prerender stack:
+
+bool PrerenderingAdapter::CanPrerender() const {
+  return prerender::PrerenderManager::ActuallyPrerendering();
+}
+
+bool PrerenderingAdapter::AddPrerenderForOffline(
+    content::BrowserContext* browser_context,
     const GURL& url,
     content::SessionStorageNamespace* session_storage_namespace,
-    const gfx::Size& size,
-    const LoadPageCallback& callback) {
-  // TODO(dougarnett): implement.
-  return false;
+    const gfx::Size& size) {
+  DCHECK(!IsActive());
+  Profile* profile = Profile::FromBrowserContext(browser_context);
+  prerender::PrerenderManager* manager =
+      prerender::PrerenderManagerFactory::GetForProfile(profile);
+  DCHECK(manager);
+  active_handle_.reset(

[Pete Williamson, 2016/05/10 20:00:27]

// Start the prerender manager doing the render of the target url.

A comment here might help maintainers later, since this seems to be the
important handoff point.

[dougarnett, 2016/05/10 21:33:11]

Done.

+      manager->AddPrerenderForOffline(url, session_storage_namespace, size));
+  return active_handle_.get();
 }
 
-void PrerenderingLoader::StopLoading() {
-  // TODO(dougarnett): implement.
+void PrerenderingAdapter::SetObserver(
+    prerender::PrerenderHandle::Observer* observer) {
+  active_handle_->SetObserver(observer);
+}
+
+bool PrerenderingAdapter::IsPrerendering() const {
+  return active_handle_->IsPrerendering();
+}
+
+void PrerenderingAdapter::OnCancel() {
+  active_handle_->OnCancel();
+}
+
+content::WebContents* PrerenderingAdapter::GetPrerenderContents() const {
+  if (active_handle_->contents()) {
+    return active_handle_->contents()->prerender_contents();

[Pete Williamson, 2016/05/10 20:00:27]

Maybe add a comment about ownership of the web contents, and how it must be
freed?

[dougarnett, 2016/05/10 21:33:11]

Beefed up .h comment and added .cc comment as well.

+  }
+  return nullptr;
+}
+
+prerender::FinalStatus PrerenderingAdapter::GetFinalStatus() const {
+  DCHECK(active_handle_->contents());
+  return active_handle_->contents()->final_status();
+}
+
+bool PrerenderingAdapter::IsActive() const {
+  return active_handle_ ? true : false;

[fgorski, 2016/05/10 21:36:27]

return active_handle_;

[dougarnett, 2016/05/10 23:08:22]

Compiler not happy with that - but maybe return active_handle_.get(); is better

+}
+
+bool PrerenderingAdapter::IsActive(prerender::PrerenderHandle* handle) const {
+  return active_handle_.get() == handle;
+}
+
+void PrerenderingAdapter::DestroyActive() {
+  active_handle_.reset(nullptr);
 }
 
 }  // namespace offline_pages