Fix: Certain redirections remove sites from the history

Currently, PageTransition::CHAIN_END flag is removed from a History database
entry for a redirect source, even when the redirect is user initiated.

This change prevents the flag removal for user-initiated redirects.

TEST=Open http://www.google.com/ig and click on tabs multiple times. Without
this change, only the last tab clicked appears in the History page (CTRL+H).
With this change, all the tabs should appear.

TESTED=gcl try, manually

BUG=11355

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

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

7 files changed, 107 insertions, 30 deletions

diff --git a/chrome/browser/history/history.cc b/chrome/browser/history/history.cc
index 01d3e35..49b76c6 100644
--- a/chrome/browser/history/history.cc
+++ b/chrome/browser/history/history.cc
@@ -253,8 +253,10 @@ void HistoryService::AddPage(const GURL& url,
                              int32 page_id,
                              const GURL& referrer,
                              PageTransition::Type transition,
-                             const RedirectList& redirects) {
-  AddPage(url, Time::Now(), id_scope, page_id, referrer, transition, redirects);
+                             const RedirectList& redirects,
+                             bool did_replace_entry) {
+  AddPage(url, Time::Now(), id_scope, page_id, referrer, transition, redirects,
+          did_replace_entry);
 }
 
 void HistoryService::AddPage(const GURL& url,
@@ -263,7 +265,8 @@ void HistoryService::AddPage(const GURL& url,
                              int32 page_id,
                              const GURL& referrer,
                              PageTransition::Type transition,
-                             const RedirectList& redirects) {
+                             const RedirectList& redirects,
+                             bool did_replace_entry) {
   DCHECK(history_backend_) << "History service being called after cleanup";
 
   // Filter out unwanted URLs. We don't add auto-subframe URLs. They are a
@@ -292,7 +295,8 @@ void HistoryService::AddPage(const GURL& url,
 
   scoped_refptr<history::HistoryAddPageArgs> request(
       new history::HistoryAddPageArgs(url, time, id_scope, page_id,
-                                      referrer, redirects, transition));
+                                      referrer, redirects, transition,
+                                      did_replace_entry));
   ScheduleAndForget(PRIORITY_NORMAL, &HistoryBackend::AddPage, request);
 }
 
diff --git a/chrome/browser/history/history.h b/chrome/browser/history/history.h
index 8ca8878..fcea84f 100644
--- a/chrome/browser/history/history.h
+++ b/chrome/browser/history/history.h
@@ -155,13 +155,18 @@ class HistoryService : public CancelableRequestProvider,
   // one entry). If there are no redirects, this array may also be empty for
   // the convenience of callers.
   //
+  // 'did_replace_entry' is true when the navigation entry for this page has
+  // replaced the existing entry. A non-user initiated redirect causes such
+  // replacement.
+  //
   // All "Add Page" functions will update the visited link database.
   void AddPage(const GURL& url,
                const void* id_scope,
                int32 page_id,
                const GURL& referrer,
                PageTransition::Type transition,
-               const RedirectList& redirects);
+               const RedirectList& redirects,
+               bool did_replace_entry);
 
   // For adding pages to history with a specific time. This is for testing
   // purposes. Call the previous one to use the current time.
@@ -171,12 +176,13 @@ class HistoryService : public CancelableRequestProvider,
                int32 page_id,
                const GURL& referrer,
                PageTransition::Type transition,
