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

11 files changed, 170 insertions, 62 deletions

diff --git a/chrome/browser/alternate_nav_url_fetcher.cc b/chrome/browser/alternate_nav_url_fetcher.cc
index a791106..e86105b 100644
--- a/chrome/browser/alternate_nav_url_fetcher.cc
+++ b/chrome/browser/alternate_nav_url_fetcher.cc
@@ -41,14 +41,14 @@ AlternateNavURLFetcher::AlternateNavURLFetcher(
     state_(NOT_STARTED),
     navigated_to_entry_(false) {
   NotificationService::current()->AddObserver(this,
-      NOTIFY_NAV_STATE_CHANGED, NotificationService::AllSources());
+      NOTIFY_NAV_ENTRY_PENDING, NotificationService::AllSources());
 }
 
 AlternateNavURLFetcher::~AlternateNavURLFetcher() {
   if (state_ == NOT_STARTED) {
     // Never caught the NavigationController notification.
     NotificationService::current()->RemoveObserver(this,
-        NOTIFY_NAV_STATE_CHANGED, NotificationService::AllSources());
+        NOTIFY_NAV_ENTRY_PENDING, NotificationService::AllSources());
   }  // Otherwise, Observe() removed the observer already.
 }
 
@@ -69,7 +69,7 @@ void AlternateNavURLFetcher::Observe(NotificationType type,
   controller_->SetAlternateNavURLFetcher(this);
 
   NotificationService::current()->RemoveObserver(this,
-      NOTIFY_NAV_STATE_CHANGED, NotificationService::AllSources());
+      NOTIFY_NAV_ENTRY_PENDING, NotificationService::AllSources());
 
   DCHECK_EQ(NOT_STARTED, state_);
   state_ = IN_PROGRESS;
