Make PrerenderHandle an observer of PrerenderContents.

chrome/browser/prerender/prerender_link_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/prerender/prerender_link_manager.h"
 
 #include <limits>
 #include <queue>
 #include <utility>
 
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/prerender/prerender_handle.h"
 #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"
 #include "googleurl/src/gurl.h"
 #include "ui/gfx/size.h"
 
 using content::RenderViewHost;
 using content::SessionStorageNamespace;
 
 namespace prerender {
 
 PrerenderLinkManager::PrerenderLinkManager(PrerenderManager* manager)
     : manager_(manager) {
 }
 
 PrerenderLinkManager::~PrerenderLinkManager() {
   for (IdPairToPrerenderHandleMap::iterator it = ids_to_handle_map_.begin();
        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;
   }
 }
 
 bool PrerenderLinkManager::OnAddPrerender(int child_id,
                                           int prerender_id,
                                           const GURL& url,
                                           const content::Referrer& referrer,
                                           const gfx::Size& size,
                                           int render_view_route_id) {
   DVLOG(2) << "OnAddPrerender, child_id = " << child_id
            << ", prerender_id = " << prerender_id
            << ", url = " << url.spec();
   DVLOG(3) << "... referrer url = " << referrer.url.spec()
            << ", size = (" << size.width() << ", " << size.height() << ")"
            << ", 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;
 }
 
 // TODO(gavinp): Once an observer interface is provided down to the WebKit
 // layer, we should add DCHECK_NE(0L, ids_to_url_map_.count(...)) to both
 // OnCancelPrerender and OnAbandonPrerender. We can't do this now, since
 // the WebKit layer isn't even aware if we didn't add the prerender to the map
 // in OnAddPrerender above.

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

This TODO will no longer work - if a prerender is deleted because it's does
something bad, and then the prerendering page cancels the prerender before it
learns of this, we'll have a count of 0.

Also, "ids_to_url_map_" no longer exists.

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

Gone. I still am thinking about how to keep all these different sets in sync to
avoid leaks. See a FIXME I put in
WebKit/Source/WebKit/chromium/tests/PrerenderingTest.cpp around the redundant
messaging from WebKit, which is part of the problem.

 void PrerenderLinkManager::OnCancelPrerender(int child_id, int prerender_id) {
   DVLOG(2) << "OnCancelPrerender, child_id = " << child_id
            << ", prerender_id = " << prerender_id;
   const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
   IdPairToPrerenderHandleMap::iterator id_to_handle_iter =
       ids_to_handle_map_.find(child_and_prerender_id);
   if (id_to_handle_iter == ids_to_handle_map_.end()) {
     DVLOG(5) << "... canceling a prerender that doesn't exist.";
     return;
   }
   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) {
   DVLOG(2) << "OnAbandonPrerender, child_id = " << child_id
            << ", prerender_id = " << prerender_id;
   const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
   IdPairToPrerenderHandleMap::iterator id_to_handle_iter =
       ids_to_handle_map_.find(child_and_prerender_id);
   if (id_to_handle_iter == ids_to_handle_map_.end())
     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) {
   DVLOG(2) << "OnChannelClosing, child id = " << child_id;
   const ChildAndPrerenderIdPair child_and_minimum_prerender_id(
       child_id, std::numeric_limits<int>::min());
   const ChildAndPrerenderIdPair child_and_maximum_prerender_id(
       child_id, std::numeric_limits<int>::max());
   std::queue<int> prerender_ids_to_abandon;
   for (IdPairToPrerenderHandleMap::iterator
@@ skipping 14 matching lines @@
   return ids_to_handle_map_.empty();
 }
 
 void PrerenderLinkManager::RemovePrerender(
     const IdPairToPrerenderHandleMap::iterator& id_to_handle_iter) {
   PrerenderHandle* prerender_handle = id_to_handle_iter->second;
   delete prerender_handle;
   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