Change multi-prerender API to include per launcher slots.

chrome/browser/prerender/prerender_unittest.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 "base/command_line.h"
+#include "base/format_macros.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/prerender/prerender_handle.h"
 #include "chrome/browser/prerender/prerender_link_manager.h"
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/prerender/prerender_origin.h"
 #include "chrome/common/chrome_switches.h"
@@ skipping 280 matching lines @@
   }
 
   UnitTestPrerenderManager* prerender_manager() {
     return prerender_manager_.get();
   }
 
   PrerenderLinkManager* prerender_link_manager() {
     return prerender_link_manager_.get();
   }
 
+  void SetConcurrency(size_t concurrency) {
+    prerender_manager()->mutable_config().max_link_concurrency_per_launcher =
+        concurrency;
+    prerender_manager()->mutable_config().max_link_concurrency =
+        std::max(prerender_manager()->mutable_config().max_link_concurrency,
+                 concurrency);
+  }
+
   bool IsEmptyPrerenderLinkManager() {
     return prerender_link_manager_->IsEmpty();
   }
 
   int last_prerender_id() const {
     return last_prerender_id_;
   }
 
   int GetNextPrerenderID() {
     return ++last_prerender_id_;
   }
 
-  // Shorthand to add a simple preload with a reasonable source.
+  bool PrerenderHasHandle(int child_id, int prerender_id) {

[mmenke, 2012/12/28 18:25:33]

Think this name is confusing.  LauncherHasRunningPrerender?

[gavinp, 2012/12/28 19:53:46]

Done.

+    PrerenderLinkManager::Prerender* prerender =
+        prerender_link_manager()-> FindByLauncherChildIdAndPrerenderId(

[mmenke, 2012/12/28 18:25:33]

nit:  Remove space

[gavinp, 2012/12/28 19:53:46]

Done.

+            child_id, prerender_id);
+    return prerender && prerender->handle;
+  }
+
+  // Shorthand to add a simple prerender with a reasonable source. Returns
+  // true iff the prerender has been added to the PrerenderManager by the
+  // PrerenderLinkManager and the PrerenderManager returned a handle.
   bool AddSimplePrerender(const GURL& url) {
-    return prerender_link_manager()->OnAddPrerender(
-        kDefaultChildId, GetNextPrerenderID(),
-        url, content::Referrer(),
-        kSize, kDefaultRenderViewRouteId);
+    prerender_link_manager()->OnAddPrerender(kDefaultChildId,
+                                             GetNextPrerenderID(),
+                                             url, content::Referrer(),
+                                             kSize, kDefaultRenderViewRouteId);
+    return PrerenderHasHandle(kDefaultChildId, last_prerender_id());
   }
 
  private:
   PrerenderTracker* prerender_tracker() {
     return g_browser_process->prerender_tracker();
   }
 
   // Needed to pass PrerenderManager's DCHECKs.
   TestingProfile profile_;
   MessageLoop message_loop_;
   content::TestBrowserThread ui_thread_;
   scoped_ptr<UnitTestPrerenderManager> prerender_manager_;
   scoped_ptr<PrerenderLinkManager> prerender_link_manager_;
   int last_prerender_id_;
 };
 
 TEST_F(PrerenderTest, FoundTest) {
   GURL url("http://www.google.com/");
   DummyPrerenderContents* prerender_contents =
       prerender_manager()->CreateNextPrerenderContents(
           url,
           FINAL_STATUS_USED);
   EXPECT_TRUE(AddSimplePrerender(url));
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
   ASSERT_EQ(prerender_contents, prerender_manager()->FindAndUseEntry(url));
 }
 
