Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(311)

Issue 9875026: **NOTFORLANDING** New link rel=prerender API (Closed)

Created:
8 years, 6 months ago by gavinp
Modified:
8 years, 5 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, jam, darin-cc_chromium.org, brettw-cc_chromium.org, michaeln
Visibility:
Public.

Description

**NOTFORLANDING** New link rel=prerender API This new API implements chrome-side of the new API for <link rel=prerender...> elements. The webkit side of this is https://bugs.webkit.org/show_bug.cgi?id=82478 This patch includes what will end up being many commits! Please review the architecture; after this review starts settling down into nits, I'll split it up into separate reviews for each part as I stage both this change and the webkit bug into the respective projects' trunks. BUG=None

Patch Set 1 : wdyt? #

Total comments: 27

Patch Set 2 : no changes: merged patch set for diffs #

Patch Set 3 : Preparation: move WebReferrerPolicy.h header #

Patch Set 4 : Preparation: move content::Referrer into CommonParamTraits #

Patch Set 5 : rename a file (no changes) to make rietveld easier to read #

Patch Set 6 : Add PrerenderLinkManager #

Patch Set 7 : Cleanup: remove old launching API #

Patch Set 8 : patch sets 3-7 against trunk, for combined browsing #

Total comments: 60
Unified diffs Side-by-side diffs Delta from patch set Stats (+1042 lines, -725 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 7 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/external_tab/external_tab_container_win.cc View 1 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/prerender/prerender_browsertest.cc View 1 2 3 4 5 7 5 chunks +37 lines, -1 line 4 comments Download
M chrome/browser/prerender/prerender_contents.h View 1 2 3 4 5 7 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/browser/prerender/prerender_link_manager.h View 1 2 3 4 5 7 1 chunk +76 lines, -0 lines 10 comments Download
A chrome/browser/prerender/prerender_link_manager.cc View 1 2 3 4 5 7 1 chunk +163 lines, -0 lines 25 comments Download
A chrome/browser/prerender/prerender_link_manager_factory.h View 1 2 3 4 5 7 1 chunk +37 lines, -0 lines 2 comments Download
A chrome/browser/prerender/prerender_link_manager_factory.cc View 1 2 3 4 5 7 1 chunk +49 lines, -0 lines 5 comments Download
M chrome/browser/prerender/prerender_manager.h View 1 2 3 4 5 7 2 chunks +24 lines, -18 lines 0 comments Download
D chrome/browser/prerender/prerender_manager_unittest.cc View 1 2 3 4 6 7 1 chunk +0 lines, -601 lines 0 comments Download
A chrome/browser/prerender/prerender_message_filter.h View 1 2 3 4 5 7 1 chunk +60 lines, -0 lines 6 comments Download
A chrome/browser/prerender/prerender_message_filter.cc View 1 2 3 4 5 7 1 chunk +103 lines, -0 lines 4 comments Download
A + chrome/browser/prerender/prerender_unittest.cc View 1 2 3 4 5 6 7 25 chunks +138 lines, -61 lines 4 comments Download
M chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc View 1 3 4 5 6 2 chunks +0 lines, -26 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 7 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/chrome_renderer.gypi View 1 2 3 4 5 7 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 7 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/prerender_messages.h View 1 2 3 4 5 7 1 chunk +28 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.cc View 1 2 3 4 5 7 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/renderer/prerender/prerender_dispatcher.cc View 1 2 3 4 5 7 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerender_extra_data.h View 1 2 3 4 5 7 1 chunk +39 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerender_extra_data.cc View 1 2 3 4 5 7 1 chunk +27 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerenderer_client.h View 1 2 3 4 5 7 1 chunk +34 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerenderer_client.cc View 1 2 3 4 5 7 1 chunk +38 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerendering_platform.h View 1 2 3 4 5 7 1 chunk +28 lines, -0 lines 0 comments Download
A chrome/renderer/prerender/prerendering_platform.cc View 1 2 3 4 5 7 1 chunk +39 lines, -0 lines 0 comments Download
A chrome/test/data/prerender/prerender_loader_removing_links.html View 1 2 3 4 5 7 1 chunk +39 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 5 6 7 2 chunks +0 lines, -6 lines 0 comments Download
M content/public/browser/resource_request_info.h View 1 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M content/public/common/common_param_traits.h View 1 2 3 5 6 7 4 chunks +20 lines, -1 line 0 comments Download
M content/public/common/common_param_traits.cc View 1 2 3 5 6 7 2 chunks +30 lines, -0 lines 0 comments Download
M content/public/common/context_menu_params.h View 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M content/public/common/referrer.h View 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M webkit/glue/resource_loader_bridge.h View 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M webkit/glue/resource_type.cc View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
M webkit/glue/weburlrequest_extradata_impl.h View 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 11 (0 generated)
gavinp
WDYT?
8 years, 6 months ago (2012-03-28 19:10:05 UTC) #1
gavinp
I'd like to note: I want to add more to the prerender_manager API in the ...
8 years, 6 months ago (2012-03-28 19:51:31 UTC) #2
cbentzel
Thanks. I'll try to do a first pass tonight, but will not worry about nits. ...
8 years, 6 months ago (2012-03-28 20:15:05 UTC) #3
cbentzel
http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/prerender_browsertest.cc File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/prerender_browsertest.cc#newcode877 chrome/browser/prerender/prerender_browsertest.cc:877: IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderPageRemovingLink) { Should probably have some tests for ...
8 years, 6 months ago (2012-03-29 00:13:37 UTC) #4
mmenke
http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc#newcode65 chrome/browser/prerender/prerender_link_manager.cc:65: url_map_.erase(to_erase); I suggest that you only delete entries in ...
8 years, 6 months ago (2012-03-29 01:15:38 UTC) #5
gavinp
Remember this patch from three weeks ago? Here's another view on it. I've uploaded it ...
8 years, 5 months ago (2012-04-20 17:54:14 UTC) #6
cbentzel
Will try to review today. On Fri, Apr 20, 2012 at 1:54 PM, <gavinp@chromium.org> wrote: ...
8 years, 5 months ago (2012-04-23 11:35:34 UTC) #7
cbentzel
On 2012/04/23 11:35:34, cbentzel wrote: > Will try to review today. Oops. Tomorrow morning, too ...
8 years, 5 months ago (2012-04-24 01:38:33 UTC) #8
mmenke
Mostly nit-ish comments, though I've tried to refrain from going overboard on them, this pass. ...
8 years, 5 months ago (2012-04-24 15:26:15 UTC) #9
cbentzel
I just reviewed the browser process changes. http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/prerender_link_manager.cc File chrome/browser/prerender/prerender_link_manager.cc (right): http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/prerender_link_manager.cc#newcode23 chrome/browser/prerender/prerender_link_manager.cc:23: bool CheckMapsAreInverseOfEachOther(const ...
8 years, 5 months ago (2012-04-24 15:42:59 UTC) #10
gavinp
8 years, 5 months ago (2012-04-26 23:55:39 UTC) #11
Thank you everyone for your reviews!

I've answered comments below, but the remediations are available in
http://codereview.chromium.org/10198040/ , and I'd like to continue hacking at
this bug there.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:826:
loader_query_and_fragment_);
On 2012/04/24 15:26:15, Matt Menke wrote:
> We have "src_url" but "loader_query_and_fragment_" and "loader_path".  Know it
> was like this before, but I find it a little confusing.  I suggest renaming
the
> latter two to something like "src_url_query_and_fragment_" and "src_path"
> respectively.
> 
> Alternatively, could rename src_url to loader_url.  That makes the variable
> names a little clearer, at the cost of losing the src/dest symmetry.

