Revert 18512 - Revert 18373 Consider a redirect following user gesture as userinitiated in maintaining

navigation entries. Also, ignore redirect or machineinitiated new subframe
navigations.

The current code treats all redirects as machineinitiated in processing
navigation to a new page (to fix Bugs 9663 and 10531). This is not always
appropriate, because some sites, e.g., www.google.com/ig, use redirect to
implement userinitiated navigation (Bug 11896).

This change assumes that a machineinitiated redirect happens within 300ms
since the last document load was completed, while  a userinitiated one
happens later.

This assumption is not always correct, e.g., a user may cause transition within
300ms. But I cannot think of any better ways to tell if a redirect is machine
initiated or userinitiated.

I believe this change works good enough, at least better than the status quo.

Review URL: http://codereview.chromium.org/115919

TEST=Open http://www.hp.com and observe it redirects to
http://www.hp.com/#Product . Hit Back button and observe
the former URL is not visited. Open http://www.google.com/ig and
click tabs inside the page, and try hitting Back and Forward to see if the
navigation is right. Open http://www.google.com/codesearch, search for
something, click on a result item, and try hitting Back.

BUG=11896,12820





TBR=yuzo@chromium.org

Review URL: http://codereview.chromium.org/125202

TBR=laforge@chromium.org

Review URL: http://codereview.chromium.org/126221

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

4 files changed, 76 insertions, 5 deletions

diff --git a/chrome/browser/tab_contents/navigation_controller.cc b/chrome/browser/tab_contents/navigation_controller.cc
index eb6441a..b5ae80b 100644
--- a/chrome/browser/tab_contents/navigation_controller.cc
+++ b/chrome/browser/tab_contents/navigation_controller.cc
@@ -90,6 +90,12 @@ bool AreURLsInPageNavigation(const GURL& existing_url, const GURL& new_url) {
       new_url.ReplaceComponents(replacements);
 }
 
+// Navigation within this limit since the last document load is considered to
+// be automatic (i.e., machine-initiated) rather than user-initiated unless
+// a user gesture has been observed.
+const base::TimeDelta kMaxAutoNavigationTimeDelta =
+    base::TimeDelta::FromSeconds(5);
+
 }  // namespace
 
 // NavigationController ---------------------------------------------------
@@ -424,6 +430,14 @@ const SkBitmap& NavigationController::GetLazyFavIcon() const {
   }
 }
 