diff --git a/chrome/browser/jsmessage_box_handler.cc b/chrome/browser/jsmessage_box_handler.cc
index 09b9757..549981a 100644
--- a/chrome/browser/jsmessage_box_handler.cc
+++ b/chrome/browser/jsmessage_box_handler.cc
@@ -63,7 +63,7 @@ void JavascriptMessageBoxHandler::RunJavascriptMessageBox(
 
 JavascriptMessageBoxHandler::~JavascriptMessageBoxHandler() {
   NotificationService::current()->
-      RemoveObserver(this, NOTIFY_NAV_STATE_CHANGED,
+      RemoveObserver(this, NOTIFY_NAV_ENTRY_COMMITTED,
                      NotificationService::AllSources());
   NotificationService::current()->
       RemoveObserver(this, NOTIFY_TAB_CONTENTS_DESTROYED,
@@ -188,7 +188,7 @@ void JavascriptMessageBoxHandler::Observe(NotificationType type,
   if (!web_contents_)
     return;
 
-  if (type == NOTIFY_NAV_STATE_CHANGED &&
+  if (type == NOTIFY_NAV_ENTRY_COMMITTED &&
       Source<NavigationController>(source).ptr() == web_contents_->controller())
     web_contents_gone = true;
 
@@ -230,7 +230,7 @@ JavascriptMessageBoxHandler::JavascriptMessageBoxHandler(
   // Make sure we get navigation notifications so we know when our parent
   // contents will disappear or navigate to a different page.
   NotificationService::current()->
-      AddObserver(this, NOTIFY_NAV_STATE_CHANGED,
+      AddObserver(this, NOTIFY_NAV_ENTRY_COMMITTED,
                   NotificationService::AllSources());
   NotificationService::current()->
       AddObserver(this, NOTIFY_TAB_CONTENTS_DESTROYED,
diff --git a/chrome/browser/navigation_controller.cc b/chrome/browser/navigation_controller.cc
index 91b404a4..5100922 100644
--- a/chrome/browser/navigation_controller.cc
+++ b/chrome/browser/navigation_controller.cc
@@ -349,13 +349,6 @@ const SkBitmap& NavigationController::GetLazyFavIcon() const {
   }
 }
 
-void NavigationController::EntryUpdated(NavigationEntry* entry) {
-  if (entry == GetActiveEntry()) {
-    // Someone has modified our active navigation entry.
-    NotifyNavigationStateChanged();
-  }
-}
-
 void NavigationController::SetAlternateNavURLFetcher(
     AlternateNavURLFetcher* alternate_nav_url_fetcher) {
   DCHECK(!alternate_nav_url_fetcher_.get());
@@ -423,17 +416,10 @@ void NavigationController::DiscardPendingEntry() {
     DCHECK(from_contents != active_contents_);
     ScheduleTabContentsCollection(from_contents->type());
   }
-
-  // Note: this may be redundant in some cases.  we may want to optimize away
-  // the redundant notifications.
-  NotifyNavigationStateChanged();
 }
 
 void NavigationController::InsertEntry(NavigationEntry* entry) {
   NavigationControllerBase::InsertEntry(entry);
-  NotificationService::current()->Notify(NOTIFY_NAV_INDEX_CHANGED,
-                                         Source<NavigationController>(this),
-                                         NotificationService::NoDetails());
   active_contents_->NotifyDidNavigate(NAVIGATION_NEW, 0);
 }
 
@@ -473,19 +459,16 @@ void NavigationController::NavigateToPendingEntry(bool reload) {
       from_contents->delegate()->ReplaceContents(from_contents, contents);
   }
 
-  if (contents->Navigate(*pending_entry_, reload)) {
-    // Note: this is redundant if navigation completed synchronously because
-    // DidNavigateToEntry call this as well.
-    NotifyNavigationStateChanged();
-  } else {
+  if (!contents->Navigate(*pending_entry_, reload))
     DiscardPendingEntry();
-  }
 }
 
-void NavigationController::NotifyNavigationStateChanged() {
+void NavigationController::NotifyNavigationEntryCommitted() {
   // Reset the Alternate Nav URL Fetcher if we're loading some page it doesn't
   // care about.  We must do this before calling Notify() below as that may
   // result in the creation of a new fetcher.
+  //
+  // TODO(brettw) bug 1324500: this logic should be moved out of the controller!
   const NavigationEntry* const entry = GetActiveEntry();
   if (!entry ||
       (entry->unique_id() != alternate_nav_url_fetcher_entry_unique_id_)) {
@@ -500,7 +483,7 @@ void NavigationController::NotifyNavigationStateChanged() {
   active_contents_->NotifyNavigationStateChanged(
       TabContents::INVALIDATE_EVERYTHING);
 
-  NotificationService::current()->Notify(NOTIFY_NAV_STATE_CHANGED,
+  NotificationService::current()->Notify(NOTIFY_NAV_ENTRY_COMMITTED,
                                          Source<NavigationController>(this),
                                          NotificationService::NoDetails());
 }
@@ -520,12 +503,6 @@ void NavigationController::IndexOfActiveEntryChanged(
     nav_type = NAVIGATION_REPLACE;
   }
   active_contents_->NotifyDidNavigate(nav_type, relative_navigation_offset);
-  if (GetCurrentEntryIndex() != -1) {
-    NotificationService::current()->Notify(
-        NOTIFY_NAV_INDEX_CHANGED,
-        Source<NavigationController>(this),
-        NotificationService::NoDetails());
-  }
 }
 
 TabContents* NavigationController::GetTabContentsCreateIfNecessary(
diff --git a/chrome/browser/navigation_controller.h b/chrome/browser/navigation_controller.h
index 136540e..4378b04 100644
--- a/chrome/browser/navigation_controller.h
+++ b/chrome/browser/navigation_controller.h
@@ -133,9 +133,6 @@ class NavigationController : public NavigationControllerBase {
   const std::wstring& GetLazyTitle() const;
   const SkBitmap& GetLazyFavIcon() const;
 
-  // Called when |entry| has been updated outside its NavigationController.
-  void EntryUpdated(NavigationEntry* entry);
-
   void SetAlternateNavURLFetcher(
       AlternateNavURLFetcher* alternate_nav_url_fetcher);
 
@@ -200,7 +197,7 @@ class NavigationController : public NavigationControllerBase {
 
   virtual int GetMaxPageID() const;
   virtual void NavigateToPendingEntry(bool reload);
-  virtual void NotifyNavigationStateChanged();
+  virtual void NotifyNavigationEntryCommitted();
 
   // Lets the history database know navigation entries have been removed.
   virtual void NotifyPrunedEntries();
diff --git a/chrome/browser/navigation_controller_base.cc b/chrome/browser/navigation_controller_base.cc
index f2b921d..e086e82 100644
--- a/chrome/browser/navigation_controller_base.cc
+++ b/chrome/browser/navigation_controller_base.cc
@@ -33,6 +33,8 @@
 
 #include "base/logging.h"
 #include "chrome/browser/navigation_entry.h"
+#include "chrome/common/notification_service.h"
+#include "chrome/common/notification_types.h"
 #include "webkit/glue/webkit_glue.h"
 
 // The maximum number of entries that a navigation controller can store.
@@ -209,6 +211,12 @@ void NavigationControllerBase::LoadEntry(NavigationEntry* entry) {
   // TODO(pkasting): http://b/1113085 Should this use DiscardPendingEntry()?
   DiscardPendingEntryInternal();
   pending_entry_ = entry;
+  // TODO(brettw) the reinterpret cast can be removed once we combine the
+  // NavigationController and the NavigationControllerBase.
+  NotificationService::current()->Notify(
+      NOTIFY_NAV_ENTRY_PENDING,
+      Source<NavigationController>(reinterpret_cast<NavigationController*>(this)),
+      NotificationService::NoDetails());
   NavigateToPendingEntry(false);
 }
 
@@ -241,6 +249,7 @@ void NavigationControllerBase::DidNavigateToEntry(NavigationEntry* entry) {
   // active WebContents, because we have just navigated to it.
   if (entry->GetPageID() > GetMaxPageID()) {
     InsertEntry(entry);
+    NotifyNavigationEntryCommitted();
     return;
   }
 
@@ -298,7 +307,7 @@ void NavigationControllerBase::DidNavigateToEntry(NavigationEntry* entry) {
 
   delete entry;
 
-  NotifyNavigationStateChanged();
+  NotifyNavigationEntryCommitted();
 }
 
 void NavigationControllerBase::DiscardPendingEntry() {
@@ -337,12 +346,15 @@ void NavigationControllerBase::InsertEntry(NavigationEntry* entry) {
 
   // Prune any entries which are in front of the current entry.
   if (current_size > 0) {
+    bool pruned = false;
     while (last_committed_entry_index_ < (current_size - 1)) {
+      pruned = true;
       delete entries_[current_size - 1];
       entries_.pop_back();
       current_size--;
     }
-    NotifyPrunedEntries();
+    if (pruned)  // Only notify if we did prune something.
+      NotifyPrunedEntries();
   }
 
   if (entries_.size() >= max_entry_count_)
@@ -350,8 +362,6 @@ void NavigationControllerBase::InsertEntry(NavigationEntry* entry) {
 
   entries_.push_back(entry);
   last_committed_entry_index_ = static_cast<int>(entries_.size()) - 1;
-
-  NotifyNavigationStateChanged();
 }
 
 void NavigationControllerBase::RemoveLastEntry() {
@@ -370,10 +380,11 @@ void NavigationControllerBase::RemoveLastEntry() {
 
     NotifyPrunedEntries();
   }
-  NotifyNavigationStateChanged();
 }
 
 void NavigationControllerBase::RemoveEntryAtIndex(int index) {
+  // TODO(brettw) this is only called to remove the first one when we've got
+  // too many entries. It should probably be more specific for this case.
   if (index >= static_cast<int>(entries_.size()) ||
       index == pending_entry_index_ || index == last_committed_entry_index_) {
     NOTREACHED();
@@ -390,8 +401,8 @@ void NavigationControllerBase::RemoveEntryAtIndex(int index) {
       last_committed_entry_index_ = -1;
   }
 
-  NotifyPrunedEntries();
-  NotifyNavigationStateChanged();
+  // TODO(brettw) bug 1324021: we probably need some notification here so the
+  // session service can stay in sync.
 }
 
 void NavigationControllerBase::ResetInternal() {
diff --git a/chrome/browser/navigation_controller_base.h b/chrome/browser/navigation_controller_base.h
index e712ff3..0faf365 100644
--- a/chrome/browser/navigation_controller_base.h
+++ b/chrome/browser/navigation_controller_base.h
@@ -175,7 +175,7 @@ class NavigationControllerBase {
 
   // Allows the derived class to issue notifications that a load has been
   // committed.
-  virtual void NotifyNavigationStateChanged() {}
+  virtual void NotifyNavigationEntryCommitted() {}
 
   // Invoked when entries have been pruned, or removed. For example, if the
   // current entries are [google, digg, yahoo], with the current entry google,
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);
diff --git a/chrome/browser/session_service.cc b/chrome/browser/session_service.cc
index 9ed3d9f..da72060 100644
--- a/chrome/browser/session_service.cc
+++ b/chrome/browser/session_service.cc
@@ -160,7 +160,7 @@ SessionService::~SessionService() {
   NotificationService::current()->RemoveObserver(
       this, NOTIFY_NAV_ENTRY_CHANGED, NotificationService::AllSources());
   NotificationService::current()->RemoveObserver(
-      this, NOTIFY_NAV_INDEX_CHANGED, NotificationService::AllSources());
+      this, NOTIFY_NAV_ENTRY_COMMITTED, NotificationService::AllSources());
 }
 
 void SessionService::ResetFromCurrentBrowsers() {
@@ -417,7 +417,7 @@ void SessionService::Init(const std::wstring& path) {
   NotificationService::current()->AddObserver(
       this, NOTIFY_NAV_ENTRY_CHANGED, NotificationService::AllSources());
   NotificationService::current()->AddObserver(
-      this, NOTIFY_NAV_INDEX_CHANGED, NotificationService::AllSources());
+      this, NOTIFY_NAV_ENTRY_COMMITTED, NotificationService::AllSources());
 
   DCHECK(!path.empty());
   commands_since_reset_ = 0;
@@ -450,7 +450,7 @@ void SessionService::Observe(NotificationType type,
                           changed->index, *changed->changed_entry);
       break;
     }
-    case NOTIFY_NAV_INDEX_CHANGED:
+    case NOTIFY_NAV_ENTRY_COMMITTED:
       SetSelectedNavigationIndex(controller->window_id(),
                                  controller->session_id(),
                                  controller->GetCurrentEntryIndex());
diff --git a/chrome/browser/ssl_manager.cc b/chrome/browser/ssl_manager.cc
index be58f24..8454c09 100644
--- a/chrome/browser/ssl_manager.cc
+++ b/chrome/browser/ssl_manager.cc
@@ -247,10 +247,8 @@ void SSLManager::SetMaxSecurityStyle(SecurityStyle style) {
     return;
   }
 
-  if (entry->ssl().security_style() > style) {
+  if (entry->ssl().security_style() > style)
     entry->ssl().set_security_style(style);
-    controller_->EntryUpdated(entry);
-  }
 }
 
 // Delegate API method.
@@ -652,10 +650,10 @@ void SSLManager::DidCommitProvisionalLoad(ProvisionalLoadDetails* details) {
       // We may not have an entry if this is a navigation to an initial blank
       // page. Reset the SSL information and add the new data we have.
       entry->ssl() = NavigationEntry::SSLStatus();
+      InitializeEntryIfNeeded(entry);  // For security_style.
       entry->ssl().set_cert_id(details->ssl_cert_id());
       entry->ssl().set_cert_status(details->ssl_cert_status());
       entry->ssl().set_security_bits(details->ssl_security_bits());
-      controller_->EntryUpdated(entry);
     }
 
     if (details->interstitial_page()) {
diff --git a/chrome/browser/ssl_policy.cc b/chrome/browser/ssl_policy.cc
index dd83357..03e16cc 100644
--- a/chrome/browser/ssl_policy.cc
+++ b/chrome/browser/ssl_policy.cc
@@ -346,7 +346,6 @@ class DefaultPolicy : public SSLPolicy {
 
     NavigationEntry* entry = navigation_controller->GetActiveEntry();
     entry->ssl().set_has_mixed_content();
-    navigation_controller->EntryUpdated(entry);
   }
 
   void OnDenyCertificate(SSLManager::CertError* error) {
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index f354ef4..edf43bc 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -362,9 +362,9 @@ void LocationBarView::OnAutocompleteAccept(
 
     scoped_ptr<AlternateNavURLFetcher> fetcher(
         new AlternateNavURLFetcher(alternate_nav_url));
-    // The AlternateNavURLFetcher will listen for the next navigation state
-    // update notification (expecting it to be a new page load) and hook
-    // itself in to that loading process.
+    // The AlternateNavURLFetcher will listen for the pending navigation
+    // notification that will be issued as a result of the "open URL." It
+    // will automatically install itself into that navigation controller.
     controller_->ExecuteCommand(IDC_OPENURL);
     if (fetcher->state() == AlternateNavURLFetcher::NOT_STARTED) {
       // I'm not sure this should be reachable, but I'm not also sure enough