I went with the change to loader_url; what src/dest symmetry are we losing? 
What was called src was the intermediate product, made by translating the loader
file (the src src?) by the replacement text.  That's not src!  That's
intermediate state!

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:965: NavigateToDestURL();
On 2012/04/24 15:26:15, Matt Menke wrote:
> I'd suggest you make sure that the link manager is empty after the tab has
been
> navigated.  Same goes for the 3 new tests.  There may be other tests where
this
> is a good idea, too (Like the crash one).

Done.  These tests can even land without being disabled until after the API is
turned on (a subsequent step due to staging), since the prerender_link_manager
will be empty at the end of a run using the phantom request launcher!

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:23: bool
CheckMapsAreInverseOfEachOther(const MapForwards& map_forwards,
On 2012/04/24 15:26:15, Matt Menke wrote:
> nit:  Suggest you rename this "MapsAreInverseOfEachOther".  Mostly because
> DCHECK(Check...) is just a little weird.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:27: for (typename
MapForwards::const_iterator i = map_forwards.begin(),
On 2012/04/24 15:42:59, cbentzel wrote:
> 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.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:34: e = map_backwards.end();
On 2012/04/24 15:26:15, Matt Menke wrote:
> Is this necessary?  Given that the maps have the same size, I'd think not,
> assuming that the underlying map classes work.

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

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:54: void
PrerenderLinkManager::RemovePrerender(
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: ordering of class definitions should match declarations.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:73: void
PrerenderLinkManager::OnAddPrerender(const int prerender_id,
On 2012/04/24 15:42:59, cbentzel wrote:
> 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?

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.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:85: // TODO(gavinp): Add
tests to insure fragments work, then remove this fragment
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: ensure

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:95:
id_map_.insert(std::make_pair(child_and_prerender_id, url));
On 2012/04/24 15:42:59, cbentzel wrote:
> 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.

See my reply to mmenke above...

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:117: // TODO(gavinp): Track
down the correct prerender and stop it, rather than
On 2012/04/24 15:42:59, cbentzel wrote:
> Why not fix this now?

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.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:125: int prerender_id,
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: could this fit on the previous line?

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

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:150: i =
id_map_.lower_bound(child_and_minimum_prerender_id),
On 2012/04/24 15:42:59, cbentzel wrote:
> 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()?

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().

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:158:
OnAbandonPrerender(prerender_ids_to_abandon.front(), child_id);
On 2012/04/24 15:42:59, cbentzel wrote:
> 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. 

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...

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:31: // Launch and cancel
prerenders based on the LinkPrerender element events.
On 2012/04/24 15:42:59, cbentzel wrote:
> I'd prefer a bit more description here.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:40: int child_id,
On 2012/04/24 15:26:15, Matt Menke wrote:
> nit:  I suggest always putting child_id first.  This is both the order of the
id
> pair, and it makes more sense to have the "more significant" id first.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:52: // renderer processed was
fast-closed when the last render view in it was
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: renderer process

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:62: typedef
std::map<ChildAndPrerenderIdPair, GURL> PrerenderIdToUrlMap;
On 2012/04/24 15:26:15, Matt Menke wrote:
> nit:  Suggest you rename this either IdToUrlMap, IdPairToUrlMap,
> PrerenderIdsToUrlMap, or some such.  Just make clear it's not just a
> prerender_id.  And a corresponding change to the other typedef, of course.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:69: UrlToPrerenderIdMap
url_map_;
On 2012/04/24 15:26:15, Matt Menke wrote:
> nit:  Suggest you rename these ids_to_url_map_, etc.  It's not uncommon to
name
> maps after the values instead of the keys.  In fact, a quick search indicates
> that naming maps after the values may be more common.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager_factory.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.cc:18: if
(!PrerenderManager::IsPrerenderingPossible())
On 2012/04/24 15:42:59, cbentzel wrote:
> This check doesn't seem necessary - kind of covered by the
> PrerenderManagerFactory::GetForProfile() which is called in
> BuildServiceInstanceFor

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.cc:30: :
ProfileKeyedServiceFactory("PrerenderLinkmanager",
On 2012/04/24 15:42:59, cbentzel wrote:
> It seems like this factory should depend on PrerenderManagerFactory, so the
> PrerenderLinkManager is created after the PrerenderManager for a profile. You
> can just use DependsOn to do this.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager_factory.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.h:35: }
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: // namespace prerender 

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_message_filter.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.cc:34: {
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: move this up with profile_

Done.

NitFromMeToTheUniverse: WebKit and chrome have different style guides on this!

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.cc:52: if (message.type() ==
PrerenderHostMsg_AddPrerender::ID ||
On 2012/04/24 15:42:59, cbentzel wrote:
> What are the lifetime guarantees for both PrerenderMessageFilter and profile_
> when these are handled on the UI thread? For example, is it possible for the
> profile_ to be destroyed between when OnChannelClosing is called on IO thread
> and OnChannelClosingInUIThread is invoked on UI thread?

At creation, the PrerenderMessageFilter is placed on the IPC::ChannelProxy, and
that occurs on the UI thread.  The scope of that proxy should be the life of the
channel.  However, in the channel closing case, I don't know if it's legitimate,
as I'm posting my own task to another thread.  In the case of the other
messages, they should be good, since the PrerenderMessageFilter should be scoped
inside of the ChannelProxy which itself should be scoped inside of the
RendererProcessHost.

So in the OnChannelClosingInUIThread case, I believe I need to call
g_browser_process->profile_manager()->IsValidProfile(....), but I think I'm fine
in the other cases for the reason stated.  Who should we ask to double check
this logic for us?

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_message_filter.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.h:40: virtual
~PrerenderMessageFilter() { }
On 2012/04/24 15:42:59, cbentzel wrote:
> Why are you inlining this destructor?

It's against the style guide, even.  Fixed.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.h:50: 
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: Remove extra newline.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.h:53: Profile*const profile_;
On 2012/04/24 15:42:59, cbentzel wrote:
> spaces please. I haven't really seen use of *const in the codebase but it does
> seem appropriate.

Done.  I'm a fan of making strong statements with const where you can.  It's
when you start seeing const_cast<> that I am willing to talk compromise...

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_unittest.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:250: bool
AddSimplePrerenderViaManager(const GURL& url) {
On 2012/04/24 15:26:15, Matt Menke wrote:
> This is little weird.  Since all link rel prerenders now go through the
> PrerenderLinkManager, we're basically testing things that can't happen when we
> use this.
> 
> I suggest using the omnibox path instead.  Also should have a test where one
url
> is added by the omnibox before the same URL is added via a link tag.
> 
> Alternatively, could make prerender_link_manager()->OnAddPrerender return the
> bool it gets back from AddPrerenderFromLinkRelPrerender.

Done, with the second option.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:638: TEST_F(PrerenderTest,
LinkManagerCancel) {
On 2012/04/24 15:26:15, Matt Menke wrote:
> Other test suggestions:
> 
> Cancel a prerender that's already long since been cancelled.

Yes, but expired instead of cancelled.


> 
> Add a prerender twice and make sure we don't cancel the original until it's
been
> cancelled twice by the link manager.

Done.


> 
> Add a prerender again after it's already been cancelled by the
PrerenderManager
> for some other reason.

Done.


> Add a prerender, cancel it through the link manager, and then add one again. 
We
> don't have a test that the prerender manager cleans up its maps correctly in
the
> general case.

Done, and PrerenderLinkManager::IsEmpty() was great here.

Powered by Google App Engine
This is Rietveld 408576698