Handle interface to prerenders.

chrome/browser/prerender/prerender_manager.h

Index: chrome/browser/prerender/prerender_manager.h
diff --git a/chrome/browser/prerender/prerender_manager.h b/chrome/browser/prerender/prerender_manager.h
index 5d357ac99087d7b15bbeea0f3e6594a3362747e4..06039fb20e07d559c11a2cae0e9fddc2b5ba1f3c 100644
--- a/chrome/browser/prerender/prerender_manager.h
+++ b/chrome/browser/prerender/prerender_manager.h
@@ -7,6 +7,7 @@
 #pragma once
 
 #include <list>
+#include <map>
 #include <string>
 #include <utility>
 
@@ -21,6 +22,7 @@
 #include "chrome/browser/prerender/prerender_config.h"
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/prerender/prerender_final_status.h"
+#include "chrome/browser/prerender/prerender_handle.h"
 #include "chrome/browser/prerender/prerender_origin.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
 #include "googleurl/src/gurl.h"
@@ -104,10 +106,10 @@ class PrerenderManager : public base::SupportsWeakPtr<PrerenderManager>,
   // the RenderView that the prerender request came from.  The |size| may be
   // empty, and the current tab size will be used if it is. If the current
   // active tab size cannot be found, we use a default from PrerenderConfig.
