PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle

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 "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. 

+}
 
-bool PrerenderingLoader::LoadPage(
-    const GURL& url,
-    content::SessionStorageNamespace* session_storage_namespace,
-    const gfx::Size& size,
-    const LoadPageCallback& callback) {
-  // TODO(dougarnett): implement.
-  return false;
+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() {
-  // TODO(dougarnett): implement.
+  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

+}
+
+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) {
+  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 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