Apply following CLs which are applied for recent 2 years to

typed_url_change_processor.cc. And also port unittests from
typed_url_model_associator_unittest.cc.

https://codereview.chromium.org/1087773002
https://codereview.chromium.org/631253002
https://codereview.chromium.org/1126633005
https://codereview.chromium.org/1156583006
https://codereview.chromium.org/300843004
https://codereview.chromium.org/1304423002
https://codereview.chromium.org/1127353004
https://codereview.chromium.org/773103004
https://codereview.chromium.org/1231723004
https://chromiumcodereview.appspot.com/16770005
https://chromiumcodereview.appspot.com/694843002/

BUG=77819

Review URL: https://codereview.chromium.org/1410243006

Cr-Commit-Position: refs/heads/master@{#357386}

4 files changed, 254 insertions, 66 deletions

diff --git a/components/history/core/browser/history_backend.cc b/components/history/core/browser/history_backend.cc
index 6937454..542e952 100644
--- a/components/history/core/browser/history_backend.cc
+++ b/components/history/core/browser/history_backend.cc
@@ -2528,28 +2528,26 @@ void HistoryBackend::NotifyURLVisited(ui::PageTransition transition,
                                       const URLRow& row,
                                       const RedirectList& redirects,
                                       base::Time visit_time) {
-  URLRow url_info(row);
   if (typed_url_syncable_service_)
-    typed_url_syncable_service_->OnUrlVisited(transition, &url_info);
+    typed_url_syncable_service_->OnURLVisited(this, transition, row, redirects,
+                                              visit_time);
 
-  FOR_EACH_OBSERVER(
-      HistoryBackendObserver, observers_,
-      OnURLVisited(this, transition, url_info, redirects, visit_time));
+  FOR_EACH_OBSERVER(HistoryBackendObserver, observers_,
+                    OnURLVisited(this, transition, row, redirects, visit_time));
 
   if (delegate_)
-    delegate_->NotifyURLVisited(transition, url_info, redirects, visit_time);
+    delegate_->NotifyURLVisited(transition, row, redirects, visit_time);
 }
 
 void HistoryBackend::NotifyURLsModified(const URLRows& rows) {
-  URLRows changed_urls(rows);
   if (typed_url_syncable_service_)
-    typed_url_syncable_service_->OnUrlsModified(&changed_urls);
+    typed_url_syncable_service_->OnURLsModified(this, rows);
 
   FOR_EACH_OBSERVER(HistoryBackendObserver, observers_,
-                    OnURLsModified(this, changed_urls));
+                    OnURLsModified(this, rows));
 
   if (delegate_)
-    delegate_->NotifyURLsModified(changed_urls);
+    delegate_->NotifyURLsModified(rows);
 }
 
 void HistoryBackend::NotifyURLsDeleted(bool all_history,
@@ -2558,8 +2556,8 @@ void HistoryBackend::NotifyURLsDeleted(bool all_history,
                                        const std::set<GURL>& favicon_urls) {
   URLRows copied_rows(rows);
   if (typed_url_syncable_service_) {
-    typed_url_syncable_service_->OnUrlsDeleted(all_history, expired,
-                                               &copied_rows);
+    typed_url_syncable_service_->OnURLsDeleted(this, all_history, expired,
+                                               copied_rows, favicon_urls);
   }
 
   FOR_EACH_OBSERVER(
diff --git a/components/history/core/browser/typed_url_syncable_service.cc b/components/history/core/browser/typed_url_syncable_service.cc
index 932498c..7fb5410 100644
--- a/components/history/core/browser/typed_url_syncable_service.cc
+++ b/components/history/core/browser/typed_url_syncable_service.cc
@@ -263,9 +263,10 @@ syncer::SyncError TypedUrlSyncableService::ProcessSyncChanges(
   return syncer::SyncError();
 }
 
-void TypedUrlSyncableService::OnUrlsModified(URLRows* changed_urls) {
+void TypedUrlSyncableService::OnURLsModified(
+    history::HistoryBackend* history_backend,
+    const history::URLRows& changed_urls) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(changed_urls);
 
   if (processing_syncer_changes_)
     return;  // These are changes originating from us, ignore.
@@ -275,13 +276,12 @@ void TypedUrlSyncableService::OnUrlsModified(URLRows* changed_urls) {
   // Create SyncChangeList.
   syncer::SyncChangeList changes;
 
-  for (URLRows::iterator url = changed_urls->begin();
-       url != changed_urls->end(); ++url) {
+  for (const auto& row : changed_urls) {
     // Only care if the modified URL is typed.
-    if (url->typed_count() > 0) {
+    if (row.typed_count() >= 0) {
       // If there were any errors updating the sync node, just ignore them and
       // continue on to process the next URL.
-      CreateOrUpdateSyncNode(*url, &changes);
+      CreateOrUpdateSyncNode(row, &changes);
     }
   }
 
@@ -290,31 +290,37 @@ void TypedUrlSyncableService::OnUrlsModified(URLRows* changed_urls) {
     sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
-void TypedUrlSyncableService::OnUrlVisited(ui::PageTransition transition,
-                                           URLRow* row) {
+void TypedUrlSyncableService::OnURLVisited(
+    history::HistoryBackend* history_backend,
+    ui::PageTransition transition,
+    const history::URLRow& row,
+    const history::RedirectList& redirects,
+    base::Time visit_time) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(row);
 
   if (processing_syncer_changes_)
     return;  // These are changes originating from us, ignore.
   if (!sync_processor_.get())
     return;  // Sync processor not yet initialized, don't sync.
-  if (!ShouldSyncVisit(transition, row))
+  if (!ShouldSyncVisit(row.typed_count(), transition))
     return;
 
   // Create SyncChangeList.
   syncer::SyncChangeList changes;
 
-  CreateOrUpdateSyncNode(*row, &changes);
+  CreateOrUpdateSyncNode(row, &changes);
 
   // Send SyncChangeList to server if there are any changes.
   if (changes.size() > 0)
     sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
-void TypedUrlSyncableService::OnUrlsDeleted(bool all_history,
-                                            bool expired,
-                                            URLRows* rows) {
+void TypedUrlSyncableService::OnURLsDeleted(
+    history::HistoryBackend* history_backend,
+    bool all_history,
+    bool expired,
+    const history::URLRows& deleted_rows,
+    const std::set<GURL>& favicon_urls) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (processing_syncer_changes_)
@@ -335,27 +341,24 @@ void TypedUrlSyncableService::OnUrlsDeleted(bool all_history,
 
   if (all_history) {
     // Delete all synced typed urls.
-    for (std::set<GURL>::const_iterator url = synced_typed_urls_.begin();
-         url != synced_typed_urls_.end(); ++url) {
+    for (const auto& url : synced_typed_urls_) {
       VisitVector visits;
-      URLRow row(*url);
+      URLRow row(url);
       AddTypedUrlToChangeList(syncer::SyncChange::ACTION_DELETE, row, visits,
-                              url->spec(), &changes);
+                              url.spec(), &changes);
     }
     // Clear cache of server state.
     synced_typed_urls_.clear();
   } else {
-    DCHECK(rows);
     // Delete rows.
-    for (URLRows::const_iterator row = rows->begin(); row != rows->end();
-         ++row) {
+    for (const auto& row : deleted_rows) {
       // Add specifics to change list for all synced urls that were deleted.
-      if (synced_typed_urls_.find(row->url()) != synced_typed_urls_.end()) {
+      if (synced_typed_urls_.find(row.url()) != synced_typed_urls_.end()) {
         VisitVector visits;
-        AddTypedUrlToChangeList(syncer::SyncChange::ACTION_DELETE, *row, visits,
-                                row->url().spec(), &changes);
+        AddTypedUrlToChangeList(syncer::SyncChange::ACTION_DELETE, row, visits,
+                                row.url().spec(), &changes);
         // Delete typed url from cache.
-        synced_typed_urls_.erase(row->url());
+        synced_typed_urls_.erase(row.url());
       }
     }
   }
@@ -763,15 +766,8 @@ bool TypedUrlSyncableService::ShouldIgnoreVisits(
   return true;
 }
 
-bool TypedUrlSyncableService::ShouldSyncVisit(
-    ui::PageTransition page_transition,
-    URLRow* row) {
-  if (!row)
-    return false;
-  int typed_count = row->typed_count();
-  ui::PageTransition transition = ui::PageTransitionFromInt(
-      page_transition & ui::PAGE_TRANSITION_CORE_MASK);
-
+bool TypedUrlSyncableService::ShouldSyncVisit(int typed_count,
+                                              ui::PageTransition transition) {
   // Just use an ad-hoc criteria to determine whether to ignore this
   // notification. For most users, the distribution of visits is roughly a bell
   // curve with a long tail - there are lots of URLs with < 5 visits so we want
@@ -779,7 +775,8 @@ bool TypedUrlSyncableService::ShouldSyncVisit(
   // suggestions. But there are relatively few URLs with > 10 visits, and those
   // tend to be more broadly distributed such that there's no need to sync up
   // every visit to preserve their relative ordering.
-  return (transition == ui::PAGE_TRANSITION_TYPED && typed_count > 0 &&
+  return (ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_TYPED) &&
+          typed_count >= 0 &&
           (typed_count < kTypedUrlVisitThrottleThreshold ||
            (typed_count % kTypedUrlVisitThrottleMultiple) == 0));
 }
@@ -787,7 +784,7 @@ bool TypedUrlSyncableService::ShouldSyncVisit(
 bool TypedUrlSyncableService::CreateOrUpdateSyncNode(
     URLRow url,
     syncer::SyncChangeList* changes) {
-  DCHECK_GT(url.typed_count(), 0);
+  DCHECK_GE(url.typed_count(), 0);
 
   if (ShouldIgnoreUrl(url.url()))
     return true;
@@ -798,6 +795,15 @@ bool TypedUrlSyncableService::CreateOrUpdateSyncNode(
     DLOG(ERROR) << "Could not load visits for url: " << url.url();
     return false;
   }
+
+  if (std::find_if(visit_vector.begin(), visit_vector.end(),
+                   [](const history::VisitRow& visit) {
+                     return ui::PageTransitionCoreTypeIs(
+                         visit.transition, ui::PAGE_TRANSITION_TYPED);
+                   }) == visit_vector.end())
+    // This URL has no TYPED visits, don't sync it
+    return false;
+
   DCHECK(!visit_vector.empty());
 
   std::string title = url.url().spec();
@@ -956,6 +962,13 @@ bool TypedUrlSyncableService::FixupURLAndGetVisits(URLRow* url,
   // This is a workaround for http://crbug.com/84258.
   if (visits->empty()) {
     DVLOG(1) << "Found empty visits for URL: " << url->url();
+    if (url->last_visit().is_null()) {
+      // If modified URL is bookmarked, history backend treats it as modified
+      // even if all its visits are deleted. Return false to stop further
+      // processing because sync expects valid visit time for modified entry.
+      return false;
+    }
+
     VisitRow visit(url->id(), url->last_visit(), 0, ui::PAGE_TRANSITION_TYPED,
                    0);
     visits->push_back(visit);
diff --git a/components/history/core/browser/typed_url_syncable_service.h b/components/history/core/browser/typed_url_syncable_service.h
index 6c6e943..ed7d492 100644
--- a/components/history/core/browser/typed_url_syncable_service.h
+++ b/components/history/core/browser/typed_url_syncable_service.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/threading/thread_checker.h"
+#include "components/history/core/browser/history_backend_observer.h"
 #include "components/history/core/browser/history_types.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_data.h"
@@ -33,7 +34,8 @@ class HistoryBackend;
 class TypedUrlSyncableServiceTest;
 class URLRow;
 
-class TypedUrlSyncableService : public syncer::SyncableService {
+class TypedUrlSyncableService : public syncer::SyncableService,
+                                public history::HistoryBackendObserver {
  public:
   explicit TypedUrlSyncableService(HistoryBackend* history_backend);
   ~TypedUrlSyncableService() override;
@@ -52,10 +54,19 @@ class TypedUrlSyncableService : public syncer::SyncableService {
       const tracked_objects::Location& from_here,
       const syncer::SyncChangeList& change_list) override;
 
-  // Called directly by HistoryBackend when local url data changes.
-  void OnUrlsModified(URLRows* changed_urls);
-  void OnUrlVisited(ui::PageTransition transition, URLRow* row);
-  void OnUrlsDeleted(bool all_history, bool expired, URLRows* rows);
+  // history::HistoryBackendObserver:
+  void OnURLVisited(history::HistoryBackend* history_backend,
+                    ui::PageTransition transition,
+                    const history::URLRow& row,
+                    const history::RedirectList& redirects,
+                    base::Time visit_time) override;
+  void OnURLsModified(history::HistoryBackend* history_backend,
+                      const history::URLRows& changed_urls) override;
+  void OnURLsDeleted(history::HistoryBackend* history_backend,
+                     bool all_history,
+                     bool expired,
+                     const history::URLRows& deleted_rows,
+                     const std::set<GURL>& favicon_urls) override;
 
  private:
   friend class TypedUrlSyncableServiceTest;
@@ -142,7 +153,7 @@ class TypedUrlSyncableService : public syncer::SyncableService {
   // notification. We use this to throttle the number of sync changes we send
   // to the server so we don't hit the server for every
   // single typed URL visit.
-  bool ShouldSyncVisit(ui::PageTransition transition, URLRow* row);
+  bool ShouldSyncVisit(int typed_count, ui::PageTransition transition);
 
   // Utility routine that either updates an existing sync node or creates a
   // new one for the passed |typed_url| if one does not already exist. Returns
diff --git a/components/history/core/browser/typed_url_syncable_service_unittest.cc b/components/history/core/browser/typed_url_syncable_service_unittest.cc
index 47e3c98c..977506d 100644
--- a/components/history/core/browser/typed_url_syncable_service_unittest.cc
+++ b/components/history/core/browser/typed_url_syncable_service_unittest.cc
@@ -57,6 +57,14 @@ bool URLsEqual(URLRow& row, sync_pb::TypedUrlSpecifics& specifics) {
           (row.hidden() == specifics.hidden()));
 }
 
+bool URLsEqual(history::URLRow& lhs, history::URLRow& rhs) {
+  // Only compare synced fields (ignore typed_count and visit_count as those
+  // are maintained by the history subsystem).
+  return (lhs.url().spec().compare(rhs.url().spec()) == 0) &&
+         (lhs.title().compare(rhs.title()) == 0) &&
+         (lhs.hidden() == rhs.hidden());
+}
+
 void AddNewestVisit(ui::PageTransition transition,
                     int64 visit_time,
                     URLRow* url,
@@ -118,6 +126,19 @@ URLRow MakeTypedUrlRow(const std::string& url,
   return history_url;
 }
 
+static sync_pb::TypedUrlSpecifics MakeTypedUrlSpecifics(const char* url,
+                                                        const char* title,
+                                                        int64 last_visit,
+                                                        bool hidden) {
+  sync_pb::TypedUrlSpecifics typed_url;
+  typed_url.set_url(url);
+  typed_url.set_title(title);
+  typed_url.set_hidden(hidden);
+  typed_url.add_visits(last_visit);
+  typed_url.add_visit_transitions(ui::PAGE_TRANSITION_TYPED);
+  return typed_url;
+}
+
 class TestHistoryBackend;
 
 class TestHistoryBackendDelegate : public HistoryBackend::Delegate {
@@ -253,6 +274,15 @@ class TypedUrlSyncableServiceTest : public testing::Test {
   static history::VisitRow CreateVisit(ui::PageTransition type,
                                        int64 timestamp);
 
+  static const TypedUrlSyncableService::MergeResult DIFF_NONE =
+      TypedUrlSyncableService::DIFF_NONE;
+  static const TypedUrlSyncableService::MergeResult DIFF_UPDATE_NODE =
+      TypedUrlSyncableService::DIFF_UPDATE_NODE;
+  static const TypedUrlSyncableService::MergeResult DIFF_LOCAL_ROW_CHANGED =
+      TypedUrlSyncableService::DIFF_LOCAL_ROW_CHANGED;
+  static const TypedUrlSyncableService::MergeResult DIFF_LOCAL_VISITS_ADDED =
+      TypedUrlSyncableService::DIFF_LOCAL_VISITS_ADDED;
+
  protected:
   base::MessageLoop message_loop_;
   base::FilePath test_dir_;
@@ -297,14 +327,15 @@ bool TypedUrlSyncableServiceTest::BuildAndPushLocalChanges(
       int typed = i < num_typed_urls ? 1 : 0;
       VisitVector visits;
       visit_vectors->push_back(visits);
-      rows->push_back(MakeTypedUrlRow(
-          urls[i], "pie", typed, i + 3, false, &visit_vectors->back()));
+      rows->push_back(MakeTypedUrlRow(urls[i], kTitle, typed, i + 3, false,
+                                      &visit_vectors->back()));
       fake_history_backend_->SetVisitsForUrl(rows->back(),
                                              visit_vectors->back());
       changed_urls.push_back(rows->back());
     }
 
-    typed_url_sync_service_->OnUrlsModified(&changed_urls);
+    typed_url_sync_service_->OnURLsModified(fake_history_backend_.get(),
+                                            changed_urls);
   }
 
   // Check that communication with sync was successful.
@@ -445,7 +476,8 @@ TEST_F(TypedUrlSyncableServiceTest, UpdateLocalTypedUrl) {
   changed_urls.push_back(url_row);
 
   // Notify typed url sync service of the update.
-  typed_url_sync_service_->OnUrlsModified(&changed_urls);
+  typed_url_sync_service_->OnURLsModified(fake_history_backend_.get(),
+                                          changed_urls);
 
   ASSERT_EQ(1U, changes.size());
   ASSERT_TRUE(changes[0].IsValid());
@@ -504,7 +536,9 @@ TEST_F(TypedUrlSyncableServiceTest, ReloadVisitLocalTypedUrl) {
   changed_urls.push_back(url_row);
 
   // Notify typed url sync service of the update.
-  typed_url_sync_service_->OnUrlVisited(ui::PAGE_TRANSITION_RELOAD, &url_row);
+  typed_url_sync_service_->OnURLVisited(
+      fake_history_backend_.get(), ui::PAGE_TRANSITION_RELOAD, url_row,
+      RedirectList(), base::Time::FromInternalValue(7));
 
   ASSERT_EQ(0U, changes.size());
 
@@ -538,7 +572,9 @@ TEST_F(TypedUrlSyncableServiceTest, LinkVisitLocalTypedUrl) {
 
   ui::PageTransition transition = ui::PAGE_TRANSITION_LINK;
   // Notify typed url sync service of non-typed visit, expect no change.
-  typed_url_sync_service_->OnUrlVisited(transition, &url_row);
+  typed_url_sync_service_->OnURLVisited(fake_history_backend_.get(), transition,
+                                        url_row, RedirectList(),
+                                        base::Time::FromInternalValue(6));
   ASSERT_EQ(0u, changes.size());
 }
 
@@ -566,7 +602,9 @@ TEST_F(TypedUrlSyncableServiceTest, TypedVisitLocalTypedUrl) {
 
   // Notify typed url sync service of typed visit.
   ui::PageTransition transition = ui::PAGE_TRANSITION_TYPED;
-  typed_url_sync_service_->OnUrlVisited(transition, &url_row);
+  typed_url_sync_service_->OnURLVisited(fake_history_backend_.get(), transition,
+                                        url_row, RedirectList(),
+                                        base::Time::Now());
 
   ASSERT_EQ(1U, changes.size());
   ASSERT_TRUE(changes[0].IsValid());
@@ -636,7 +674,8 @@ TEST_F(TypedUrlSyncableServiceTest, DeleteLocalTypedUrl) {
   }
 
   // Notify typed url sync service.
-  typed_url_sync_service_->OnUrlsDeleted(false, false, &rows);
+  typed_url_sync_service_->OnURLsDeleted(fake_history_backend_.get(), false,
+                                         false, rows, std::set<GURL>());
 
   ASSERT_EQ(3u, changes.size());
   for (size_t i = 0; i < changes.size(); ++i) {
@@ -686,7 +725,9 @@ TEST_F(TypedUrlSyncableServiceTest, DeleteAllLocalTypedUrl) {
   bool all_history = true;
 
   // Notify typed url sync service.
-  typed_url_sync_service_->OnUrlsDeleted(all_history, false, NULL);
+  typed_url_sync_service_->OnURLsDeleted(fake_history_backend_.get(),
+                                         all_history, false, URLRows(),
+                                         std::set<GURL>());
 
   ASSERT_EQ(4u, changes.size());
   for (size_t i = 0; i < changes.size(); ++i) {
@@ -729,7 +770,9 @@ TEST_F(TypedUrlSyncableServiceTest, MaxVisitLocalTypedUrl) {
 
   // Notify typed url sync service of typed visit.
   ui::PageTransition transition = ui::PAGE_TRANSITION_TYPED;
-  typed_url_sync_service_->OnUrlVisited(transition, &url_row);
+  typed_url_sync_service_->OnURLVisited(fake_history_backend_.get(), transition,
+                                        url_row, RedirectList(),
+                                        base::Time::Now());
 
   syncer::SyncChangeList& changes = fake_change_processor_->changes();
   ASSERT_EQ(1U, changes.size());
@@ -780,7 +823,9 @@ TEST_F(TypedUrlSyncableServiceTest, ThrottleVisitLocalTypedUrl) {
 
   // Notify typed url sync service of typed visit.
   ui::PageTransition transition = ui::PAGE_TRANSITION_TYPED;
-  typed_url_sync_service_->OnUrlVisited(transition, &url_row);
+  typed_url_sync_service_->OnURLVisited(fake_history_backend_.get(), transition,
+                                        url_row, RedirectList(),
+                                        base::Time::Now());
 
   // Should throttle, so sync and local cache should not update.
   syncer::SyncChangeList& changes = fake_change_processor_->changes();
@@ -795,7 +840,9 @@ TEST_F(TypedUrlSyncableServiceTest, ThrottleVisitLocalTypedUrl) {
   fake_history_backend_->SetVisitsForUrl(url_row, visits);
 
   // Notify typed url sync service of typed visit.
-  typed_url_sync_service_->OnUrlVisited(transition, &url_row);
+  typed_url_sync_service_->OnURLVisited(fake_history_backend_.get(), transition,
+                                        url_row, RedirectList(),
+                                        base::Time::Now());
 
   ASSERT_EQ(1u, changes.size());
   ASSERT_TRUE(changes[0].IsValid());
@@ -1282,4 +1329,123 @@ TEST_F(TypedUrlSyncableServiceTest, NoTypedVisits) {
             ui::PageTransitionFromInt(typed_url.visit_transitions(0)));
 }
 
+TEST_F(TypedUrlSyncableServiceTest, MergeUrls) {
+  history::VisitVector visits1;
+  history::URLRow row1(MakeTypedUrlRow(kURL, kTitle, 2, 3, false, &visits1));
+  sync_pb::TypedUrlSpecifics specs1(
+      MakeTypedUrlSpecifics(kURL, kTitle, 3, false));
+  history::URLRow new_row1((GURL(kURL)));
+  std::vector<history::VisitInfo> new_visits1;
+  EXPECT_TRUE(TypedUrlSyncableServiceTest::MergeUrls(specs1, row1, &visits1,
+                                                     &new_row1, &new_visits1) ==
+              TypedUrlSyncableServiceTest::DIFF_NONE);
+
+  history::VisitVector visits2;
+  history::URLRow row2(MakeTypedUrlRow(kURL, kTitle, 2, 3, false, &visits2));
+  sync_pb::TypedUrlSpecifics specs2(
+      MakeTypedUrlSpecifics(kURL, kTitle, 3, true));
+  history::VisitVector expected_visits2;
+  history::URLRow expected2(
+      MakeTypedUrlRow(kURL, kTitle, 2, 3, true, &expected_visits2));
+  history::URLRow new_row2((GURL(kURL)));
+  std::vector<history::VisitInfo> new_visits2;
+  EXPECT_TRUE(TypedUrlSyncableServiceTest::MergeUrls(specs2, row2, &visits2,
+                                                     &new_row2, &new_visits2) ==
+              TypedUrlSyncableServiceTest::DIFF_LOCAL_ROW_CHANGED);
+  EXPECT_TRUE(URLsEqual(new_row2, expected2));
+
+  history::VisitVector visits3;
+  history::URLRow row3(MakeTypedUrlRow(kURL, kTitle, 2, 3, false, &visits3));
+  sync_pb::TypedUrlSpecifics specs3(
+      MakeTypedUrlSpecifics(kURL, kTitle2, 3, true));
+  history::VisitVector expected_visits3;
+  history::URLRow expected3(
+      MakeTypedUrlRow(kURL, kTitle2, 2, 3, true, &expected_visits3));
+  history::URLRow new_row3((GURL(kURL)));
+  std::vector<history::VisitInfo> new_visits3;
+  EXPECT_EQ(TypedUrlSyncableServiceTest::DIFF_LOCAL_ROW_CHANGED |
+                TypedUrlSyncableServiceTest::DIFF_NONE,
+            TypedUrlSyncableServiceTest::MergeUrls(specs3, row3, &visits3,
+                                                   &new_row3, &new_visits3));
+  EXPECT_TRUE(URLsEqual(new_row3, expected3));
+
+  // Create one node in history DB with timestamp of 3, and one node in sync
+  // DB with timestamp of 4. Result should contain one new item (4).
+  history::VisitVector visits4;
+  history::URLRow row4(MakeTypedUrlRow(kURL, kTitle, 2, 3, false, &visits4));
+  sync_pb::TypedUrlSpecifics specs4(
+      MakeTypedUrlSpecifics(kURL, kTitle2, 4, false));
+  history::VisitVector expected_visits4;
+  history::URLRow expected4(
+      MakeTypedUrlRow(kURL, kTitle2, 2, 4, false, &expected_visits4));
+  history::URLRow new_row4((GURL(kURL)));
+  std::vector<history::VisitInfo> new_visits4;
+  EXPECT_EQ(TypedUrlSyncableServiceTest::DIFF_UPDATE_NODE |
+                TypedUrlSyncableServiceTest::DIFF_LOCAL_ROW_CHANGED |
+                TypedUrlSyncableServiceTest::DIFF_LOCAL_VISITS_ADDED,
+            TypedUrlSyncableServiceTest::MergeUrls(specs4, row4, &visits4,
+                                                   &new_row4, &new_visits4));
+  EXPECT_EQ(1U, new_visits4.size());
+  EXPECT_EQ(specs4.visits(0), new_visits4[0].first.ToInternalValue());
+  EXPECT_TRUE(URLsEqual(new_row4, expected4));
+  EXPECT_EQ(2U, visits4.size());
+
+  history::VisitVector visits5;
+  history::URLRow row5(MakeTypedUrlRow(kURL, kTitle, 1, 4, false, &visits5));
+  sync_pb::TypedUrlSpecifics specs5(
+      MakeTypedUrlSpecifics(kURL, kTitle, 3, false));
+  history::VisitVector expected_visits5;
+  history::URLRow expected5(
+      MakeTypedUrlRow(kURL, kTitle, 2, 3, false, &expected_visits5));
+  history::URLRow new_row5((GURL(kURL)));
+  std::vector<history::VisitInfo> new_visits5;
+
+  // UPDATE_NODE should be set because row5 has a newer last_visit timestamp.
+  EXPECT_EQ(TypedUrlSyncableServiceTest::DIFF_UPDATE_NODE |
+                TypedUrlSyncableServiceTest::DIFF_NONE,
+            TypedUrlSyncableServiceTest::MergeUrls(specs5, row5, &visits5,
+                                                   &new_row5, &new_visits5));
+  EXPECT_TRUE(URLsEqual(new_row5, expected5));
+  EXPECT_EQ(0U, new_visits5.size());
+}
+
+TEST_F(TypedUrlSyncableServiceTest, MergeUrlsAfterExpiration) {
+  // Tests to ensure that we don't resurrect expired URLs (URLs that have been
+  // deleted from the history DB but still exist in the sync DB).
+
+  // First, create a history row that has two visits, with timestamps 2 and 3.
+  history::VisitVector(history_visits);
+  history_visits.push_back(history::VisitRow(
+      0, base::Time::FromInternalValue(2), 0, ui::PAGE_TRANSITION_TYPED, 0));
+  history::URLRow history_url(
+      MakeTypedUrlRow(kURL, kTitle, 2, 3, false, &history_visits));
+
+  // Now, create a sync node with visits at timestamps 1, 2, 3, 4.
+  sync_pb::TypedUrlSpecifics node(
+      MakeTypedUrlSpecifics(kURL, kTitle, 1, false));
+  node.add_visits(2);
+  node.add_visits(3);
+  node.add_visits(4);
+  node.add_visit_transitions(2);
+  node.add_visit_transitions(3);
+  node.add_visit_transitions(4);
+  history::URLRow new_history_url(history_url.url());
+  std::vector<history::VisitInfo> new_visits;
+  EXPECT_EQ(
+      TypedUrlSyncableServiceTest::DIFF_NONE |
+          TypedUrlSyncableServiceTest::DIFF_LOCAL_VISITS_ADDED,
+      TypedUrlSyncableServiceTest::MergeUrls(node, history_url, &history_visits,
+                                             &new_history_url, &new_visits));
+  EXPECT_TRUE(URLsEqual(history_url, new_history_url));
+  EXPECT_EQ(1U, new_visits.size());
+  EXPECT_EQ(4U, new_visits[0].first.ToInternalValue());
+  // We should not sync the visit with timestamp #1 since it is earlier than
+  // any other visit for this URL in the history DB. But we should sync visit
+  // #4.
+  EXPECT_EQ(3U, history_visits.size());
+  EXPECT_EQ(2U, history_visits[0].visit_time.ToInternalValue());
+  EXPECT_EQ(3U, history_visits[1].visit_time.ToInternalValue());
+  EXPECT_EQ(4U, history_visits[2].visit_time.ToInternalValue());
+}
+
 }  // namespace history