PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle

chrome/browser/android/offline_pages/prerendering_loader.h

Index: chrome/browser/android/offline_pages/prerendering_loader.h
diff --git a/chrome/browser/android/offline_pages/prerendering_loader.h b/chrome/browser/android/offline_pages/prerendering_loader.h
index f4af6a2b7a2e0b37892acaf3ec965e59df8f1a2e..74f19563b90222736a6b1dfff3f6a31cf0c5f897 100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.h
+++ b/chrome/browser/android/offline_pages/prerendering_loader.h
@@ -5,7 +5,10 @@
 #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/android/offline_pages/prerender_adapter.h"
 #include "components/offline_pages/background/offliner.h"
 
 class GURL;
@@ -23,29 +26,99 @@ class Size;
 namespace offline_pages {
 
 // A client-side page loader that integrates with the PrerenderManager to do
-// the page loading in the background.
-class PrerenderingLoader {
+// the page loading in the background. It operates on a single thread and
+// needs to run on BrowserThread::UI to work with the PrerenderManager.
+// It supports a single load request at a time.
+class PrerenderingLoader : public PrerenderAdapter::Observer {
  public:
   // Reports status of a load page request with loaded contents if available.
-  typedef base::Callback<void(Offliner::CompletionStatus,
-                              content::WebContents*)>
+  typedef base::Callback<void(Offliner::RequestStatus, content::WebContents*)>
       LoadPageCallback;
 
   explicit PrerenderingLoader(content::BrowserContext* browser_context);
-  virtual ~PrerenderingLoader();
+  ~PrerenderingLoader() override;
 
   // 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 will be accepted.
+  // request is still in progress it must be stopped before a new
+  // request will be accepted. The callback may be called more than
+  // once - first for a successful load and then if canceled after the
+  // load (which may be from resources being reclaimed) at which point
+  // the retrieved WebContents should no longer be used.
   virtual bool LoadPage(const GURL& url, const LoadPageCallback& callback);

[pasko, 2016/05/23 20:05:41]

A question related to adding virtual methods here.

Does it mean that there is a plan to inherit from this class (in which case I am
worried), or this area of code has a convention to default to virtual methods
(in which case I am less worried, but still those ~1.5MiB of relocations to
perform on every startup, and a policy like this would not be helpful to shrink
this amount)?

[dougarnett, 2016/05/23 21:43:54]

The virtual methods here are to be able to mock it for testing. Specifically,
there is a MockPrerenderingLoader in prerendering_offliner_unittests.cc

[pasko, 2016/05/26 11:00:28]

Makes sense, I did not know the mock already exists. So the only method not
mocked out is CanPrerender(). Is it a TODO?

Can the layers above be tested by reusing the TestAdapter or it is more
difficult?

[dougarnett, 2016/06/01 15:05:48]

Yes, I do need to mock CanPrerender as well, thanks! 

I think it is good idea to revisit merging Loader and Offliner and writing
Offliner tests that use Loader and just mock the Adapter would be good way to
start a merge. The Adapter has turned out to be a bit more comprehensive layer
than when I first wrote the shells of Offliner and Loader so seems like it would
work.

[pasko, 2016/06/01 15:09:06]

nice, +1 here, thanks

 
   // 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).

[pasko, 2016/05/23 20:05:41]

nit: s/called/run/

[dougarnett, 2016/05/23 21:43:54]

Done.

   // This loader should also be responsible for stopping offline
   // prerenders when Chrome is transitioned to foreground.
   virtual void StopLoading();
+
+  // Returns whether prerendering is possible for this device's configuration
+  // and the browser context.
+  virtual bool CanPrerender();
+
+  // Returns whether the loader is idle and able to accept new LoadPage
+  // request.
+  virtual bool IsIdle();
+
+  // Returns whether the loader has successfully loaded web contents.
+  // Note that |StopLoading()| should be used to clear this state once
+  // the loaded web contents are no longer needed.
+  virtual bool IsLoaded();
+
+  // Overrides the prerender stack adapter for unit testing.
+  void SetAdapterForTesting(
+      std::unique_ptr<PrerenderAdapter> prerender_adapter);
+
+  // PrerenderAdapter::Observer implementation:
+  void OnPrerenderStart() override;
+  void OnPrerenderStopLoading() override;
+  void OnPrerenderDomContentLoaded() override;
+  void OnPrerenderStop() override;
+
+ private:
+  // State of the loader (only one request may be active at a time).
+  enum class State {
+    IDLE,     // No active load request.
+    PENDING,  // Load request is pending the start of prerendering.
+    LOADING,  // Loading in progress.
+    LOADED,   // Loaded and now waiting for requestor to StopLoading().
+  };
+
+  // Handles some event/signal that the load request has succeeded or failed.
+  // It may be due to some asynchronous trigger that occurs after the request
+  // has completed for some other reason/event.
+  void HandleLoadEvent();
+
+  // Handles some event/signal that loading has stopped (whether due to a
+  // failue, cancel, or stop request). It may be due to some asynchronous

[pasko, 2016/05/23 20:05:41]

typo: s/failue/failure/

[dougarnett, 2016/05/23 21:43:54]

Done.

+  // trigger that occurs after the request has stopped for some other reason.
+  void HandleLoadingStopped();
+
+  // Cancels any current prerender and moves loader to idle state.
+  void CancelPrerender();
+
+  // Tracks loading state including whether the Loader is idle.
+  State state_;
+
+  // Not owned.
+  content::BrowserContext* browser_context_;
+
+  // Adapter for handling calls to the prerender stack.
+  std::unique_ptr<PrerenderAdapter> adapter_;
+
+  // A WebContents for the active load request that is used to hold the session
+  // storage namespace for rendering. This will NOT have the loaded page.
+  std::unique_ptr<content::WebContents> session_contents_;
+
+  // Callback to call when the active load request completes, fails, or is
+  // canceled.
+  LoadPageCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrerenderingLoader);
 };
 
 }  // namespace offline_pages