Make PrerenderHandle an observer of PrerenderContents.

chrome/renderer/prerender/prerender_dispatcher_unittest.cc

Index: chrome/renderer/prerender/prerender_dispatcher_unittest.cc
diff --git a/chrome/renderer/prerender/prerender_dispatcher_unittest.cc b/chrome/renderer/prerender/prerender_dispatcher_unittest.cc
index f7170ba8b4c6a49c1bb7dc4fed4f8283f2ea5807..9dbca2ad12840c3636f8d92e9f050ca00f8bf5b4 100644
--- a/chrome/renderer/prerender/prerender_dispatcher_unittest.cc
+++ b/chrome/renderer/prerender/prerender_dispatcher_unittest.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/renderer/prerender/prerender_dispatcher.h"
 
+#include <map>
+#include <utility>
+
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "googleurl/src/gurl.h"
@@ -11,29 +14,52 @@
 
 namespace prerender {
 
+namespace {
+
+int g_next_prerender_id = 0;
+
+}  // namespace
+
+using WebKit::WebPrerender;
+
+// Since we can't mock out WebKit::WebPrerender in chrome, this test can't test
+// signalling to or from the WebKit side. Instead, it checks only that the
+// messages received from the browser generate consistant state in the
+// PrerenderDispatcher. Since prerenders couldn't even start or stop without the
+// WebKit signalling, we can expect PrerenderBrowserTest to provide adequate
+// coverage of this.
 class PrerenderDispatcherTest : public testing::Test {
  public:
   PrerenderDispatcherTest() {}
 
-  bool is_prerender_url(const GURL& url) const {
+  bool IsPrerenderURL(const GURL& url) const {
     return prerender_dispatcher_.IsPrerenderURL(url);
   }
 
-  const PrerenderDispatcher::PrerenderMap& urls() const {
-    return prerender_dispatcher_.prerender_urls_;
+  const std::map<int, GURL>& prerenders() const {
+    return prerender_dispatcher_.prerenders_;
+  }
+
+  int StartPrerender(const GURL& url) {
+    std::pair<std::map<int, GURL>::iterator, bool> insert_result =
+        prerender_dispatcher_.prerenders_.insert(
+            std::make_pair(g_next_prerender_id, url));
+    CHECK(insert_result.second);

[mmenke, 2012/12/12 16:54:12]

I think that the following is much clearer:

DCHECK_EQ(0u, prerender_dispatcher_.prerenders_.count(g_next_prerender_id));
prerender_dispatcher_.prerenders_[g_next_prerender_id] = url;

I don't think I've ever seen the insertion result of adding something to a map
used in Chrome.  Probably because the data type is rather complicated.

[gavinp, 2012/12/13 13:38:03]

Done. You are slowly winning me over to your crazy "use
std::map<>::operator[]()" arguments.

With respect to std::map<>::insert(), the return data type is complicated, but
it's not very high cost to allocate; if used wisely, the virtue is that you save
a traversal of the structure: you can use insert as a test for existence.

However, every comment you've made on this has been spot on and correct: I was
using the insert result only for a DCHECK, and that's bad form. In debug, an
extra search is fine if the benefit is huge simplification of code. So I'm sold.

If the insert result was being checked in the non-debug code path? Well, that's
a topic for a different review...

+
+    prerender_dispatcher_.OnPrerenderStart(g_next_prerender_id);
+    return g_next_prerender_id++;
+  }
+
+  void AddAliasToPrerender(int prerender_id, const GURL& url) {
+    prerender_dispatcher_.OnPrerenderAddAlias(prerender_id, url);
   }
 
-  void AddURL(const GURL& url) { prerender_dispatcher_.OnAddPrerenderURL(url); }
-  void RemoveURL(const GURL& url) {
-    prerender_dispatcher_.OnRemovePrerenderURL(url);
+  void StopPrerender(int prerender_id) {
+    prerender_dispatcher_.OnPrerenderStop(prerender_id);
   }
 
   int GetCountForURL(const GURL& url) const {
-    PrerenderDispatcher::PrerenderMap::const_iterator entry = urls().find(url);
-    if (entry == urls().end())
-      return 0;
-    EXPECT_GT(entry->second, 0);
-    return entry->second;
+    return prerender_dispatcher_.running_prerender_urls_.count(url);
   }
 
  private:
@@ -42,14 +68,14 @@ class PrerenderDispatcherTest : public testing::Test {
 };
 
 TEST_F(PrerenderDispatcherTest, PrerenderDispatcherEmpty) {
-  EXPECT_EQ(0U, urls().size());
+  EXPECT_TRUE(prerenders().empty());
 }
 
 TEST_F(PrerenderDispatcherTest, PrerenderDispatcherSingleAdd) {
   GURL foo_url = GURL("http://foo.com");
-  EXPECT_FALSE(is_prerender_url(foo_url));
-  AddURL(foo_url);
-  EXPECT_TRUE(is_prerender_url(foo_url));
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
+  StartPrerender(foo_url);
+  EXPECT_TRUE(IsPrerenderURL(foo_url));
   EXPECT_EQ(1, GetCountForURL(foo_url));
 }
 
@@ -57,69 +83,66 @@ TEST_F(PrerenderDispatcherTest, PrerenderDispatcherMultipleAdd) {
   GURL foo_url = GURL("http://foo.com");
   GURL bar_url = GURL("http://bar.com");
 
-  EXPECT_FALSE(is_prerender_url(foo_url));
-  EXPECT_FALSE(is_prerender_url(bar_url));
-  AddURL(foo_url);
-  EXPECT_TRUE(is_prerender_url(foo_url));
-  EXPECT_FALSE(is_prerender_url(bar_url));
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
+  EXPECT_FALSE(IsPrerenderURL(bar_url));
+  int foo_id = StartPrerender(foo_url);
+  EXPECT_TRUE(IsPrerenderURL(foo_url));
+  EXPECT_FALSE(IsPrerenderURL(bar_url));
 
-  AddURL(foo_url);
-  EXPECT_TRUE(is_prerender_url(foo_url));
-  EXPECT_FALSE(is_prerender_url(bar_url));
+  AddAliasToPrerender(foo_id, foo_url);
+  EXPECT_TRUE(IsPrerenderURL(foo_url));
+  EXPECT_FALSE(IsPrerenderURL(bar_url));
   EXPECT_EQ(2, GetCountForURL(foo_url));
 
-  AddURL(bar_url);
-  EXPECT_TRUE(is_prerender_url(foo_url));
-  EXPECT_TRUE(is_prerender_url(bar_url));
+  StartPrerender(bar_url);
+  EXPECT_TRUE(IsPrerenderURL(foo_url));
+  EXPECT_TRUE(IsPrerenderURL(bar_url));
   EXPECT_EQ(2, GetCountForURL(foo_url));
   EXPECT_EQ(1, GetCountForURL(bar_url));
 }
 
 TEST_F(PrerenderDispatcherTest, PrerenderDispatcherSingleRemove) {
   GURL foo_url = GURL("http://foo.com");
-  EXPECT_FALSE(is_prerender_url(foo_url));
-  AddURL(foo_url);
-  EXPECT_TRUE(is_prerender_url(foo_url));
-  RemoveURL(foo_url);
-  EXPECT_FALSE(is_prerender_url(foo_url));
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
+  int foo_id = StartPrerender(foo_url);
+  EXPECT_TRUE(IsPrerenderURL(foo_url));
+  StopPrerender(foo_id);
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
   EXPECT_EQ(0, GetCountForURL(foo_url));
 }
 
 TEST_F(PrerenderDispatcherTest, PrerenderDispatcherMultipleRemove) {
   GURL foo_url = GURL("http://foo.com");
-  EXPECT_FALSE(is_prerender_url(foo_url));
-  AddURL(foo_url);
-  EXPECT_TRUE(is_prerender_url(foo_url));
-  AddURL(foo_url);
-  EXPECT_TRUE(is_prerender_url(foo_url));
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
+  int foo_id = StartPrerender(foo_url);
+  EXPECT_TRUE(IsPrerenderURL(foo_url));
+  AddAliasToPrerender(foo_id, foo_url);
+  EXPECT_TRUE(IsPrerenderURL(foo_url));
   EXPECT_EQ(2, GetCountForURL(foo_url));
 

[mmenke, 2012/12/12 16:54:12]

May want to add a second url as well.

[gavinp, 2012/12/13 13:38:03]

Throwing out the test, as we seem to be able to DCHECK on non-duplicate Stop
events.

-  RemoveURL(foo_url);
-  EXPECT_TRUE(is_prerender_url(foo_url));
-  EXPECT_EQ(1, GetCountForURL(foo_url));
-
-  RemoveURL(foo_url);
-  EXPECT_FALSE(is_prerender_url(foo_url));
+  StopPrerender(foo_id);
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
   EXPECT_EQ(0, GetCountForURL(foo_url));
 }
 
 TEST_F(PrerenderDispatcherTest, PrerenderDispatcherRemoveWithoutAdd) {

[mmenke, 2012/12/12 16:54:12]

Does this ever happen?

[gavinp, 2012/12/13 13:38:03]

I don't think so.

   GURL foo_url = GURL("http://foo.com");
-  EXPECT_FALSE(is_prerender_url(foo_url));
-  RemoveURL(foo_url);
-  EXPECT_FALSE(is_prerender_url(foo_url));
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
+  int crazy_prerender_id = 13;
+  StopPrerender(crazy_prerender_id);
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
   EXPECT_EQ(0, GetCountForURL(foo_url));
 }
 
 TEST_F(PrerenderDispatcherTest, PrerenderDispatcherRemoveTooMany) {

[mmenke, 2012/12/12 16:54:12]

Does this ever happen?

[gavinp, 2012/12/13 13:38:03]

I don't think so.

   GURL foo_url = GURL("http://foo.com");
-  EXPECT_FALSE(is_prerender_url(foo_url));
-  AddURL(foo_url);
-  EXPECT_TRUE(is_prerender_url(foo_url));
-  RemoveURL(foo_url);
-  EXPECT_FALSE(is_prerender_url(foo_url));
-  RemoveURL(foo_url);
-  EXPECT_FALSE(is_prerender_url(foo_url));
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
+  int foo_id = StartPrerender(foo_url);
+  EXPECT_TRUE(IsPrerenderURL(foo_url));
+  StopPrerender(foo_id);
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
+  StopPrerender(foo_id);
+  EXPECT_FALSE(IsPrerenderURL(foo_url));
   EXPECT_EQ(0, GetCountForURL(foo_url));
 }