-  // Returns true if the URL was added, false if it was not.
-  // If the launching RenderView is itself prerendering, the prerender is added
-  // as a pending prerender.
-  bool AddPrerenderFromLinkRelPrerender(
+  // Returns a caller owned PrerenderHandle* if the URL was added, NULL if it

[mmenke, 2012/07/09 18:06:57]

nit:  believe this should be caller-owned here and below.

[gavinp, 2012/07/11 17:04:00]

Done.

+  // was not. If the launching RenderView is itself prerendering, the prerender
+  // is added as a pending prerender.
+  PrerenderHandle* AddPrerenderFromLinkRelPrerender(
       int process_id,
       int route_id,
       const GURL& url,
@@ -118,17 +120,13 @@ class PrerenderManager : public base::SupportsWeakPtr<PrerenderManager>,
   // from a source without a RenderViewHost (i.e., the omnibox) we don't have a
   // child or route id, or a referrer. This method uses sensible values for
   // those. The |session_storage_namespace| matches the namespace of the active
-  // tab at the time the prerender is generated from the omnibox.
-  bool AddPrerenderFromOmnibox(
+  // tab at the time the prerender is generated from the omnibox. Returns a
+  // caller owned PrerenderHandle*.

[mmenke, 2012/07/09 18:06:57]

nit:  or NULL.

[gavinp, 2012/07/11 17:04:00]

Done.

+  PrerenderHandle* AddPrerenderFromOmnibox(
       const GURL& url,
       content::SessionStorageNamespace* session_storage_namespace,
       gfx::Size size);
 
-  // Request cancelation of a previously added prerender. If the |active_count_|
-  // of the prerender is one, it will be canceled.  Otherwise, |active_count_|
-  // will be decremented by one.
-  void MaybeCancelPrerender(const GURL& url);
-
   // Destroy all prerenders for the given child route id pair and assign a final
   // status to them.

[mmenke, 2012/07/10 18:01:13]

This comment looks wrong to me.  From both the old (And new) code and usage, it
looks to me like this destroys the (necessarily unique) prerender that
process_id, view_id identifies.  The description makes it sound like it destroys
all prerenders initiated by process_id, view_id.

While you're here, mind fixing the comment?  Maybe "Destroys the prerender using
the specified RenderView..."

[gavinp, 2012/07/11 17:04:00]

Done.

   virtual void DestroyPrerenderForRenderView(int process_id,
@@ -184,10 +182,6 @@ class PrerenderManager : public base::SupportsWeakPtr<PrerenderManager>,
   // is prerendering a page.
   bool IsWebContentsPrerendering(content::WebContents* web_contents) const;
 
-  // Returns true if there is a prerendered page for the given URL and it has
-  // finished loading. Only valid if called before MaybeUsePrerenderedPage.
-  bool DidPrerenderFinishLoading(const GURL& url) const;
-
   // Maintaining and querying the set of WebContents belonging to this
   // PrerenderManager that are currently showing prerendered pages.
   void MarkWebContentsAsPrerendered(content::WebContents* web_contents);
@@ -237,11 +231,6 @@ class PrerenderManager : public base::SupportsWeakPtr<PrerenderManager>,
   // Adds a condition. This is owned by the PrerenderManager.
   void AddCondition(const PrerenderCondition* condition);
 
-  bool IsPendingEntry(const GURL& url) const;
-
-  // Returns true if |url| matches any URLs being prerendered.
-  bool IsPrerendering(const GURL& url) const;
-
   // Records that some visible tab navigated (or was redirected) to the
   // provided URL.
   void RecordNavigation(const GURL& url);
@@ -251,70 +240,55 @@ class PrerenderManager : public base::SupportsWeakPtr<PrerenderManager>,
   PrerenderHistograms* histograms() const { return histograms_.get(); }
 
  protected:
+  friend class PrerenderContents;
+  friend class PrerenderHandle;

[mmenke, 2012/07/09 18:06:57]

All friends should be together, in the private section.

[gavinp, 2012/07/11 17:04:00]

Done.

+
   void SetPrerenderContentsFactory(
       PrerenderContents::Factory* prerender_contents_factory);
 
+  // Adds a prerender from a pending Prerender, called by
+  // PrerenderContents::StartPendingPrerenders.
+  void StartPendingPrerender(
+      PrerenderHandle* existing_prerender_handle,
+      Origin origin,
+      int process_id,
+      const GURL& url,
+      const content::Referrer& referrer,
+      const gfx::Size& size,
+      content::SessionStorageNamespace* session_storage_namespace);

[mmenke, 2012/07/09 18:06:57]

The other functions take Sizes by value rather than reference.  Should either
be:  By value everywhere, or by reference except when needed to be by value.

[gavinp, 2012/07/11 17:04:00]

Done.

+
+  void DestroyPendingPrerenderData(
+      PrerenderHandle::PrerenderData* pending_prerender_data);
+
   // Utility method that is called from the virtual Shutdown method on this
   // class but is called directly from the TestPrerenderManager in the unit
   // tests.
   void DoShutdown();
 
  private:
-  // Test that needs needs access to internal functions.
   friend class PrerenderBrowserTest;
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, AliasURLTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, CancelAllTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest,
-                           CancelOmniboxRemovesOmniboxTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest,
-                           CancelOmniboxDoesNotRemoveLinkTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, ClearTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, ControlGroup);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, DropOldestRequestTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, DropSecondRequestTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, ExpireTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, FoundTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, FragmentMatchesFragmentTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, FragmentMatchesPageTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, LinkManagerAbandon);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, LinkManagerAddTwiceAbandonTwice);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, LinkManagerAddTwiceCancelTwice);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest,
-                           LinkManagerAddTwiceCancelTwiceThenAbandonTwice);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, LinkManagerCancel);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, LinkManagerCancelThenAbandon);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, LinkManagerCancelThenAddAgain);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, LinkManagerCancelTwice);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, LinkManagerExpireThenAddAgain);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, LinkManagerExpireThenCancel);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, NotSoRecentlyVisited);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, PageMatchesFragmentTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, PendingPrerenderTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, PPLTDummy);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, RateLimitInWindowTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, RateLimitOutsideWindowTest);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, RecentlyVisitedPPLTDummy);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, SourceRenderViewClosed);
-  FRIEND_TEST_ALL_PREFIXES(PrerenderTest, TwoElementPrerenderTest);
-
-  struct PrerenderContentsData;
-  struct NavigationRecord;
+  friend class UnitTestPrerenderManager;
 
   class OnCloseTabContentsDeleter;
+  struct NavigationRecord;
+
+  typedef PrerenderHandle::PrerenderData PrerenderData;
+  typedef std::map<const PrerenderHandle::PrerenderData*,
+                   base::WeakPtrFactory<PrerenderHandle::PrerenderData>*>

