**NOTFORLANDING** New link rel=prerender API

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 <utility>
+
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/prerender/prerender_manager.h"
+#include "chrome/browser/prerender/prerender_manager_factory.h"
+#include "content/public/common/referrer.h"
+#include "googleurl/src/gurl.h"
+#include "googleurl/src/url_canon.h"
+#include "ui/gfx/size.h"
+
+namespace prerender {
+
+PrerenderLinkManager::PrerenderLinkManager(
+    PrerenderManager* manager)
+    : manager_(manager) {
+}
+
+PrerenderLinkManager::~PrerenderLinkManager() {
+}
+
+void PrerenderLinkManager::OnNewLinkPrerenderImpl(
+    int prerender_id,
+    int child_id,
+    int render_view_route_id,
+    const GURL& orig_url,
+    const content::Referrer& referrer,
+    const gfx::Size& ALLOW_UNUSED size) {

[gavinp, 2012/03/28 19:51:31]

I carried this size all the way up for two reasons:

1. Because I hope to get rid of the render_view_route_id in a future patch, by
making the StorageNameSpace able to move back to the browser (the API right now
only really supports moving them the other way).

2. Because this size, from the WebView(), is actually more reliable than
GetContainerSize() on ChromeOS, and identical on all other platforms.  If we
start using it, that will make prerendering work better on ChromeOS, or at least
our brwoser tests.

+  // TODO(gavinp): Add tests to insure fragments work.
+  url_canon::Replacements<char> replacements;
+  replacements.ClearRef();
+  const GURL url = orig_url.ReplaceComponents(replacements);

[gavinp, 2012/03/28 19:51:31]

In the spirit of not changing too much as  I add this new API, I'm not turning
on fragments just yet.  This code has the same effect as the CachedResource in
WebKit to remove fragments.

+
+  manager_->AddPrerenderFromLinkRelPrerender(
+      child_id, render_view_route_id, url, referrer);
+  const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);

[cbentzel, 2012/03/29 00:13:37]

Why don't you need to include route_id as well? Do you assume that the
prerender_id is unique per process rather than per RenderView? Is this an
appropriate assumption to make?

[gavinp, 2012/04/20 17:54:14]

The prerender_id should be unique across the WebKit instance, and there's only
one WebKit instance per renderer process.  This is an appropriate assumption.

+  DCHECK(id_map_.find(child_and_prerender_id) == id_map_.end());
+  id_map_.insert(std::make_pair(child_and_prerender_id, url));
+  url_map_.insert(std::make_pair(url, child_and_prerender_id));

[cbentzel, 2012/03/29 00:13:37]

I recommend adding a CheckInvariant() function which is only called in debug in
DCHECKs() to ensure that id_map_ and url_map_ are consistent. I would put it at
the start/end of each of the mutation methods.

[gavinp, 2012/04/20 17:54:15]

Done.

+}
+
+void PrerenderLinkManager::OnRemovedLinkPrerenderImpl(const int prerender_id,
+                                                      const int child_id) {
+  const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
+
+  PrerenderIdToUrlMap::iterator id_url_iter =
+      id_map_.find(child_and_prerender_id);
+  if (id_url_iter == id_map_.end())
+    return;
+  const GURL url = id_url_iter->second;
+  id_map_.erase(id_url_iter);
+
+  // Are any other link elements prerendering this URL?
+  UrlToPrerenderIdMap::iterator url_id_iter = url_map_.find(url);

[cbentzel, 2012/03/29 00:13:37]

This doesn't guarantee to find the first entry in the multimap for url. Use
lower_bound instead. Or you could use equal_range, but you are mutating the
multimap as you go along. 

[gavinp, 2012/04/20 17:54:15]

_The C++ Programming Language_ claims otherwise in 17.4.1.6 (pg 486 in the 1st
edition).  The C++03 standard, however, does not go so far.  In ISO 14882:2003,
the claim is only that it finds a matching element. But in <algorithm>, the
standard says in 25.1.2 [lib.alg.find] that the std::find(multimap.begin(),
multimap.end(), i) is claimed to find the first match.  I wonder why the
inconsistancy?  A review of the code to my local STL shows std::find() on a
multimap is the same as multimap<>::find, so I'm good, but I'm switching to
lower_bound here.

+  int remaining_prerender_link_count = 0;
+  while (url_id_iter != url_map_.end() && url_id_iter->first == url) {
+    if (url_id_iter->second == child_and_prerender_id) {
+      UrlToPrerenderIdMap::iterator to_erase = url_id_iter;
+      ++url_id_iter;
+      url_map_.erase(to_erase);

[cbentzel, 2012/03/29 00:13:37]

I always get nervous about erasing while iterating, although this appears to
handle it correctly. 

I think it would be easier to do something like to_erase = url_map_.end()
outside of the loop, have the loop bounds defined by url_map_.equal_range, and
after the loop is done erase if the iterator does not == url_map_.end(). What do
you think?

[mmenke, 2012/03/29 01:15:39]

I suggest that you only delete entries in one place.  It's a bit slower, but
think just calling "RemovePrerender(blah)" first from here and from
OnUnloadedLinkPrerenderImpl makes things simpler.

Also, then you won't need a loop here.  Just remove the link, and then do a
find.

[gavinp, 2012/04/20 17:54:15]

Done.

+      continue;
+    }
+    ++remaining_prerender_link_count;
+    ++url_id_iter;
+  }
+  if (remaining_prerender_link_count)
+    return;
+
+  // TODO(gavinp): Track down the correct prerender and stop it, rather than

[cbentzel, 2012/03/29 00:13:37]

Please fix this. Also, it doesn't look like you have feedback from the
PrerenderManager back into here in case the URL that this added was evicted by a
different prerender.

[gavinp, 2012/04/20 17:54:15]

Done, in a basic way using the existing PrerenderManager APIs.

After this CL lands, and we have the plumbing in, I intend to land a followup CL
that connects the PrerenderLinkManager and the PrerenderManager much more
closely.

+  // this nuclear option, which assumes that only one prerender at a time
+  // runs.
+  if (manager_->IsPrerendering(url))
+    manager_->CancelAllPrerenders();
+}
+
+void PrerenderLinkManager::OnUnloadedLinkPrerenderImpl(
+    int prerender_id,
+    int child_id) {
+  // TODO(gavinp,cbentzel): Implement reasonable behaviour for
+  // navigation away from launcher.
+  const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
+  PrerenderIdToUrlMap::iterator id_url_iter =
+      id_map_.find(child_and_prerender_id);
+  if (id_url_iter == id_map_.end())
+    return;
+  const GURL& url = id_url_iter->second;
+  DCHECK(url_map_.find(url) != url_map_.end());
+  ignore_result(url_map_.erase(url));
+  id_map_.erase(id_url_iter);
+}
+
+// static
+void PrerenderLinkManager::OnNewLinkPrerender(
+      Profile* profile,
+      int prerender_id,
+      int child_id,
+      int render_view_route_id,
+      const GURL& url,
+      const content::Referrer& referrer,
+      const gfx::Size& size) {
+  if (prerender::PrerenderManager* prerender_manager =
+      prerender::PrerenderManagerFactory::GetForProfile(profile)) {
+    prerender_manager->link_manager()->OnNewLinkPrerenderImpl(

[cbentzel, 2012/03/29 00:13:37]

Not sure I like that this goes "Get the prerender managers link manager, and
have that call into the prerender manager".

In general I'd like this to exist completely outside of the PrerenderManager.
Perhaps make it a ProfileKeyedService as well? 

[gavinp, 2012/04/20 17:54:15]

Done.

+        prerender_id, child_id, render_view_route_id,
+        url, referrer, size);
+  }
+}
+
+// static
+void PrerenderLinkManager::OnRemovedLinkPrerender(Profile* profile,
+                                                  int prerender_id,
+                                                  int child_id) {
+  if (prerender::PrerenderManager* prerender_manager =
+      prerender::PrerenderManagerFactory::GetForProfile(profile))
+    prerender_manager->link_manager()->OnRemovedLinkPrerenderImpl(prerender_id,
+                                                                  child_id);
+}
+
+// static
+void PrerenderLinkManager::OnUnloadedLinkPrerender(Profile* profile,
+                                                   int prerender_id,
+                                                   int child_id) {
+  if (prerender::PrerenderManager* prerender_manager =
+      prerender::PrerenderManagerFactory::GetForProfile(profile))
+    prerender_manager->link_manager()->OnUnloadedLinkPrerenderImpl(prerender_id,
+                                                                   child_id);
+}
+
+}  // namespace prerender