-               const RedirectList& redirects);
+               const RedirectList& redirects,
+               bool did_replace_entry);
 
   // For adding pages to history where no tracking information can be done.
   void AddPage(const GURL& url) {
     AddPage(url, NULL, 0, GURL::EmptyGURL(), PageTransition::LINK,
-            RedirectList());
+            RedirectList(), false);
   }
 
   // Sets the title for the given page. The page should be in history. If it
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index 3200355..7060c45 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -402,12 +402,13 @@ void HistoryBackend::AddPage(scoped_refptr<HistoryAddPageArgs> request) {
         DCHECK(request->referrer == request->redirects[0]);
         request->redirects.erase(request->redirects.begin());
 
-        // Make sure to remove the CHAIN_END marker from the first visit. This
+        // If the navigation entry for this visit has replaced that for the
+        // first visit, remove the CHAIN_END marker from the first visit. This
         // can be called a lot, for example, the page cycler, and most of the
         // time we won't have changed anything.
-        // TODO(brettw) this should be unit tested.
         VisitRow visit_row;
-        if (db_->GetRowForVisit(last_ids.second, &visit_row) &&
+        if (request->did_replace_entry &&
+            db_->GetRowForVisit(last_ids.second, &visit_row) &&
             visit_row.transition | PageTransition::CHAIN_END) {
           visit_row.transition &= ~PageTransition::CHAIN_END;
           db_->UpdateVisitRow(visit_row);
diff --git a/chrome/browser/history/history_backend_unittest.cc b/chrome/browser/history/history_backend_unittest.cc
index e6c2879..beb73fa 100644
--- a/chrome/browser/history/history_backend_unittest.cc
+++ b/chrome/browser/history/history_backend_unittest.cc
@@ -70,10 +70,43 @@ class HistoryBackendTest : public testing::Test {
     scoped_refptr<history::HistoryAddPageArgs> request(
         new history::HistoryAddPageArgs(
             redirects.back(), Time::Now(), scope, page_id, GURL(),
-            redirects, PageTransition::LINK));
+            redirects, PageTransition::LINK, true));
     backend_->AddPage(request);
   }
 
+  // Adds CLIENT_REDIRECT page transition.
+  // |url1| is the source URL and |url2| is the destination.
+  // |did_replace| is true if the transition is non-user initiated and the
+  // navigation entry for |url2| has replaced that for |url1|. The possibly
+  // updated transition code of the visit records for |url1| and |url2| is
+  // returned by filling in |*transition1| and |*transition2|, respectively.
+  void  AddClientRedirect(const GURL& url1, const GURL& url2, bool did_replace,
+                          int* transition1, int* transition2) {
+    void* const dummy_scope = reinterpret_cast<void*>(0x87654321);
+    HistoryService::RedirectList redirects;
+    if (url1.is_valid())
+      redirects.push_back(url1);
+    if (url2.is_valid())
+      redirects.push_back(url2);
+    scoped_refptr<HistoryAddPageArgs> request(
+        new HistoryAddPageArgs(url2, base::Time(), dummy_scope, 0, url1,
+            redirects, PageTransition::CLIENT_REDIRECT, did_replace));
+    backend_->AddPage(request);
+
+    *transition1 = getTransition(url1);
+    *transition2 = getTransition(url2);
+  }
+
+  int getTransition(const GURL& url) {
+    if (!url.is_valid())
+      return 0;
+    URLRow row;
+    URLID id = backend_->db()->GetRowForURL(url, &row);
+    VisitVector visits;
+    EXPECT_TRUE(backend_->db()->GetVisitsForURL(id, &visits));
+    return visits[0].transition;
+  }
+
   BookmarkModel bookmark_model_;
 
  protected:
@@ -406,7 +439,7 @@ TEST_F(HistoryBackendTest, KeywordGenerated) {
   scoped_refptr<HistoryAddPageArgs> request(
       new HistoryAddPageArgs(url, visit_time, NULL, 0, GURL(),
                              HistoryService::RedirectList(),
-                             PageTransition::KEYWORD_GENERATED));
+                             PageTransition::KEYWORD_GENERATED, false));
   backend_->AddPage(request);
 
   // A row should have been added for the url.
@@ -444,4 +477,28 @@ TEST_F(HistoryBackendTest, KeywordGenerated) {
   ASSERT_EQ(0, backend_->db()->GetRowForURL(url, &row));
 }
 
+TEST_F(HistoryBackendTest, ClientRedirect) {
+  ASSERT_TRUE(backend_.get());
+
+  int transition1;
+  int transition2;
+
+  // Initial transition to page A.
+  GURL url_a("http://google.com/a");
+   AddClientRedirect(GURL(), url_a, false, &transition1, &transition2);
+  EXPECT_TRUE(transition2 & PageTransition::CHAIN_END);
+
+  // User initiated redirect to page B.
+  GURL url_b("http://google.com/b");
+   AddClientRedirect(url_a, url_b, false, &transition1, &transition2);
+  EXPECT_TRUE(transition1 & PageTransition::CHAIN_END);
+  EXPECT_TRUE(transition2 & PageTransition::CHAIN_END);
+
+  // Non-user initiated redirect to page C.
+  GURL url_c("http://google.com/c");
+   AddClientRedirect(url_b, url_c, true, &transition1, &transition2);
+  EXPECT_FALSE(transition1 & PageTransition::CHAIN_END);
+  EXPECT_TRUE(transition2 & PageTransition::CHAIN_END);
+}
+
 }  // namespace history
