Now does not sync URLs that only have imported visits.

During ModelAssociation we ignore URLs that only contain imported visits. We
still look at them in the change processor because we still want to sync them
if the user ever visits the URLs in Chrome.

BUG=99963
TEST=Run firefox, visit URLs, run chrome with fresh profile, see imported URLs are not synced.

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

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

8 files changed, 120 insertions, 11 deletions

diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index cc11b94..45f12ef 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -1000,6 +1000,15 @@ bool HistoryBackend::RemoveVisits(const VisitVector& visits) {
   return true;
 }
 
+bool HistoryBackend::GetVisitsSource(const VisitVector& visits,
+                                     VisitSourceMap* sources) {
+  if (!db_.get())
+    return false;
+
+  db_->GetVisitsSource(visits, sources);
+  return true;
+}
+
 bool HistoryBackend::GetURL(const GURL& url, history::URLRow* url_row) {
   if (db_.get())
     return db_->GetRowForURL(url, url_row) != 0;
diff --git a/chrome/browser/history/history_backend.h b/chrome/browser/history/history_backend.h
index 4c0f503..057b0f5 100644
--- a/chrome/browser/history/history_backend.h
+++ b/chrome/browser/history/history_backend.h
@@ -301,6 +301,11 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
 
   virtual bool RemoveVisits(const VisitVector& visits);
 
+  // Returns the VisitSource associated with each one of the passed visits.
+  // If there is no entry in the map for a given visit, that means the visit
+  // was SOURCE_BROWSED. Returns false if there is no HistoryDatabase..
+  bool GetVisitsSource(const VisitVector& visits, VisitSourceMap* sources);
+
   virtual bool GetURL(const GURL& url, history::URLRow* url_row);
 
   // Deleting ------------------------------------------------------------------
diff --git a/chrome/browser/history/history_backend_unittest.cc b/chrome/browser/history/history_backend_unittest.cc
index e72dbfd..9840e09 100644
--- a/chrome/browser/history/history_backend_unittest.cc
+++ b/chrome/browser/history/history_backend_unittest.cc
@@ -700,7 +700,7 @@ TEST_F(HistoryBackendTest, AddPageVisitSource) {
   // Check if all the visits to the url are stored in database.
   ASSERT_EQ(3U, visits.size());
   VisitSourceMap visit_sources;
-  backend_->db_->GetVisitsSource(visits, &visit_sources);
+  ASSERT_TRUE(backend_->GetVisitsSource(visits, &visit_sources));
   ASSERT_EQ(3U, visit_sources.size());
   int sources = 0;
   for (int i = 0; i < 3; i++) {
@@ -754,7 +754,7 @@ TEST_F(HistoryBackendTest, AddPageArgsSource) {
   ASSERT_TRUE(backend_->db()->GetVisitsForURL(id, &visits));
   ASSERT_EQ(3U, visits.size());
   VisitSourceMap visit_sources;
-  backend_->db_->GetVisitsSource(visits, &visit_sources);
+  ASSERT_TRUE(backend_->GetVisitsSource(visits, &visit_sources));
   ASSERT_EQ(1U, visit_sources.size());
   EXPECT_EQ(history::SOURCE_SYNCED, visit_sources.begin()->second);
 }
@@ -793,14 +793,14 @@ TEST_F(HistoryBackendTest, AddVisitsSource) {
   ASSERT_TRUE(backend_->db()->GetVisitsForURL(id, &visits));
   ASSERT_EQ(3U, visits.size());
   VisitSourceMap visit_sources;
-  backend_->db_->GetVisitsSource(visits, &visit_sources);
+  ASSERT_TRUE(backend_->GetVisitsSource(visits, &visit_sources));
   ASSERT_EQ(3U, visit_sources.size());
   for (int i = 0; i < 3; i++)
     EXPECT_EQ(history::SOURCE_IE_IMPORTED, visit_sources[visits[i].visit_id]);
   id = backend_->db()->GetRowForURL(url2, &row);
   ASSERT_TRUE(backend_->db()->GetVisitsForURL(id, &visits));
   ASSERT_EQ(2U, visits.size());
-  backend_->db_->GetVisitsSource(visits, &visit_sources);
+  ASSERT_TRUE(backend_->GetVisitsSource(visits, &visit_sources));
   ASSERT_EQ(2U, visit_sources.size());
   for (int i = 0; i < 2; i++)
     EXPECT_EQ(history::SOURCE_SYNCED, visit_sources[visits[i].visit_id]);
@@ -925,12 +925,12 @@ TEST_F(HistoryBackendTest, RemoveVisitsSource) {
 
   // Now check only url2's source in visit_source table.
   VisitSourceMap visit_sources;
-  backend_->db_->GetVisitsSource(visits, &visit_sources);
+  ASSERT_TRUE(backend_->GetVisitsSource(visits, &visit_sources));
   ASSERT_EQ(0U, visit_sources.size());
   id = backend_->db()->GetRowForURL(url2, &row);
   ASSERT_TRUE(backend_->db()->GetVisitsForURL(id, &visits));
   ASSERT_EQ(2U, visits.size());
-  backend_->db_->GetVisitsSource(visits, &visit_sources);
+  ASSERT_TRUE(backend_->GetVisitsSource(visits, &visit_sources));
   ASSERT_EQ(2U, visit_sources.size());
   for (int i = 0; i < 2; i++)
     EXPECT_EQ(history::SOURCE_SYNCED, visit_sources[visits[i].visit_id]);
diff --git a/chrome/browser/sync/glue/typed_url_model_associator.cc b/chrome/browser/sync/glue/typed_url_model_associator.cc
index f49711f..ed471d8 100644
--- a/chrome/browser/sync/glue/typed_url_model_associator.cc
+++ b/chrome/browser/sync/glue/typed_url_model_associator.cc
@@ -100,6 +100,27 @@ bool TypedUrlModelAssociator::FixupURLAndGetVisits(
   return true;
 }
 
+bool TypedUrlModelAssociator::ShouldIgnoreUrl(
+    const history::URLRow& url, const history::VisitVector& visits) {
+  // We ignore URLs that where imported, but have never been visited by
+  // chromium.
+  static const int kLastImportedSource = history::SOURCE_EXTENSION;
+  history::VisitSourceMap map;
+  if (!history_backend_->GetVisitsSource(visits, &map))
+    return false;  // If we can't read the visit, assume it's not imported.
+
+  // Walk the list of visits and look for a non-imported item.
+  for (history::VisitVector::const_iterator it = visits.begin();
+       it != visits.end(); ++it) {
+    if (map.count(it->visit_id) == 0 ||
+        map[it->visit_id] <= kLastImportedSource) {
+      return false;
+    }
+  }
+  // We only saw imported visits, so tell the caller to ignore them.
+  return true;
+}
+
 bool TypedUrlModelAssociator::AssociateModels(SyncError* error) {
   VLOG(1) << "Associating TypedUrl Models";
   DCHECK(expected_loop_ == MessageLoop::current());
@@ -116,7 +137,7 @@ bool TypedUrlModelAssociator::AssociateModels(SyncError* error) {
   // Get all the visits.
   std::map<history::URLID, history::VisitVector> visit_vectors;
   for (std::vector<history::URLRow>::iterator ix = typed_urls.begin();
-       ix != typed_urls.end(); ++ix) {
+       ix != typed_urls.end();) {
     if (IsAbortPending())
       return false;
     if (!FixupURLAndGetVisits(
@@ -124,6 +145,11 @@ bool TypedUrlModelAssociator::AssociateModels(SyncError* error) {
       error->Reset(FROM_HERE, "Could not get the url's visits.", model_type());
       return false;
     }
+
+    if (ShouldIgnoreUrl(*ix, visit_vectors[ix->id()]))
+      ix = typed_urls.erase(ix);
+    else
+      ++ix;
   }
 
   TypedUrlTitleVector titles;
diff --git a/chrome/browser/sync/glue/typed_url_model_associator.h b/chrome/browser/sync/glue/typed_url_model_associator.h
index 87de4e5..e875ecc 100644
--- a/chrome/browser/sync/glue/typed_url_model_associator.h
+++ b/chrome/browser/sync/glue/typed_url_model_associator.h
@@ -166,6 +166,11 @@ class TypedUrlModelAssociator
   // association map.
   bool IsAssociated(const std::string& node_tag);
 
+  // Helper function that determines if we should ignore a URL for the purposes
+  // of sync, because it's import-only.
+  bool ShouldIgnoreUrl(const history::URLRow& url,
+                       const history::VisitVector& visits);
+
   ProfileSyncService* sync_service_;
   history::HistoryBackend* history_backend_;
   int64 typed_url_node_id_;
diff --git a/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc b/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
index 0e0b6f5..f1ee3f2 100644
--- a/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
+++ b/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
@@ -12,6 +12,7 @@
 #include "chrome/browser/sync/test/integration/typed_urls_helper.h"
 
 using typed_urls_helper::AddUrlToHistory;
+using typed_urls_helper::AddUrlToHistoryWithTransition;
 using typed_urls_helper::AssertAllProfilesHaveSameURLsAsVerifier;
 using typed_urls_helper::DeleteUrlFromHistory;
 using typed_urls_helper::GetTypedUrlsFromClient;
@@ -176,3 +177,44 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest,
   // Both clients should have this URL added again.
   AssertAllProfilesHaveSameURLsAsVerifier();
 }
+
+IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest,
+                       SkipImportedVisits) {
+
+  GURL imported_url("http://imported_url.com");
+  GURL browsed_url("http://browsed_url.com");
+  GURL browsed_and_imported_url("http://browsed_and_imported_url.com");
+  ASSERT_TRUE(SetupClients());
+
+  // Create 3 items in our first client - 1 imported, one browsed, one with
+  // both imported and browsed entries.
+  AddUrlToHistoryWithTransition(0, imported_url,
+                                content::PAGE_TRANSITION_TYPED,
+                                history::SOURCE_FIREFOX_IMPORTED);
+  AddUrlToHistoryWithTransition(0, browsed_url,
+                                content::PAGE_TRANSITION_TYPED,
+                                history::SOURCE_BROWSED);
+  AddUrlToHistoryWithTransition(0, browsed_and_imported_url,
+                                content::PAGE_TRANSITION_TYPED,
+                                history::SOURCE_FIREFOX_IMPORTED);
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  std::vector<history::URLRow> urls = GetTypedUrlsFromClient(1);
+  ASSERT_EQ(1U, urls.size());
+  ASSERT_EQ(browsed_url, urls[0].url());
+
+  // Now browse to 3rd URL - this should cause it to be synced, even though it
+  // was initially imported.
+  AddUrlToHistoryWithTransition(0, browsed_and_imported_url,
+                                content::PAGE_TRANSITION_TYPED,
+                                history::SOURCE_BROWSED);
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  urls = GetTypedUrlsFromClient(1);
+  ASSERT_EQ(2U, urls.size());
+
+  // Make sure the imported URL didn't make it over.
+  for (size_t i = 0; i < urls.size(); ++i) {
+    ASSERT_NE(imported_url, urls[i].url());
+  }
+}
diff --git a/chrome/browser/sync/test/integration/typed_urls_helper.cc b/chrome/browser/sync/test/integration/typed_urls_helper.cc
index de67e74..d8c29be 100644
--- a/chrome/browser/sync/test/integration/typed_urls_helper.cc
+++ b/chrome/browser/sync/test/integration/typed_urls_helper.cc
@@ -66,18 +66,21 @@ void WaitForHistoryDBThread(int index) {
   wait_event.Wait();
 }
 
-// Creates a URLRow in the specified HistoryService.
+// Creates a URLRow in the specified HistoryService with the passed transition
+// type.
 void AddToHistory(HistoryService* service,
                   const GURL& url,
+                  content::PageTransition transition,
+                  history::VisitSource source,
                   const base::Time& timestamp) {
   service->AddPage(url,
                    timestamp,
                    NULL, // scope
                    1234, // page_id
                    GURL(),  // referrer
-                   content::PAGE_TRANSITION_TYPED,
+                   transition,
                    history::RedirectList(),
-                   history::SOURCE_BROWSED,
+                   source,
                    false);
 }
 
@@ -117,14 +120,25 @@ base::Time GetTimestamp() {
 }
 
 void AddUrlToHistory(int index, const GURL& url) {
+  AddUrlToHistoryWithTransition(index, url, content::PAGE_TRANSITION_TYPED,
+                                history::SOURCE_BROWSED);
+}
+void AddUrlToHistoryWithTransition(int index,
+                                   const GURL& url,
+                                   content::PageTransition transition,
+                                   history::VisitSource source) {
   base::Time timestamp = GetTimestamp();
   AddToHistory(test()->GetProfile(index)->GetHistoryServiceWithoutCreating(),
                url,
+               transition,
+               source,
                timestamp);
   if (test()->use_verifier())
     AddToHistory(
         test()->verifier()->GetHistoryService(Profile::IMPLICIT_ACCESS),
         url,
+        transition,
+        source,
         timestamp);
 
   // Wait until the AddPage() request has completed so we know the change has
diff --git a/chrome/browser/sync/test/integration/typed_urls_helper.h b/chrome/browser/sync/test/integration/typed_urls_helper.h
index b7923b17..86b99c8 100644
--- a/chrome/browser/sync/test/integration/typed_urls_helper.h
+++ b/chrome/browser/sync/test/integration/typed_urls_helper.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "chrome/browser/history/history_types.h"
+#include "content/public/common/page_transition_types.h"
 
 namespace base {
 class Time;
@@ -20,9 +21,16 @@ namespace typed_urls_helper {
 std::vector<history::URLRow> GetTypedUrlsFromClient(int index);
 
 // Adds a URL to the history DB for a specific sync profile (just registers a
-// new visit if the URL already exists).
+// new visit if the URL already exists) using a TYPED PageTransition.
 void AddUrlToHistory(int index, const GURL& url);
 
+// Adds a URL to the history DB for a specific sync profile (just registers a
+// new visit if the URL already exists), using the passed PageTransition.
+void AddUrlToHistoryWithTransition(int index,
+                                   const GURL& url,
+                                   content::PageTransition transition,
+                                   history::VisitSource source);
+
 // Deletes a URL from the history DB for a specific sync profile.
 void DeleteUrlFromHistory(int index, const GURL& url);