+void NavigationController::DocumentLoadedInFrame() {
+  last_document_loaded_ = base::TimeTicks::Now();
+}
+
+void NavigationController::OnUserGesture() {
+  user_gesture_observed_ = true;
+}
+
 bool NavigationController::RendererDidNavigate(
     const ViewHostMsg_FrameNavigate_Params& params,
     LoadCommittedDetails* details) {
@@ -505,6 +519,8 @@ bool NavigationController::RendererDidNavigate(
   details->http_status_code = params.http_status_code;
   NotifyNavigationEntryCommitted(details);
 
+  user_gesture_observed_ = false;
+
   return true;
 }
 
@@ -585,6 +601,21 @@ NavigationType::Type NavigationController::ClassifyNavigation(
   return NavigationType::EXISTING_PAGE;
 }
 
+bool NavigationController::IsRedirect(
+  const ViewHostMsg_FrameNavigate_Params& params) {
+  // For main frame transition, we judge by params.transition.
+  // Otherwise, by params.redirects.
+  if (PageTransition::IsMainFrame(params.transition)) {
+    return PageTransition::IsRedirect(params.transition);
+  }
+  return params.redirects.size() > 1;
+}
+
+bool NavigationController::IsLikelyAutoNavigation(base::TimeTicks now) {
+  return !user_gesture_observed_ &&
+         (now - last_document_loaded_) < kMaxAutoNavigationTimeDelta;
+}
+
 void NavigationController::RendererDidNavigateToNewPage(
     const ViewHostMsg_FrameNavigate_Params& params) {
   NavigationEntry* new_entry;
@@ -611,11 +642,14 @@ void NavigationController::RendererDidNavigateToNewPage(
   new_entry->set_site_instance(tab_contents_->GetSiteInstance());
   new_entry->set_has_post_data(params.is_post);
 
-  // If the current entry is a redirection source, it needs to be replaced with
-  // the new entry to avoid unwanted redirections in navigating backward /
-  // forward. Otherwise, just insert the new entry.
+  // If the current entry is a redirection source and the redirection has
+  // occurred within kMaxAutoNavigationTimeDelta since the last document load,
+  // this is likely to be machine-initiated redirect and the entry needs to be
+  // replaced with the new entry to avoid unwanted redirections in navigating
+  // backward/forward.
+  // Otherwise, just insert the new entry.
   InsertOrReplaceEntry(new_entry,
-      PageTransition::IsRedirect(new_entry->transition_type()));
+      IsRedirect(params) && IsLikelyAutoNavigation(base::TimeTicks::Now()));
 }
 
 void NavigationController::RendererDidNavigateToExistingPage(
@@ -687,11 +721,22 @@ void NavigationController::RendererDidNavigateInPage(
   NavigationEntry* new_entry = new NavigationEntry(*existing_entry);
   new_entry->set_page_id(params.page_id);
   new_entry->set_url(params.url);
-  InsertOrReplaceEntry(new_entry, false);
+  InsertOrReplaceEntry(new_entry,
+      IsRedirect(params) && IsLikelyAutoNavigation(base::TimeTicks::Now()));
 }
 
 void NavigationController::RendererDidNavigateNewSubframe(
     const ViewHostMsg_FrameNavigate_Params& params) {
+  if (PageTransition::StripQualifier(params.transition) ==
+      PageTransition::AUTO_SUBFRAME) {
+    // This is not user-initiated. Ignore.
+    return;
+  }
+  if (IsRedirect(params)) {
+    // This is redirect. Ignore.
+    return;
+  }
+
   // Manual subframe navigations just get the current entry cloned so the user
   // can go back or forward to it. The actual subframe information will be
   // stored in the page state for each of those entries. This happens out of
diff --git a/chrome/browser/tab_contents/navigation_controller.h b/chrome/browser/tab_contents/navigation_controller.h
index 5a7fba9..8419e0b 100644
--- a/chrome/browser/tab_contents/navigation_controller.h
+++ b/chrome/browser/tab_contents/navigation_controller.h
@@ -11,6 +11,7 @@
 
 #include "base/linked_ptr.h"
 #include "base/string16.h"
+#include "base/time.h"
 #include "googleurl/src/gurl.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/browser/ssl/ssl_manager.h"
@@ -288,6 +289,12 @@ class NavigationController {
     return tab_contents_;
   }
 
+  // Called when a document has been loaded in a frame.
+  void DocumentLoadedInFrame();
+
+  // Called when the user presses the mouse, enter key or space bar.
+  void OnUserGesture();
+
   // For use by TabContents ----------------------------------------------------
 
   // Handles updating the navigation state after the renderer has navigated.
@@ -432,6 +439,13 @@ class NavigationController {
   // Discards the transient entry.
   void DiscardTransientEntry();
 
+  // Returns true if the navigation is redirect.
+  bool IsRedirect(const ViewHostMsg_FrameNavigate_Params& params);
+
+  // Returns true if the navigation is likley to be automatic rather than
+  // user-initiated.
+  bool IsLikelyAutoNavigation(base::TimeTicks now);
+
   // ---------------------------------------------------------------------------
 
   // The user profile associated with this controller
@@ -491,6 +505,12 @@ class NavigationController {
   // Unique identifier of the window we're in. Used by session restore.
   SessionID window_id_;
 
+  // The time ticks at which the last document was loaded.
+  base::TimeTicks last_document_loaded_;
+
+  // Whether a user gesture has been observed since the last navigation.
+  bool user_gesture_observed_;
+
   // Should Reload check for post data? The default is true, but is set to false
   // when testing.
   static bool check_for_repost_;
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index 0380c2b..2d6d997 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -1841,6 +1841,10 @@ void TabContents::ProcessDOMUIMessage(const std::string& message,
   render_manager_.dom_ui()->ProcessDOMUIMessage(message, content);
 }
 
+void TabContents::DocumentLoadedInFrame() {
+  controller_.DocumentLoadedInFrame();
+}
+
 void TabContents::ProcessExternalHostMessage(const std::string& message,
                                              const std::string& origin,
                                              const std::string& target) {
@@ -2177,6 +2181,7 @@ void TabContents::OnUserGesture() {
   if (drm)
     drm->OnUserGesture(this);
 #endif
+  controller_.OnUserGesture();
 }
 
 void TabContents::OnFindReply(int request_id,
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index c344a8a..ddf42b5 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -783,6 +783,7 @@ class TabContents : public PageNavigator,
                                     int automation_id);
   virtual void ProcessDOMUIMessage(const std::string& message,
                                    const std::string& content);
+  virtual void DocumentLoadedInFrame();
   virtual void ProcessExternalHostMessage(const std::string& message,
                                           const std::string& origin,
                                           const std::string& target);