+// Make sure that if queue a request, and a second prerender request for the
+// same URL comes in, that the second request attaches to the first prerender,
+// and we don't use the second prerender contents.
 TEST_F(PrerenderTest, DuplicateTest) {
+  SetConcurrency(2);
   GURL url("http://www.google.com/");
   DummyPrerenderContents* prerender_contents =
       prerender_manager()->CreateNextPrerenderContents(
-          url,
-          FINAL_STATUS_USED);
-  EXPECT_TRUE(AddSimplePrerender(url));
-  EXPECT_TRUE(prerender_contents->prerendering_has_started());
-
-  scoped_ptr<PrerenderHandle> duplicate_prerender_handle(
-      prerender_manager()->AddPrerenderFromLinkRelPrerender(
-          kDefaultChildId, kDefaultRenderViewRouteId, url,
-          Referrer(url, WebKit::WebReferrerPolicyDefault), kSize));
-
-  EXPECT_TRUE(duplicate_prerender_handle->IsPrerendering());
-
-  ASSERT_EQ(prerender_contents, prerender_manager()->FindAndUseEntry(url));
-
-  EXPECT_FALSE(duplicate_prerender_handle->IsPrerendering());
-}
-
-// Make sure that if queue a request, and a second prerender request for the
-// same URL comes in, that we drop the second request and keep the first one.
-TEST_F(PrerenderTest, DropSecondRequestTest) {
-  GURL url("http://www.google.com/");
-  DummyPrerenderContents* prerender_contents =
-      prerender_manager()->CreateNextPrerenderContents(
           url,
           FINAL_STATUS_USED);
   DummyPrerenderContents* null = NULL;
   EXPECT_TRUE(AddSimplePrerender(url));
   EXPECT_EQ(null, prerender_manager()->next_prerender_contents());
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
 
   DummyPrerenderContents* prerender_contents1 =
       prerender_manager()->CreateNextPrerenderContents(
           url,
@@ skipping 89 matching lines @@
                                                last_prerender_id());
   EXPECT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
 
   DummyPrerenderContents* null = NULL;
   EXPECT_EQ(null, prerender_manager()->next_prerender_contents());
 
   prerender_manager()->AdvanceTimeTicks(second_advance);
   EXPECT_EQ(null, prerender_manager()->FindEntry(url));
 }
 
-// LRU Test. Make sure that if we prerender more requests than we support, that
-// the oldest one will be dropped.
-TEST_F(PrerenderTest, DropOldestRequestTest) {
-  std::vector<int> concurrencies_to_test;
-  concurrencies_to_test.push_back(
-      prerender_manager()->config().max_concurrency);
-  concurrencies_to_test.push_back(1);
-  concurrencies_to_test.push_back(2);
+// Make sure that if we prerender more requests than we support, that we launch
+// them in the order given up until we reach MaxConcurrency, at which point we
+// queue them and launch them in the order given. As well, insure that limits
+// are enforced for the system as a whole and on a per launcher basis.
+TEST_F(PrerenderTest, MaxConcurrencyTest) {

[mmenke, 2012/12/28 18:25:33]

Three test suggestions, with per_launcher = 1, max_link_concurrency = 2:

Two prerenders from:

1)  Same RenderView.
2)  Same process, different RenderViews.
3)  Different processes, same RenderView ID (Since IDs are only unique within a
process).

[gavinp, 2012/12/28 19:53:46]

(1) is already tested in this test, right?

I've added (2). Good catch.

(3) scares me. child_id == -1 is special cased everywhere...

[mmenke, 2012/12/28 20:15:13]

Yup.  Wrote this comment early, before I looked at the others.
SGTM.

+  struct TestConcurrency {
+    size_t max_link_concurrency;
+    size_t max_link_concurrency_per_launcher;
+  };
+
+  TestConcurrency concurrencies_to_test[] = {
+    { prerender_manager()->config().max_link_concurrency,
+      prerender_manager()->config(). max_link_concurrency_per_launcher},

[mmenke, 2012/12/28 18:25:33]

nit:  Remove space.

[gavinp, 2012/12/28 19:53:46]

Done.

+
+    // With the system limit higher than the per launcher limit, the per
+    // launcher limit should be in effect.
+    { 2, 1 },
+
+    // With the per launcher limit higher than system limit, the system limit
+    // should be in effect.
+    { 2, 4 },
+  };
 
   DummyPrerenderContents* null = NULL;
