Make PrerenderHandle an observer of PrerenderContents.

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 192f2c60af67c908476ead25f5bc850c81d0a215..d027720377140f2d94c0cd2100e22292438b7c6d 100644
--- a/chrome/browser/prerender/prerender_manager.cc
+++ b/chrome/browser/prerender/prerender_manager.cc
@@ -223,7 +223,6 @@ PrerenderManager::~PrerenderManager() {
   // The earlier call to ProfileKeyedService::Shutdown() should have emptied
   // these vectors already.
   DCHECK(active_prerenders_.empty());
-  DCHECK(pending_prerenders_.empty());
   DCHECK(to_delete_prerenders_.empty());
 }
 
@@ -238,7 +237,6 @@ void PrerenderManager::Shutdown() {
   profile_ = NULL;
 
   DCHECK(active_prerenders_.empty());
-  pending_prerenders_.clear();
 }
 
 PrerenderHandle* PrerenderManager::AddPrerenderFromLinkRelPrerender(
@@ -278,11 +276,8 @@ PrerenderHandle* PrerenderManager::AddPrerenderFromLinkRelPrerender(
     // Instead of prerendering from inside of a running prerender, we will defer
     // this request until its launcher is made visible.
     if (PrerenderContents* contents = parent_prerender_data->contents()) {
-      pending_prerenders_.push_back(new PrerenderData(this));
       PrerenderHandle* prerender_handle =
-          new PrerenderHandle(pending_prerenders_.back());
-      DCHECK(prerender_handle->IsPending());
-
+          new PrerenderHandle(static_cast<PrerenderData*>(NULL));
       scoped_ptr<PrerenderContents::PendingPrerenderInfo>
           pending_prerender_info(new PrerenderContents::PendingPrerenderInfo(
               prerender_handle->weak_ptr_factory_.GetWeakPtr(),
@@ -855,10 +850,6 @@ struct PrerenderManager::PrerenderData::OrderByExpiryTime {
   }
 };
 
-PrerenderManager::PrerenderData::PrerenderData(PrerenderManager* manager)
-    : manager_(manager), contents_(NULL), handle_count_(0) {
-}
-
 PrerenderManager::PrerenderData::PrerenderData(PrerenderManager* manager,
                                                PrerenderContents* contents,
                                                base::TimeTicks expiry_time)
@@ -881,38 +872,30 @@ void PrerenderManager::PrerenderData::MakeIntoMatchCompleteReplacement() {
   manager_->to_delete_prerenders_.push_back(to_delete);
 }
 
-void PrerenderManager::PrerenderData::OnNewHandle() {
-  DCHECK(contents_ || handle_count_ == 0) <<
-      "Cannot create multiple handles to a pending prerender.";
+void PrerenderManager::PrerenderData::OnNewHandle(PrerenderHandle* handle) {
   ++handle_count_;
+  if (contents_)

[mmenke, 2012/12/06 22:22:15]

contents_ can no longer be null.

[gavinp, 2012/12/10 17:55:10]

Done.

+    contents_->AddObserver(handle);
 }
 
-void PrerenderManager::PrerenderData::OnNavigateAwayByHandle() {
-  if (!contents_) {
-    DCHECK_EQ(1, handle_count_);
-    // Pending prerenders are not maintained in the active_prerenders_, so they
-    // will not get normal expiry. Since this prerender hasn't even been
-    // launched yet, and it's held by a page that is being prerendered, we will
-    // just delete it.
-    manager_->DestroyPendingPrerenderData(this);
-  } else {
-    DCHECK_LE(0, handle_count_);
-    // We intentionally don't decrement the handle count here, so that the
-    // prerender won't be canceled until it times out.
-    manager_->SourceNavigatedAway(this);
-  }
+void PrerenderManager::PrerenderData::OnNavigateAwayByHandle(
+    PrerenderHandle* handle) {
+  DCHECK_LT(0, handle_count_);
+  // We intentionally don't decrement the handle count here, so that the
+  // prerender won't be canceled until it times out.
+  manager_->SourceNavigatedAway(this);
 }
 
-void PrerenderManager::PrerenderData::OnCancelByHandle() {
+void PrerenderManager::PrerenderData::OnCancelByHandle(
+    PrerenderHandle* handle) {

[mmenke, 2012/12/06 22:22:15]

Why do we need the handle here?  Just for consistency?

[gavinp, 2012/12/10 17:55:10]

Just for consistency.

   DCHECK_LE(1, handle_count_);
-  DCHECK(contents_ || handle_count_ == 1);
+  DCHECK(contents_ || handle_count_ == 1)
+      << "Pending prerenders should never have multiple prerenders";
 
   if (--handle_count_ == 0) {
     if (contents_) {

[mmenke, 2012/12/06 22:22:15]

contents_ can't be null.

[gavinp, 2012/12/10 17:55:10]

Done.

       // This will eventually remove this object from active_prerenders_.
       contents_->Destroy(FINAL_STATUS_CANCELLED);
-    } else {
-      manager_->DestroyPendingPrerenderData(this);
     }
   }
 }
@@ -937,31 +920,26 @@ void PrerenderManager::StartPendingPrerenders(
     PrerenderContents::PendingPrerenderInfo* info = *it;
     PrerenderHandle* existing_prerender_handle =
         info->weak_prerender_handle.get();
-    if (!existing_prerender_handle || !existing_prerender_handle->IsValid())
+    if (!existing_prerender_handle)
+      continue;
+    if (existing_prerender_handle->has_been_canceled())
       continue;
 
-    DCHECK(existing_prerender_handle->IsPending());
+    DCHECK(!existing_prerender_handle->IsValid());
     DCHECK(process_id == -1 || session_storage_namespace);
 
-    scoped_ptr<PrerenderHandle> swap_prerender_handle(AddPrerender(
+    scoped_ptr<PrerenderHandle> new_prerender_handle(AddPrerender(
         info->origin, process_id,
         info->url, info->referrer, info->size,
         session_storage_namespace));

[mmenke, 2012/12/07 16:46:08]

optional (Or feel free to think about in a later CL):  May make life simpler to
have AddPrerender just return a PrerenderContents*, and have the calling
function wrap it into the PrerenderHandle.

Think creating a handle only to destroy it makes things overly complicated here,
and creating the handle in a couple places isn't too terribly ugly.

[gavinp, 2012/12/10 17:55:10]

Definitely a good idea, and I'm definitely punting to a later CL.

-    if (swap_prerender_handle.get()) {
+    if (new_prerender_handle.get()) {

[mmenke, 2012/12/07 16:46:08]

nit:  Can remove the .get() while you're here.

[gavinp, 2012/12/10 17:55:10]

enne is Awesome! Done.

       // AddPrerender has returned a new prerender handle to us. We want to make
-      // |existing_prerender_handle| active, so swap the underlying
-      // PrerenderData between the two handles, and delete our old handle (which
-      // will release our entry in the pending_prerender_list_).
-      existing_prerender_handle->SwapPrerenderDataWith(
-          swap_prerender_handle.get());
-      swap_prerender_handle->OnCancel();
+      // |existing_prerender_handle| active, so move the underlying
+      // PrerenderData to our new handle.
+      existing_prerender_handle->AdoptPrerenderDataFrom(
+          new_prerender_handle.get());
       continue;
     }

[mmenke, 2012/12/07 16:46:08]

Random comment for the future - if a prerender is cancelled without really being
started...Should there be some notification of that?

[gavinp, 2012/12/13 13:38:03]

I'm going to go with no. The launcher has no promises about which ones will be
started...

-
-    // We could not start our Prerender. Canceling the existing handle will make
-    // it return false for PrerenderHandle::IsPending(), and will release the
-    // PrerenderData from pending_prerender_list_.
-    existing_prerender_handle->OnCancel();
   }
 }
 
@@ -980,16 +958,6 @@ void PrerenderManager::SourceNavigatedAway(PrerenderData* prerender_data) {
   SortActivePrerenders();
 }
 
-void PrerenderManager::DestroyPendingPrerenderData(
-    PrerenderData* pending_prerender_data) {
-  ScopedVector<PrerenderData>::iterator it =
-      std::find(pending_prerenders_.begin(), pending_prerenders_.end(),
-                pending_prerender_data);
-  if (it == pending_prerenders_.end())
-    return;
-  pending_prerenders_.erase(it);
-}
-
 // private
 PrerenderHandle* PrerenderManager::AddPrerender(
     Origin origin,