PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle

chrome/browser/android/offline_pages/prerender_adapter.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/prerender_adapter.h"
+
+#include "base/location.h"
+#include "base/logging.h"
+#include "chrome/browser/android/offline_pages/prerendering_loader.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 {
+
+PrerenderAdapter::PrerenderAdapter() {}
+
+PrerenderAdapter::~PrerenderAdapter() {
+  DestroyActive();
+}
+
+bool PrerenderAdapter::CanPrerender() const {
+  return prerender::PrerenderManager::ActuallyPrerendering();
+}
+
+bool PrerenderAdapter::StartPrerender(
+    content::BrowserContext* browser_context,
+    const GURL& url,
+    content::SessionStorageNamespace* session_storage_namespace,
+    const gfx::Size& size,
+    prerender::PrerenderHandle::Observer* observer) {
+  DCHECK(!IsActive());
+  DCHECK(CanPrerender());
+
+  Profile* profile = Profile::FromBrowserContext(browser_context);
+  prerender::PrerenderManager* manager =
+      prerender::PrerenderManagerFactory::GetForProfile(profile);
+  DCHECK(manager);
+
+  // Start prerendering the url and capture the handle for the prerendering.
+  active_handle_.reset(
+      manager->AddPrerenderForOffline(url, session_storage_namespace, size));
+  if (!active_handle_)
+    return false;
+
+  active_handle_->SetObserver(observer);
+  return true;
+}
+
+bool PrerenderAdapter::IsPrerendering() const {
+  DCHECK(IsActive());

[fgorski, 2016/05/17 05:09:51]

I am wondering why you decided on a DCHECK here.
why won't this be:
return IsActive() && active_handle_->IsPrerendering(); ?

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

I would support the DCHECK because the adapter is supposed to be 'just a
mockable adapter' talking to the prerender stack. All cleverness about the
prerendering state should sit on levels above.

[fgorski, 2016/05/17 15:54:54]

IsActive() is not a fall through but statement of internal state of the adapter.
The call site is forced to do checks like this:
if (IsActive() && IsPrerendering())

That is why I have a problem with DCHECK in this method.
It does not affect the state, it is const and yet it may DCHECK.

If we choose to keep the DCHECK I'll be fine with that, but
a) let's document it
b) let's stay aware of the fact that this is asking for trouble :)

[pasko, 2016/05/17 16:26:31]

There are only two methods that can affect this bit of state (StartPrerender()
and DestroyActive()), and their behavior towards IsActive() is part of the
contract between the Adapter and its clients. This means one can omit checks to
IsActive() in many reasonable codepaths.

I understand that this might be not the easiest contract. Alternatively the
client could manage PrerenderHandleAdapter, created by injectable factory, and
matching the lifetime of the PrerenderHandle. Created roughly by something like
StartPrerender(), destroyed when necessary (before creating another prerender).
Checks for IsActive() are not necessary then. Same mockability. WDYT?

[fgorski, 2016/05/17 17:06:19]

Let's have Doug digest it and try to come up with necessary changes.
I think this could be a sound approach, but anything with a solid contract
between adapter and callers will work for me. I am looking for consistency in
both design and implementation.

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

Simplifying by dropping IsPrerendering() altogether!

[I have ended up using the OnPrerenderStart() observer call instead of this
method to know when loading is in progress]

+  return active_handle_->IsPrerendering();
+}
+
+content::WebContents* PrerenderAdapter::GetWebContents() const {
+  DCHECK(IsActive());
+  if (active_handle_->contents()) {
+    // Note: the prerender stack maintains ownership of these contents
+    // and PrerenderingLoader::StopLoading() must be called to report
+    // the Loader is done with the contents.
+    return active_handle_->contents()->prerender_contents();
+  }
+  return nullptr;
+}
+
+prerender::FinalStatus PrerenderAdapter::GetFinalStatus() const {
+  DCHECK(IsActive());
+  DCHECK(active_handle_->contents());

[fgorski, 2016/05/17 05:09:51]

why is there a difference between GetWebContents and GetFinalStatus, with
respect to how they fail when active_handle_->contents() == nullptr?

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

Put DCHECK in other one

+  return active_handle_->contents()->final_status();
+}
+
+bool PrerenderAdapter::IsActive() const {
+  return active_handle_.get();
+}
+
+bool PrerenderAdapter::HasHandle(prerender::PrerenderHandle* handle) const {
+  return active_handle_.get() == handle;
+}
+
+void PrerenderAdapter::DestroyActive() {
+  if (IsActive()) {

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

We are checking for the same condition in prerendering_loader. I understand that
it's probably for moar safety, but it makes the contract between the two classes
less clear.

For simplicity we should have DCHECK(IsActive()) here, and move all the
cleverness away from the PrerenderAdapter. It is supposed to be the minimal
possible surface for mocking.

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

Ok, I will move this IsActive() condition into the destructor as that is the
usage it is covering.

+    active_handle_->OnCancel();
+    active_handle_.reset(nullptr);
+  }
+}
+
+}  // namespace offline_pages