**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 <limits>
+#include <queue>
+#include <utility>
+
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/prerender/prerender_contents.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 {
+
+template<typename MapForwards, typename MapBackwards>
+bool CheckMapsAreInverseOfEachOther(const MapForwards& map_forwards,

[mmenke, 2012/04/24 15:26:15]

nit:  Suggest you rename this "MapsAreInverseOfEachOther".  Mostly because
DCHECK(Check...) is just a little weird.

[cbentzel, 2012/04/24 15:42:59]

Nit: Perhaps AreMapsInverseOfEachOther? No actual CHECK'ing is done this.

[gavinp, 2012/04/26 23:55:39]

Done.

+                                    const MapBackwards& map_backwards) {
+  if (map_forwards.size() != map_backwards.size())
+    return false;
+  for (typename MapForwards::const_iterator i = map_forwards.begin(),

[cbentzel, 2012/04/24 15:42:59]

This doesn't quite test inversion. For example, if map_forwards is {'a': 1, 'b':
2} and map_backwards is {1: 'b', 2: 'a'} this would still return true.

[gavinp, 2012/04/26 23:55:39]

Done.

+                                            e = map_forwards.end();
+       i != e; ++i) {
+    if (map_backwards.find(i->second) == map_backwards.end())
+      return false;
+  }
+  for (typename MapBackwards::const_iterator i = map_backwards.begin(),
+                                             e = map_backwards.end();

[mmenke, 2012/04/24 15:26:15]

Is this necessary?  Given that the maps have the same size, I'd think not,
assuming that the underlying map classes work.

[gavinp, 2012/04/26 23:55:39]

No it's not, especially now that I fixed the terrible brokenness that Chris
found and commented on, above.

+       i != e; ++i) {
+    if (map_forwards.find(i->second) == map_forwards.end())
+      return false;
+  }
+  return true;
+}
+
+}
+
+namespace prerender {
+
+PrerenderLinkManager::PrerenderLinkManager(
+    PrerenderManager* manager)
+    : manager_(manager) {
+}
+
+PrerenderLinkManager::~PrerenderLinkManager() {
+}
+
+void PrerenderLinkManager::RemovePrerender(

[cbentzel, 2012/04/24 15:42:59]

Nit: ordering of class definitions should match declarations.

[gavinp, 2012/04/26 23:55:39]

Done.

+    const PrerenderIdToUrlMap::iterator& id_url_iter) {
+  DCHECK(CheckMapsAreInverseOfEachOther(id_map_, url_map_));
+  const GURL url = id_url_iter->second;
+  const ChildAndPrerenderIdPair child_and_prerender_id = id_url_iter->first;
+  id_map_.erase(id_url_iter);
+
+  for (UrlToPrerenderIdMap::iterator i = url_map_.lower_bound(url),
+                                     e = url_map_.upper_bound(url);
+       i != e; ++i) {
+    if (i->second == child_and_prerender_id) {
+      url_map_.erase(i);
+      DCHECK(CheckMapsAreInverseOfEachOther(id_map_, url_map_));
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
+void PrerenderLinkManager::OnAddPrerender(const int prerender_id,

[cbentzel, 2012/04/24 15:42:59]

Curious - this doesn't match the signature in the header, due to the const
arguments. Does this build? Also unsure if the const is needed for pass-by-value
cases, WDYT?

[gavinp, 2012/04/26 23:55:39]

This does compile.  You can always add "const" in the definition of a function
to impose a limitation on the definition by making the automatic portion const. 
There's no real point putting const in the declaration of a function, since the
pass-by-value semantic makes it not interesting to the caller.  I would argue as
a style issue the automatic portion should never been given as const in the
definition, but in the declaration it can make sense.

Some examples of declaration types, and the definition type you might use that
is const:

int  -->  const int
int*  -->  int* const
const int*(*f)(int) -->  const int*(*const f)(int)
const int(*(*f)(int (*)())))(const int*)  --> const int (*(*const f)(int
(*)()))(const int*)

and so on.

But, if you think they detract from readability, I'm removing all of the const
from the automatic parts here.

+                                          const int child_id,
+                                          const GURL& orig_url,
+                                          const content::Referrer& referrer,
+                                          const gfx::Size& ALLOW_UNUSED size,
+                                          const int render_view_route_id) {
+  DVLOG(2) << "OnAddPrerender, child_id = " << child_id
+           << ", prerender_id = " << prerender_id
+           << ", url = " << orig_url.spec();
+  DVLOG(3) << "... render_view_route_id = " << render_view_route_id
+           << ", referrer url = " << referrer.url.spec();
+  DCHECK(CheckMapsAreInverseOfEachOther(id_map_, url_map_));
+  // TODO(gavinp): Add tests to insure fragments work, then remove this fragment

[cbentzel, 2012/04/24 15:42:59]

Nit: ensure

[gavinp, 2012/04/26 23:55:39]

Done.

+  // clearing code.
+  url_canon::Replacements<char> replacements;
+  replacements.ClearRef();
+  const GURL url = orig_url.ReplaceComponents(replacements);
+
+  manager_->AddPrerenderFromLinkRelPrerender(
+      child_id, render_view_route_id, url, referrer);

[mmenke, 2012/04/24 15:26:15]

Random comment:  When this fails, is there anything to be gained by tracking the
prerender?  We have a NOTREACHED in OnCancelPrerender, which is nice for
testing, but beyond that...

Nice thing about not tracking ids that don't have prerenders, is that we could
probably simplify things a bit here - get rid of the maps, for example, though
we'd probably want to remove ids when a prerender is cancelled by the
PrerenderManager, in that case.  Then we could simplify this class a little - no
need for two maps, for example.  The limit on prerenders will always be low
enough that we wouldn't gain anything from having inverse maps.  As it is,
they're currently only useful when a website behaves somewhat bizarrely,
spamming us with prerender requests.

Downside is that the list of requested prerenders that we ignored could one day
be useful.

+  const ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
+  DCHECK(id_map_.find(child_and_prerender_id) == id_map_.end());
+  id_map_.insert(std::make_pair(child_and_prerender_id, url));

[cbentzel, 2012/04/24 15:42:59]

Should this be done prior to the AddPrerender case? Wondering if this will be
needed if AddPrerender synchronously fails and we want to signal the error back
to the link element.

[gavinp, 2012/04/26 23:55:39]

See my reply to mmenke above...

+  url_map_.insert(std::make_pair(url, child_and_prerender_id));
+  DCHECK(CheckMapsAreInverseOfEachOther(id_map_, url_map_));
+}
+
+void PrerenderLinkManager::OnCancelPrerender(const int prerender_id,
+                                             const int child_id) {
+  DVLOG(2) << "OnCancelPrerender, child_id = " << child_id
+           << ", prerender_id = " << prerender_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()) {
+    NOTREACHED() << "Canceling a prerender that doesn't exist.";
+    return;
+  }
+  const GURL url = id_url_iter->second;
+  RemovePrerender(id_url_iter);
+
+  if (url_map_.find(url) != url_map_.end())
+    return;
+
+  // TODO(gavinp): Track down the correct prerender and stop it, rather than

[cbentzel, 2012/04/24 15:42:59]

Why not fix this now?

[gavinp, 2012/04/26 23:55:39]

This here is advanced quite a lot from what was in the last upload; if that same
url is prerendering, it kills it.  To be really correct, we'd need to key off of
the session storage namespace, too.

But getting this perfect probably requires refactoring the interface on
PrerenderManager, and that's a whole other kettle of fish.  At present, this
change consists of thousands of lines across chrome and webkit, requiring at
least four crossings of the WebKit/Chrome boundary to commit properly....

If I can land an interface in the PrerenderLinkManager that is sufficient, if
not ideal, I'm much happier doing that than making this patch larger and keeping
it in flight longer.

+  // this nuclear option, which assumes that only one prerender at a time
+  // runs.
+  if (PrerenderContents* contents = manager_->GetEntry(url))
+    contents->Destroy(FINAL_STATUS_CANCELLED);

[cbentzel, 2012/04/24 15:42:59]

Yes, this seems like the appropriate FINAL_STATUS to use. Matches omnibox
cancellations.

+}
+
+void PrerenderLinkManager::OnAbandonPrerender(
+    int prerender_id,

[cbentzel, 2012/04/24 15:42:59]

Nit: could this fit on the previous line?

[gavinp, 2012/04/26 23:55:39]

Done.  This removes the last sign of the horrible *Impl names from before.  :)

+    int child_id) {
+  DVLOG(2) << "OnAbandonPrerender, child_id = " << child_id
+           << ", prerender_id = " << prerender_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()) {
+    // FIXME(gavinp): Currently, canceled prerenders also get abandoned later.
+    // When the WebKit fix to stop this lands, add a NOTREACHED() here.
+    return;
+  }
+  RemovePrerender(id_url_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 (PrerenderIdToUrlMap::iterator
+           i = id_map_.lower_bound(child_and_minimum_prerender_id),

[cbentzel, 2012/04/24 15:42:59]

Clever.

Just to be sure though - if id_map_ only consists of entries with child id's <
child_id, both i and e should be equivalent to id_map_.end()?

[gavinp, 2012/04/26 23:55:39]

Yes.  But we'd never run the loop body in that case, so we'll be fine.  We might
also get two iterators in the middle of the map, or both at begin().

+           e = id_map_.upper_bound(child_and_maximum_prerender_id);
+       i != e; ++i) {
+    prerender_ids_to_abandon.push(i->first.second);
+  }
+  while (!prerender_ids_to_abandon.empty()) {
+    DVLOG(4) << "---> abandon prerender_id = "
+             << prerender_ids_to_abandon.front();
+    OnAbandonPrerender(prerender_ids_to_abandon.front(), child_id);

[cbentzel, 2012/04/24 15:42:59]

Why is this overloading OnAbandonPrerender, if we also want to use that for
navigating away? Seems like we could instantaneously remove prerenders in this
case. 

[gavinp, 2012/04/26 23:55:39]

What about the case of the user navigating away from the last site for a
renderer, through a redirect chain to their final site?  We'll fast kill the
site we're leaving pretty much as soon as the new renderer commits, killing the
prerender.  But a redirect chain would almost certainly still be in flight...

+    prerender_ids_to_abandon.pop();
+  }
+}
+
+}  // namespace prerender