Add pending preloads indexed by routing id. Start preloading once we navigate.

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 95d3ba1a5975bbccf34672d10e7f228386219766..d65c9f7e265e2debff01c40f86b7c7b24d6f5908 100644
--- a/chrome/browser/prerender/prerender_manager.cc
+++ b/chrome/browser/prerender/prerender_manager.cc
@@ -60,6 +60,17 @@ struct PrerenderManager::PrerenderContentsData {
   }
 };
 
+struct PrerenderManager::PendingContentsData {
+  PendingContentsData(const GURL& url, const std::vector<GURL>& alias_urls,
+                      const GURL& referrer)
+      : url_(url), alias_urls_(alias_urls), referrer_(referrer) { }
+  ~PendingContentsData() {}
+  GURL url_;
+  std::vector<GURL> alias_urls_;
+  GURL referrer_;
+};
+
+
 PrerenderManager::PrerenderManager(Profile* profile)
     : profile_(profile),
       max_prerender_age_(base::TimeDelta::FromSeconds(
@@ -117,6 +128,53 @@ bool PrerenderManager::AddPreload(const GURL& url,
   return true;
 }
 
+void PrerenderManager::AddPendingPreload(
+    const std::pair<int,int>& child_route_id_pair,
+    const GURL& url,
+    const std::vector<GURL>& alias_urls,
+    const GURL& referrer) {
+  // Check if this is coming from a valid prerender rvh.
+
+  bool is_valid_prerender = false;
+  for (std::list<PrerenderContentsData>::iterator it = prerender_list_.begin();
+       it != prerender_list_.end(); ++it) {
+    PrerenderContents* pc = it->contents_;
+
+    int child_id;
+    int route_id;
+    bool has_child_id = pc->child_id(&child_id);

[cbentzel, 2011/03/21 19:46:04]

Probably not important, but you could do

if (pc->child_id(&child_id) && pc->route_id(&route_id) && child_id == ...)

to short-circuit the route_id call when child_id fails.

[dominich, 2011/03/21 20:25:51]

Short-circuited, but slightly differently to retain what I think is the more
readable conditional statement.

+    bool has_route_id = pc->route_id(&route_id);
+
+    if (has_child_id && has_route_id &&
+        child_id == child_route_id_pair.first &&
+        route_id == child_route_id_pair.second) {
+      is_valid_prerender = true;
+      break;
+    }
+  }
+
+  // If not, we could check to see if the RenderViewHost specified by the
+  // child_route_id_pair exists and if so just start prerendering, as this
+  // suggests that the link was clicked, though this might prerender something
+  // that the user has already navigated away from. For now, we'll be
+  // conservative and skip the prerender which will mean some prerender requests
+  // from prerendered pages will be missed if the user navigates quickly.
+  if (!is_valid_prerender) {
+    RecordFinalStatus(FINAL_STATUS_PENDING_SKIPPED);
+    return;
+  }
+
+  PendingPrerenderList::iterator it =
+      pending_prerender_list_.find(child_route_id_pair);
+  if (it == pending_prerender_list_.end()) {
+    PendingPrerenderList::value_type el = std::make_pair(child_route_id_pair,
+                                            std::vector<PendingContentsData>());
+    it = pending_prerender_list_.insert(el).first;
+  }
+
+  it->second.push_back(PendingContentsData(url, alias_urls, referrer));
+}
+
 void PrerenderManager::DeleteOldEntries() {
   while (!prerender_list_.empty()) {
     PrerenderContentsData data = prerender_list_.front();
@@ -164,6 +222,11 @@ bool PrerenderManager::MaybeUsePreloadedPage(TabContents* tc, const GURL& url) {
     RecordTimeUntilUsed(base::TimeTicks::Now() - pc->load_start_time());
   pc->set_final_status(FINAL_STATUS_USED);
 
+  int child_id;
+  int route_id;
+  CHECK(pc->child_id(&child_id));

[cbentzel, 2011/03/21 19:46:04]

CHECK is probably OK here, although I'd hate to crash in a release if we are
wrong. 

Maybe DCHECK and have child_id, route_id assign an invalid_number in the false
case?

[dominich, 2011/03/21 20:25:51]

I considered that, but I wasn't sure what an invalid child or route id would be.
-2 is reserved for 'we don't have one yet' and kint32max is reserved for general
messages. There isn't an INVALID routing ID that I can find.

I could assume -1, but I'm not comfortable with that. I could also introduce -1
as invalid.

Thanks to the prerendering_has_started() check above we know we have a valid
RenderViewHost at this point. If you prefer, I can replace the CHECK with a
DCHECK on the returned booleans?

[mmenke, 2011/03/21 21:18:57]

I've seen -1 used to check for invalid elsewhere, though I can't recall the
context.  I believe it was in either RenderViewHost or a different
ResourceHandler.

+  CHECK(pc->route_id(&route_id));
+
   RenderViewHost* rvh = pc->render_view_host();
   // RenderViewHosts in PrerenderContents start out hidden.
   // Since we are actually using it now, restore it.
@@ -173,6 +236,21 @@ bool PrerenderManager::MaybeUsePreloadedPage(TabContents* tc, const GURL& url) {
   tc->SwapInRenderViewHost(rvh);
   MarkTabContentsAsPrerendered(tc);
 
+  // See if we have any pending prerender requests for this routing id and start
+  // the preload if we do.
+  std::pair<int, int> child_route_pair = std::make_pair(child_id, route_id);
+  PendingPrerenderList::iterator pending_it =
+      pending_prerender_list_.find(child_route_pair);
+  if (pending_it != pending_prerender_list_.end()) {
+    for (std::vector<PendingContentsData>::iterator content_it =
+            pending_it->second.begin();
+         content_it != pending_it->second.end(); ++content_it) {
+      AddPreload(content_it->url_, content_it->alias_urls_,
+                 content_it->referrer_);
+    }
+    pending_prerender_list_.erase(pending_it);
+  }
+
   ViewHostMsg_FrameNavigate_Params* p = pc->navigate_params();
   if (p != NULL)
     tc->DidNavigate(rvh, *p);
@@ -197,6 +275,7 @@ void PrerenderManager::RemoveEntry(PrerenderContents* entry) {
        it != prerender_list_.end();
        ++it) {
     if (it->contents_ == entry) {
+      RemovePendingPreload(entry);
       prerender_list_.erase(it);
       break;
     }
@@ -280,6 +359,24 @@ PrerenderContents* PrerenderManager::FindEntry(const GURL& url) {
   return NULL;
 }
 
+PrerenderManager::PendingContentsData*
+    PrerenderManager::FindPendingEntry(const GURL& url) {
+  for (PendingPrerenderList::iterator map_it = pending_prerender_list_.begin();
+       map_it != pending_prerender_list_.end();
+       ++map_it) {
+    for (std::vector<PendingContentsData>::iterator content_it =
+            map_it->second.begin();
+         content_it != map_it->second.end();
+         ++content_it) {
+      if (content_it->url_ == url) {
+        return &(*content_it);
+      }
+    }
+  }
+
+  return NULL;
+}
+
 // static
 void PrerenderManager::RecordPrefetchTagObserved() {
   // Ensure that we are in the UI thread, and post to the UI thread if
@@ -306,6 +403,23 @@ void PrerenderManager::RecordPrefetchTagObservedOnUIThread() {
   last_prefetch_seen_time_ = base::TimeTicks::Now();
 }
 
+void PrerenderManager::RemovePendingPreload(PrerenderContents* entry) {
+  RenderViewHost* rvh = entry->render_view_host();

[cbentzel, 2011/03/21 19:46:04]

You could remove the rvh here, remove the CHECK's from child_id and route_id,
and test if those work. 

[dominich, 2011/03/21 20:25:51]

Done.

+
+  // If the entry doesn't have a RenderViewHost then it didn't start
+  // prerendering and there shouldn't be any pending preloads to remove.
+  if (rvh == NULL)
+    return;
+
+  int child_id;
+  int route_id;
+  CHECK(entry->child_id(&child_id));
+  CHECK(entry->route_id(&route_id));
+
+  std::pair<int, int> child_route_pair = std::make_pair(child_id, route_id);
+  pending_prerender_list_.erase(child_route_pair);
+}
+
 // static
 bool PrerenderManager::ShouldRecordWindowedPPLT() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));