PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle

chrome/browser/android/offline_pages/prerendering_loader.h

 // 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_PRERENDERING_LOADER_H_
 #define CHROME_BROWSER_ANDROID_OFFLINE_PAGES_PRERENDERING_LOADER_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;
-class PrerenderManager;
 
 namespace content {
+class BrowserContext;
 class WebContents;
 class SessionStorageNamespace;
 }  // namespace content
 
 namespace gfx {
 class Size;
 }  // namespace gfx
 
 namespace offline_pages {
 
 // A client-side page loader that integrates with the PrerenderManager to do
 // the page loading in the background.
-class PrerenderingLoader {
+class PrerenderingLoader : public prerender::PrerenderHandle::Observer {
  public:
+  // Adapter for making prerender stack calls internally. This provides virtual
+  // methods that may be overridden or mocked to allow for unit testing.
+  class PrerenderingAdapter {

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

Would this be better in its own file?  It is starting to seem a bit heavyweight
for a nested class.

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

Yes, maybe so. Would like to get some review from Filip and Egor that this
adapter approach is acceptable and then will do it.

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

+1 what to Pete suggested, and also, PrerenderAdapter seems like a good name
too.

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

Done.

+   public:
+    PrerenderingAdapter() {}
+    virtual ~PrerenderingAdapter() {}
+
+    // Returns whether prerendering is enabled and configured.
+    virtual bool CanPrerender() const;
+
+    // Requests prerendering of |url|.
+    virtual bool AddPrerenderForOffline(
+        content::BrowserContext* browser_context,
+        const GURL& url,
+        content::SessionStorageNamespace* session_storage_namespace,
+        const gfx::Size& size);
+
+    // Sets an observer on prerendering events.
+    virtual void SetObserver(prerender::PrerenderHandle::Observer* observer);
+
+    // Returns whether actively prerendering.
+    virtual bool IsPrerendering() const;
+
+    // Reports that prerendering should be canceled.
+    virtual void OnCancel();
+
+    // Returns the prerendered WebContents. This should only be called once
+    // prerendering observer events indicate content is loaded. It may be
+    // used for snapshotting the page.
+    virtual content::WebContents* GetPrerenderContents() const;
+
+    // Returns the final status of prerendering.
+    virtual prerender::FinalStatus GetFinalStatus() const;
+
+    // Returns whether there is an active prerendering.
+    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 IsActive(prerender::PrerenderHandle* handle) const;
+
+    // Destroys and clears any current prerendering operation and state.
+    virtual void DestroyActive();
+
+   private:
+    std::unique_ptr<prerender::PrerenderHandle> active_handle_;
+  };
+
   // Reports status of a load page request with loaded contents if available.
-  typedef base::Callback<void(Offliner::CompletionStatus,
-                              content::WebContents*)>
+  typedef base::Callback<void(bool /* loaded */, content::WebContents*)>
       LoadPageCallback;
 
-  explicit PrerenderingLoader(PrerenderManager* prerender_manager);
-  ~PrerenderingLoader();
+  explicit PrerenderingLoader(content::BrowserContext* browser_context);
+  virtual ~PrerenderingLoader();

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

scratch the virtual, add override;

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

Done.

 
   // Loads a page in the background if possible and returns whether the
   // request was accepted. If so, the LoadPageCallback will be informed
   // of status. Only one load request may exist as a time. If a previous
-  // request is still in progress it must be canceled before a new
+  // request is still in progress it must be stopped before a new
   // request will be accepted.
-  bool LoadPage(const GURL& url,
-                content::SessionStorageNamespace* session_storage_namespace,
-                const gfx::Size& size,
-                const LoadPageCallback& callback);
+  bool LoadPage(const GURL& url, const LoadPageCallback& callback);
 
   // Stops (completes or cancels) the load request. Must be called when
-  // LoadPageCallback is done with consuming the contents.
+  // LoadPageCallback is done with consuming the contents. May be called
+  // prior to LoadPageCallback in order to cancel the current request (in
+  // which case the callback will not be called).
   // This loader should also be responsible for stopping offline
   // prerenders when Chrome is transitioned to foreground.
   void StopLoading();
+
+  // Returns whether prerendering is possible for this device's configuration
+  // and the browser context.
+  bool CanPrerender();
+
+  // Returns whether the loader is idle and able to accept new LoadPage
+  // request.

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

IsAvailable() may be a better name than IsIdle().

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

Maybe, or IsReady

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

Kept as IsIdle(). Available and Ready seem more potentially confused wrt whether
it is the Loader or some loaded contents that is Available/Ready.

+  bool IsIdle();
+
+  // Overrides the prerender stack adapter for unit testing.
+  void SetAdapterForTesting(PrerenderingAdapter* prerendering_adapter);
+
+ private:
+  // Determines and returns the session storage namespace to use for the
+  // prerendering request.
+  content::SessionStorageNamespace* GetSessionStorageNamespace();
+
+  // Determines and returns the window size to use for the prerendering request.
+  const gfx::Size GetSize();
+
+  // Reports the load result to the LoadPageCallback.
+  void ReportLoaded();
+
+  // Reports a load failure to the LoadPageCallback.
+  void ReportLoadFailed();
+
+  // Releases the current load operation resources.
+  void ReleaseCurrentLoad();
+
+  // PrerenderHandle::Observer overrides:
+  void OnPrerenderStart(prerender::PrerenderHandle* handle) override;
+  void OnPrerenderStopLoading(prerender::PrerenderHandle* handle) override;
+  void OnPrerenderDomContentLoaded(prerender::PrerenderHandle* handle) override;
+  void OnPrerenderStop(prerender::PrerenderHandle* handle) override;
+  void OnPrerenderCreatedMatchCompleteReplacement(
+      prerender::PrerenderHandle* handle) override;
+
+  // kIdle - no active request.
+  // kPending - request pending the start of prerendering.
+  // kLoading - loading in progress.
+  // kLoaded - loaded and now waiting for requestor to StopLoading().
+  enum class State { kIdle, kPending, kLoading, kLoaded };

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

I don't think this is wrong or bad as is, but as I recall I normally see one per
line, and a end of line comment explaining it

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

Think I saw this style from recent dimich CL so tried it out. The other style
will scale better.

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

Revised to other style.

+  State state_;
+
+  // Not owned.

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

Suggestion to avoid ambiguity:  browser_context_ is an unowned pointer.

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

Moved to be a suffix comment. I don't want to set up the comment for
inconsistency on renaming member. 

+  content::BrowserContext* browser_context_;
+  std::unique_ptr<PrerenderingAdapter> adapter_;
+  LoadPageCallback callback_;
 };
 
 }  // namespace offline_pages
 
 #endif  // CHROME_BROWSER_ANDROID_OFFLINE_PAGES_PRERENDERING_LOADER_H_