Make PrerenderHandle an observer of PrerenderContents.

chrome/browser/prerender/prerender_link_manager.cc

Index: chrome/browser/prerender/prerender_link_manager.cc
diff --git a/chrome/browser/prerender/prerender_link_manager.cc b/chrome/browser/prerender/prerender_link_manager.cc
index ea559977e272d5c0eeace49991130a88ad22457d..8304407869141a091e9adfbfc9f75c47d5e60812 100644
--- a/chrome/browser/prerender/prerender_link_manager.cc
+++ b/chrome/browser/prerender/prerender_link_manager.cc
@@ -13,6 +13,8 @@
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/prerender/prerender_manager_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/prerender_messages.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/session_storage_namespace.h"
 #include "content/public/common/referrer.h"
@@ -33,7 +35,7 @@ PrerenderLinkManager::~PrerenderLinkManager() {
        it != ids_to_handle_map_.end();
        ++it) {
     PrerenderHandle* prerender_handle = it->second;
-    prerender_handle->OnCancel();
+    DCHECK(!prerender_handle->IsValid());

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

Think this is worth a comment, along the lines "All prerenders handles that were
started should have been destroyed when the PrerenderManager was destroyed."

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

Done.

     delete prerender_handle;
   }
 }
@@ -52,18 +54,20 @@ bool PrerenderLinkManager::OnAddPrerender(int child_id,
            << ", render_view_route_id = " << render_view_route_id;
 
   const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
-  DCHECK_EQ(0U, ids_to_handle_map_.count(child_and_prerender_id));
+  DCHECK_EQ(0u, ids_to_handle_map_.count(child_and_prerender_id));
 
-  scoped_ptr<PrerenderHandle> prerender_handle(
-      manager_->AddPrerenderFromLinkRelPrerender(
-          child_id, render_view_route_id, url, referrer, size));
-  if (prerender_handle.get()) {
+  if (PrerenderHandle* prerender_handle = manager_->
+          AddPrerenderFromLinkRelPrerender(child_id, render_view_route_id,
+                                           url, referrer, size)) {

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

I don't think we should be putting variable declarations in if statements.

I've thought about this a bit, and the fact is that while it does in fact obey
the scoping rule, it's not the prevailing style, and it's used rarely enough
that I think it decreases readability for the average Chromian (Chromus? 
Chromeite?  Chromizan?  Chromizone?).

That goes for the rest of this file as well.

     std::pair<IdPairToPrerenderHandleMap::iterator, bool> insert_result =
-        ids_to_handle_map_.insert(std::make_pair(
-            child_and_prerender_id, static_cast<PrerenderHandle*>(NULL)));
+        ids_to_handle_map_.insert(std::make_pair(child_and_prerender_id,
+                                                 prerender_handle));
     DCHECK(insert_result.second);

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

I think this would be much cleaner if we just did:

DCHECK_EQ(0u, ids_to_handle_map_.count(child_and_prerender_id));
ids_to_handle_map_[child_and_prerender_id] = prerender_handle;

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

Done, and I moved around some other code to avoid double DCHECKs.

-    delete insert_result.first->second;
-    insert_result.first->second = prerender_handle.release();
+    // If we are given a prerender that is already prerendering, we have missed
+    // the start event.
+    if (prerender_handle->IsPrerendering())
+      OnPrerenderStart(prerender_handle);
+    prerender_handle->AddObserver(this);
     return true;
   }
   return false;
@@ -86,7 +90,6 @@ void PrerenderLinkManager::OnCancelPrerender(int child_id, int prerender_id) {
   }
   PrerenderHandle* prerender_handle = id_to_handle_iter->second;
   prerender_handle->OnCancel();
-  RemovePrerender(id_to_handle_iter);
 }
 
 void PrerenderLinkManager::OnAbandonPrerender(int child_id, int prerender_id) {
@@ -99,7 +102,6 @@ void PrerenderLinkManager::OnAbandonPrerender(int child_id, int prerender_id) {
     return;
   PrerenderHandle* prerender_handle = id_to_handle_iter->second;
   prerender_handle->OnNavigateAway();

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

Hmm...we can now cancel and then navigate aware the same prerender handle... 
This seems kinda strange.  Since all handles for a prerender have the same
SiteInstance, I think this is fine, but it's kinda unexpected.

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

The reason I thought to keep them in was so that events could keep travelling
down to Prerenders after these notices came up. But the underlying cause of
Cancel is that there's no element in the document (which I _guess_ could catch
events, but come on...), and Abandon means there isn't even a document anymore.

So, sure, let's remove here. Addresses the concerns you raised below, too.

-  RemovePrerender(id_to_handle_iter);
 }
 
 void PrerenderLinkManager::OnChannelClosing(int child_id) {
@@ -134,4 +136,59 @@ void PrerenderLinkManager::RemovePrerender(
   ids_to_handle_map_.erase(id_to_handle_iter);
 }
 
+PrerenderLinkManager::IdPairToPrerenderHandleMap::iterator
+PrerenderLinkManager::FindPrerenderHandle(
+    PrerenderHandle* prerender_handle) {
+  for (IdPairToPrerenderHandleMap::iterator it = ids_to_handle_map_.begin();
+       it != ids_to_handle_map_.end(); ++it) {
+    if (it->second == prerender_handle)
+      return it;
+  }
+  NOTREACHED();

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

While this is true for all the places that this function is called, I don't
think this behavior is implied by the function name, and could easily see that
resulting in bugs.

I'd suggest removing the NOTREACHED, and adding a:
DCHECK_NE(ids_to_handle_map_.end(), FindPrerenderHandle(prerender_handle));

Just before each place it's called.  I'm open to other ideas.  I could live with
a comment in the function definition, but I'd prefer a code change to a
documentation change.

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

Done. I like simple names, and I like names that correspond to functionality.

+  return ids_to_handle_map_.end();
+}
+
+void PrerenderLinkManager::OnPrerenderStart(
+    PrerenderHandle* prerender_handle) {
+  ChildAndPrerenderIdPair child_and_prerender_id_pair =
+      FindPrerenderHandle(prerender_handle)->first;
+  const int child_id = child_and_prerender_id_pair.first;
+  const int prerender_id = child_and_prerender_id_pair.second;
+
+  if (content::RenderProcessHost* render_process_host =
+              content::RenderProcessHost::FromID(child_id)) {
+    render_process_host->Send(new PrerenderMsg_OnPrerenderStart(prerender_id));
+  }

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

Think it would be a little cleaner to have a function in a private namespace: 
Send(child_id, IPC::Message* msg); and just use it in these 3 methods.  Doesn't
savea huge amount of code, but increases readability.

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

Done.

+}
+
+void PrerenderLinkManager::OnPrerenderAddAlias(
+    PrerenderHandle* prerender_handle,
+    const GURL& alias_url) {
+  ChildAndPrerenderIdPair child_and_prerender_id_pair =
+      FindPrerenderHandle(prerender_handle)->first;
+  const int child_id = child_and_prerender_id_pair.first;
+  const int prerender_id = child_and_prerender_id_pair.second;
+
+  if (content::RenderProcessHost* render_process_host =
+              content::RenderProcessHost::FromID(child_id)) {
+    render_process_host->Send(new PrerenderMsg_OnPrerenderAddAlias(prerender_id,
+                                                                   alias_url));
+  }
+}
+
+void PrerenderLinkManager::OnPrerenderStop(
+    PrerenderHandle* prerender_handle) {
+  IdPairToPrerenderHandleMap::iterator
+      it = FindPrerenderHandle(prerender_handle);
+  ChildAndPrerenderIdPair child_and_prerender_id_pair = it->first;
+  const int child_id = child_and_prerender_id_pair.first;
+  const int prerender_id = child_and_prerender_id_pair.second;
+
+  if (content::RenderProcessHost* render_process_host =
+              content::RenderProcessHost::FromID(child_id)) {
+    render_process_host->Send(new PrerenderMsg_OnPrerenderStop(prerender_id));
+  }
+  RemovePrerender(it);
+}
+
 }  // namespace prerender