Change multi-prerender API to include per launcher slots.

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 da68f892f2c793cbf602a75554e4404cdc2f715b..254fd58d415d85e9c2c985a4c6afe69c2e97bdf1 100644
--- a/chrome/browser/prerender/prerender_link_manager.cc
+++ b/chrome/browser/prerender/prerender_link_manager.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/prerender/prerender_link_manager.h"
 
 #include <limits>
+#include <set>
 #include <utility>
 
 #include "base/memory/scoped_ptr.h"
@@ -41,131 +42,226 @@ void Send(int child_id, IPC::Message* raw_message) {
 namespace prerender {
 
 PrerenderLinkManager::PrerenderLinkManager(PrerenderManager* manager)
-    : manager_(manager) {
+    : is_shutdown_(false),
+      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;
-    DCHECK(!prerender_handle->IsPrerendering())
-        << "All running prerenders should stop at the same time as the "
-        << "PrerenderManager.";
-    delete prerender_handle;
+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (i->handle) {
+      DCHECK(!i->handle->IsPrerendering())
+          << "All running prerenders should stop at the same time as the "
+          << "PrerenderManager.";
+      delete i->handle;
+      i->handle = 0;
+    }
   }
 }
 
-bool PrerenderLinkManager::OnAddPrerender(int child_id,
+void PrerenderLinkManager::OnAddPrerender(int launcher_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;
-
-
-  PrerenderHandle* prerender_handle =
-      manager_->AddPrerenderFromLinkRelPrerender(
-          child_id, render_view_route_id, url, referrer, size);
-  if (!prerender_handle)
-    return false;
-
-  const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
-  DCHECK_EQ(0u, ids_to_handle_map_.count(child_and_prerender_id));
-  ids_to_handle_map_[child_and_prerender_id] = prerender_handle;
-
-  // 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->SetObserver(this);
-  return true;
+  Prerender prerender(launcher_child_id, prerender_id, url, referrer, size,
+                      render_view_route_id);
+  prerenders_.push_back(prerender);
+  StartPrerenders();
 }
 
 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.";
+  Prerender* prerender = FindByLauncherChildIdAndPrerenderId(child_id,
+                                                             prerender_id);
+  if (!prerender)
     return;
-  }
 
-  scoped_ptr<PrerenderHandle> prerender_handle(id_to_handle_iter->second);
-  ids_to_handle_map_.erase(id_to_handle_iter);
-  prerender_handle->OnCancel();
+  scoped_ptr<PrerenderHandle> own_prerender_handle(prerender->handle);
+  prerender->handle = NULL;
+  RemovePrerender(prerender);

[mmenke, 2012/12/18 17:15:15]

Is the goal of all this not to send stop events for prerenders cancelled by the
other process, or to make sure we delete prerenders that haven't yet been
started?

Think it's worth a comment.

[gavinp, 2012/12/18 20:15:12]

Done.

 
-  // Because OnCancel() can remove the prerender from the map, we need to
-  // consider our iterator invalid.
-  id_to_handle_iter = ids_to_handle_map_.find(child_and_prerender_id);
-  if (id_to_handle_iter != ids_to_handle_map_.end())
-    RemovePrerender(id_to_handle_iter);
-}
+  if (own_prerender_handle)
+    own_prerender_handle->OnCancel();

[mmenke, 2012/12/18 17:15:15]

Just a general comment:  Think it's rather unfortunate that in a couple of these
functions we have to deal with 3 or 4 cases separately:

1)  It doesn't exist due to an expected race (Or because of a deliberate design
decision).
2)  We haven't started prerendering.
3)  We have started prerendering, but it's a pending prerender.
4)  It has started, and has a WebContents.

Suppose we can get rid of 3) in a future CL by handling all the pending
prerender logic here, though it may be a little polling heavy (Check if the
requester is prerendering before starting a prerender.  If it is, don't start. 
We'll also need a callback from the PrerenderManager when a prerender is
displayed).

We need to make sure we testing all the cases for all these functions.

[gavinp, 2012/12/18 20:15:12]

Yes, it is unfortunate. Pulling things into the LinkManager will simplify a lot
of these; the special cases though are already mirrored through PrerenderManager
and other places. I wish Prerendering could be made a lot less special case
heavy.

+
+  StartPrerenders();
+ }

[mmenke, 2012/12/18 17:15:15]

nit:  Remove indent.

[gavinp, 2012/12/18 20:15:12]

Done.

 
 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())
+  Prerender* prerender = FindByLauncherChildIdAndPrerenderId(child_id,
+                                                             prerender_id);
+  if (!prerender || !prerender->handle)

[mmenke, 2012/12/18 17:15:15]

If !prerender->handle, we should still be removing the prerender, right?  Do we
have a unit test that covers this?

[gavinp, 2012/12/18 20:15:12]

Added  a few more tests walking us through these cases, and confirmed these
lines are key to them passing.

     return;
