Remove DidNavigate from the tab contents delegate and all the related plumbing.

I added additional information to the regular load commit notification so all
interested parties can listen for that instead.

I removed the old navigation type enum, and replaced it with the enum from
the NavigationController, so it's now public.
Review URL: http://codereview.chromium.org/3112

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

9 files changed, 129 insertions, 140 deletions

diff --git a/chrome/browser/external_tab_container.cc b/chrome/browser/external_tab_container.cc
index 50f5123..c6188c3 100644
--- a/chrome/browser/external_tab_container.cc
+++ b/chrome/browser/external_tab_container.cc
@@ -81,6 +81,8 @@ bool ExternalTabContainer::Init(Profile* profile) {
 
   NavigationController* controller = tab_contents_->controller();
   DCHECK(controller);
+  registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                 Source<NavigationController>(controller));
   NotificationService::current()->
       Notify(NOTIFY_EXTERNAL_TAB_CREATED,
               Source<NavigationController>(controller),
@@ -208,15 +210,6 @@ void ExternalTabContainer::ToolbarSizeChanged(TabContents* source,
                                             bool finished) {
 }
 
-void ExternalTabContainer::DidNavigate(NavigationType nav_type,
-                                       int relative_navigation_offet) {
-  if (automation_) {
-    automation_->Send(
-        new AutomationMsg_DidNavigate(0, nav_type,
-                                      relative_navigation_offet));
-  }
-}
-
 void ExternalTabContainer::ForwardMessageToExternalHost(
     const std::string& receiver, const std::string& message) {
   if(automation_) {
@@ -228,6 +221,24 @@ void ExternalTabContainer::ForwardMessageToExternalHost(
 void ExternalTabContainer::Observe(NotificationType type,
                                    const NotificationSource& source,
                                    const NotificationDetails& details) {
+  switch (type) {
+    case NOTIFY_NAV_ENTRY_COMMITTED:
+      if (automation_) {
+        const NavigationController::LoadCommittedDetails* commit =
+            Details<NavigationController::LoadCommittedDetails>(details).ptr();
+
+        // When the previous entry index is invalid, it will be -1, which will
+        // still make the computation come out right (navigating to the 0th
+        // entry will be +1).
+        automation_->Send(new AutomationMsg_DidNavigate(
+            0, commit->type,
+            commit->previous_entry_index -
+                tab_contents_->controller()->GetLastCommittedEntryIndex()));
+      }
+      break;
+    default:
+      NOTREACHED();
+  }
 }
 
 void ExternalTabContainer::GetBounds(CRect *out, bool including_frame) const {
diff --git a/chrome/browser/external_tab_container.h b/chrome/browser/external_tab_container.h
index 6cc1160..b0f4909 100644
--- a/chrome/browser/external_tab_container.h
+++ b/chrome/browser/external_tab_container.h
@@ -13,6 +13,7 @@
 #include "base/basictypes.h"
 #include "chrome/browser/tab_contents_delegate.h"
 #include "chrome/common/chrome_constants.h"
+#include "chrome/common/notification_registrar.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/views/focus_manager.h"
 #include "chrome/views/root_view.h"
@@ -77,8 +78,6 @@ class ExternalTabContainer : public TabContentsDelegate,
   virtual void UpdateTargetURL(TabContents* source, const GURL& url);
   virtual void ContentsZoomChange(bool zoom_in);
   virtual void ToolbarSizeChanged(TabContents* source, bool is_animating);
-  virtual void DidNavigate(NavigationType nav_type,
-                           int relative_navigation_offet);
   virtual void ForwardMessageToExternalHost(const std::string& receiver,
                                             const std::string& message);
 
@@ -132,6 +131,9 @@ class ExternalTabContainer : public TabContentsDelegate,
  protected:
   TabContents *tab_contents_;
   AutomationProvider* automation_;
+
+  NotificationRegistrar registrar_;
+
   // Root view
   ChromeViews::RootView root_view_;
   // The accelerator table of the external host.
diff --git a/chrome/browser/navigation_controller.cc b/chrome/browser/navigation_controller.cc
index 91c1fb4..38fcf65 100644
--- a/chrome/browser/navigation_controller.cc
+++ b/chrome/browser/navigation_controller.cc
@@ -492,9 +492,14 @@ bool NavigationController::RendererDidNavigate(
     const ViewHostMsg_FrameNavigate_Params& params,
     bool is_interstitial,
     LoadCommittedDetails* details) {
-  // Save the previous URL before we clobber it.
-  if (GetLastCommittedEntry())
+  // Save the previous state before we clobber it.
+  if (GetLastCommittedEntry()) {
     details->previous_url = GetLastCommittedEntry()->url();
+    details->previous_entry_index = GetLastCommittedEntryIndex();
+  } else {
+    details->previous_url = GURL();
+    details->previous_entry_index = -1;
+  }
 
   // Assign the current site instance to any pending entry, so we can find it
   // later by calling GetEntryIndexWithPageID. We only care about this if the
@@ -508,27 +513,28 @@ bool NavigationController::RendererDidNavigate(
     pending_entry_->set_site_instance(active_contents_->GetSiteInstance());
 
   // Do navigation-type specific actions. These will make and commit an entry.
-  switch (ClassifyNavigation(params)) {
-    case NAV_NEW_PAGE:
+  details->type = ClassifyNavigation(params);
+  switch (details->type) {
+    case NavigationType::NEW_PAGE:
       RendererDidNavigateToNewPage(params);
       break;
-    case NAV_EXISTING_PAGE:
+    case NavigationType::EXISTING_PAGE:
       RendererDidNavigateToExistingPage(params);
       break;
-    case NAV_SAME_PAGE:
+    case NavigationType::SAME_PAGE:
       RendererDidNavigateToSamePage(params);
       break;
-    case NAV_IN_PAGE:
+    case NavigationType::IN_PAGE:
       RendererDidNavigateInPage(params);
       break;
-    case NAV_NEW_SUBFRAME:
+    case NavigationType::NEW_SUBFRAME:
       RendererDidNavigateNewSubframe(params);
       break;
-    case NAV_AUTO_SUBFRAME:
+    case NavigationType::AUTO_SUBFRAME:
       if (!RendererDidNavigateAutoSubframe(params))
         return false;
       break;
-    case NAV_IGNORE:
+    case NavigationType::NAV_IGNORE:
       // There is nothing we can do with this navigation, so we just return to
       // the caller that nothing has happened.
       return false;
@@ -570,7 +576,7 @@ bool NavigationController::RendererDidNavigate(
   return true;
 }
 
-NavigationController::NavClass NavigationController::ClassifyNavigation(
+NavigationType::Type NavigationController::ClassifyNavigation(
     const ViewHostMsg_FrameNavigate_Params& params) const {
   // If a page makes a popup navigated to about blank, and then writes stuff
   // like a subframe navigated to a real site, we'll get a notification with an
@@ -578,7 +584,7 @@ NavigationController::NavClass NavigationController::ClassifyNavigation(
   if (params.page_id == -1) {
     DCHECK(!GetActiveEntry()) << "Got an invalid page ID but we seem to be "
       " navigated to a valid page. This should be impossible.";
-    return NAV_IGNORE;
+    return NavigationType::NAV_IGNORE;
   }
 
   if (params.page_id > active_contents_->GetMaxPageID()) {
@@ -586,7 +592,7 @@ NavigationController::NavClass NavigationController::ClassifyNavigation(
     // not have a pending entry for the page, and this may or may not be the
     // main frame.
     if (PageTransition::IsMainFrame(params.transition))
-      return NAV_NEW_PAGE;
+      return NavigationType::NEW_PAGE;
 
     // When this is a new subframe navigation, we should have a committed page
     // for which it's a suframe in. This may not be the case when an iframe is
@@ -594,10 +600,10 @@ NavigationController::NavClass NavigationController::ClassifyNavigation(
     // written into the popup by script on the main page). For these cases,
     // there isn't any navigation stuff we can do, so just ignore it.
     if (!GetLastCommittedEntry())
-      return NAV_IGNORE;
+      return NavigationType::NAV_IGNORE;
 
     // Valid subframe navigation.
-    return NAV_NEW_SUBFRAME;
+    return NavigationType::NEW_SUBFRAME;
   }
 
   // Now we know that the notification is for an existing page. Find that entry.
@@ -610,7 +616,7 @@ NavigationController::NavClass NavigationController::ClassifyNavigation(
     // back/forward entries (not likely since we'll usually tell it to navigate
     // to such entries). It could also mean that the renderer is smoking crack.
     NOTREACHED();
-    return NAV_IGNORE;
+    return NavigationType::NAV_IGNORE;
   }
   NavigationEntry* existing_entry = entries_[existing_entry_index].get();
 
@@ -626,23 +632,23 @@ NavigationController::NavClass NavigationController::ClassifyNavigation(
     // (the user doesn't want to have a new back/forward entry when they do
     // this). In this case, we want to just ignore the pending entry and go
     // back to where we were (the "existing entry").
-    return NAV_SAME_PAGE;
+    return NavigationType::SAME_PAGE;
   }
 
   if (AreURLsInPageNavigation(existing_entry->url(), params.url))
-    return NAV_IN_PAGE;
+    return NavigationType::IN_PAGE;
 
   if (!PageTransition::IsMainFrame(params.transition)) {
     // All manual subframes would get new IDs and were handled above, so we
     // know this is auto. Since the current page was found in the navigation
     // entry list, we're guaranteed to have a last committed entry.
     DCHECK(GetLastCommittedEntry());
-    return NAV_AUTO_SUBFRAME;
+    return NavigationType::AUTO_SUBFRAME;
   }
 
   // Since we weeded out "new" navigations above, we know this is an existing
   // (back/forward) navigation.
-  return NAV_EXISTING_PAGE;
+  return NavigationType::EXISTING_PAGE;
 }
 
 void NavigationController::RendererDidNavigateToNewPage(
@@ -706,9 +712,7 @@ void NavigationController::RendererDidNavigateToExistingPage(
   if (entry == pending_entry_)
     DiscardPendingEntryInternal();
   
-  int old_committed_entry_index = last_committed_entry_index_;
   last_committed_entry_index_ = entry_index;
-  IndexOfActiveEntryChanged(old_committed_entry_index);
 }
 
 void NavigationController::RendererDidNavigateToSamePage(
@@ -782,9 +786,7 @@ bool NavigationController::RendererDidNavigateAutoSubframe(
 
   // Update the current navigation entry in case we're going back/forward.
   if (entry_index != last_committed_entry_index_) {
-    int old_committed_entry_index = last_committed_entry_index_;
     last_committed_entry_index_ = entry_index;
-    IndexOfActiveEntryChanged(old_committed_entry_index);
     return true;
   }
   return false;
@@ -796,19 +798,22 @@ void NavigationController::CommitPendingEntry() {
 
   // Need to save the previous URL for the notification.
   LoadCommittedDetails details;
-  if (GetLastCommittedEntry())
+  if (GetLastCommittedEntry()) {
     details.previous_url = GetLastCommittedEntry()->url();
+    details.previous_entry_index = GetLastCommittedEntryIndex();
+  } else {
+    details.previous_entry_index = -1;
+  }
 
   if (pending_entry_index_ >= 0) {
     // This is a previous navigation (back/forward) that we're just now
     // committing. Just mark it as committed.
+    details.type = NavigationType::EXISTING_PAGE;
     int new_entry_index = pending_entry_index_;
     DiscardPendingEntryInternal();
 
     // Mark that entry as committed.
-    int old_committed_entry_index = last_committed_entry_index_;
     last_committed_entry_index_ = new_entry_index;
-    IndexOfActiveEntryChanged(old_committed_entry_index);
   } else {
     // This is a new navigation. It's easiest to just copy the entry and insert
     // it new again, since InsertEntry expects to take ownership and also
@@ -816,6 +821,7 @@ void NavigationController::CommitPendingEntry() {
     // only do this because this function will only be called by our custom
     // TabContents types. For WebContents, the IDs are generated by the
     // renderer, so we can't do this.
+    details.type = NavigationType::NEW_PAGE;
     pending_entry_->set_page_id(active_contents_->GetMaxPageID() + 1);
     active_contents_->UpdateMaxPageID(pending_entry_->page_id());
     InsertEntry(new NavigationEntry(*pending_entry_));
@@ -963,10 +969,6 @@ void NavigationController::InsertEntry(NavigationEntry* entry) {
 
   // This is a new page ID, so we need everybody to know about it.
   active_contents_->UpdateMaxPageID(entry->page_id());
-  
-  // TODO(brettw) this seems bogus. The tab contents can listen for the
-  // notification or use the details that we pass back to it.
-  active_contents_->NotifyDidNavigate(NAVIGATION_NEW, 0);
 }
 
 void NavigationController::SetWindowID(const SessionID& id) {
@@ -1026,19 +1028,6 @@ void NavigationController::NotifyNavigationEntryCommitted(
       Details<LoadCommittedDetails>(details));
 }
 
-void NavigationController::IndexOfActiveEntryChanged(int prev_committed_index) {
-  NavigationType nav_type = NAVIGATION_BACK_FORWARD;
-  int relative_navigation_offset =
-      GetLastCommittedEntryIndex() - prev_committed_index;
-  if (relative_navigation_offset == 0)
-    nav_type = NAVIGATION_REPLACE;
-
-  // TODO(brettw) I don't think this call should be necessary. There is already
-  // a notification of this event that could be used, or maybe all the tab
-  // contents' know when we navigate (WebContents does).
-  active_contents_->NotifyDidNavigate(nav_type, relative_navigation_offset);
-}
-
 TabContents* NavigationController::GetTabContentsCreateIfNecessary(
     HWND parent,
     const NavigationEntry& entry) {
diff --git a/chrome/browser/navigation_controller.h b/chrome/browser/navigation_controller.h
index 7d99876..1cb39aa 100644
--- a/chrome/browser/navigation_controller.h
+++ b/chrome/browser/navigation_controller.h
@@ -13,6 +13,7 @@
 #include "chrome/browser/site_instance.h"
 #include "chrome/browser/ssl_manager.h"
 #include "chrome/browser/tab_contents_type.h"
+#include "chrome/common/navigation_types.h"
 
 class GURL;
 class Profile;
@@ -57,6 +58,15 @@ class NavigationController {
     // The committed entry. This will be the active entry in the controller.
     NavigationEntry* entry;
 
+    // The type of navigation that just occurred. Note that not all types of
+    // navigations in the enum are valid here, since some of them don't actually
+    // cause a "commit" and won't generate this notification.
+    NavigationType::Type type;
+
+    // The index of the previously committed navigation entry. This will be -1
+    // if there are no previous entries.
+    int previous_entry_index;
+
     // The previous URL that the user was on. This may be empty if none.
     GURL previous_url;
 
@@ -369,52 +379,8 @@ class NavigationController {
   friend class RestoreHelper;
   friend class TabContents;  // For invoking OnReservedPageIDRange.
 
-  // Indicates different types of navigations that can occur that we will handle
-  // separately. This is computed by ClassifyNavigation.
-  enum NavClass {
-    // A new page was navigated in the main frame.
-    NAV_NEW_PAGE,
-
-    // Renavigating to an existing navigation entry. The entry is guaranteed to
-    // exist in the list, or else it would be a new page or IGNORE navigation.
-    NAV_EXISTING_PAGE,
-
-    // The same page has been reloaded as a result of the user requesting
-    // navigation to that same page (like pressing Enter in the URL bar). This
-    // is not the same as an in-page navigation because we'll actually have a
-    // pending entry for the load, which is then meaningless.
-    NAV_SAME_PAGE,
-
-    // In page navigations are when the reference fragment changes. This will
-    // be in the main frame only (we won't even get notified of in-page
-    // subframe navigations). It may be for any page, not necessarily the last
-    // committed one (for example, whey going back to a page with a ref).
-    NAV_IN_PAGE,
-
-    // A new subframe was manually navigated by the user. We will create a new
-    // NavigationEntry so they can go back to the previous subframe content
-    // using the back button.
-    NAV_NEW_SUBFRAME,
-
-    // A subframe in the page was automatically loaded or navigated to such that
-    // a new navigation entry should not be created. There are two cases:
-    //  1. Stuff like iframes containing ads that the page loads automatically.
-    //     The user doesn't want to see these, so we just update the existing
-    //     navigation entry.
-    //  2. Going back/forward to previous subframe navigations. We don't create
-    //     a new entry here either, just update the last committed entry.
-    // These two cases are actually pretty different, they just happen to
-    // require almost the same code to handle.
-    NAV_AUTO_SUBFRAME,
-
-    // Nothing happened. This happens when we get information about a page we
-    // don't know anything about. It can also happen when an iframe in a popup
-    // navigated to about:blank is navigated. Nothing needs to be done.
-    NAV_IGNORE,
-  };
-
   // Classifies the given renderer navigation (see the NavigationType enum).
-  NavClass ClassifyNavigation(
+  NavigationType::Type ClassifyNavigation(
       const ViewHostMsg_FrameNavigate_Params& params) const;
 
   // Causes the controller to load the specified entry. The function assumes
@@ -450,11 +416,6 @@ class NavigationController {
   // committed. This will fill in the active entry to the details structure.
   void NotifyNavigationEntryCommitted(LoadCommittedDetails* details);
 
-  // Invoked when the index of the active entry may have changed.
-  // The prev_commited_index parameter specifies the previous value
-  // of the last commited index before this navigation event happened
-  void IndexOfActiveEntryChanged(int prev_commited_index);
-
   // Returns the TabContents for the |entry|'s type. If the TabContents
   // doesn't yet exist, it is created. If a new TabContents is created, its
   // parent is |parent|.  Becomes part of |entry|'s SiteInstance.
diff --git a/chrome/browser/tab_contents.cc b/chrome/browser/tab_contents.cc
index a0d39f3..17fd07c 100644
--- a/chrome/browser/tab_contents.cc
+++ b/chrome/browser/tab_contents.cc
@@ -270,12 +270,6 @@ void TabContents::NotifyNavigationStateChanged(unsigned changed_flags) {
     delegate_->NavigationStateChanged(this, changed_flags);
 }
 
-void TabContents::NotifyDidNavigate(NavigationType nav_type,
-                                    int relative_navigation_offset) {
-  if (delegate_)
-    delegate_->DidNavigate(nav_type, relative_navigation_offset);
-}
-
 static BOOL CALLBACK InvalidateWindow(HWND hwnd, LPARAM lparam) {
   // Note: erase is required to properly paint some widgets borders. This can be
   // seen with textfields.
diff --git a/chrome/browser/tab_contents.h b/chrome/browser/tab_contents.h
index e8990d6..5630cc4 100644
--- a/chrome/browser/tab_contents.h
+++ b/chrome/browser/tab_contents.h
@@ -298,11 +298,6 @@ class TabContents : public PageNavigator,
   // change. See TabContentsDelegate.
   void NotifyNavigationStateChanged(unsigned changed_flags);
 
-  // Convenience method for notifying the delegate of a navigation.
-  // See TabContentsDelegate.
-  void NotifyDidNavigate(NavigationType nav_type,
-                         int relative_navigation_offset);
-
   // Invoked when the tab contents becomes selected. If you override, be sure
   // and invoke super's implementation.
   virtual void DidBecomeSelected();
diff --git a/chrome/browser/tab_contents_delegate.h b/chrome/browser/tab_contents_delegate.h
index 63cbcde..ae3db93 100644
--- a/chrome/browser/tab_contents_delegate.h
+++ b/chrome/browser/tab_contents_delegate.h
@@ -123,16 +123,6 @@ class TabContentsDelegate : public PageNavigator {
   // a WebContents with a valid WebApp set.
   virtual void ConvertContentsToApplication(TabContents* source) { }
 
-  // Notifies the delegate that a navigation happened. nav_type indicates the
-  // type of navigation. If nav_type is NAVIGATION_BACK_FORWARD then the
-  // relative_navigation_offset indicates the relative offset of the navigation
-  // within the session history (a negative value indicates a backward
-  // navigation and a positive value indicates a forward navigation). If
-  // nav_type is any other value, the relative_navigation_offset parameter
-  // is not defined and should be ignored.
-  virtual void DidNavigate(NavigationType nav_type,
-                           int relative_navigation_offset) { return; }
-
   // Informs the TabContentsDelegate that some of our state has changed 
   // for this tab.
   virtual void ContentsStateChanged(TabContents* source) {}
diff --git a/chrome/common/common.vcproj b/chrome/common/common.vcproj
index a09fcba..203aa3f 100644
--- a/chrome/common/common.vcproj
+++ b/chrome/common/common.vcproj
@@ -506,6 +506,10 @@
 			>
 		</File>
 		<File
+			RelativePath=".\navigation_types.h"
+			>
+		</File>
+		<File
 			RelativePath=".\notification_details.h"
 			>
 		</File>
diff --git a/chrome/common/navigation_types.h b/chrome/common/navigation_types.h
index c7e3a90..bc27cbd 100644
--- a/chrome/common/navigation_types.h
+++ b/chrome/common/navigation_types.h
@@ -2,20 +2,63 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_COMMON_NAVIGATION_TYPES_H__
-#define CHROME_COMMON_NAVIGATION_TYPES_H__
-
-// An enum for the type of navigation. This is used when calling
-// the NotifyDidNavigate method on the active TabContents
-enum NavigationType {
-  // This is a new navigation resulting in a new entry in the session history
-  NAVIGATION_NEW = 0,
-  // Back or forward navigation within the session history
-  NAVIGATION_BACK_FORWARD = 1,
-  // This navigation simply replaces the URL of an existing entry in the
-  // seesion history
-  NAVIGATION_REPLACE = 2,
+#ifndef CHROME_COMMON_NAVIGATION_TYPES_H_
+#define CHROME_COMMON_NAVIGATION_TYPES_H_
+
+#include "base/basictypes.h"
+
+// Indicates different types of navigations that can occur that we will handle
+// separately.
+class NavigationType {
+ public:
+  enum Type {
+    // A new page was navigated in the main frame.
+    NEW_PAGE,
+
+    // Renavigating to an existing navigation entry. The entry is guaranteed to
+    // exist in the list, or else it would be a new page or IGNORE navigation.
+    EXISTING_PAGE,
+
+    // The same page has been reloaded as a result of the user requesting
+    // navigation to that same page (like pressing Enter in the URL bar). This
+    // is not the same as an in-page navigation because we'll actually have a
+    // pending entry for the load, which is then meaningless.
+    SAME_PAGE,
+
+    // In page navigations are when the reference fragment changes. This will
+    // be in the main frame only (we won't even get notified of in-page
+    // subframe navigations). It may be for any page, not necessarily the last
+    // committed one (for example, whey going back to a page with a ref).
+    IN_PAGE,
+
+    // A new subframe was manually navigated by the user. We will create a new
+    // NavigationEntry so they can go back to the previous subframe content
+    // using the back button.
+    NEW_SUBFRAME,
+
+    // A subframe in the page was automatically loaded or navigated to such that
+    // a new navigation entry should not be created. There are two cases:
+    //  1. Stuff like iframes containing ads that the page loads automatically.
+    //     The user doesn't want to see these, so we just update the existing
+    //     navigation entry.
+    //  2. Going back/forward to previous subframe navigations. We don't create
+    //     a new entry here either, just update the last committed entry.
+    // These two cases are actually pretty different, they just happen to
+    // require almost the same code to handle.
+    AUTO_SUBFRAME,
+
+    // Nothing happened. This happens when we get information about a page we
+    // don't know anything about. It can also happen when an iframe in a popup
+    // navigated to about:blank is navigated. Nothing needs to be done.
+    NAV_IGNORE,
+  };
+
+ private:
+  // This class is for scoping only, so you shouldn't create an instance of it.
+  NavigationType() {}
+
+  DISALLOW_COPY_AND_ASSIGN(NavigationType);
 };
 
-#endif  // CHROME_COMMON_NAVIGATION_TYPES_H__
+#endif  // CHROME_COMMON_NAVIGATION_TYPES_H_