PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle

chrome/browser/android/offline_pages/prerender_adapter.h

Index: chrome/browser/android/offline_pages/prerender_adapter.h
diff --git a/chrome/browser/android/offline_pages/prerender_adapter.h b/chrome/browser/android/offline_pages/prerender_adapter.h
new file mode 100644
index 0000000000000000000000000000000000000000..2de50fce98fda3b2f7662ac7b3a606a3a6ec120b
--- /dev/null
+++ b/chrome/browser/android/offline_pages/prerender_adapter.h
@@ -0,0 +1,84 @@
+// 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.
+
+#ifndef CHROME_BROWSER_ANDROID_OFFLINE_PAGES_PRERENDER_ADAPTER_H_
+#define CHROME_BROWSER_ANDROID_OFFLINE_PAGES_PRERENDER_ADAPTER_H_
+
+#include <memory>
+
+#include "base/callback.h"
+#include "chrome/browser/prerender/prerender_handle.h"
+#include "chrome/browser/prerender/prerender_manager.h"
+#include "components/offline_pages/background/offliner.h"
+
+class GURL;
+
+namespace content {
+class BrowserContext;
+class WebContents;
+class SessionStorageNamespace;
+}  // namespace content
+
+namespace gfx {
+class Size;
+}  // namespace gfx
+
+namespace offline_pages {
+
+// An adapter for making calls into the prerender stack for managing an

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

for managing a prerendering...

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

Done.

+// prerendering request for offlining. This provides virtual methods

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

This provides... you are explaining what interfaces are for in general terms. I
don't think this description is needed here.

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

Done.

+// that may be overridden or mocked to allow for unit testing.
+class PrerenderAdapter {
+ public:
+  PrerenderAdapter();
+  virtual ~PrerenderAdapter();
+
+  // Returns whether prerendering is enabled and configured.
+  virtual bool CanPrerender() const;
+
+  // Starts prerendering |url| for offlining. There must be no active
+  // prerender request when calling this. Returns whether it was able
+  // to start the prerendering operation.
+  virtual bool StartPrerender(
+      content::BrowserContext* browser_context,
+      const GURL& url,
+      content::SessionStorageNamespace* session_storage_namespace,
+      const gfx::Size& size,
+      prerender::PrerenderHandle::Observer* observer);
+
+  // Returns whether actively prerendering.

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

nit: whether adapter is actively?

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

dropped method

+  virtual bool IsPrerendering() const;
+
+  // Returns a pointer to the prerendered WebContents. This should only be
+  // called once prerendering observer events indicate content is loaded.
+  // It may be used for snapshotting the page. The caller does NOT get
+  // ownership on the contents and must call PrerenderingLoader::StopLoading()
+  // to report when it no longer needs the contents.

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

the web contents.

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

Done.

+  virtual content::WebContents* GetWebContents() const;

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

Since Prerendering can destroy WebContents behind the scenes (via timeout, for
example), we should have OnWebContentsDestroyed in the adapter to properly wrap
up the uses of this instance of WebContents (such as serialization).

I think the best way to observe it is by attaching a WebContentsObserver with
::WebContentsDestroyed() immediately after we successfully
AddPrerenderForOffline().

With that, IsPrerendering() could be implemented via the observer you expose
here, and not needed in the Adapter.

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

As discussed OnPrerenderStop() seems to be the better signal. Added some note
about that in the comment.

+
+  // Returns the final status of prerendering.
+  virtual prerender::FinalStatus GetFinalStatus() const;
+
+  // Returns whether there is an active prerendering operation.

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

Compare to documentation of IsPrerendering() and try to explain how these are
different. Perhaps one of these should be private/protected.

Also, I made a comment about implementation of IsPrerendering() that is
relevant.

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

Comments to both methods are indeed confusing here. My today's reading of the
prerender code suggests that:

1. PrerenderHandle is owned by the client of Prerendering, hence we control when
we are IsActive() here.

2. IsPrerendering(), in contrary, can change behind the scenes. It is controlled
by lifetime of PrerenderData in two lists: PrerenderManager::active_prerenders_
and PrerenderManager::to_delete_prerenders_;

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

As long as it is documented I am OK with having both of course.

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

Please see if new wording is clear enough. Dropped IsPrerendering as not
currently used anyway.

+  virtual bool IsActive() const;
+
+  // Returns whether there is an active prerendering associated with
+  // the |handle|. This may be used to confirm that an observed prerender
+  // event for |handle| applies to the active prerendering.
+  virtual bool HasHandle(prerender::PrerenderHandle* handle) const;
+
+  // Cancels any current prerendering operation and destroys its local handle.
+  virtual void DestroyActive();
+
+ private:
+  // At most one prerender request may be active for this adapter and this
+  // holds its handle if one is active.
+  std::unique_ptr<prerender::PrerenderHandle> active_handle_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrerenderAdapter);
+};
+
+}  // namespace offline_pages
+
+#endif  // CHROME_BROWSER_ANDROID_OFFLINE_PAGES_PRERENDER_ADAPTER_H_