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..8a11db8f594b157a5c031b7a68b366129c9dd506 100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.h
+++ b/chrome/browser/android/offline_pages/prerendering_loader.h
@@ -5,7 +5,12 @@
 #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 "chrome/browser/prerender/prerender_handle.h"
+#include "chrome/browser/prerender/prerender_manager.h"
 #include "components/offline_pages/background/offliner.h"
 
 class GURL;
@@ -23,12 +28,12 @@ class Size;
 namespace offline_pages {
 
 // A client-side page loader that integrates with the PrerenderManager to do
-// the page loading in the background.
+// the page loading in the background. It operates on a single thread and
+// needs to run on BrowserThread::UI to work with the PrerenderManager.
 class PrerenderingLoader {
  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);
@@ -37,15 +42,94 @@ class PrerenderingLoader {
   // 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.
   virtual 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.
   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 contents.

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

nit: web contents.

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

Done.

+  // Note that StopLoading() should be used to clear this state once

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

nit: |StopLoading()|

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

Done.

+  // the loaded contents are no longer needed.

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

the loaded web contents is

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

Done.

+  virtual bool IsLoaded();
+
+  // Overrides the prerender stack adapter for unit testing.
+  void SetAdapterForTesting(PrerenderAdapter* prerender_adapter);
+
+ private:
+  friend class ObserverDelegate;
+
+  // Delegating observer for PrerenderHandle events. Used to restrict access
+  // to loader's observer methods.
+  class ObserverDelegate : public prerender::PrerenderHandle::Observer {

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

Can you name it PrerenderHandleObserver or HandleObserver.

ObserverDelegate feel a bit like calling something HandlerManager or
ManagerHandler and thinking of the two interchangeably.

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

Done.

+   public:
+    explicit ObserverDelegate(PrerenderingLoader* loader);
+    ~ObserverDelegate() override;
+
+    void OnPrerenderStart(prerender::PrerenderHandle* handle) override;
+    void OnPrerenderStopLoading(prerender::PrerenderHandle* handle) override;
+    void OnPrerenderDomContentLoaded(
+        prerender::PrerenderHandle* handle) override;
+    void OnPrerenderStop(prerender::PrerenderHandle* handle) override;
+
+   private:
+    // Not owned.
+    PrerenderingLoader* loader_;
+  };
+
+  // State of the loader (only one request may be active at a time).

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

the comment in () should be in the documentation of the class I think.

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

Done.

+  enum class State {
+    IDLE,     // No active request.
+    PENDING,  // Request pending the start of prerendering.
+    LOADING,  // Loading in progress.
+    LOADED,   // Loaded and now waiting for requestor to StopLoading().
+  };
+
+  // PrerenderHandle::Observer handling logic:
+  void OnPrerenderStart(prerender::PrerenderHandle* handle);
+  void OnPrerenderStopLoading(prerender::PrerenderHandle* handle);
+  void OnPrerenderDomContentLoaded(prerender::PrerenderHandle* handle);
+  void OnPrerenderStop(prerender::PrerenderHandle* handle);

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

why should PrerenderingLoader know about the handle? I think management of the
handle and events on it is logically closer to the adapter.

The handle is used only for a DCHECK, and via a call to Adapter anyway. Looks
out of place to me.

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

Done.

+
+  // Returns the session storage namespace to use for the prerendering request.
+  content::SessionStorageNamespace* GetSessionStorageNamespace();
+
+  // Returns the window size to render.
+  const gfx::Size GetSize();
+
+  // Reports the load result to the LoadPageCallback if load still in progress.
+  void ReportLoadedIfStillLoading();
+
+  // Reports a load failure to the LoadPageCallback if load still in progress.
+  void ReportLoadFailedIfStillLoading();
+
+  // Cancels any current prerender and moves loader to idle state.
+  void CancelPrerender();
+
+  // Tracks loading state including whether the Loader is idle.
+  State state_;
+
+  content::BrowserContext* browser_context_;  // Not owned.
+  std::unique_ptr<PrerenderAdapter> adapter_;

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

More documentation on each one, please. This is important to understanding this
class a bit better.

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

Done.

+  std::unique_ptr<ObserverDelegate> observer_;
+  std::unique_ptr<content::WebContents> session_contents_;
+  LoadPageCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrerenderingLoader);
 };
 
 }  // namespace offline_pages