-  GURL url_to_evict("http://www.google.com/evictme");
+  GURL url_to_delay("http://www.google.com/delayme");
 
-  for (std::vector<int>::const_iterator i = concurrencies_to_test.begin();
-       i != concurrencies_to_test.end();
-       ++i) {
-    const int max_concurrency = *i;
-    prerender_manager()->mutable_config().max_concurrency = max_concurrency;
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(concurrencies_to_test); ++i) {
+    prerender_manager()->mutable_config().max_link_concurrency =
+        concurrencies_to_test[i].max_link_concurrency;
+    prerender_manager()->mutable_config().max_link_concurrency_per_launcher =
+        concurrencies_to_test[i].max_link_concurrency_per_launcher;
 
-    DummyPrerenderContents* prerender_contents_to_evict =
-        prerender_manager()->CreateNextPrerenderContents(
-            url_to_evict, FINAL_STATUS_EVICTED);
-    EXPECT_TRUE(AddSimplePrerender(url_to_evict));
-    EXPECT_EQ(null, prerender_manager()->next_prerender_contents());
-    EXPECT_TRUE(prerender_contents_to_evict->prerendering_has_started());
+    const size_t effective_max_link_concurrency =
+        std::min(concurrencies_to_test[i].max_link_concurrency,
+                 concurrencies_to_test[i].max_link_concurrency_per_launcher);
 
     std::vector<GURL> urls;
     std::vector<PrerenderContents*> prerender_contentses;
 
-    for (int j = 0; j < max_concurrency; ++j) {
-      urls.push_back(GURL(base::StringPrintf("http://google.com/use#%d", j)));
+    for (size_t j = 0; j < effective_max_link_concurrency; ++j) {
+      urls.push_back(
+          GURL(base::StringPrintf("http://google.com/use#%" PRIuS, j)));
       prerender_contentses.push_back(
           prerender_manager()->CreateNextPrerenderContents(urls.back(),
                                                            FINAL_STATUS_USED));
       EXPECT_TRUE(AddSimplePrerender(urls.back()));
       EXPECT_EQ(null, prerender_manager()->next_prerender_contents());
       EXPECT_TRUE(prerender_contentses.back()->prerendering_has_started());
     }
 
-    ASSERT_EQ(null, prerender_manager()->FindEntry(url_to_evict));
-    for (int j = 0; j < max_concurrency; ++j) {
-      ASSERT_EQ(prerender_contentses[j],
+    DummyPrerenderContents* prerender_contents_to_delay =
+        prerender_manager()->CreateNextPrerenderContents(url_to_delay,
+                                                         FINAL_STATUS_USED);
+    EXPECT_FALSE(AddSimplePrerender(url_to_delay));
+    EXPECT_FALSE(prerender_contents_to_delay->prerendering_has_started());
+    EXPECT_NE(null, prerender_manager()->next_prerender_contents());
+    EXPECT_EQ(null, prerender_manager()->FindEntry(url_to_delay));
+    for (size_t j = 0; j < effective_max_link_concurrency; ++j) {
+      EXPECT_EQ(prerender_contentses[j],
                 prerender_manager()->FindAndUseEntry(urls[j]));
     }
+
+    EXPECT_TRUE(prerender_contents_to_delay->prerendering_has_started());

[mmenke, 2012/12/28 18:25:33]

Suggest checking this at the end of the above loop (Should start when the first
one is used, not just after the last one).

[gavinp, 2012/12/28 19:53:46]

Done.

+    EXPECT_EQ(prerender_contents_to_delay,
+              prerender_manager()->FindAndUseEntry(url_to_delay));
+    EXPECT_EQ(null, prerender_manager()->next_prerender_contents());
   }
 }
 
 TEST_F(PrerenderTest, AliasURLTest) {
+  SetConcurrency(7);
+
   GURL url("http://www.google.com/");
   GURL alias_url1("http://www.google.com/index.html");
   GURL alias_url2("http://google.com/");
   GURL not_an_alias_url("http://google.com/index.html");
   std::vector<GURL> alias_urls;
   alias_urls.push_back(alias_url1);
   alias_urls.push_back(alias_url2);
 
   // Test that all of the aliases work, but not_an_alias_url does not.
   DummyPrerenderContents* prerender_contents =
@@ skipping 17 matching lines @@
   prerender_contents = prerender_manager()->CreateNextPrerenderContents(
           url, alias_urls, FINAL_STATUS_USED);
   EXPECT_TRUE(AddSimplePrerender(url));
   EXPECT_TRUE(AddSimplePrerender(url));
   EXPECT_TRUE(AddSimplePrerender(alias_url1));
   EXPECT_TRUE(AddSimplePrerender(alias_url2));
   ASSERT_EQ(prerender_contents, prerender_manager()->FindAndUseEntry(url));
 }
 
 // Ensure that we ignore prerender requests within the rate limit.
 TEST_F(PrerenderTest, RateLimitInWindowTest) {

[mmenke, 2012/12/28 18:25:33]

Test comment and name no longer make any sense.  Suggest deleting it, since it
doesn't seem to be testing anything MaxConcurrencyTest doesn't test for.

[gavinp, 2012/12/28 19:53:46]

Done.

+  prerender_manager()->set_rate_limit_enabled(true);
+  SetConcurrency(2);
   GURL url("http://www.google.com/");
   DummyPrerenderContents* prerender_contents =
       prerender_manager()->CreateNextPrerenderContents(
           url,
           FINAL_STATUS_MANAGER_SHUTDOWN);
   DummyPrerenderContents* null = NULL;
   EXPECT_TRUE(AddSimplePrerender(url));
   EXPECT_EQ(null, prerender_manager()->next_prerender_contents());
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
 
-  prerender_manager()->set_rate_limit_enabled(true);
   prerender_manager()->AdvanceTimeTicks(TimeDelta::FromMilliseconds(1));
 
   GURL url1("http://news.google.com/");
   prerender_manager()->CreateNextPrerenderContents(
-      url,
+      url1,
       FINAL_STATUS_MANAGER_SHUTDOWN);
   EXPECT_FALSE(AddSimplePrerender(url1));
   prerender_manager()->set_rate_limit_enabled(false);
 }
 
 // Ensure that we don't ignore prerender requests outside the rate limit.
 TEST_F(PrerenderTest, RateLimitOutsideWindowTest) {

[mmenke, 2012/12/28 18:25:33]

This is now identical to the above test, so should be deleted.

-  // Setting concurrency to one lets us force eviction by adding only one more
-  // prerender.
-  prerender_manager()->mutable_config().max_concurrency = 1;
+  SetConcurrency(2);
 
   GURL url("http://www.google.com/");
   DummyPrerenderContents* prerender_contents =
       prerender_manager()->CreateNextPrerenderContents(
           url,
-          FINAL_STATUS_EVICTED);
+          FINAL_STATUS_MANAGER_SHUTDOWN);

[mmenke, 2012/12/28 18:25:33]

If we wanted to test the same thing as before, this should be TIMED_OUT, but
that doesn't really get us anything...  Closesy analog would be to modify
ExpireTest (Or make a modified copy of it) to have a pending prerender when the
first prerender times out.

Not sure if we absolutely need such a test, but I think it would be a reasonable
test to have, and seems to fit with the 3 abandon/timeout tests at the bottom
(None of which currently start the pending prerender).

[gavinp, 2012/12/28 19:53:46]

I just tossed it.

   DummyPrerenderContents* null = NULL;
   EXPECT_TRUE(AddSimplePrerender(url));
   EXPECT_EQ(null, prerender_manager()->next_prerender_contents());
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
 
   prerender_manager()->set_rate_limit_enabled(true);
   prerender_manager()->AdvanceTimeTicks(
       TimeDelta::FromMilliseconds(
           UnitTestPrerenderManager::kMinTimeBetweenPrerendersMs + 500));
 
@@ skipping 122 matching lines @@
 }
 
 // Tests that prerendering is cancelled when the source render view does not
 // exist.  On failure, the DCHECK in CreatePrerenderContents() above should be
 // triggered.
 TEST_F(PrerenderTest, SourceRenderViewClosed) {
   GURL url("http://www.google.com/");
   prerender_manager()->CreateNextPrerenderContents(
       url,
       FINAL_STATUS_MANAGER_SHUTDOWN);
-  EXPECT_FALSE(prerender_link_manager()->OnAddPrerender(
-      100, GetNextPrerenderID(), url,
-      Referrer(), kSize, 200));
+  prerender_link_manager()->OnAddPrerender(100, GetNextPrerenderID(), url,
+                                           Referrer(), kSize, 200);
+  EXPECT_FALSE(PrerenderHasHandle(100, last_prerender_id()));
 }
 
 // Tests that prerendering is cancelled when we launch a second prerender of
 // the same target within a short time interval.
 TEST_F(PrerenderTest, RecentlyVisited) {
   GURL url("http://www.google.com/");
 
   prerender_manager()->RecordNavigation(url);
 
   DummyPrerenderContents* prerender_contents =
@@ skipping 254 matching lines @@
 TEST_F(PrerenderTest, LinkManagerCancelTwice) {
   EXPECT_TRUE(IsEmptyPrerenderLinkManager());
   GURL url("http://www.myexample.com");
   DummyPrerenderContents* prerender_contents =
       prerender_manager()->CreateNextPrerenderContents(
           url, FINAL_STATUS_CANCELLED);
 
   EXPECT_TRUE(AddSimplePrerender(url));
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
   EXPECT_FALSE(prerender_contents->prerendering_has_been_cancelled());
-  ASSERT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
+  EXPECT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
   prerender_link_manager()->OnCancelPrerender(kDefaultChildId,
                                               last_prerender_id());
 
   EXPECT_TRUE(IsEmptyPrerenderLinkManager());
   EXPECT_TRUE(prerender_contents->prerendering_has_been_cancelled());
   DummyPrerenderContents* null = NULL;
   ASSERT_EQ(null, prerender_manager()->FindEntry(url));
   prerender_link_manager()->OnCancelPrerender(kDefaultChildId,
                                               last_prerender_id());
 }
 
 TEST_F(PrerenderTest, LinkManagerAddTwiceCancelTwice) {
+  SetConcurrency(2);
   EXPECT_TRUE(IsEmptyPrerenderLinkManager());
   GURL url("http://www.myexample.com");
   DummyPrerenderContents* prerender_contents =
       prerender_manager()->CreateNextPrerenderContents(
           url, FINAL_STATUS_CANCELLED);
 
   EXPECT_TRUE(AddSimplePrerender(url));
 
   const int first_prerender_id = last_prerender_id();
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
   EXPECT_FALSE(prerender_contents->prerendering_has_been_cancelled());
-  ASSERT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
+  EXPECT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
   EXPECT_TRUE(AddSimplePrerender(url));
 
   const int second_prerender_id = last_prerender_id();
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
   EXPECT_FALSE(prerender_contents->prerendering_has_been_cancelled());
-  ASSERT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
+  EXPECT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
   prerender_link_manager()->OnCancelPrerender(kDefaultChildId,
                                               first_prerender_id);
 
   EXPECT_FALSE(prerender_contents->prerendering_has_been_cancelled());
-  ASSERT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
+  EXPECT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
   prerender_link_manager()->OnCancelPrerender(kDefaultChildId,
                                               second_prerender_id);
 
   EXPECT_TRUE(IsEmptyPrerenderLinkManager());
   EXPECT_TRUE(prerender_contents->prerendering_has_been_cancelled());
   DummyPrerenderContents* null = NULL;
   ASSERT_EQ(null, prerender_manager()->FindEntry(url));
 }
 
 TEST_F(PrerenderTest, LinkManagerAddTwiceCancelTwiceThenAbandonTwice) {
+  SetConcurrency(2);
   EXPECT_TRUE(IsEmptyPrerenderLinkManager());
   GURL url("http://www.myexample.com");
   DummyPrerenderContents* prerender_contents =
       prerender_manager()->CreateNextPrerenderContents(
           url, FINAL_STATUS_CANCELLED);
 
   EXPECT_TRUE(AddSimplePrerender(url));
 
   const int first_prerender_id = last_prerender_id();
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
@@ skipping 24 matching lines @@
                                                second_prerender_id);
 
   EXPECT_TRUE(IsEmptyPrerenderLinkManager());
   EXPECT_TRUE(prerender_contents->prerendering_has_been_cancelled());
   DummyPrerenderContents* null = NULL;
   ASSERT_EQ(null, prerender_manager()->FindEntry(url));
 }
 
 // TODO(gavinp): Update this test after abandon has an effect on Prerenders,
 // like shortening the timeouts.
 TEST_F(PrerenderTest, LinkManagerAddTwiceAbandonTwice) {

[mmenke, 2012/12/28 18:25:33]

nit:  May want to rename to LinkManagerAddTwiceAbandonTwiceUseTwice while you're
here.

[gavinp, 2012/12/28 19:53:46]

Done.

+  SetConcurrency(2);
   EXPECT_TRUE(IsEmptyPrerenderLinkManager());
   GURL url("http://www.myexample.com");
   DummyPrerenderContents* prerender_contents =
       prerender_manager()->CreateNextPrerenderContents(
           url, FINAL_STATUS_USED);
 
   EXPECT_TRUE(AddSimplePrerender(url));
 
   const int first_prerender_id = last_prerender_id();
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
   EXPECT_FALSE(prerender_contents->prerendering_has_been_cancelled());
   ASSERT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
   EXPECT_TRUE(AddSimplePrerender(url));
 
   const int second_prerender_id = last_prerender_id();
   EXPECT_TRUE(prerender_contents->prerendering_has_started());
   EXPECT_FALSE(prerender_contents->prerendering_has_been_cancelled());
   ASSERT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
   prerender_link_manager()->OnAbandonPrerender(kDefaultChildId,
                                                first_prerender_id);
 
   EXPECT_FALSE(prerender_contents->prerendering_has_been_cancelled());
   ASSERT_EQ(prerender_contents, prerender_manager()->FindEntry(url));
   prerender_link_manager()->OnAbandonPrerender(kDefaultChildId,
                                                second_prerender_id);
 
   EXPECT_FALSE(prerender_contents->prerendering_has_been_cancelled());
   ASSERT_EQ(prerender_contents, prerender_manager()->FindAndUseEntry(url));
+  EXPECT_TRUE(IsEmptyPrerenderLinkManager());
 }
 
 // TODO(gavinp): After abandon shortens the expire time on a Prerender,
 // add a series of tests testing advancing the time by either the abandon
 // or normal expire, and verifying the expected behaviour with groups
 // of links.
 TEST_F(PrerenderTest, LinkManagerExpireThenCancel) {
   EXPECT_TRUE(IsEmptyPrerenderLinkManager());
   GURL url("http://www.myexample.com");
   DummyPrerenderContents* prerender_contents =
@@ skipping 83 matching lines @@
 
   prerender_manager()->AdvanceTimeTicks(
       prerender_manager()->config().abandon_time_to_live +
       TimeDelta::FromSeconds(1));
 
   DummyPrerenderContents* null = NULL;
   EXPECT_EQ(null, prerender_manager()->FindEntry(url));
   EXPECT_TRUE(IsEmptyPrerenderLinkManager());
 }
 
+// Creates two prerenders, one of which should be blocked by the
+// max_link_concurrency; abandons both of them and waits to make sure both
+// are cleared from the PrerenderLinkManager.
+TEST_F(PrerenderTest, LinkManagerClearOnAbandon) {

[mmenke, 2012/12/28 18:25:33]

I think "LinkManagerAbandonPendingPrerender" (Slowly redefining "pending" - see
note below) would be clearer.  If you prefer not to use "pending", then maybe
"LinkManagerAbandonInactivePrerender"?

[mmenke, 2012/12/28 18:25:33]

You're depending on max_link_concurrency being 1 in this test.  Suggest
explicitly setting it in tests that depend on it.  Suspect at some point we'll
want to be able to have multiple prerenders active from a single site.

[gavinp, 2012/12/28 19:53:46]

I set the concurrency in the test now. I went with "Inactive", for reasons why
see my comment on line 1312 below.

+  ASSERT_LT(prerender_manager()->config().abandon_time_to_live,
+            prerender_manager()->config().time_to_live);
+  GURL first_url("http://www.myexample.com");
+  DummyPrerenderContents* prerender_contents =
+      prerender_manager()->CreateNextPrerenderContents(
+          first_url, FINAL_STATUS_TIMED_OUT);
+  EXPECT_TRUE(AddSimplePrerender(first_url));
+  const int first_prerender_id = last_prerender_id();
+
+  GURL second_url("http://www.neverlaunched.com");
+  EXPECT_FALSE(AddSimplePrerender(second_url));
+  const int second_prerender_id = last_prerender_id();
+
+  EXPECT_FALSE(IsEmptyPrerenderLinkManager());
+
+  DummyPrerenderContents* null = NULL;
+  EXPECT_EQ(prerender_contents, prerender_manager()->FindEntry(first_url));
+  EXPECT_EQ(null, prerender_manager()->FindEntry(second_url));
+
+  prerender_link_manager()->OnAbandonPrerender(kDefaultChildId,
+                                               first_prerender_id);
+  prerender_link_manager()->OnAbandonPrerender(kDefaultChildId,
+                                               second_prerender_id);
+
+  prerender_manager()->AdvanceTimeTicks(
+      prerender_manager()->config().abandon_time_to_live +
+      TimeDelta::FromSeconds(1));
+  EXPECT_EQ(null, prerender_manager()->FindEntry(first_url));
+  EXPECT_EQ(null, prerender_manager()->FindEntry(second_url));
+  EXPECT_TRUE(IsEmptyPrerenderLinkManager());
+}
+
+// Creates two prerenders, one of which should be blocked by the
+// max_link_concurrency; abandons both of them and waits to make sure both
+// are cleared from the PrerenderLinkManager.

[mmenke, 2012/12/28 18:25:33]

Description should mention the second prerender is launched by the first.

[gavinp, 2012/12/28 19:53:46]

Done.

+TEST_F(PrerenderTest, LinkManagerClearOnPendingAbandon) {

[mmenke, 2012/12/28 18:25:33]

Suggest renaming the test.  Now that PrerenderLinkManager has prerenders that
haven't yet started, but may be started in the future, we have another type of
prerender that is "pending".

I think using "pending" solely to describe prerenders that haven't yet started
due to being prerenders that a prerendered page tried to start is confusing, and
is terminology we should move away from.

Suggested name:  LinkManagerAbandonPrerenderWithChildPrerender (Or
"WithLinkRelPrerender")

[gavinp, 2012/12/28 19:53:46]

I went with Inactive on the rename request on the earlier test, and kept Pending
here with the overly careful distinction that you are complaining about. I
concur about the name distinction being confusing, but I think the better path
forward is to move pending prerenders out of the prerender manager into the
prerender link manager. The only reason that wasn't in this CL is that it's not
small, and seemed separate enough not to include here.

[mmenke, 2012/12/28 20:15:13]

SGTM.  Still think this test needs renaming, though - Tests can be named after
the activities they simulate, or what the results of those activities are that
they test for.  For the most part, tests in this file are named for what they
simulate, so think the "Clear" in the test name is confusing.

+  ASSERT_LT(prerender_manager()->config().abandon_time_to_live,
+            prerender_manager()->config().time_to_live);
+  GURL first_url("http://www.myexample.com");
+  DummyPrerenderContents* prerender_contents =
+      prerender_manager()->CreateNextPrerenderContents(
+          first_url, FINAL_STATUS_TIMED_OUT);
+  EXPECT_TRUE(AddSimplePrerender(first_url));
+  const int first_prerender_id = last_prerender_id();
+
+  int child_id;
+  int route_id;
+  ASSERT_TRUE(prerender_contents->GetChildId(&child_id));
+  ASSERT_TRUE(prerender_contents->GetRouteId(&route_id));
+
+  GURL pending_url("http://www.neverlaunched.com");
+  prerender_link_manager()->OnAddPrerender(child_id,
+                                           GetNextPrerenderID(),
+                                           pending_url, content::Referrer(),
+                                           kSize, route_id);
+  const int second_prerender_id = last_prerender_id();
+
+  EXPECT_FALSE(IsEmptyPrerenderLinkManager());
+
+  DummyPrerenderContents* null = NULL;
+  EXPECT_EQ(prerender_contents, prerender_manager()->FindEntry(first_url));
+  EXPECT_EQ(null, prerender_manager()->FindEntry(pending_url));
+
+  prerender_link_manager()->OnAbandonPrerender(kDefaultChildId,
+                                               first_prerender_id);
+  prerender_link_manager()->OnAbandonPrerender(kDefaultChildId,
+                                               second_prerender_id);
+
+  prerender_manager()->AdvanceTimeTicks(
+      prerender_manager()->config().abandon_time_to_live +
+      TimeDelta::FromSeconds(1));
+  EXPECT_EQ(null, prerender_manager()->FindEntry(first_url));
+  EXPECT_EQ(null, prerender_manager()->FindEntry(pending_url));
+  EXPECT_TRUE(IsEmptyPrerenderLinkManager());
+}
+
+// Creates two prerenders, one of which should be blocked by the
+// max_link_concurrency; uses one after the max wait to launch, and
+// ensures the second prerender does not start.
+TEST_F(PrerenderTest, LinkManagerWaitToLaunch) {

[mmenke, 2012/12/28 18:25:33]

Suggest calling this LinkManagerWaitToLaunchNotLaunched or somesuch -
"WaitToLaunch" kinda has the implication of actually being launched after
waiting to do so.

[gavinp, 2012/12/28 19:53:46]

Done.

+  ASSERT_LT(prerender_manager()->config().max_wait_to_launch,
+            prerender_manager()->config().time_to_live);
+  GURL first_url("http://www.myexample.com");
+  DummyPrerenderContents* prerender_contents =
+      prerender_manager()->CreateNextPrerenderContents(
+          first_url, FINAL_STATUS_TIMED_OUT);
+  EXPECT_TRUE(AddSimplePrerender(first_url));
+
+  GURL second_url("http://www.neverlaunched.com");
+  EXPECT_FALSE(AddSimplePrerender(second_url));
+
+  EXPECT_FALSE(IsEmptyPrerenderLinkManager());
+
+  DummyPrerenderContents* null = NULL;
+  EXPECT_EQ(prerender_contents, prerender_manager()->FindEntry(first_url));
+  EXPECT_EQ(null, prerender_manager()->FindEntry(second_url));
+
+  prerender_manager()->AdvanceTimeTicks(
+      prerender_manager()->config().max_wait_to_launch +
+      TimeDelta::FromSeconds(1));
+  EXPECT_EQ(prerender_contents, prerender_manager()->FindEntry(first_url));
+  EXPECT_EQ(null, prerender_manager()->FindEntry(second_url));
+
+  EXPECT_EQ(prerender_contents,
+            prerender_manager()->FindAndUseEntry(first_url));
+
+  EXPECT_EQ(null, prerender_manager()->FindEntry(first_url));
+  EXPECT_EQ(null, prerender_manager()->FindEntry(second_url));
+  EXPECT_TRUE(IsEmptyPrerenderLinkManager());
+}
+
 }  // namespace prerender