-  PrerenderHandle* prerender_handle = id_to_handle_iter->second;
-  prerender_handle->OnNavigateAway();
+  prerender->handle->OnNavigateAway();
+
+  // If the prerender is not running, remove it from the list so it does not
+  // leak. If it is running, it will send a cancel event when it stops which
+  // will remove it.
+  if (!prerender->handle || prerender->handle->IsPrerendering())

[mmenke, 2012/12/18 17:15:15]

Why check prernder->handle here?  If OnNavigateAway clears it, then it will have
presumably deleted the prerender as well, and we'll have more serious issues...

[gavinp, 2012/12/18 20:15:12]

Done.

+    RemovePrerender(prerender);
 }
 
 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());
-
-  IdPairToPrerenderHandleMap::iterator
-      it = ids_to_handle_map_.lower_bound(child_and_minimum_prerender_id);
-  IdPairToPrerenderHandleMap::iterator
-      end = ids_to_handle_map_.upper_bound(child_and_maximum_prerender_id);
-  while (it != end) {
-    IdPairToPrerenderHandleMap::iterator next = it;
+  std::list<Prerender>::iterator next = prerenders_.begin();
+  while (next != prerenders_.end()) {
+    std::list<Prerender>::iterator it = next;
     ++next;
 
-    size_t size_before_abandon = ids_to_handle_map_.size();
-    OnAbandonPrerender(child_id, it->first.second);
-    DCHECK_EQ(size_before_abandon, ids_to_handle_map_.size());
-    RemovePrerender(it);
+    if (child_id != it->launcher_child_id)
+      continue;
 
-    it = next;
+    const size_t running_prerender_count = CountRunningPrerenders();
+    OnAbandonPrerender(child_id, it->prerender_id);
+    DCHECK_EQ(running_prerender_count, CountRunningPrerenders());
   }
 }
 
+PrerenderLinkManager::Prerender::Prerender(int launcher_child_id,
+                                           int prerender_id,
+                                           const GURL& url,
+                                           const content::Referrer& referrer,
+                                           const gfx::Size& size,
+                                           int render_view_route_id)
+    : launcher_child_id(launcher_child_id),
+      prerender_id(prerender_id),
+      url(url),
+      referrer(referrer),
+      size(size),
+      render_view_route_id(render_view_route_id),
+      handle(NULL) {
+}
+
+PrerenderLinkManager::Prerender::~Prerender() {
+  DCHECK_EQ(static_cast<PrerenderHandle*>(NULL), handle);
+}
+
 bool PrerenderLinkManager::IsEmpty() const {
-  return ids_to_handle_map_.empty();
+  return prerenders_.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);
+size_t PrerenderLinkManager::CountRunningPrerenders() const {
+  size_t retval = 0;
+  for (std::list<Prerender>::const_iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (i->handle && i->handle->IsPrerendering())
+      ++retval;
+  }
+  return retval;
 }
 
-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;
+void PrerenderLinkManager::StartPrerenders() {
+  if (is_shutdown_)
+    return;
+
+  std::set<std::pair<int, int> > launcher_and_prerender_id_set;
+
+  size_t running_count = 0;
+  std::multiset<std::pair<int, int> >
+      running_launcher_and_render_view_route_set;
+
+  // Scan the list, counting how many prerenders are running and how many are
+  // running for each launcher.
+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (i->handle) {
+      ++running_count;
+      std::pair<int, int> launcher_and_render_view_route(
+          i->launcher_child_id, i->render_view_route_id);
+      running_launcher_and_render_view_route_set.insert(
+          launcher_and_render_view_route);
+      DCHECK_GE(manager_->config().max_concurrency_per_launcher,
+                running_launcher_and_render_view_route_set.count(
+                    launcher_and_render_view_route));
+    }
+
+    std::pair<int, int> launcher_and_prerender_id(i->launcher_child_id,
+                                                  i->prerender_id);
+    DCHECK_EQ(0u,
+              launcher_and_prerender_id_set.count(launcher_and_prerender_id));
+    launcher_and_prerender_id_set.insert(launcher_and_prerender_id);
   }
-  return ids_to_handle_map_.end();
+  DCHECK_GE(manager_->config().max_concurrency, running_count);
+
+
+  // Scan the list again, starting prerenders as our counts allow.
+  std::list<Prerender>::iterator next = prerenders_.begin();
+  while (next != prerenders_.end()) {
+    std::list<Prerender>::iterator i = next;
+    ++next;

[mmenke, 2012/12/18 17:15:15]

Maybe a DCHECK_EQ(CountRunningPrerenders(), running_count);?

Could also (Or alternatively) put one just before this loop.

[gavinp, 2012/12/18 20:15:12]

I think it'd DCHECK_LE, right? Some prerenders have handles but not all are
running. I'll change names to make this less confusing.

[mmenke, 2012/12/28 18:25:33]

Correct - I made an incorrect assumption about what CountRunningPrerenders()
did, based on the naming.  Changing the names seems like a good idea.  :)

