Handle interface to prerenders.

chrome/browser/prerender/prerender_manager.cc

Index: chrome/browser/prerender/prerender_manager.cc
diff --git a/chrome/browser/prerender/prerender_manager.cc b/chrome/browser/prerender/prerender_manager.cc
index 18b2dec4670aed13d1b931bf44133b1935284623..a91a8df2dfa08eaada7173d4730f61e363ac1a0e 100644
--- a/chrome/browser/prerender/prerender_manager.cc
+++ b/chrome/browser/prerender/prerender_manager.cc
@@ -170,18 +170,159 @@ int PrerenderManager::prerenders_per_session_count_ = 0;
 PrerenderManager::PrerenderManagerMode PrerenderManager::mode_ =
     PRERENDER_MODE_ENABLED;
 
-struct PrerenderManager::PrerenderContentsData {
-  PrerenderContents* contents_;
+// A PrerenderContentsData represents a running prerender. A list of WeakPtr
+// to these objects is kept in prerender_list_ as the canonical list of running
+// prerenders.
+class PrerenderManager::PrerenderContentsData :
+      public PrerenderHandleInterface {

[dominich, 2012/06/18 15:32:44]

I don't understand why this has a PrerenderHandle interface. Either it /is/ a
PrerenderHandle, or it contains a PrerenderHandle, or it is related to a
PrerenderHandle.

[gavinp, 2012/06/18 16:40:48]

Every other instance of Handle in our code base is as a reference, ala a weakptr
or scoped ptr to an object. I think this is a PrerenderInterface, or maybe a
Prerender itself... But it isn't a Handle if it isn't indirect. I suppose maybe
the name Handle is bad, then.

+ public:
+  // These STL predicates all operate on WeakPtrs, for compatibility with
+  // PrerenderManager::PrerenderContentsList, which contains
+  // base::WeakPtr<PrerenderContentsData>.
+
+  // This predicate tests if the weak-pointed to object has been deleted.
+  class DeletedPredicate;
+  // This predicate compares the object to another PrerenderHandle for equality.
+  class EqualsPredicate;
+  // Given a |base::Time|, is the current prerender expired?
+  class ExpiredPredicate;
+  // Given an |url| and a |session_storage_namespace|, This predicate tests if
+  // this PrerenderContentsData
+  class MatchPredicate;

[dominich, 2012/06/18 15:32:44]

I smell over-engineering. These were already handled in a very simple to
understand way in the code. Adding these seems unrelated to PrerenderHandle and,
to me, adds complexity to the code that is unnecessary.

[gavinp, 2012/06/18 16:40:48]

Yes, I hate these and plan to remove them.

On 2012/06/18 15:32:44, dominich wrote:

+
+  // Creates a PrerenderContentsData for a prerender starting right now.
+  PrerenderContentsData(PrerenderManager* manager,
+                        PrerenderContents* contents);
+
+  base::WeakPtr<PrerenderContentsData> AsWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+  PrerenderContents* contents() { return contents_; }
+  const PrerenderContents* contents() const { return contents_; }
+
+  void set_contents(PrerenderContents* contents) { contents_ = contents; }
+
+  bool IsExpiredAt(base::Time time) const;
+  bool IsDestroyed() const;
+  bool IsStarted() const;
+
+  // from PrerenderHandleInterface
+  virtual bool DidFinishLoading() const OVERRIDE;
+  virtual bool IsPending() const OVERRIDE;
+  virtual void Destroy() OVERRIDE;
+
+  static PrerenderContentsData* FromPrerenderHandle(PrerenderHandleInterface* interface);
+
+ private:
+  virtual ~PrerenderContentsData();
+
+  base::WeakPtrFactory<PrerenderContentsData> weak_factory_;
+  PrerenderManager const* manager_;
   base::Time start_time_;
-  int active_count_;
-  PrerenderContentsData(PrerenderContents* contents, base::Time start_time)
-      : contents_(contents),
-        start_time_(start_time),
-        active_count_(1) {
-    CHECK(contents);
+  base::Time expiry_time_;
+  PrerenderContents* contents_;
+};
+
+class PrerenderManager::PrerenderContentsData::DeletedPredicate {
+ public:
+  bool operator()(base::WeakPtr<PrerenderContentsData> item) const {
+    return !item;
+  }
+};
+
+class PrerenderManager::PrerenderContentsData::EqualsPredicate {
+ public:
+  EqualsPredicate(PrerenderHandle handle) : handle_(handle) {}
+
+  bool operator()(const base::WeakPtr<PrerenderContentsData>& item) const {
+    return handle_.get() == item.get();
   }
+
+ private:
+  PrerenderHandle handle_;
 };
 
+class PrerenderManager::PrerenderContentsData::ExpiredPredicate {
+ public:
+  ExpiredPredicate(base::Time now) : now_(now) {}
+  
+  bool operator()(base::WeakPtr<PrerenderContentsData> item) const {
+    return item && item->IsExpiredAt(now_);
+  }
+ private:
+  base::Time now_;
+};
+
+class PrerenderManager::PrerenderContentsData::MatchPredicate {
+ public:
+  MatchPredicate(const GURL& url,
+                 content::SessionStorageNamespace* session_storage_namespace)
+      : url_(url),
+        session_storage_namespace_(session_storage_namespace) {
+  }
+
+  bool operator()(base::WeakPtr<PrerenderContentsData> item) const {
+    return item && item->contents_ &&
+        item->contents_->Matches(url_, session_storage_namespace_);
+  }
+
+ private:
+  GURL url_;
+  content::SessionStorageNamespace* session_storage_namespace_;
+};
+
+PrerenderManager::PrerenderContentsData::PrerenderContentsData(
+    PrerenderManager* manager,
+    PrerenderContents* contents) : weak_factory_(this),
+                                   manager_(manager),
+                                   start_time_(manager_->GetCurrentTime()),

[dominich, 2012/06/18 15:32:44]

why is the manager the arbiter of what the time is? shouldn't this be
base::TimeTicks::Now() ?

+                                   expiry_time_(start_time_ +

[dominich, 2012/06/18 15:32:44]

if this can be inferred from the start_time_ there's no need for it to be a
member. Or, if this might be mutable as it can be extended, there's no need to
keep the start_time_.

+                                                manager_->GetMaxAge()),
+                                   contents_(contents) {
+}
+
+bool PrerenderManager::PrerenderContentsData::IsExpiredAt(
+    base::Time time) const {
+  return time > expiry_time_;
+}
+
+bool PrerenderManager::PrerenderContentsData::IsDestroyed() const {
+  return !manager_ && !contents_;
+}
+
+bool PrerenderManager::PrerenderContentsData::IsStarted() const {
+  return manager_ && contents_;
+}
+
+bool PrerenderManager::PrerenderContentsData::DidFinishLoading() const {
+  return contents_ ? contents_->has_finished_loading() : false;
+}
+
+bool PrerenderManager::PrerenderContentsData::IsPending() const {
+  DCHECK(!IsDestroyed());
+  return manager_ && !contents_;
+}
+
+// static
+PrerenderManager::PrerenderContentsData*
+PrerenderManager::PrerenderContentsData::FromPrerenderHandle(
+    PrerenderHandleInterface* interface) {
+  return static_cast<PrerenderContentsData*>(interface);

[dominich, 2012/06/18 15:32:44]

Code smell. If you need a static method to cast from the base class to the
derived class you're doing something wrong.

+}
+
+PrerenderManager::PrerenderContentsData::~PrerenderContentsData() {
+  DVLOG(4) << "PrerenderContentsData::~PrerenderContentsData()";
+  if (contents_)
+    contents_->Destroy(FINAL_STATUS_CANCELLED);
+}
+
+void PrerenderManager::PrerenderContentsData::Destroy() {
+  DCHECK(!IsDestroyed());
+  contents_ = NULL;
+  manager_ = NULL;
+}
+
 struct PrerenderManager::NavigationRecord {
   GURL url_;
   base::TimeTicks time_;
@@ -215,7 +356,7 @@ void PrerenderManager::Shutdown() {
   DoShutdown();
 }
 
-bool PrerenderManager::AddPrerenderFromLinkRelPrerender(
+PrerenderHandle PrerenderManager::AddPrerenderFromLinkRelPrerender(
     int process_id,
     int route_id,
     const GURL& url,
@@ -224,16 +365,14 @@ bool PrerenderManager::AddPrerenderFromLinkRelPrerender(
 #if defined(OS_ANDROID)
   // TODO(jcivelli): http://crbug.com/113322 We should have an option to disable
   //                link-prerender and enable omnibox-prerender only.
-  return false;
+  return PrerenderHandle();
 #else
-  std::pair<int, int> child_route_id_pair(process_id, route_id);
-  PrerenderContentsDataList::iterator it =
-      FindPrerenderContentsForChildRouteIdPair(child_route_id_pair);
-  if (it != prerender_list_.end()) {
-    // Instead of prerendering from inside of a running prerender, we will defer
-    // this request until its launcher is made visible.
-    it->contents_->AddPendingPrerender(url, referrer, size);
-    return true;
+  if (PrerenderContentsData* contents_data =
+          FindContentsDataForChildAndRouteId(process_id, route_id)) {
+    // If we are attempting to prerender from inside of a prerender, make a
+    // pending prerender in the contents, which will launch again when the
+    // user navigates to the launcher.
+    return contents_data->contents()->AddPendingPrerender(url, referrer, size);

[dominich, 2012/06/18 15:32:44]

Ah, now I see why you need this to return a valid PrerenderHandle. I still think
conflating the pending prerender info with the prerender handle is a bad idea
and this can all be simplified by making the PrerenderHandle a much more simple
class.

   }
 
   // Unit tests pass in a process_id == -1.
@@ -243,7 +382,7 @@ bool PrerenderManager::AddPrerenderFromLinkRelPrerender(
     source_render_view_host =
         RenderViewHost::FromID(process_id, route_id);
     if (!source_render_view_host || !source_render_view_host->GetView())
-      return false;
+      return PrerenderHandle();
     session_storage_namespace =
         source_render_view_host->GetSessionStorageNamespace();
   }
@@ -254,79 +393,68 @@ bool PrerenderManager::AddPrerenderFromLinkRelPrerender(
 #endif
 }
 
-bool PrerenderManager::AddPrerenderFromOmnibox(
+PrerenderHandle PrerenderManager::AddPrerenderFromOmnibox(
     const GURL& url,
     SessionStorageNamespace* session_storage_namespace) {
   if (!IsOmniboxEnabled(profile_))
-    return false;
+    return PrerenderHandle();
   return AddPrerender(ORIGIN_OMNIBOX, -1, url,
                       content::Referrer(), gfx::Size(),
                       session_storage_namespace);
 }
 
-void PrerenderManager::MaybeCancelPrerender(const GURL& url) {
-  PrerenderContentsDataList::iterator it = FindPrerenderContentsForURL(url);
-  if (it == prerender_list_.end())
-    return;
-  PrerenderContentsData& prerender_contents_data = *it;
-  if (--prerender_contents_data.active_count_ == 0)
-    prerender_contents_data.contents_->Destroy(FINAL_STATUS_CANCELLED);
-}
-
 void PrerenderManager::DestroyPrerenderForRenderView(
     int process_id, int view_id, FinalStatus final_status) {
   DCHECK(CalledOnValidThread());
-  PrerenderContentsDataList::iterator it =
-      FindPrerenderContentsForChildRouteIdPair(
-          std::make_pair(process_id, view_id));
-  if (it != prerender_list_.end()) {
-    PrerenderContents* prerender_contents = it->contents_;
-    prerender_contents->Destroy(final_status);
+  if (PrerenderContentsData* contents_data =
+          FindContentsDataForChildAndRouteId(process_id, view_id)) {
+    contents_data->contents()->Destroy(final_status);
   }
 }
 
 void PrerenderManager::CancelAllPrerenders() {
   DCHECK(CalledOnValidThread());
   while (!prerender_list_.empty()) {
-    PrerenderContentsData data = prerender_list_.front();
-    DCHECK(data.contents_);
-    data.contents_->Destroy(FINAL_STATUS_CANCELLED);
-  }
-}
-
-void PrerenderManager::CancelOmniboxPrerenders() {
-  DCHECK(CalledOnValidThread());
-  for (PrerenderContentsDataList::iterator it = prerender_list_.begin();
-       it != prerender_list_.end(); ) {
-    PrerenderContentsDataList::iterator cur = it++;
-    if (cur->contents_->origin() == ORIGIN_OMNIBOX)
-      cur->contents_->Destroy(FINAL_STATUS_CANCELLED);
+    if (PrerenderContentsData* contents_data = prerender_list_.front()) {
+      DCHECK(contents_data->contents());
+      contents_data->contents()->Destroy(FINAL_STATUS_CANCELLED);
+    }
   }
 }
 
 bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
                                                const GURL& url) {
+  DCHECK(web_contents);
   DCHECK(CalledOnValidThread());
   DCHECK(!IsWebContentsPrerendering(web_contents));
 
+  DVLOG(6) << "entering MaybeUsePrerenderedPage...";
+  
+  RenderViewHost* old_render_view_host = web_contents->GetRenderViewHost();
+  content::SessionStorageNamespace* old_session_storage_namespace =
+      old_render_view_host->GetSessionStorageNamespace();
+
   scoped_ptr<PrerenderContents> prerender_contents(
-      GetEntryButNotSpecifiedWC(url, web_contents));
+      ClaimPrerender(web_contents, url, old_session_storage_namespace));
+
   if (prerender_contents.get() == NULL)
     return false;
 
+
+  DVLOG(2) << "MaybeUsePrerenderedPage claimed a prerender";
+
   // Do not use the prerendered version if there is an opener object.
   if (web_contents->HasOpener()) {
     prerender_contents.release()->Destroy(FINAL_STATUS_WINDOW_OPENER);
+    DVLOG(3) << "... but it's a window opener.";
     return false;
   }
 
-  // Even if we match, the location.hash might be different. Record this as a
-  // separate final status.
-  GURL matching_url;
-  bool url_matches = prerender_contents->MatchesURL(url, &matching_url);
-  DCHECK(url_matches);
-  if (url_matches && url.ref() != matching_url.ref()) {
-    prerender_contents.release()->Destroy(FINAL_STATUS_FRAGMENT_MISMATCH);
+
+
+  if (!prerender_contents->Matches(url, old_session_storage_namespace)) {
+    NOTREACHED() << "Something didn't match.";
+    DVLOG(3) << "... but there's a bad session storage namespace";
     return false;
   }
 
@@ -336,6 +464,7 @@ bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
   if (!prerender_contents->prerendering_has_started()) {
     MarkWebContentsAsWouldBePrerendered(web_contents);
     prerender_contents.release()->Destroy(FINAL_STATUS_WOULD_HAVE_BEEN_USED);
+    DVLOG(3) << "... but we're in the control group, so we'll destroy it unused.";
     return false;
   }
 
@@ -344,6 +473,7 @@ bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
   if (content::DevToolsAgentHostRegistry::IsDebuggerAttached(web_contents)) {
     DestroyAndMarkMatchCompleteAsUsed(prerender_contents.release(),
                                       FINAL_STATUS_DEVTOOLS_ATTACHED);
+    DVLOG(3) << "... but the dev tools are attached.";
     return false;
   }
 
@@ -353,21 +483,7 @@ bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
     DestroyAndMarkMatchCompleteAsUsed(
         prerender_contents.release(),
         FINAL_STATUS_CROSS_SITE_NAVIGATION_PENDING);
-    return false;
-  }
-
-  // If the session storage namespaces don't match, cancel the prerender.
-  RenderViewHost* old_render_view_host = web_contents->GetRenderViewHost();
-  RenderViewHost* new_render_view_host =
-      prerender_contents->prerender_contents()->web_contents()->
-          GetRenderViewHost();
-  DCHECK(old_render_view_host);
-  DCHECK(new_render_view_host);
-  if (old_render_view_host->GetSessionStorageNamespace() !=
-      new_render_view_host->GetSessionStorageNamespace()) {
-    DestroyAndMarkMatchCompleteAsUsed(
-        prerender_contents.release(),
-        FINAL_STATUS_SESSION_STORAGE_NAMESPACE_MISMATCH);
+    DVLOG(3) << "... but cross site navigation is pending.";
     return false;
   }
 
@@ -377,18 +493,21 @@ bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
   if (GetMode() == PRERENDER_MODE_EXPERIMENT_NO_USE_GROUP) {
     MarkWebContentsAsWouldBePrerendered(web_contents);
     prerender_contents.release()->Destroy(FINAL_STATUS_WOULD_HAVE_BEEN_USED);
+    DVLOG(3) << "... we are in the no use experimental group...";
     return false;
   }
 
   int child_id, route_id;
-  CHECK(prerender_contents->GetChildId(&child_id));
-  CHECK(prerender_contents->GetRouteId(&route_id));
+  DCHECK(prerender_contents->GetChildId(&child_id));
+  DCHECK(prerender_contents->GetRouteId(&route_id));
 
   // Try to set the prerendered page as used, so any subsequent attempts to
   // cancel on other threads will fail.  If this fails because the prerender
   // was already cancelled, possibly on another thread, fail.
-  if (!prerender_tracker_->TryUse(child_id, route_id))
+  if (!prerender_tracker_->TryUse(child_id, route_id)) {
+    DVLOG(3) << "... prerender_tracker_->TryUse failed!";
     return false;
+  }
 
   if (!prerender_contents->load_start_time().is_null()) {
     histograms_->RecordTimeUntilUsed(GetCurrentTimeTicks() -
@@ -400,6 +519,10 @@ bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
   histograms_->RecordUsedPrerender(prerender_contents->origin());
   prerender_contents->set_final_status(FINAL_STATUS_USED);
 
+  RenderViewHost* new_render_view_host =
+      prerender_contents->prerender_contents()->web_contents()->
+          GetRenderViewHost();
+
   new_render_view_host->Send(
       new PrerenderMsg_SetIsPrerendering(new_render_view_host->GetRoutingID(),
                                          false));
@@ -455,63 +578,80 @@ bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
   //                 list, instead of deleting directly here?
   AddToHistory(prerender_contents.get());
   RecordNavigation(url);
+  DVLOG(3) << "... success!";
   return true;
 }
 
-void PrerenderManager::MoveEntryToPendingDelete(PrerenderContents* entry,
+void PrerenderManager::MoveEntryToPendingDelete(WeakPrerenderHandle prerender,
                                                 FinalStatus final_status) {
+  if (!prerender)
+    return;
   DCHECK(CalledOnValidThread());
-  DCHECK(entry);
-  // Confirm this entry has not already been moved to the pending delete list.
-  DCHECK_EQ(0, std::count(pending_delete_list_.begin(),
-                          pending_delete_list_.end(), entry));
 
-  for (PrerenderContentsDataList::iterator it = prerender_list_.begin();
-       it != prerender_list_.end();
-       ++it) {
-    if (it->contents_ == entry) {
-      bool swapped_in_dummy_replacement = false;
-
-      // If this PrerenderContents is being deleted due to a cancellation,
-      // we need to create a dummy replacement for PPLT accounting purposes
-      // for the Match Complete group.
-      // This is the case if the cancellation is for any reason that would not
-      // occur in the control group case.
-      if (entry->match_complete_status() ==
-          PrerenderContents::MATCH_COMPLETE_DEFAULT &&
-          NeedMatchCompleteDummyForFinalStatus(final_status) &&
-          ActuallyPrerendering()) {
-        // TODO(tburkard): I'd like to DCHECK that we are actually prerendering.
-        // However, what if new conditions are added and
-        // NeedMatchCompleteDummyForFinalStatus, is not being updated.  Not sure
-        // what's the best thing to do here.  For now, I will just check whether
-        // we are actually prerendering.
-        entry->set_match_complete_status(
-            PrerenderContents::MATCH_COMPLETE_REPLACED);
-        if (PrerenderContents* dummy_replacement_prerender_contents =
-                CreatePrerenderContents(entry->prerender_url(),
-                                        entry->referrer(),
-                                        entry->origin(),
-                                        entry->experiment_id())) {
-          dummy_replacement_prerender_contents->set_match_complete_status(
-              PrerenderContents::MATCH_COMPLETE_REPLACEMENT_PENDING);
-          if (!dummy_replacement_prerender_contents->Init())
-            break;
-          dummy_replacement_prerender_contents->
-              AddAliasURLsFromOtherPrerenderContents(entry);
-          dummy_replacement_prerender_contents->set_match_complete_status(
-              PrerenderContents::MATCH_COMPLETE_REPLACEMENT);
-          it->contents_ = dummy_replacement_prerender_contents;
-          swapped_in_dummy_replacement = true;
-        }
+  DVLOG(2) << "MoveEntryToPendingDelete";
+  DVLOG(2) << "... final_status = \"" << NameFromFinalStatus(final_status) << "\"";
+
+  PrerenderContentsData* contents_data =
+      PrerenderContentsData::FromPrerenderHandle(prerender.get());
+  DVLOG(3) << "... contents()->prerender_url() = "
+           << contents_data->contents()->prerender_url().spec();
+  DCHECK(contents_data->contents());
+  // Confirm this prerender has not already been moved to the pending delete
+  // list.
+  DCHECK_EQ(0, std::count(pending_delete_list_.begin(),
+                          pending_delete_list_.end(),
+                          contents_data->contents()));
+
+  bool swapped_in_dummy_replacement = false;
+
+  // If this PrerenderContents is being deleted due to a cancellation,
+  // we need to create a dummy replacement for PPLT accounting purposes
+  // for the Match Complete group.
+  // This is the case if the cancellation is for any reason that would not
+  // occur in the control group case.
+  if (contents_data->contents()->match_complete_status() ==

[dominich, 2012/06/18 15:32:44]

If you cache contents_data->contents() in a const PrerenderContentsData* const
local var, the changes in this file will be minimized and you'll help the
compiler make some decisions about how to render this code.

[gavinp, 2012/06/18 16:40:48]

Yes on the minimized changes, but probably not so helpful to the compiler.

+      PrerenderContents::MATCH_COMPLETE_DEFAULT &&
+      NeedMatchCompleteDummyForFinalStatus(final_status) &&
+      ActuallyPrerendering()) {
+    DVLOG(8) << "swapping!";
+    // TODO(tburkard): I'd like to DCHECK that we are actually prerendering.
+    // However, what if new conditions are added and
+    // NeedMatchCompleteDummyForFinalStatus, is not being updated.  Not sure
+    // what's the best thing to do here.  For now, I will just check whether
+    // we are actually prerendering.
+    contents_data->contents()->set_match_complete_status(
+        PrerenderContents::MATCH_COMPLETE_REPLACED);
+    if (PrerenderContents* dummy_replacement_prerender_contents =
+            CreatePrerenderContents(
+                contents_data->contents()->prerender_url(),
+                contents_data->contents()->referrer(),
+                contents_data->contents()->origin(),
+                contents_data->contents()->experiment_id())) {
+      DVLOG(8) << "really swapping";
+      dummy_replacement_prerender_contents->set_match_complete_status(
+          PrerenderContents::MATCH_COMPLETE_REPLACEMENT_PENDING);
+      if (dummy_replacement_prerender_contents->Init(prerender)) {
+        DVLOG(8) << "i totally am going to swap";
+        dummy_replacement_prerender_contents->
+            AddAliasURLsFromOtherPrerenderContents(contents_data->contents());
+        dummy_replacement_prerender_contents->set_match_complete_status(
+            PrerenderContents::MATCH_COMPLETE_REPLACEMENT);
+        contents_data->set_contents(dummy_replacement_prerender_contents);
+        swapped_in_dummy_replacement = true;
       }
-      if (!swapped_in_dummy_replacement)
-        prerender_list_.erase(it);
-      break;
     }
   }
-  AddToHistory(entry);
-  pending_delete_list_.push_back(entry);
+  if (!swapped_in_dummy_replacement) {
+    DCHECK_EQ(1, std::count(prerender_list_.begin(), prerender_list_.end(),
+                            contents_data));
+    prerender_list_.erase(
+        std::find(prerender_list_.begin(), prerender_list_.end(),
+                  contents_data));
+  }
+
+  AddToHistory(contents_data->contents());
+  pending_delete_list_.push_back(contents_data->contents());
+  contents_data->Destroy();
 
   // Destroy the old WebContents relatively promptly to reduce resource usage,
   // and in the case of HTML5 media, reduce the change of playing any sound.
@@ -623,11 +763,13 @@ bool PrerenderManager::IsWebContentsPrerendering(
   for (PrerenderContentsDataList::const_iterator it = prerender_list_.begin();
        it != prerender_list_.end();
        ++it) {
-    TabContents* prerender_tab_contents = it->contents_->prerender_contents();
-    if (prerender_tab_contents &&
-        prerender_tab_contents->web_contents() == web_contents) {
+    const PrerenderContentsData* contents_data = *it;
+    if (!contents_data) continue;
+    TabContents* prerender_tab_contents =
+        contents_data->contents()->prerender_contents();
+    if (!prerender_tab_contents) continue;
+    if (prerender_tab_contents->web_contents() == web_contents)
       return true;
-    }
   }
 
   // Also look through the pending-deletion list.
@@ -644,12 +786,6 @@ bool PrerenderManager::IsWebContentsPrerendering(
   return false;
 }
 
-bool PrerenderManager::DidPrerenderFinishLoading(const GURL& url) const {
-  DCHECK(CalledOnValidThread());
-  PrerenderContents* contents = FindEntry(url);
-  return contents ? contents->has_finished_loading() : false;
-}
-
 void PrerenderManager::MarkWebContentsAsPrerendered(WebContents* web_contents) {
   DCHECK(CalledOnValidThread());
   prerendered_tab_contents_set_.insert(web_contents);
@@ -765,22 +901,6 @@ void PrerenderManager::AddCondition(const PrerenderCondition* condition) {
   prerender_conditions_.push_back(condition);
 }
 
-bool PrerenderManager::IsPendingEntry(const GURL& url) const {
-  DCHECK(CalledOnValidThread());
-  for (PrerenderContentsDataList::const_iterator it = prerender_list_.begin();
-       it != prerender_list_.end();
-       ++it) {
-    if (it->contents_->IsPendingEntry(url))
-      return true;
-  }
-  return false;
-}
-
-bool PrerenderManager::IsPrerendering(const GURL& url) const {
-  DCHECK(CalledOnValidThread());
-  return (FindEntry(url) != NULL);
-}
-
 void PrerenderManager::RecordNavigation(const GURL& url) {
   DCHECK(CalledOnValidThread());
 
@@ -802,18 +922,53 @@ void PrerenderManager::DoShutdown() {
   profile_ = NULL;
 }
 
-// private
-bool PrerenderManager::AddPrerender(
+const PrerenderContents* PrerenderManager::FindPrerender(
+    const GURL& url) const {
+  DVLOG(4) << "PrerenderManager::FindPrerender";
+  DVLOG(5) << "... prerender_list_.size() = " << prerender_list_.size();
+  
+  for (PrerenderContentsDataList::const_iterator
+           it = prerender_list_.begin(), end = prerender_list_.end();
+       it != end; ++it) {
+    if (!*it || !(*it)->contents()) continue;
+    if ((*it)->contents()->Matches(url, NULL))

[dominich, 2012/06/18 15:32:44]

nit: if (*it && (*it)->contents() && (*it)->contents()->Matches(url, NULL))

+      return (*it)->contents();
+  }
+  return NULL;
+}
+
+PrerenderContents* PrerenderManager::ClaimPrerender(
+    content::WebContents* web_contents,
+    const GURL& url,
+    content::SessionStorageNamespace* session_storage_namespace) {
+  for (PrerenderContentsDataList::iterator
+           it = prerender_list_.begin(), end = prerender_list_.end();
+       it != end; ++it) {
+    if (!*it || !(*it)->contents()) continue;
+    PrerenderContents* contents = (*it)->contents();
+    if (contents->Matches(url, session_storage_namespace) &&
+        !IsNoSwapInExperiment(contents->experiment_id()) &&
+        contents->prerender_contents()->web_contents() != web_contents) {
+      (*it)->Destroy();
+      prerender_list_.erase(it);
+      return contents;
+    }
+  }
+  return NULL;
+}
+
+PrerenderHandle PrerenderManager::AddPrerender(
     Origin origin,
     int process_id,
     const GURL& url_arg,
     const content::Referrer& referrer,
     const gfx::Size& size,
     SessionStorageNamespace* session_storage_namespace) {
+  DVLOG(4) << "PrerenderManager::AddPrerender";
   DCHECK(CalledOnValidThread());
 
   if (!IsEnabled())
-    return false;
+    return PrerenderHandle();
 
   if (origin == ORIGIN_LINK_REL_PRERENDER &&
       IsGoogleSearchResultURL(referrer.url)) {
@@ -837,12 +992,15 @@ bool PrerenderManager::AddPrerender(
   // histogram tracking.
   histograms_->RecordPrerender(origin, url_arg);
 
-  if (PrerenderContentsData* prerender_contents_data = FindEntryData(url)) {
-    ++prerender_contents_data->active_count_;
+  PrerenderContentsDataList::const_iterator duplicate =
+      std::find_if(prerender_list_.begin(), prerender_list_.end(),
+                   PrerenderContentsData::MatchPredicate(
+                       url, session_storage_namespace));
+  if (duplicate != prerender_list_.end()) {
     RecordFinalStatus(origin, experiment, FINAL_STATUS_DUPLICATE);
-    return true;
+    return PrerenderHandle(duplicate->get());
   }
-
+    
   // Do not prerender if there are too many render processes, and we would
   // have to use an existing one.  We do not want prerendering to happen in
   // a shared process, so that we can always reliably lower the CPU
@@ -858,7 +1016,7 @@ bool PrerenderManager::AddPrerender(
           profile_, url) &&
       !content::RenderProcessHost::run_renderer_in_process()) {
     RecordFinalStatus(origin, experiment, FINAL_STATUS_TOO_MANY_PROCESSES);
-    return false;
+    return PrerenderHandle();
   }
 #endif
 
@@ -868,61 +1026,47 @@ bool PrerenderManager::AddPrerender(
     // this doesn't make sense as the next prerender request will be triggered
     // by a navigation and is unlikely to be the same site.
     RecordFinalStatus(origin, experiment, FINAL_STATUS_RATE_LIMIT_EXCEEDED);
-    return false;
+    return PrerenderHandle();
   }
 
   PrerenderContents* prerender_contents = CreatePrerenderContents(
       url, referrer, origin, experiment);
-  if (!prerender_contents || !prerender_contents->Init())
-    return false;
-
-  histograms_->RecordPrerenderStarted(origin);
 
   // TODO(cbentzel): Move invalid checks here instead of PrerenderContents?
-  PrerenderContentsData data(prerender_contents, GetCurrentTime());
+  PrerenderContentsData* data;

[dominich, 2012/06/18 15:32:44]

PrerenderContentsData* data =
    new PrerenderContentsData(this, prerender_contents);
PrerenderHandle handle(data);

+  PrerenderHandle handle(
+      data = new PrerenderContentsData(this, prerender_contents));
+  
+  DVLOG(5) << "handle initial ref_count_ = " << handle->ref_count_;
 
-  prerender_list_.push_back(data);
+  if (!prerender_contents || !prerender_contents->Init(handle->AsWeakPtr()))
+    return PrerenderHandle();
+
+  histograms_->RecordPrerenderStarted(origin);
+  
+  prerender_list_.push_back(data->AsWeakPtr());
 
   last_prerender_start_time_ = GetCurrentTimeTicks();
 
-  data.contents_->StartPrerendering(process_id, size, session_storage_namespace,
-                                    control_group_behavior);
+  data->contents()->StartPrerendering(
+      process_id, size, session_storage_namespace, control_group_behavior);
+
+  // Compact the list of prerenders by removing all of the deleted
+  // PrerenderContentsData objects, so that the size() will equal the
+  // number of running prerenders.
+  prerender_list_.erase(
+      std::remove_if(prerender_list_.begin(), prerender_list_.end(),
+                     PrerenderContentsData::DeletedPredicate()),
+      prerender_list_.end());
 
   while (prerender_list_.size() > config_.max_elements) {
     data = prerender_list_.front();
     prerender_list_.pop_front();
-    data.contents_->Destroy(FINAL_STATUS_EVICTED);
+    data->contents()->Destroy(FINAL_STATUS_EVICTED);
   }
   StartSchedulingPeriodicCleanups();
-  return true;
-}
-
-PrerenderContents* PrerenderManager::GetEntry(const GURL& url) {
-  return GetEntryButNotSpecifiedWC(url, NULL);
-}
-
-PrerenderContents* PrerenderManager::GetEntryButNotSpecifiedWC(
-    const GURL& url,
-    WebContents* wc) {
-  DCHECK(CalledOnValidThread());
-  DeleteOldEntries();
-  DeletePendingDeleteEntries();
-  for (PrerenderContentsDataList::iterator it = prerender_list_.begin();
-       it != prerender_list_.end();
-       ++it) {
-    PrerenderContents* prerender_contents = it->contents_;
-    if (prerender_contents->MatchesURL(url, NULL) &&
-        !IsNoSwapInExperiment(prerender_contents->experiment_id())) {
-      if (!prerender_contents->prerender_contents() ||
-          !wc ||
-          prerender_contents->prerender_contents()->web_contents() != wc) {
-        prerender_list_.erase(it);
-        return prerender_contents;
-      }
-    }
-  }
-  // Entry not found.
-  return NULL;
+  DVLOG(5) << "on exit ref_count_ = " << handle->ref_count_;
+  return handle;
 }
 
 void PrerenderManager::StartSchedulingPeriodicCleanups() {
@@ -950,17 +1094,17 @@ void PrerenderManager::PeriodicCleanup() {
 
   // Grab a copy of the current PrerenderContents pointers, so that we
   // will not interfere with potential deletions of the list.
-  std::vector<PrerenderContents*> prerender_contents;
+  typedef std::vector<PrerenderContents*> PrerenderContentsVector;
+  PrerenderContentsVector prerender_contents;
+  prerender_contents.reserve(prerender_list_.size());
   for (PrerenderContentsDataList::iterator it = prerender_list_.begin();
        it != prerender_list_.end();
        ++it) {
-    DCHECK(it->contents_);
-    prerender_contents.push_back(it->contents_);
+    if (!*it || !(*it)->contents()) continue;
+    prerender_contents.push_back((*it)->contents());
   }
-  for (std::vector<PrerenderContents*>::iterator it =
-           prerender_contents.begin();
-       it != prerender_contents.end();
-       ++it) {
+  for (PrerenderContentsVector::iterator it = prerender_contents.begin();
+       it != prerender_contents.end(); ++it) {
     (*it)->DestroyWhenUsingTooManyResources();
   }
 
@@ -980,31 +1124,34 @@ base::TimeDelta PrerenderManager::GetMaxAge() const {
       base::TimeDelta::FromSeconds(300) : config_.max_age);
 }
 
-bool PrerenderManager::IsPrerenderElementFresh(const base::Time start) const {
-  DCHECK(CalledOnValidThread());
-  base::Time now = GetCurrentTime();
-  return (now - start < GetMaxAge());
-}
-
 void PrerenderManager::DeleteOldEntries() {
   DCHECK(CalledOnValidThread());
-  while (!prerender_list_.empty()) {
-    PrerenderContentsData data = prerender_list_.front();
-    if (IsPrerenderElementFresh(data.start_time_))
-      return;
-    data.contents_->Destroy(FINAL_STATUS_TIMED_OUT);
+  PrerenderContentsData::ExpiredPredicate expired_predicate(GetCurrentTime());

[dominich, 2012/06/18 15:32:44]

this code is longer and more complicated than the original. I suggest it's not
worth the change.

[gavinp, 2012/06/18 16:40:48]

Yes. I hate the predicates too.

+
+  PrerenderContentsDataList::iterator it =
+      std::find_if(prerender_list_.begin(), prerender_list_.end(),
+                   expired_predicate);
+  while (it != prerender_list_.end()) {
+    (*it)->contents()->Destroy(FINAL_STATUS_TIMED_OUT);
+
+    ++it;
+    it = std::find_if(it, prerender_list_.end(), expired_predicate);
   }
+
   MaybeStopSchedulingPeriodicCleanups();
 }
 
+// virtual (to allow testing)
 base::Time PrerenderManager::GetCurrentTime() const {
   return base::Time::Now();
 }
 
+// virtual (to allow testing)
 base::TimeTicks PrerenderManager::GetCurrentTimeTicks() const {
   return base::TimeTicks::Now();
 }
 
+// virtual (to allow testing)
 PrerenderContents* PrerenderManager::CreatePrerenderContents(
     const GURL& url,
     const content::Referrer& referrer,
@@ -1024,58 +1171,47 @@ void PrerenderManager::DeletePendingDeleteEntries() {
   }
 }
 
-PrerenderManager::PrerenderContentsData* PrerenderManager::FindEntryData(
-    const GURL& url) {
+const PrerenderManager::PrerenderContentsData*
+PrerenderManager::FindContentsData(PrerenderHandle handle) const {
   DCHECK(CalledOnValidThread());
-  PrerenderContentsDataList::iterator it = FindPrerenderContentsForURL(url);
+  // Confirm this handle occurs at most once in our prerender list.
+  DCHECK_GE(1, std::count_if(prerender_list_.begin(), prerender_list_.end(),

[dominich, 2012/06/18 15:32:44]

pointless test - find_if would return prerender_list_.end() if it doesn't so
just DCHECK on that instead.

+                             PrerenderContentsData::EqualsPredicate(handle)));
+  PrerenderContentsDataList::const_iterator it =
+      std::find_if(prerender_list_.begin(), prerender_list_.end(),
+                   PrerenderContentsData::EqualsPredicate(handle));
   if (it == prerender_list_.end())
     return NULL;
-  PrerenderContentsData& prerender_contents_data = *it;
-  return &prerender_contents_data;
-}
-
-PrerenderContents* PrerenderManager::FindEntry(const GURL& url) const {
-  DCHECK(CalledOnValidThread());
-  for (PrerenderContentsDataList::const_iterator it = prerender_list_.begin();
-       it != prerender_list_.end();
-       ++it) {
-    if (it->contents_->MatchesURL(url, NULL))
-      return it->contents_;
-  }
-  // Entry not found.
-  return NULL;
-}
-
-PrerenderManager::PrerenderContentsDataList::iterator
-    PrerenderManager::FindPrerenderContentsForChildRouteIdPair(
-        const std::pair<int, int>& child_route_id_pair) {
-  PrerenderContentsDataList::iterator it = prerender_list_.begin();
-  for (; it != prerender_list_.end(); ++it) {
-    PrerenderContents* prerender_contents = it->contents_;
-
-    int child_id;
-    int route_id;
-    bool has_child_id = prerender_contents->GetChildId(&child_id);
-    bool has_route_id = has_child_id &&
-                        prerender_contents->GetRouteId(&route_id);
-
-    if (has_child_id && has_route_id &&
-        child_id == child_route_id_pair.first &&
-        route_id == child_route_id_pair.second) {
-      break;
+  // Make sure this WeakPtr is still dereferencable. This only needs to be a
+  // DCHECK because the in-scope PrerenderHandle should hold a ref which keeps
+  // this WeakPtr as being valid.
+  DCHECK(*it);
+
+  return *it;
+}
+
+PrerenderManager::PrerenderContentsData*
+PrerenderManager::FindContentsDataMutable(PrerenderHandle handle) {
+  return const_cast<PrerenderManager::PrerenderContentsData*>(

[dominich, 2012/06/18 15:32:44]

Code smell: const_cast?

+      FindContentsData(handle));
+}
+
+PrerenderManager::PrerenderContentsData*
+PrerenderManager::FindContentsDataForChildAndRouteId(const int child_id,
+                                                     const int route_id) {
+  for (PrerenderContentsDataList::iterator
+           it = prerender_list_.begin(), end = prerender_list_.end();
+       it != end; ++it) {
+    if (!*it) continue;

[dominich, 2012/06/18 15:32:44]

nit:
int contents_child_id, contents_route_id;
if (*it && (*it)->contents()->GetChildId(&contents_child_id) &&
        child_id == contents_child_id
        (*it)->contents()->GetRouteId(&contents_route_id) &&
        route_id == contents_route_id) {
    return *it;
}

+    int contents_child_id;
+    if (!(*it)->contents()->GetChildId(&contents_child_id)) continue;
+    int contents_route_id;
+    if (!(*it)->contents()->GetRouteId(&contents_route_id)) continue;
+    if (child_id == contents_child_id && route_id == contents_route_id) {
+      return *it;
     }
   }
-  return it;
-}
-
-PrerenderManager::PrerenderContentsDataList::iterator
-    PrerenderManager::FindPrerenderContentsForURL(const GURL& url) {
-  for (PrerenderContentsDataList::iterator it = prerender_list_.begin();
-       it != prerender_list_.end(); ++it) {
-    if (it->contents_->MatchesURL(url, NULL))
-      return it;
-  }
-  return prerender_list_.end();
+  return NULL;
 }
 
 bool PrerenderManager::DoesRateLimitAllowPrerender() const {
@@ -1128,6 +1264,9 @@ void PrerenderManager::ScheduleDeleteOldTabContents(
 }
 
 void PrerenderManager::AddToHistory(PrerenderContents* contents) {
+  DVLOG(4) << "PrerenderManager::AddToHistory";
+  DVLOG(5) << "... contents = " << contents;
+  DVLOG(5) << "... fs = " << contents->final_status();
   PrerenderHistory::Entry entry(contents->prerender_url(),
                                 contents->final_status(),
                                 contents->origin(),
@@ -1140,9 +1279,9 @@ Value* PrerenderManager::GetActivePrerendersAsValue() const {
   for (PrerenderContentsDataList::const_iterator it = prerender_list_.begin();
        it != prerender_list_.end();
        ++it) {
-    Value* prerender_value = it->contents_->GetAsValue();
-    if (!prerender_value)
-      continue;
+    if (!*it || (*it)->IsStarted()) continue;
+    Value* prerender_value = (*it)->contents()->GetAsValue();
+    if (!prerender_value) continue;
     list_value->Append(prerender_value);
   }
   return list_value;
@@ -1151,9 +1290,15 @@ Value* PrerenderManager::GetActivePrerendersAsValue() const {
 void PrerenderManager::DestroyAllContents(FinalStatus final_status) {
   DeleteOldTabContents();
   while (!prerender_list_.empty()) {
-    PrerenderContentsData data = prerender_list_.front();
+    if (PrerenderContentsData* data = prerender_list_.front()) {
+      if (data->contents()) {
+        DVLOG(4) << "destroy .. contents = " << data->contents();
+        DVLOG(4) << "fs = " << data->contents()->final_status();
+      }
+      if (data->IsStarted())
+        data->contents()->Destroy(final_status);

[dominich, 2012/06/18 15:32:44]

We shouldn't have a valid contents if we haven't started prerendering...

+    }
     prerender_list_.pop_front();
-    data.contents_->Destroy(final_status);
   }
   DeletePendingDeleteEntries();
 }