Change multi-prerender API to include per launcher slots.

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 <set>
 #include <utility>
 
 #include "base/memory/scoped_ptr.h"
 #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"
@@ skipping 16 matching lines @@
   if (!render_process_host)
     return;
   render_process_host->Send(own_message.release());
 }
 
 }  // namespace
 
 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;
-  }
-}
-
-bool PrerenderLinkManager::OnAddPrerender(int child_id,
+  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;
+    }
+  }
+}
+
+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();
-
-  // 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);
-}
+
+  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.

+
+  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);
-
-    it = next;
-  }
+    if (child_id != it->launcher_child_id)
+      continue;
+
+    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();
-}
-
-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;
-  }
-  return ids_to_handle_map_.end();
+  return prerenders_.empty();
+}
+
+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;
+}
+
+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);
+  }
+  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
 // PrerenderLinkManager::OnAddPrerender in the regular case, or in the pending
 // 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