Cleans up notifications for the NavigationController. There were several

notifications before and some of them were very unclear and misused (STATE_CHANGED). This one, and PRUNED were called unnecessarily in some cases as well.

I replaced STATE_CHANGED and INDEX_CHANGED with ENTRY_COMMITTED which is more clear and covers (I think!) all the cases that the callers care about.

I added a simple notification testing helper class, and used in the navigation controller unit tests to make sure we get the proper notifications. I had to change NotificationSource/Details to have a = and copy constructor so I can track them easily in my helper. I don't see why this would be bad.

BUG=1325636,1321376,1325779

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@1039 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 131 insertions, 5 deletions

diff --git a/chrome/browser/navigation_controller_unittest.cc b/chrome/browser/navigation_controller_unittest.cc
index 9176df6..d815a54 100644
--- a/chrome/browser/navigation_controller_unittest.cc
+++ b/chrome/browser/navigation_controller_unittest.cc
@@ -39,7 +39,9 @@
 #include "chrome/browser/tab_contents.h"
 #include "chrome/browser/tab_contents_delegate.h"
 #include "chrome/browser/tab_contents_factory.h"
+#include "chrome/common/notification_types.h"
 #include "chrome/common/stl_util-inl.h"
+#include "chrome/test/test_notification_tracker.h"
 #include "chrome/test/testing_profile.h"
 #include "net/base/net_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -290,6 +292,16 @@ class NavigationControllerHistoryTest : public NavigationControllerTest {
   std::wstring profile_path_;
 };
 