+
+    if (running_count >= manager_->config().max_concurrency ||
+        running_count >= prerenders_.size()) {
+      return;
+    }
+
+    if (i->handle)
+      continue;
+
+    std::pair<int, int> launcher_and_render_view_route(
+        i->launcher_child_id, i->render_view_route_id);
+
+    if (manager_->config().max_concurrency_per_launcher <=
+        running_launcher_and_render_view_route_set.count(
+            launcher_and_render_view_route)) {
+      continue;
+    }
+
+    PrerenderHandle* handle = manager_->AddPrerenderFromLinkRelPrerender(
+        i->launcher_child_id, i->render_view_route_id,
+        i->url, i->referrer, i->size);
+    if (!handle) {
+      prerenders_.erase(i);
+      continue;
+    }
+    i->handle = handle;
+    ++running_count;
+    handle->SetObserver(this);
+    if (handle->IsPrerendering())
+      OnPrerenderStart(handle);
+
+    running_launcher_and_render_view_route_set.insert(
+        launcher_and_render_view_route);
+  }
+}
+
+PrerenderLinkManager::Prerender*
+PrerenderLinkManager::FindByLauncherChildIdAndPrerenderId(int launcher_child_id,
+                                                          int prerender_id) {
+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (launcher_child_id == i->launcher_child_id &&
+        prerender_id == i->prerender_id) {
+      return &(*i);
+    }
+  }
+  return NULL;
+}
+
+PrerenderLinkManager::Prerender*
+PrerenderLinkManager::FindByPrerenderHandle(PrerenderHandle* prerender_handle) {

[mmenke, 2012/12/18 17:15:15]

DCHECK(prerender_handle);

[gavinp, 2012/12/18 20:15:12]

Done.

+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (prerender_handle == i->handle)
+      return &(*i);
+  }
+  return NULL;
+}
+
+void PrerenderLinkManager::RemovePrerender(Prerender* prerender) {
+  for (std::list<Prerender>::iterator i = prerenders_.begin();
+       i != prerenders_.end(); ++i) {
+    if (&(*i) == prerender) {
+      scoped_ptr<PrerenderHandle> own_handle(i->handle);
+      i->handle = NULL;
+      prerenders_.erase(i);
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
+void PrerenderLinkManager::Shutdown() {
+  is_shutdown_ = true;
 }
 
 // In practice, this is always called from either
@@ -173,39 +269,34 @@ PrerenderLinkManager::FindPrerenderHandle(
 // prerender case, from PrerenderHandle::AdoptPrerenderDataFrom.
 void PrerenderLinkManager::OnPrerenderStart(
     PrerenderHandle* prerender_handle) {
-  IdPairToPrerenderHandleMap::iterator it =
-      FindPrerenderHandle(prerender_handle);
-  DCHECK(it != ids_to_handle_map_.end());
-  const int child_id = it->first.first;
-  const int prerender_id = it->first.second;
-
-  Send(child_id, new PrerenderMsg_OnPrerenderStart(prerender_id));
+  Prerender* prerender = FindByPrerenderHandle(prerender_handle);
+  if (!prerender)
+    return;
+  Send(prerender->launcher_child_id,
+       new PrerenderMsg_OnPrerenderStart(prerender->prerender_id));
 }
 
 void PrerenderLinkManager::OnPrerenderAddAlias(
     PrerenderHandle* prerender_handle,
     const GURL& alias_url) {
-  IdPairToPrerenderHandleMap::iterator it =
-      FindPrerenderHandle(prerender_handle);
-  if (it == ids_to_handle_map_.end())
+  Prerender* prerender = FindByPrerenderHandle(prerender_handle);
+  if (!prerender)
     return;
-  const int child_id = it->first.first;
-  const int prerender_id = it->first.second;
-
-  Send(child_id, new PrerenderMsg_OnPrerenderAddAlias(prerender_id, alias_url));
+  Send(prerender->launcher_child_id,
+       new PrerenderMsg_OnPrerenderAddAlias(prerender->prerender_id,
+                                            alias_url));
 }
 
 void PrerenderLinkManager::OnPrerenderStop(
     PrerenderHandle* prerender_handle) {
-  IdPairToPrerenderHandleMap::iterator it =
-      FindPrerenderHandle(prerender_handle);
-  if (it == ids_to_handle_map_.end())
+  Prerender* prerender = FindByPrerenderHandle(prerender_handle);
+  if (!prerender)
     return;
-  const int child_id = it->first.first;
-  const int prerender_id = it->first.second;
 
-  Send(child_id, new PrerenderMsg_OnPrerenderStop(prerender_id));
-  RemovePrerender(it);
+  Send(prerender->launcher_child_id,
+       new PrerenderMsg_OnPrerenderStop(prerender->prerender_id));
+  RemovePrerender(prerender);
+  StartPrerenders();
 }
 
 }  // namespace prerender