Make a step on refactoring navigation. The eventual plan is to have the NavigationController create and commit the new NavigationEntries (currently WebContents does a bunch of the details of this which is hard to understand and not easily testable).

This tries to consolidate the logic that I want to move to the NavigationController without actually moving it there yet. I removed all of the "PreCommit" functions in WebContents, since when the NavigationController does
all of the committing, there won't be a phase where the NavigationEntry exists but isn't committed.

Most of the logic could be moved to the PostCommit functions without any problem, which is an indication that the current design was busted anyway. I had to precompute some data and pass it to the *PostCommit function to work around some of the components that required old data. I had to change InfoBars around since it relied on having both the committed and uncommitted entries, but I think the new design is much better anyway.

BUG=1343593,1343146

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

12 files changed, 299 insertions, 255 deletions

diff --git a/chrome/browser/native_ui_contents.cc b/chrome/browser/native_ui_contents.cc
index 2046ba0..73cd614 100644
--- a/chrome/browser/native_ui_contents.cc
+++ b/chrome/browser/native_ui_contents.cc
@@ -269,7 +269,10 @@ bool NativeUIContents::Navigate(const NavigationEntry& entry, bool reload) {
   state_->GetByteRepresentation(&content_state);
   new_entry->set_content_state(content_state);
   const int32 page_id = new_entry->page_id();
-  DidNavigateToEntry(new_entry);
+
+  // The default details is "new navigation", and that's OK with us.
+  NavigationController::LoadCommittedDetails details;
+  DidNavigateToEntry(new_entry, &details);
   // This is not a WebContents, so we use a NULL SiteInstance.
   controller()->NotifyEntryChangedByPageID(type(), NULL, page_id);
   return true;
diff --git a/chrome/browser/navigation_controller.cc b/chrome/browser/navigation_controller.cc
index 715e33f7..c944d99 100644
--- a/chrome/browser/navigation_controller.cc
+++ b/chrome/browser/navigation_controller.cc
@@ -493,7 +493,8 @@ void NavigationController::SetAlternateNavURLFetcher(
   alternate_nav_url_fetcher_entry_unique_id_ = pending_entry_->unique_id();
 }
 
-void NavigationController::DidNavigateToEntry(NavigationEntry* entry) {
+void NavigationController::DidNavigateToEntry(NavigationEntry* entry,
+                                              LoadCommittedDetails* details) {
   DCHECK(active_contents_);
   DCHECK(entry->tab_type() == active_contents_->type());
 
@@ -501,6 +502,11 @@ void NavigationController::DidNavigateToEntry(NavigationEntry* entry) {
 
   entry->set_restored(false);
 
+  // Update the details to list the last URL. Later, we'll update the current
+  // entry (after it's committed) and the details will be complete.
+  if (GetLastCommittedEntry())
+    details->previous_url = GetLastCommittedEntry()->url();
+
   // If the entry is that of a page with PageID larger than any this Tab has
   // seen before, then consider it a new navigation.  Note that if the entry
   // has a SiteInstance, it should be the same as the SiteInstance of the
@@ -508,7 +514,7 @@ void NavigationController::DidNavigateToEntry(NavigationEntry* entry) {
   DCHECK(entry->page_id() >= 0) << "Page ID must be set before calling us.";
   if (entry->page_id() > GetMaxPageID()) {
     InsertEntry(entry);
-    NotifyNavigationEntryCommitted();
+    NotifyNavigationEntryCommitted(details);
     // It is now a safe time to schedule collection for any tab contents of a
     // different type, because a navigation is necessary to get back to them.
     ScheduleTabContentsCollectionForInactiveTabs();
@@ -566,7 +572,7 @@ void NavigationController::DidNavigateToEntry(NavigationEntry* entry) {
   }
 
   delete entry;
-  NotifyNavigationEntryCommitted();
+  NotifyNavigationEntryCommitted(details);
 
   if (alternate_nav_url_fetcher_.get()) {
     // Because this call may synchronously show an infobar, we do it last, to
@@ -715,7 +721,8 @@ void NavigationController::NavigateToPendingEntry(bool reload) {
     DiscardPendingEntry();
 }
 
-void NavigationController::NotifyNavigationEntryCommitted() {
+void NavigationController::NotifyNavigationEntryCommitted(
+    LoadCommittedDetails* details) {
   // 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.
@@ -735,9 +742,11 @@ void NavigationController::NotifyNavigationEntryCommitted() {
   active_contents_->NotifyNavigationStateChanged(
       TabContents::INVALIDATE_EVERYTHING);
 
-  NotificationService::current()->Notify(NOTIFY_NAV_ENTRY_COMMITTED,
-                                         Source<NavigationController>(this),
-                                         NotificationService::NoDetails());
+  details->entry = GetActiveEntry();
+  NotificationService::current()->Notify(
+      NOTIFY_NAV_ENTRY_COMMITTED,
+      Source<NavigationController>(this),
+      Details<LoadCommittedDetails>(details));
 }
 
 void NavigationController::NotifyPrunedEntries() {
diff --git a/chrome/browser/navigation_controller.h b/chrome/browser/navigation_controller.h
index c2c517d..e43bfe1 100644
--- a/chrome/browser/navigation_controller.h
+++ b/chrome/browser/navigation_controller.h
@@ -48,6 +48,46 @@ class NavigationController {
     int index;
   };
 
+  struct LoadCommittedDetails {
+    // By default, the entry will be filled according to a new main frame
+    // navigation.
+    LoadCommittedDetails()
+        : entry(NULL),
+          is_auto(false),
+          is_in_page(false),
+          is_main_frame(true) {
+    }
+
+    // The committed entry. This will be the active entry in the controller.
+    NavigationEntry* entry;
+
+    // The previous URL that the user was on. This may be empty if none.
+    GURL previous_url;
+
+    // True when this load was non-user initated. This corresponds to a
+    // a NavigationGestureAuto call from WebKit (see webview_delegate.h).
+    // We also count reloads and meta-refreshes as "auto" to account for the
+    // fact that WebKit doesn't always set the user gesture properly in these
+    // cases (see bug 1051891).
+    bool is_auto;
+
+    // True if the navigation was in-page. This means that the active entry's
+    // URL and the |previous_url| are the same except for reference fragments.
+    bool is_in_page;
+
+    // True when the main frame was navigated. False means the navigation was a
+    // sub-frame.
+    bool is_main_frame;
+
+    // Returns whether the user probably felt like they navigated somewhere new.
+    // We often need this logic for showing or hiding something, and this
+    // returns true only for main frame loads that the user initiated, that go
+    // to a new page.
+    bool is_user_initiated_main_frame_load() const {
+      return !is_auto && !is_in_page && is_main_frame;
+    }
+  };
+
   NavigationController(TabContents* initial_contents, Profile* profile);
   // Creates a NavigationController from the specified history. Processing
   // for this is asynchronous and handled via the RestoreHelper (in
@@ -208,7 +248,15 @@ class NavigationController {
   // for a tab.  The controller takes ownership of the entry.  Any entry located
   // forward to the current entry will be deleted.  The new entry becomes the
   // current entry.
-  void DidNavigateToEntry(NavigationEntry* entry);
+  //
+  // The details are populated by the caller except for the new NavigationEntry
+  // pointer and the previous URL. We will fill these in before using it to
+  // broadcast notifications, so it can also be used by the caller.
+  //
+  // TODO(brettw) bug 1343146: The NavigationController should internally make
+  // the entry and the notification details.
+  void DidNavigateToEntry(NavigationEntry* entry,
+                          LoadCommittedDetails* details);
 
   // Calling this may cause the active tab contents to switch if the current
   // entry corresponds to a different tab contents type.
@@ -284,8 +332,8 @@ class NavigationController {
   void NavigateToPendingEntry(bool reload);
 
   // Allows the derived class to issue notifications that a load has been
-  // committed.
-  void NotifyNavigationEntryCommitted();
+  // committed. This will fill in the active entry to the details structure.
+  void NotifyNavigationEntryCommitted(LoadCommittedDetails* details);
 
   // 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/safe_browsing/safe_browsing_blocking_page.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
index c495952..8d39465 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
@@ -176,7 +176,10 @@ void SafeBrowsingBlockingPage::DisplayBlockingPage() {
     nav_entry->set_page_id(tab_->GetMaxPageID() + 1);
     nav_entry->set_page_type(NavigationEntry::INTERSTITIAL_PAGE);
     nav_entry->set_url(url_);
-    tab_->controller()->DidNavigateToEntry(nav_entry);
+
+    // The default details is "new navigation", and that's OK with us.
+    NavigationController::LoadCommittedDetails details;
+    tab_->controller()->DidNavigateToEntry(nav_entry, &details);
     created_temporary_entry_ = true;
   }
 
diff --git a/chrome/browser/ssl_blocking_page.cc b/chrome/browser/ssl_blocking_page.cc
index db0f684..a8f5152 100644
--- a/chrome/browser/ssl_blocking_page.cc
+++ b/chrome/browser/ssl_blocking_page.cc
@@ -153,7 +153,10 @@ void SSLBlockingPage::Show() {
   nav_entry->ssl().set_cert_status(ssl_info.cert_status);
   nav_entry->ssl().set_security_bits(ssl_info.security_bits);
   // The controller will own the entry.
-  tab_->controller()->DidNavigateToEntry(nav_entry);
+
+  // The default details is "new navigation", and that's OK with us.
+  NavigationController::LoadCommittedDetails details;
+  tab_->controller()->DidNavigateToEntry(nav_entry, &details);
   tab->ShowInterstitialPage(html_text, NULL);
 }
 
diff --git a/chrome/browser/tab_contents.cc b/chrome/browser/tab_contents.cc
index 27d7a0a..503c99e 100644
--- a/chrome/browser/tab_contents.cc
+++ b/chrome/browser/tab_contents.cc
@@ -256,11 +256,13 @@ void TabContents::SetIsLoading(bool is_loading,
                         NotificationService::NoDetails());
 }
 
-void TabContents::DidNavigateToEntry(NavigationEntry* entry) {
+void TabContents::DidNavigateToEntry(
+    NavigationEntry* entry,
+    NavigationController::LoadCommittedDetails* details) {
   // The entry may be deleted by DidNavigateToEntry...
   int new_page_id = entry->page_id();
 
-  controller_->DidNavigateToEntry(entry);
+  controller_->DidNavigateToEntry(entry, details);
 
   // update after informing the navigation controller so it can check the
   // previous value of the max page id.
@@ -275,7 +277,14 @@ bool TabContents::Navigate(const NavigationEntry& entry, bool reload) {
     new_entry->set_page_id(0);
     new_entry->set_title(GetDefaultTitle());
   }
-  DidNavigateToEntry(new_entry);
+
+  // When we're commanded to navigate like this, it's always a new main frame
+  // navigation (which is the default for the details).
+  NavigationController::LoadCommittedDetails details;
+  if (controller()->GetLastCommittedEntry())
+    details.previous_url = controller()->GetLastCommittedEntry()->url();
+
+  DidNavigateToEntry(new_entry, &details);
   return true;
 }
 
diff --git a/chrome/browser/tab_contents.h b/chrome/browser/tab_contents.h
index 9d117a5..7d0f784 100644
--- a/chrome/browser/tab_contents.h
+++ b/chrome/browser/tab_contents.h
@@ -489,8 +489,11 @@ class TabContents : public PageNavigator,
   void SetIsLoading(bool is_loading, LoadNotificationDetails* details);
 
   // Called by subclasses when a navigation occurs.  Ownership of the entry
-  // object is passed to this method.
-  void DidNavigateToEntry(NavigationEntry* entry);
+  // object is passed to this method. The details object should be filled in
+  // *except* for the entry (which the NavigationController will set). This
+  // behaves the same as NavigationController::DidNavigate, see that for more.
+  void DidNavigateToEntry(NavigationEntry* entry,
+                          NavigationController::LoadCommittedDetails* details);
 
   // Called by a derived class when the TabContents is resized, causing
   // suppressed constrained web popups to be repositioned to the new bounds
diff --git a/chrome/browser/views/info_bar_view.cc b/chrome/browser/views/info_bar_view.cc
index c10dfa9..56c2ecc 100644
--- a/chrome/browser/views/info_bar_view.cc
+++ b/chrome/browser/views/info_bar_view.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/views/info_bar_view.h"
 
 #include "base/logging.h"
+#include "chrome/browser/navigation_controller.h"
 #include "chrome/browser/navigation_entry.h"
 #include "chrome/browser/tab_contents_delegate.h"
 #include "chrome/browser/web_contents.h"
@@ -31,9 +32,16 @@ static const int kSeparatorHeight = 1;
 InfoBarView::InfoBarView(WebContents* web_contents)
     : web_contents_(web_contents) {
   Init();
+  NotificationService::current()->AddObserver(
+      this, NOTIFY_NAV_ENTRY_COMMITTED,
+      Source<NavigationController>(web_contents_->controller()));
 }
 
-InfoBarView::~InfoBarView() {}
+InfoBarView::~InfoBarView() {
+  NotificationService::current()->RemoveObserver(
+      this, NOTIFY_NAV_ENTRY_COMMITTED,
+      Source<NavigationController>(web_contents_->controller()));
+}
 
 void InfoBarView::AppendInfoBarItem(ChromeViews::View* view, bool auto_expire) {
   // AddChildView adds an entry to expire_map_ for view.
@@ -107,43 +115,6 @@ void InfoBarView::ChildAnimationEnded() {
     web_contents_->ToolbarSizeChanged(false);
 }
 
-void InfoBarView::DidNavigate(NavigationEntry* entry) {
-  // Determine the views to remove first.
-  std::vector<ChromeViews::View*> to_remove;
-  NavigationEntry* pending_entry =
-      web_contents_->controller()->GetPendingEntry();
-  const int active_id = pending_entry ? pending_entry->unique_id() :
-                                        entry->unique_id();
-  for (std::map<View*,int>::iterator i = expire_map_.begin();
-       i != expire_map_.end(); ++i) {
-    if ((pending_entry && 
-        pending_entry->transition_type() == PageTransition::RELOAD) ||
-        i->second != active_id)
-      to_remove.push_back(i->first);
-  }
-
-  if (to_remove.empty())
-    return;
-
-  // Remove the views.
-  for (std::vector<ChromeViews::View*>::iterator i = to_remove.begin();
-       i != to_remove.end(); ++i) {
-    // RemoveChildView takes care of removing from expire_map for us.
-    RemoveChildView(*i);
-
-    // We own the child and we're removing it, need to delete it.
-    delete *i;
-  }
-
-  if (GetChildViewCount() == 0) {
-    // All our views have been removed, no need to stay visible.
-    web_contents_->SetInfoBarVisible(false);
-  } else if (web_contents_) {
-    // This triggers a layout.
-    web_contents_->ToolbarSizeChanged(false);
-  }
-}
-
 void InfoBarView::ViewHierarchyChanged(bool is_add, View *parent,
                                        View *child) {
   if (parent == this && child->GetParent() == this) {
@@ -220,3 +191,52 @@ void InfoBarView::PaintSeparator(ChromeCanvas* canvas,
                       GetWidth(),
                       kSeparatorHeight);
 }
+
+void InfoBarView::Observe(NotificationType type,
+                          const NotificationSource& source,
+                          const NotificationDetails& in_details) {
+  // We should get only commit notifications from our controller.
+  DCHECK(type == NOTIFY_NAV_ENTRY_COMMITTED);
+  DCHECK(web_contents_->controller() ==
+         Source<NavigationController>(source).ptr());
+
+  NavigationController::LoadCommittedDetails& details =
+      *(Details<NavigationController::LoadCommittedDetails>(in_details).ptr());
+
+  // Only hide infobars when the user has done something that makes the main
+  // frame load. We don't want various automatic or subframe navigations making
+  // it disappear.
+  if (!details.is_user_initiated_main_frame_load())
+    return;
+
+  // Determine the views to remove first.
+  std::vector<ChromeViews::View*> to_remove;
+  for (std::map<View*,int>::iterator i = expire_map_.begin();
+       i != expire_map_.end(); ++i) {
+    if (PageTransition::StripQualifier(details.entry->transition_type()) ==
+            PageTransition::RELOAD ||
+        i->second != details.entry->unique_id())
+      to_remove.push_back(i->first);
+  }
+
+  if (to_remove.empty())
+    return;
+
+  // Remove the views.
+  for (std::vector<ChromeViews::View*>::iterator i = to_remove.begin();
+       i != to_remove.end(); ++i) {
+    // RemoveChildView takes care of removing from expire_map for us.
+    RemoveChildView(*i);
+
+    // We own the child and we're removing it, need to delete it.
+    delete *i;
+  }
+
+  if (GetChildViewCount() == 0) {
+    // All our views have been removed, no need to stay visible.
+    web_contents_->SetInfoBarVisible(false);
+  } else if (web_contents_) {
+    // This triggers a layout.
+    web_contents_->ToolbarSizeChanged(false);
+  }
+}
diff --git a/chrome/browser/views/info_bar_view.h b/chrome/browser/views/info_bar_view.h
index 8234c0a..0ff9b94 100644
--- a/chrome/browser/views/info_bar_view.h
+++ b/chrome/browser/views/info_bar_view.h
@@ -2,11 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_VIEWS_INFO_BAR_VIEW_H__
-#define CHROME_BROWSER_VIEWS_INFO_BAR_VIEW_H__
+#ifndef CHROME_BROWSER_VIEWS_INFO_BAR_VIEW_H_
+#define CHROME_BROWSER_VIEWS_INFO_BAR_VIEW_H_
 
 #include <map>
 
+#include "chrome/common/notification_service.h"
 #include "chrome/views/view.h"
 
 class NavigationEntry;
@@ -16,7 +17,8 @@ class WebContents;
 //
 // It will paint all of its children vertically, with the most recently
 // added child displayed at the top of the info bar.
-class InfoBarView : public ChromeViews::View {
+class InfoBarView : public ChromeViews::View,
+                    public NotificationObserver {
  public:
   explicit InfoBarView(WebContents* web_contents);
 
@@ -42,12 +44,6 @@ class InfoBarView : public ChromeViews::View {
 
   virtual void DidChangeBounds(const CRect& previous, const CRect& current);
 
-  // Notification that the user has done a navigation. Removes child views that
-  // are set to be removed after a certain number of navigations and
-  // potentially hides the info bar.  |entry| is the new entry that we are
-  // navigating to.
-  void DidNavigate(NavigationEntry* entry);
-
   WebContents* web_contents() { return web_contents_; }
 
  protected:
@@ -74,14 +70,19 @@ class InfoBarView : public ChromeViews::View {
                       ChromeViews::View* v1,
                       ChromeViews::View* v2);
 
+  // NotificationObserver implementation.
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
+
   WebContents* web_contents_;
 
   // Map from view to number of navigations before it is removed. If a child
   // doesn't have an entry in here, it is NOT removed on navigations.
   std::map<View*,int> expire_map_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(InfoBarView);
+  DISALLOW_COPY_AND_ASSIGN(InfoBarView);
 };
 
-#endif // CHROME_BROWSER_VIEWS_INFO_BAR_VIEW_H__
+#endif // CHROME_BROWSER_VIEWS_INFO_BAR_VIEW_H_
 
diff --git a/chrome/browser/web_contents.cc b/chrome/browser/web_contents.cc
index 45eec91..2367a9b 100644
--- a/chrome/browser/web_contents.cc
+++ b/chrome/browser/web_contents.cc
@@ -567,7 +567,7 @@ void WebContents::SavePage(const std::wstring& main_file,
 //   the ResourceDispatcherHost, who unpauses the response.  Data is then sent
 //   to the pending RVH.
 // - The pending renderer sends a FrameNavigate message that invokes the
-//   WebContents::DidNavigate method.  This replaces the current RVH with the
+//   DidNavigate method.  This replaces the current RVH with the
 //   pending RVH and goes back to the NORMAL RendererState.
 
 bool WebContents::Navigate(const NavigationEntry& entry, bool reload) {
@@ -1194,46 +1194,59 @@ void WebContents::DidNavigate(RenderViewHost* rvh,
     return;
   }
 
+  // Generate the details for the notification.
+  // TODO(brettw) bug 1343146: Move into the NavigationController.
+  NavigationController::LoadCommittedDetails details;
+  // WebKit doesn't set the "auto" transition on meta refreshes properly (bug
+  // 1051891) so we manually set it for redirects which we normally treat as
+  // "non-user-gestures" where we want to update stuff after navigations.
+  //
+  // Note that the redirect check also checks for a pending entry to
+  // differentiate real redirects from browser initiated navigations to a
+  // redirected entry. This happens when you hit back to go to a page that was
+  // the destination of a redirect, we don't want to treat it as a redirect
+  // even though that's what its transition will be. See bug  1117048.
+  //
+  // TODO(brettw) write a test for this complicated logic.
+  details.is_auto = (PageTransition::IsRedirect(params.transition) &&
+                     !controller()->GetPendingEntry()) ||
+      params.gesture == NavigationGestureAuto;
+  details.is_in_page = IsInPageNavigation(params.url);
+  details.is_main_frame = PageTransition::IsMainFrame(params.transition);
+
   // DO NOT ADD MORE STUFF TO THIS FUNCTION!  Don't make me come over there!
   // =======================================================================
-  // Add your code to DidNavigateAnyFramePreCommit if you have a helper object
+  // Add your code to DidNavigateAnyFramePostCommit if you have a helper object
   // that needs to know about all navigations. If it needs to do it only for
   // main frame or sub-frame navigations, add your code to
-  // DidNavigateMainFrame or DidNavigateSubFrame. If you need to run it after
-  // the navigation has been committed, put it in a *PostCommit version.
+  // DidNavigate*PostCommit.
 
   // Create the new navigation entry for this navigation and do work specific to
   // this frame type. The main frame / sub frame functions will do additional
   // updates to the NavigationEntry appropriate for the navigation type (in
   // addition to a lot of other stuff).
   scoped_ptr<NavigationEntry> entry(CreateNavigationEntryForCommit(params));
-  if (PageTransition::IsMainFrame(params.transition))
-    DidNavigateMainFramePreCommit(params, entry.get());
-  else
-    DidNavigateSubFramePreCommit(params, entry.get());
-
-  // Now we do non-frame-specific work in *AnyFramePreCommit (this depends on
-  // the entry being completed appropriately in the frame-specific versions
-  // above before the call).
-  DidNavigateAnyFramePreCommit(params, entry.get());
 
   // Commit the entry to the navigation controller.
-  DidNavigateToEntry(entry.release());
+  DidNavigateToEntry(entry.release(), &details);
   // WARNING: NavigationController will have taken ownership of entry at this
   // point, and may have deleted it. As such, do NOT use entry after this.
 
   // Run post-commit tasks.
-  if (PageTransition::IsMainFrame(params.transition))
-    DidNavigateMainFramePostCommit(params);
-  DidNavigateAnyFramePostCommit(rvh, params);
+  if (details.is_main_frame)
+    DidNavigateMainFramePostCommit(details, params);
+  DidNavigateAnyFramePostCommit(rvh, details, params);
 }
 
+// TODO(brettw) bug 1343146: This logic should be moved to NavigationController.
 NavigationEntry* WebContents::CreateNavigationEntryForCommit(
     const ViewHostMsg_FrameNavigate_Params& params) {
   // This new navigation entry will represent the navigation. Note that we
   // don't set the URL. This will happen in the DidNavigateMainFrame/SubFrame
   // because the entry's URL should represent the toplevel frame only.
   NavigationEntry* entry = new NavigationEntry(type());
+  if (PageTransition::IsMainFrame(params.transition))
+    entry->set_url(params.url);
   entry->set_page_id(params.page_id);
   entry->set_transition_type(params.transition);
   entry->set_site_instance(site_instance());
@@ -1273,97 +1286,103 @@ NavigationEntry* WebContents::CreateNavigationEntryForCommit(
     }
   }
 
+  if (PageTransition::IsMainFrame(params.transition)) {
+    NavigationEntry* pending = controller()->GetPendingEntry();
+    if (pending) {
+      // Copy fields from the pending NavigationEntry into the actual
+      // NavigationEntry that we're committing to.
+      entry->set_user_typed_url(pending->user_typed_url());
+      if (pending->has_display_url())
+        entry->set_display_url(pending->display_url());
+      if (pending->url().SchemeIsFile())
+        entry->set_title(pending->title());
+      entry->set_content_state(pending->content_state());
+    }
+    entry->set_has_post_data(params.is_post);
+  } else {
+    NavigationEntry* last_committed = controller()->GetLastCommittedEntry();
+    if (last_committed) {
+      // In the case of a sub-frame navigation within a window that was created
+      // without an URL (window.open), we may not have a committed entry yet!
+
+      // Reset entry state to match that of the pending entry.
+      entry->set_unique_id(last_committed->unique_id());
+      entry->set_url(last_committed->url());
+      entry->set_transition_type(last_committed->transition_type());
+      entry->set_user_typed_url(last_committed->user_typed_url());
+      entry->set_content_state(last_committed->content_state());
+
+      // TODO(jcampan): when navigating to an insecure/unsafe inner frame, the
+      // main entry is the one that gets notified of the mixed/unsafe contents
+      // (see SSLPolicy::OnRequestStarted()). Here we are just transferring
+      // that state. We should find a better way to do this. Note that it is OK
+      // that the mixed/unsafe contents is set on the wrong navigation entry, as
+      // that state is reset when navigating back to it.
+      DCHECK(last_committed->ssl().content_status() == 0) << "We should never "
+          "be setting the status bits from 1 to 0 on navigate.";
+      entry->ssl() = last_committed->ssl();
+    }
+  }
+
   return entry;
 }
 
-void WebContents::DidNavigateMainFramePreCommit(
-    const ViewHostMsg_FrameNavigate_Params& params,
-    NavigationEntry* entry) {
-  // Update contents MIME type of the main webframe.
-  contents_mime_type_ = params.contents_mime_type;
-
-  entry->set_url(params.url);
-
-  NavigationEntry* pending = controller()->GetPendingEntry();
-  if (pending) {
-    // Copy fields from the pending NavigationEntry into the actual
-    // NavigationEntry that we're committing to.
-    entry->set_user_typed_url(pending->user_typed_url());
-    if (pending->has_display_url())
-      entry->set_display_url(pending->display_url());
-    if (pending->url().SchemeIsFile())
-      entry->set_title(pending->title());
-    entry->set_content_state(pending->content_state());
+void WebContents::DidNavigateMainFramePostCommit(
+    const NavigationController::LoadCommittedDetails& details,
+    const ViewHostMsg_FrameNavigate_Params& params) {
+  // Hide the download shelf if all the following conditions are true:
+  // - there are no active downloads.
+  // - this is a navigation to a different TLD.
+  // - at least 5 seconds have elapsed since the download shelf was shown.
+  // TODO(jcampan): bug 1156075 when user gestures are reliable, they should
+  //                 be used to ensure we are hiding only on user initiated
+  //                 navigations.
+  DownloadManager* download_manager = profile()->GetDownloadManager();
+  // download_manager can be NULL in unit test context.
+  if (download_manager && download_manager->in_progress_count() == 0 &&
+      !details.previous_url.is_empty() &&
+      !net::RegistryControlledDomainService::SameDomainOrHost(
+          details.previous_url, details.entry->url())) {
+    TimeDelta time_delta(
+        TimeTicks::Now() - last_download_shelf_show_);
+    if (time_delta >
+        TimeDelta::FromMilliseconds(kDownloadShelfHideDelay)) {
+      SetDownloadShelfVisible(false);
+    }
   }
 
-  // We no longer know the title after this navigation.
-  has_page_title_ = false;
-
-  // Reset the starred button to false by default, but also request from
-  // history whether it's actually starred.
-  //
-  // Only save the URL in the entry for top-level frames. This will appear in
-  // the UI for the page, so we always want to use the toplevel URL.
-  //
-  // The |user_initiated_big_change| flag indicates whether we can tell the
-  // infobar/password manager about this navigation.  True for non-redirect,
-  // non-in-page user initiated navigations; assume this is true and set false
-  // below if not.
-  //
-  // TODO(pkasting): http://b/1048012 We should notify based on whether the
-  // navigation was triggered by a user action rather than most of our current
-  // heuristics.  Be careful with SSL infobars, though.
-  //
-  // See bug 1051891 for reasons why we need both a redirect check and gesture
-  // check; basically gesture checking is not always accurate.
-  //
-  // Note that the redirect check also checks for a pending entry to
-  // differentiate real redirects from browser initiated navigations to a
-  // redirected entry (like when you hit back to go to a page that was the
-  // destination of a redirect, we don't want to treat it as a redirect
-  // even though that's what its transition will be) http://b/1117048.
-  bool user_initiated_big_change = true;
-  if ((PageTransition::IsRedirect(entry->transition_type()) &&
-       !controller()->GetPendingEntry()) ||
-       (params.gesture == NavigationGestureAuto) ||
-       IsInPageNavigation(params.url)) {
-    user_initiated_big_change = false;
-  } else {
+  if (details.is_user_initiated_main_frame_load()) {
     // Clear the status bubble. This is a workaround for a bug where WebKit
     // doesn't let us know that the cursor left an element during a
     // transition (this is also why the mouse cursor remains as a hand after
     // clicking on a link); see bugs 1184641 and 980803. We don't want to
     // clear the bubble when a user navigates to a named anchor in the same
     // page.
-    UpdateTargetURL(params.page_id, GURL());
-  }
+    UpdateTargetURL(details.entry->page_id(), GURL());
 
-  // Let the infobar know about the navigation to give the infobar a chance
-  // to remove any views on navigating. Only do so if this navigation was
-  // initiated by the user, and we are not simply following a fragment to
-  // relocate within the current page.
-  //
-  // We must do this after calling DidNavigateToEntry(), since the info bar
-  // view checks the controller's active entry to determine whether to auto-
-  // expire any children.
-  if (user_initiated_big_change && IsInfoBarVisible()) {
-    InfoBarView* info_bar = GetInfoBarView();
-    DCHECK(info_bar);
-    info_bar->DidNavigate(entry);
+    // UpdateHelpersForDidNavigate will handle the case where the password_form
+    // origin is valid.
+    // TODO(brettw) bug 1343111: Password manager stuff in here needs to be
+    // cleaned up and covered by tests.
+    if (!params.password_form.origin.is_valid())
+      GetPasswordManager()->DidNavigate();
   }
 
-  // UpdateHelpersForDidNavigate will handle the case where the password_form
-  // origin is valid.
-  if (user_initiated_big_change && !params.password_form.origin.is_valid())
-    GetPasswordManager()->DidNavigate();
-
+  // The keyword generator uses the navigation entries, so must be called after
+  // the commit.
   GenerateKeywordIfNecessary(params);
 
-  // Close constrained popups if necessary.
-  MaybeCloseChildWindows(params);
+  // We no longer know the title after this navigation.
+  has_page_title_ = false;
+
+  // Update contents MIME type of the main webframe.
+  contents_mime_type_ = params.contents_mime_type;
 
   // Get the favicon, either from history or request it from the net.
-  fav_icon_helper_.FetchFavIcon(entry->url());
+  fav_icon_helper_.FetchFavIcon(details.entry->url());
+
+  // Close constrained popups if necessary.
+  MaybeCloseChildWindows(params);
 
   // We hide the FindInPage window when the user navigates away, except on
   // reload.
@@ -1371,66 +1390,14 @@ void WebContents::DidNavigateMainFramePreCommit(
       PageTransition::RELOAD)
     SetFindInPageVisible(false);
 
-  entry->set_has_post_data(params.is_post);
-}
-
-void WebContents::DidNavigateSubFramePreCommit(
-    const ViewHostMsg_FrameNavigate_Params& params,
-    NavigationEntry* entry) {
-  NavigationEntry* last_committed = controller()->GetLastCommittedEntry();
-  if (!last_committed) {
-    // In the case of a sub-frame navigation within a window that was created
-    // without an URL (via window.open), we may not have a committed entry yet!
-    return;
-  }
-
-  // Reset entry state to match that of the pending entry.
-  entry->set_unique_id(last_committed->unique_id());
-  entry->set_url(last_committed->url());
-  entry->set_transition_type(last_committed->transition_type());
-  entry->set_user_typed_url(last_committed->user_typed_url());
-  entry->set_content_state(last_committed->content_state());
-
-  // TODO(jcampan): when navigating to an insecure/unsafe inner frame, the
-  // main entry is the one that gets notified of the mixed/unsafe contents
-  // (see SSLPolicy::OnRequestStarted()).  Here we are just transferring that
-  // state.  We should find a better way to do this.
-  // Note that it is OK that the mixed/unsafe contents is set on the wrong
-  // navigation entry, as that state is reset when navigating back to it.
-  DCHECK(last_committed->ssl().content_status() == 0) << "We should never be "
-      "setting the status bits from 1 to 0 on navigate.";
-  entry->ssl() = last_committed->ssl();
+  // Update the starred state.
+  UpdateStarredStateForCurrentURL();
 }
 
-void WebContents::DidNavigateAnyFramePreCommit(
-    const ViewHostMsg_FrameNavigate_Params& params,
-    NavigationEntry* entry) {
-
-  // Hide the download shelf if all the following conditions are true:
-  // - there are no active downloads.
-  // - this is a navigation to a different TLD.
-  // - at least 5 seconds have elapsed since the download shelf was shown.
-  // TODO(jcampan): bug 1156075 when user gestures are reliable, they should
-  //                 be used to ensure we are hiding only on user initiated
-  //                 navigations.
-  NavigationEntry* current_entry = controller()->GetLastCommittedEntry();
-  DownloadManager* download_manager = profile()->GetDownloadManager();
-  // download_manager can be NULL in unit test context.
-  if (download_manager && download_manager ->in_progress_count() == 0 &&
-      current_entry && !net::RegistryControlledDomainService::SameDomainOrHost(
-          current_entry->url(), entry->url())) {
-    TimeDelta time_delta(
-        TimeTicks::Now() - last_download_shelf_show_);
-    if (time_delta >
-        TimeDelta::FromMilliseconds(kDownloadShelfHideDelay)) {
-      SetDownloadShelfVisible(false);
-    }
-  }
-
-  // Notify the password manager of the navigation or form submit.
-  if (params.password_form.origin.is_valid())
-    GetPasswordManager()->ProvisionallySavePassword(params.password_form);
-
+void WebContents::DidNavigateAnyFramePostCommit(
+    RenderViewHost* render_view_host,
+    const NavigationController::LoadCommittedDetails& details,
+    const ViewHostMsg_FrameNavigate_Params& params) {
   // If we navigate, start showing messages again. This does nothing to prevent
   // a malicious script from spamming messages, since the script could just
   // reload the page to stop blocking.
@@ -1441,27 +1408,20 @@ void WebContents::DidNavigateAnyFramePreCommit(
   if (params.should_update_history) {
     // Most of the time, the displayURL matches the loaded URL, but for about:
     // URLs, we use a data: URL as the real value.  We actually want to save
-    // the about: URL to the history db and keep the data: URL hidden.
-    UpdateHistoryForNavigation(entry->display_url(), params);
+    // the about: URL to the history db and keep the data: URL hidden. This is
+    // what the TabContents' URL getter does.
+    UpdateHistoryForNavigation(GetURL(), params);
   }
-}
 
-void WebContents::DidNavigateMainFramePostCommit(
-    const ViewHostMsg_FrameNavigate_Params& params) {
-  // The keyword generator uses the navigation entries, so must be called after
-  // the commit.
-  GenerateKeywordIfNecessary(params);
-
-  // Update the starred state.
-  UpdateStarredStateForCurrentURL();
-}
-
-void WebContents::DidNavigateAnyFramePostCommit(
-    RenderViewHost* render_view_host,
-    const ViewHostMsg_FrameNavigate_Params& params) {
   // Have the controller save the current session.
   controller()->NotifyEntryChangedByPageID(type(), site_instance(),
-                                           params.page_id);
+                                           details.entry->page_id());
+
+  // Notify the password manager of the navigation or form submit.
+  // TODO(brettw) bug 1343111: Password manager stuff in here needs to be
+  // cleaned up and covered by tests.
+  if (params.password_form.origin.is_valid())
+    GetPasswordManager()->ProvisionallySavePassword(params.password_form);
 
   BroadcastProvisionalLoadCommit(render_view_host, params);
 }
diff --git a/chrome/browser/web_contents.h b/chrome/browser/web_contents.h
index 87acbd5..43ba2e8 100644
--- a/chrome/browser/web_contents.h
+++ b/chrome/browser/web_contents.h
@@ -555,34 +555,16 @@ class WebContents : public TabContents,
   NavigationEntry* CreateNavigationEntryForCommit(
       const ViewHostMsg_FrameNavigate_Params& params);
 
-  // Handles post-navigation tasks specific to some set of frames. DidNavigate()
-  // calls these with newly created navigation entry for this navigation BEFORE
-  // that entry has been committed to the navigation controller. The functions
-  // can update the entry as needed.
-  //
-  // First the frame-specific version (main or sub) will be called to update the
-  // entry as needed after it was created by CreateNavigationEntryForCommit.
-  //
-  // Then DidNavigateAnyFramePreCommit will be called with the now-complete
-  // entry for further processing that is not specific to the type of frame.
-  void DidNavigateMainFramePreCommit(
-      const ViewHostMsg_FrameNavigate_Params& params,
-      NavigationEntry* entry);
-  void DidNavigateSubFramePreCommit(
-      const ViewHostMsg_FrameNavigate_Params& params,
-      NavigationEntry* entry);
-  void DidNavigateAnyFramePreCommit(
-      const ViewHostMsg_FrameNavigate_Params& params,
-      NavigationEntry* entry);
-
   // Handles post-navigation tasks in DidNavigate AFTER the entry has been
-  // committed to the navigation controller. See WillNavigate* above. Note that
-  // the navigation entry is not provided since it may be invalid/changed after
-  // being committed.
+  // committed to the navigation controller. Note that the navigation entry is
+  // not provided since it may be invalid/changed after being committed. The
+  // current navigation entry is in the NavigationController at this point.
   void DidNavigateMainFramePostCommit(
+      const NavigationController::LoadCommittedDetails& details,
       const ViewHostMsg_FrameNavigate_Params& params);
   void DidNavigateAnyFramePostCommit(
       RenderViewHost* render_view_host,
+      const NavigationController::LoadCommittedDetails& details,
       const ViewHostMsg_FrameNavigate_Params& params);
 
   // Called when navigating the main frame to close all child windows if the
diff --git a/chrome/common/notification_types.h b/chrome/common/notification_types.h
index 7bd9bb1..cff0655 100644
--- a/chrome/common/notification_types.h
+++ b/chrome/common/notification_types.h
@@ -43,6 +43,9 @@ enum NotificationType {
   // A new non-pending navigation entry has been created. This will correspond
   // to one NavigationController entry being created (in the case of new
   // navigations) or renavigated to (for back/forward navigations).
+  //
+  // The source will be the navigation controller doing the commit. The details
+  // will be NavigationController::LoadCommittedDetails.
   NOTIFY_NAV_ENTRY_COMMITTED,
 
   // Indicates that the NavigationController given in the Source has decreased