+void RegisterForAllNavNotifications(TestNotificationTracker* tracker,
+                                    NavigationController* controller) {
+  tracker->ListenFor(NOTIFY_NAV_ENTRY_COMMITTED,
+                     Source<NavigationController>(controller));
+  tracker->ListenFor(NOTIFY_NAV_LIST_PRUNED,
+                     Source<NavigationController>(controller));
+  tracker->ListenFor(NOTIFY_NAV_ENTRY_CHANGED,
+                     Source<NavigationController>(controller));
+}
+
 }  // namespace
 
 TEST_F(NavigationControllerTest, Defaults) {
@@ -306,12 +318,18 @@ TEST_F(NavigationControllerTest, Defaults) {
 }
 
 TEST_F(NavigationControllerTest, LoadURL) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
   const GURL url2("test1:foo2");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
+  // Creating a pending notification should not have issued any of the
+  // notifications we're listening for.
+  EXPECT_EQ(0, notifications.size());
 
-  // the load should now be pending
+  // The load should now be pending.
   EXPECT_TRUE(contents->pending_entry());
   EXPECT_EQ(contents->controller()->GetEntryCount(), 0);
   EXPECT_EQ(contents->controller()->GetLastCommittedEntryIndex(), -1);
@@ -322,9 +340,14 @@ TEST_F(NavigationControllerTest, LoadURL) {
   EXPECT_FALSE(contents->controller()->CanGoForward());
   EXPECT_EQ(contents->GetMaxPageID(), -1);
 
+  // We should have gotten no notifications from the preceeding checks.
+  EXPECT_EQ(0, notifications.size());
+
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
-  // the load should now be committed
+  // The load should now be committed.
   EXPECT_EQ(contents->controller()->GetEntryCount(), 1);
   EXPECT_EQ(contents->controller()->GetLastCommittedEntryIndex(), 0);
   EXPECT_EQ(contents->controller()->GetPendingEntryIndex(), -1);
@@ -334,10 +357,10 @@ TEST_F(NavigationControllerTest, LoadURL) {
   EXPECT_FALSE(contents->controller()->CanGoForward());
   EXPECT_EQ(contents->GetMaxPageID(), 0);
 
-  // load another...
+  // Load another...
   contents->controller()->LoadURL(url2, PageTransition::TYPED);
 
-  // the load should now be pending
+  // The load should now be pending.
   EXPECT_TRUE(contents->pending_entry());
   EXPECT_EQ(contents->controller()->GetEntryCount(), 1);
   EXPECT_EQ(contents->controller()->GetLastCommittedEntryIndex(), 0);
@@ -350,8 +373,10 @@ TEST_F(NavigationControllerTest, LoadURL) {
   EXPECT_EQ(contents->GetMaxPageID(), 0);
 
   contents->CompleteNavigation(1);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
-  // the load should now be committed
+  // The load should now be committed.
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
   EXPECT_EQ(contents->controller()->GetLastCommittedEntryIndex(), 1);
   EXPECT_EQ(contents->controller()->GetPendingEntryIndex(), -1);
@@ -365,13 +390,23 @@ TEST_F(NavigationControllerTest, LoadURL) {
 // Tests what happens when the same page is loaded again.  Should not create a
 // new session history entry.
 TEST_F(NavigationControllerTest, LoadURL_SamePage) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
+  EXPECT_EQ(0, notifications.size());
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
+
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
+  EXPECT_EQ(0, notifications.size());
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   // should not have produced a new session history entry
   EXPECT_EQ(contents->controller()->GetEntryCount(), 1);
@@ -384,14 +419,21 @@ TEST_F(NavigationControllerTest, LoadURL_SamePage) {
 }
 
 TEST_F(NavigationControllerTest, LoadURL_Discarded) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
   const GURL url2("test1:foo2");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
+  EXPECT_EQ(0, notifications.size());
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->LoadURL(url2, PageTransition::TYPED);
   contents->controller()->DiscardPendingEntry();
+  EXPECT_EQ(0, notifications.size());
 
   // should not have produced a new session history entry
   EXPECT_EQ(contents->controller()->GetEntryCount(), 1);
@@ -404,12 +446,19 @@ TEST_F(NavigationControllerTest, LoadURL_Discarded) {
 }
 
 TEST_F(NavigationControllerTest, Reload) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
+  EXPECT_EQ(0, notifications.size());
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->Reload();
+  EXPECT_EQ(0, notifications.size());
 
   // the reload is pending...
   EXPECT_EQ(contents->controller()->GetEntryCount(), 1);
@@ -421,6 +470,8 @@ TEST_F(NavigationControllerTest, Reload) {
   EXPECT_FALSE(contents->controller()->CanGoForward());
 
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   // now the reload is committed...
   EXPECT_EQ(contents->controller()->GetEntryCount(), 1);
@@ -434,17 +485,25 @@ TEST_F(NavigationControllerTest, Reload) {
 
 // Tests what happens when a reload navigation produces a new page.
 TEST_F(NavigationControllerTest, Reload_GeneratesNewPage) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
   const GURL url2("test1:foo2");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->Reload();
+  EXPECT_EQ(0, notifications.size());
 
   contents->pending_entry()->SetURL(url2);
   contents->pending_entry()->SetTransitionType(PageTransition::LINK);
   contents->CompleteNavigation(1);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   // now the reload is committed...
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
@@ -458,16 +517,24 @@ TEST_F(NavigationControllerTest, Reload_GeneratesNewPage) {
 
 // Tests what happens when we navigate back successfully
 TEST_F(NavigationControllerTest, Back) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
   const GURL url2("test1:foo2");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->LoadURL(url2, PageTransition::TYPED);
   contents->CompleteNavigation(1);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->GoBack();
+  EXPECT_EQ(0, notifications.size());
 
   // should now have a pending navigation to go back...
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
@@ -479,6 +546,8 @@ TEST_F(NavigationControllerTest, Back) {
   EXPECT_TRUE(contents->controller()->CanGoForward());
 
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   // the back navigation completed successfully
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
@@ -492,17 +561,25 @@ TEST_F(NavigationControllerTest, Back) {
 
 // Tests what happens when a back navigation produces a new page.
 TEST_F(NavigationControllerTest, Back_GeneratesNewPage) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
   const GURL url2("test1:foo2");
   const GURL url3("test1:foo3");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->LoadURL(url2, PageTransition::TYPED);
   contents->CompleteNavigation(1);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->GoBack();
+  EXPECT_EQ(0, notifications.size());
 
   // should now have a pending navigation to go back...
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
@@ -516,6 +593,8 @@ TEST_F(NavigationControllerTest, Back_GeneratesNewPage) {
   contents->pending_entry()->SetURL(url3);
   contents->pending_entry()->SetTransitionType(PageTransition::LINK);
   contents->CompleteNavigation(2);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   // the back navigation resulted in a completely new navigation.
   // TODO(darin): perhaps this behavior will be confusing to users?
@@ -530,17 +609,26 @@ TEST_F(NavigationControllerTest, Back_GeneratesNewPage) {
 
 // Tests what happens when we navigate forward successfully
 TEST_F(NavigationControllerTest, Forward) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
   const GURL url2("test1:foo2");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->LoadURL(url2, PageTransition::TYPED);
   contents->CompleteNavigation(1);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->GoBack();
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->GoForward();
 
@@ -554,6 +642,8 @@ TEST_F(NavigationControllerTest, Forward) {
   EXPECT_FALSE(contents->controller()->CanGoForward());
 
   contents->CompleteNavigation(1);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   // the forward navigation completed successfully
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
@@ -567,20 +657,30 @@ TEST_F(NavigationControllerTest, Forward) {
 
 // Tests what happens when a forward navigation produces a new page.
 TEST_F(NavigationControllerTest, Forward_GeneratesNewPage) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
   const GURL url2("test1:foo2");
   const GURL url3("test1:foo3");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->LoadURL(url2, PageTransition::TYPED);
   contents->CompleteNavigation(1);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->GoBack();
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->controller()->GoForward();
+  EXPECT_EQ(0, notifications.size());
 
   // should now have a pending navigation to go forward...
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
@@ -594,6 +694,9 @@ TEST_F(NavigationControllerTest, Forward_GeneratesNewPage) {
   contents->pending_entry()->SetURL(url3);
   contents->pending_entry()->SetTransitionType(PageTransition::LINK);
   contents->CompleteNavigation(2);
+  EXPECT_TRUE(notifications.Check3AndReset(NOTIFY_NAV_LIST_PRUNED,
+                                           NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
   EXPECT_EQ(contents->controller()->GetLastCommittedEntryIndex(), 1);
@@ -605,17 +708,24 @@ TEST_F(NavigationControllerTest, Forward_GeneratesNewPage) {
 }
 
 TEST_F(NavigationControllerTest, LinkClick) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo1");
   const GURL url2("test1:foo2");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   contents->set_pending_entry(new NavigationEntry(kTestContentsType1, NULL, 0,
                                                   url2,
                                                   std::wstring(),
                                                   PageTransition::LINK));
   contents->CompleteNavigation(1);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   // should not have produced a new session history entry
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
@@ -628,11 +738,16 @@ TEST_F(NavigationControllerTest, LinkClick) {
 }
 
 TEST_F(NavigationControllerTest, SwitchTypes) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo");
   const GURL url2("test2:foo");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   TestContents* initial_contents = contents;
 
@@ -642,6 +757,8 @@ TEST_F(NavigationControllerTest, SwitchTypes) {
   ASSERT_TRUE(initial_contents != contents);
 
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   // A second navigation entry should have been committed even though the
   // PageIDs are the same.  PageIDs are scoped to the tab contents type.
@@ -657,6 +774,8 @@ TEST_F(NavigationControllerTest, SwitchTypes) {
   contents->controller()->GoBack();
   ASSERT_TRUE(initial_contents == contents);  // switched again!
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   EXPECT_EQ(contents->controller()->GetEntryCount(), 2);
   EXPECT_EQ(contents->controller()->GetLastCommittedEntryIndex(), 0);
@@ -673,20 +792,27 @@ TEST_F(NavigationControllerTest, SwitchTypes) {
 // Tests what happens when we begin to navigate to a new contents type, but
 // then that navigation gets discarded instead.
 TEST_F(NavigationControllerTest, SwitchTypes_Discard) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, contents->controller());
+
   const GURL url1("test1:foo");
   const GURL url2("test2:foo");
 
   contents->controller()->LoadURL(url1, PageTransition::TYPED);
   contents->CompleteNavigation(0);
+  EXPECT_TRUE(notifications.Check2AndReset(NOTIFY_NAV_ENTRY_COMMITTED,
+                                           NOTIFY_NAV_ENTRY_CHANGED));
 
   TestContents* initial_contents = contents;
 
   contents->controller()->LoadURL(url2, PageTransition::TYPED);
+  EXPECT_EQ(0, notifications.size());
 
   // The tab contents should have been replaced
   ASSERT_TRUE(initial_contents != contents);
 
   contents->controller()->DiscardPendingEntry();
+  EXPECT_EQ(0, notifications.size());
 
   // The tab contents should have been replaced back
   ASSERT_TRUE(initial_contents == contents);