diff --git a/chrome/browser/history/history_marshaling.h b/chrome/browser/history/history_marshaling.h
index bf558c8..a6f449c 100644
--- a/chrome/browser/history/history_marshaling.h
+++ b/chrome/browser/history/history_marshaling.h
@@ -26,14 +26,16 @@ class HistoryAddPageArgs : public base::RefCounted<HistoryAddPageArgs> {
                      int32 arg_page_id,
                      const GURL& arg_referrer,
                      const HistoryService::RedirectList& arg_redirects,
-                     PageTransition::Type arg_transition)
+                     PageTransition::Type arg_transition,
+                     bool arg_did_replace_entry)
       : url(arg_url),
         time(arg_time),
         id_scope(arg_id_scope),
         page_id(arg_page_id),
         referrer(arg_referrer),
         redirects(arg_redirects),
-        transition(arg_transition) {
+        transition(arg_transition),
+        did_replace_entry(arg_did_replace_entry){
   }
 
   GURL url;
@@ -45,6 +47,7 @@ class HistoryAddPageArgs : public base::RefCounted<HistoryAddPageArgs> {
   GURL referrer;
   HistoryService::RedirectList redirects;
   PageTransition::Type transition;
+  bool did_replace_entry;
 
  private:
   DISALLOW_EVIL_CONSTRUCTORS(HistoryAddPageArgs);
diff --git a/chrome/browser/history/history_querying_unittest.cc b/chrome/browser/history/history_querying_unittest.cc
index 72c3825..e9625c3 100644
--- a/chrome/browser/history/history_querying_unittest.cc
+++ b/chrome/browser/history/history_querying_unittest.cc
@@ -108,7 +108,8 @@ class HistoryQueryTest : public testing::Test {
       GURL url(test_entries[i].url);
 
       history_->AddPage(url, test_entries[i].time, id_scope, page_id, GURL(),
-                        PageTransition::LINK, HistoryService::RedirectList());
+                        PageTransition::LINK, HistoryService::RedirectList(),
+                        false);
       history_->SetPageTitle(url, test_entries[i].title);
       history_->SetPageContents(url, test_entries[i].body);
     }
diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc
index 2474e6c..6915bf2 100644
--- a/chrome/browser/history/history_unittest.cc
+++ b/chrome/browser/history/history_unittest.cc
@@ -384,7 +384,7 @@ TEST_F(HistoryTest, AddPage) {
   const GURL test_url("http://www.google.com/");
   history->AddPage(test_url, NULL, 0, GURL(),
                    PageTransition::MANUAL_SUBFRAME,
-                   HistoryService::RedirectList());
+                   HistoryService::RedirectList(), false);
   EXPECT_TRUE(QueryURL(history, test_url));
   EXPECT_EQ(1, query_url_row_.visit_count());
   EXPECT_EQ(0, query_url_row_.typed_count());
@@ -392,7 +392,7 @@ TEST_F(HistoryTest, AddPage) {
 
   // Add the page once from the main frame (should unhide it).
   history->AddPage(test_url, NULL, 0, GURL(), PageTransition::LINK,
-                   HistoryService::RedirectList());
+                   HistoryService::RedirectList(), false);
   EXPECT_TRUE(QueryURL(history, test_url));
   EXPECT_EQ(2, query_url_row_.visit_count());  // Added twice.
   EXPECT_EQ(0, query_url_row_.typed_count());  // Never typed.
@@ -415,14 +415,14 @@ TEST_F(HistoryTest, AddPageSameTimes) {
   // additions have different timestamps.
   history->AddPage(test_urls[0], now, NULL, 0, GURL(),
                    PageTransition::LINK,
-                   HistoryService::RedirectList());
+                   HistoryService::RedirectList(), false);
   EXPECT_TRUE(QueryURL(history, test_urls[0]));
   EXPECT_EQ(1, query_url_row_.visit_count());
   EXPECT_TRUE(now == query_url_row_.last_visit());  // gtest doesn't like Time
 
   history->AddPage(test_urls[1], now, NULL, 0, GURL(),
                    PageTransition::LINK,
-                   HistoryService::RedirectList());
+                   HistoryService::RedirectList(), false);
   EXPECT_TRUE(QueryURL(history, test_urls[1]));
   EXPECT_EQ(1, query_url_row_.visit_count());
   EXPECT_TRUE(now + TimeDelta::FromMicroseconds(1) ==
@@ -432,7 +432,7 @@ TEST_F(HistoryTest, AddPageSameTimes) {
   history->AddPage(test_urls[2], now + TimeDelta::FromMinutes(1),
                    NULL, 0, GURL(),
                    PageTransition::LINK,
-                   HistoryService::RedirectList());
+                   HistoryService::RedirectList(), false);
   EXPECT_TRUE(QueryURL(history, test_urls[2]));
   EXPECT_EQ(1, query_url_row_.visit_count());
   EXPECT_TRUE(now + TimeDelta::FromMinutes(1) ==
@@ -455,7 +455,7 @@ TEST_F(HistoryTest, AddRedirect) {
   // Add the sequence of pages as a server with no referrer. Note that we need
   // to have a non-NULL page ID scope.
   history->AddPage(first_redirects.back(), MakeFakeHost(1), 0, GURL(),
-                   PageTransition::LINK, first_redirects);
+                   PageTransition::LINK, first_redirects, true);
 
   // The first page should be added once with a link visit type (because we set
   // LINK when we added the original URL, and a referrer of nowhere (0).
@@ -493,7 +493,7 @@ TEST_F(HistoryTest, AddRedirect) {
                    second_redirects[0],
                    static_cast<PageTransition::Type>(PageTransition::LINK |
                        PageTransition::CLIENT_REDIRECT),
-                   second_redirects);
+                   second_redirects, true);
 
   // The last page (source of the client redirect) should NOT have an
   // additional visit added, because it was a client redirect (normally it
@@ -518,7 +518,7 @@ TEST_F(HistoryTest, Typed) {
   // Add the page once as typed.
   const GURL test_url("http://www.google.com/");
   history->AddPage(test_url, NULL, 0, GURL(), PageTransition::TYPED,
-                   HistoryService::RedirectList());
+                   HistoryService::RedirectList(), false);
   EXPECT_TRUE(QueryURL(history, test_url));
 
   // We should have the same typed & visit count.
@@ -527,7 +527,7 @@ TEST_F(HistoryTest, Typed) {
 
   // Add the page again not typed.
   history->AddPage(test_url, NULL, 0, GURL(), PageTransition::LINK,
-                   HistoryService::RedirectList());
+                   HistoryService::RedirectList(), false);
   EXPECT_TRUE(QueryURL(history, test_url));
 
   // The second time should not have updated the typed count.
@@ -536,7 +536,8 @@ TEST_F(HistoryTest, Typed) {
 
   // Add the page again as a generated URL.
   history->AddPage(test_url, NULL, 0, GURL(),
-                   PageTransition::GENERATED, HistoryService::RedirectList());
+                   PageTransition::GENERATED, HistoryService::RedirectList(),
+                   false);
   EXPECT_TRUE(QueryURL(history, test_url));
 
   // This should have worked like a link click.
@@ -545,7 +546,8 @@ TEST_F(HistoryTest, Typed) {
 
   // Add the page again as a reload.
   history->AddPage(test_url, NULL, 0, GURL(),
-                   PageTransition::RELOAD, HistoryService::RedirectList());
+                   PageTransition::RELOAD, HistoryService::RedirectList(),
+                   false);
   EXPECT_TRUE(QueryURL(history, test_url));
 
   // This should not have incremented any visit counts.
@@ -594,7 +596,8 @@ TEST_F(HistoryTest, Segments) {
   // Add a URL.
   const GURL existing_url("http://www.google.com/");
   history->AddPage(existing_url, scope, 0, GURL(),
-                   PageTransition::TYPED, HistoryService::RedirectList());
+                   PageTransition::TYPED, HistoryService::RedirectList(),
+                   false);
 
   // Make sure a segment was created.
   history->QuerySegmentUsageSince(
@@ -612,7 +615,8 @@ TEST_F(HistoryTest, Segments) {
   // Add a URL which doesn't create a segment.
   const GURL link_url("http://yahoo.com/");
   history->AddPage(link_url, scope, 0, GURL(),
-                   PageTransition::LINK, HistoryService::RedirectList());
+                   PageTransition::LINK, HistoryService::RedirectList(),
+                   false);
 
   // Query again
   history->QuerySegmentUsageSince(
@@ -629,7 +633,8 @@ TEST_F(HistoryTest, Segments) {
 
   // Add a page linked from existing_url.
   history->AddPage(GURL("http://www.google.com/foo"), scope, 3, existing_url,
-                   PageTransition::LINK, HistoryService::RedirectList());
+                   PageTransition::LINK, HistoryService::RedirectList(),
+                   false);
 
   // Query again
   history->QuerySegmentUsageSince(
@@ -758,7 +763,7 @@ HistoryAddPageArgs* MakeAddArgs(const GURL& url) {
                                 0,
                                 GURL(),
                                 HistoryService::RedirectList(),
-                                PageTransition::TYPED);
+                                PageTransition::TYPED, false);
 }
 
 // Convenience version of the above to convert a char string.