[mmenke, 2012/07/09 18:06:57]

nit:  You typedefed PrerenderHandle::PrerenderData just one line up (x2).

[dominich, 2012/07/09 18:25:43]

i asked for the typedef to be removed to make it clearer who the PrerenderData
was owned by.

[gavinp, 2012/07/11 17:04:00]

Yeah, sorry. My new upload gets rid of not only the users of the typedef, but
the actual typedef too.

+      PrerenderWeakPtrFactoryMap;
 
-  typedef std::list<PrerenderContentsData> PrerenderContentsDataList;
   typedef base::hash_map<content::WebContents*, bool> WouldBePrerenderedMap;
 
   // Time window for which we record old navigations, in milliseconds.
   static const int kNavigationRecordWindowMs = 5000;
 
-  // Adds a prerender for |url| from referrer |referrer| initiated from the
-  // child process specified by |child_id|. The |origin| specifies how the
-  // prerender was added. If the |size| is empty, then
-  // PrerenderContents::StartPrerendering will instead use the size of the
-  // currently active tab. If the current active tab size cannot be found, it
-  // then uses a default from PrerenderConfig.
-  bool AddPrerender(
+  // Adds a prerender for |url| from |referrer| initiated from the process
+  // |child_id|. The |origin| specifies how the prerender was added. If |size|
+  // is empty, then PrerenderContents::StartPrerendering will instead use the
+  // size of the currently active tab. If the current active tab size cannot be
+  // found, it then uses a default from PrerenderConfig. Returns a
+  // PrerenderHandle*, owned by the caller.

[mmenke, 2012/07/09 18:06:57]

Or NULL

[gavinp, 2012/07/11 17:04:00]

Done.

+  PrerenderHandle* AddPrerender(
       Origin origin,
       int child_id,
       const GURL& url,
@@ -322,23 +296,10 @@ class PrerenderManager : public base::SupportsWeakPtr<PrerenderManager>,
       gfx::Size size,
       content::SessionStorageNamespace* session_storage_namespace);
 
-  // Retrieves the PrerenderContents object for the specified URL, if it
-  // has been prerendered.  The caller will then have ownership of the
-  // PrerenderContents object and is responsible for freeing it.
-  // Returns NULL if the specified URL has not been prerendered.
-  PrerenderContents* GetEntry(const GURL& url);
-
-  // Identical to GetEntry, with one exception:
-  // The WebContents specified indicates the WC in which to swap the
-  // prerendering into.  If the WebContents specified is the one
-  // to doing the prerendered itself, will return NULL.
-  PrerenderContents* GetEntryButNotSpecifiedWC(const GURL& url,
-                                               content::WebContents* wc);
-
-  // Starts scheduling periodic cleanups.
   void StartSchedulingPeriodicCleanups();
-  // Stops scheduling periodic cleanups if they're no longer needed.
-  void MaybeStopSchedulingPeriodicCleanups();
+  void StopSchedulingPeriodicCleanups();
+
+  void EvictOldestPrerendersIfNecessary();
 
   // Deletes stale and cancelled prerendered PrerenderContents, as well as
   // WebContents that have been replaced by prerendered WebContents.
@@ -352,7 +313,7 @@ class PrerenderManager : public base::SupportsWeakPtr<PrerenderManager>,
   void PostCleanupTask();
 
   base::TimeDelta GetMaxAge() const;
-  bool IsPrerenderElementFresh(const base::Time start) const;
+  bool IsPrerenderFresh(base::TimeTicks start) const;
   void DeleteOldEntries();
   virtual base::Time GetCurrentTime() const;
   virtual base::TimeTicks GetCurrentTimeTicks() const;
@@ -366,20 +327,28 @@ class PrerenderManager : public base::SupportsWeakPtr<PrerenderManager>,
   // list.
   void DeletePendingDeleteEntries();
 
-  // Finds the specified PrerenderContentsData/PrerenderContents and returns it,
-  // if it exists. Returns NULL otherwise.  Unlike GetEntry, the
-  // PrerenderManager maintains ownership of the PrerenderContents.
-  PrerenderContentsData* FindEntryData(const GURL& url);
-  PrerenderContents* FindEntry(const GURL& url) const;
+  PrerenderHandle* CreatePrerenderHandleForPrerenderData(
+      PrerenderHandle::PrerenderData* prerender_data);

[mmenke, 2012/07/09 18:06:57]

nit:  All of these should just be PrerenderData, unless you want to get rid of
the typedef, or move it into the cc file (Any option is fine with me, just odd
to create a typedef and then not use it).

[gavinp, 2012/07/11 17:04:00]

Done.

+
+  void InvalidatePrerenderHandlesForPrerenderData(
+      const PrerenderHandle::PrerenderData& prerender_data);
 
-  // Returns the iterator to the PrerenderContentsData entry that is being
-  // prerendered from the given child route id pair.
-  PrerenderContentsDataList::iterator
-      FindPrerenderContentsForChildRouteIdPair(
-          const std::pair<int, int>& child_route_id_pair);
+  // Finds the active PrerenderData object for a running prerender matching
+  // |url| and |session_storage_namespace|.
+  PrerenderHandle::PrerenderData* FindPrerenderData(
+      const GURL& url,
+      const content::SessionStorageNamespace* session_storage_namespace);
+
+  // Finds the active PrerenderData object for a running prerender corresponding
+  // to the renderer process |child_id| and the render view in |route_id|.

[mmenke, 2012/07/09 18:06:57]

This comment could just as easily mean a prerender started by the given
RenderView (Though that may not be unique, in the future, I still find it
confusing).  Suggest something along the lines of:  "If |child_id| and
|route_id| correspond to a RenderView that's currenly being prerendered, returns
the PrerenderData object for that prerender.  Otherwise, returns NULL."

[gavinp, 2012/07/11 17:04:00]

Done.

+  PrerenderHandle::PrerenderData* FindPrerenderDataForChildAndRoute(
+      int child_id,
+      int route_id);
 
-  PrerenderContentsDataList::iterator
-      FindPrerenderContentsForURL(const GURL& url);
+  // Given the |prerender_contents|, find the iterator in active_prerender_list_
+  // correponding to that running prerender.

[mmenke, 2012/07/09 18:06:57]

nit:  "that running" -> "the given active"

[gavinp, 2012/07/11 17:04:00]

Done.

+  std::list<PrerenderHandle::PrerenderData>::iterator
+      FindIteratorForPrerenderContents(PrerenderContents* prerender_contents);
 
   bool DoesRateLimitAllowPrerender() const;
 
@@ -440,8 +409,23 @@ class PrerenderManager : public base::SupportsWeakPtr<PrerenderManager>,
 
   PrerenderTracker* prerender_tracker_;
 
-  // List of prerendered elements.
-  PrerenderContentsDataList prerender_list_;
+  // List of all running prerenders.
+  // It is kept sorted, in increasing order by expiry time. The STL list was
+  // chosen because it does not move items in memory (this lets handles continue
+  // to point to their PrerenderData).
+  std::list<PrerenderHandle::PrerenderData> active_prerender_list_;
+
+  // List of all pending prerenders.
+  std::list<PrerenderHandle::PrerenderData> pending_prerender_list_;
+
+  // The prerenders in the above lists need WeakPtrs to instantiate handles. We
+  // keep the factories for those WeakPtrs here for the lifetime of each element
+  // in either list. In a perfect world, these factories would be a part of
+  // PrerenderHandle::PrerenderData, but that would then make the copy semantics
+  // of PrerenderHandle::PrerenderData quite confusing (copy everything except
+  // for the WeakPtrFactory). Storing this map externally is a least bad
+  // compromise.
+  PrerenderWeakPtrFactoryMap prerender_weak_ptr_factory_map_;
 
   // List of recent navigations in this profile, sorted by ascending
   // navigate_time_.