Refactored TypedUrlChangeProcessor to treat ADD and UPDATE similarly.

Updated TypedUrlChangeProcessor to handle ADDs and UPDATEs in the sync DB identically.

Also removed our code that explicitly sets the title in the history DB as that
should not be necessary (it's already set by UpdateURLRow).

Finally, removed our code that was setting last_visit_time since that is also
updated automatically by the history code when we add/remove visits. We still
set it when we initially add the URL to the DB, but that's only because the
history code requires it - we technically don't need to since we always manually
add visits immediately afterward.

BUG=101633
TEST=existing tests suffice


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

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

chrome/browser/sync/glue/typed_url_change_processor.cc

@@ -205,9 +205,9 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel(
     return;
   }
 
-  DCHECK(pending_titles_.empty() && pending_new_urls_.empty() &&
-         pending_new_visits_.empty() && pending_deleted_visits_.empty() &&
-         pending_updated_urls_.empty() && pending_deleted_urls_.empty());
+  DCHECK(pending_new_urls_.empty() && pending_new_visits_.empty() &&
+         pending_deleted_visits_.empty() && pending_updated_urls_.empty() &&
+         pending_deleted_urls_.empty());
 
   for (sync_api::ChangeRecordList::const_iterator it =
            changes.Get().begin(); it != changes.Get().end(); ++it) {
@@ -237,67 +237,21 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel(
 
     const sync_pb::TypedUrlSpecifics& typed_url(
         sync_node.GetTypedUrlSpecifics());
-    GURL url(typed_url.url());
-
-    if (sync_api::ChangeRecord::ACTION_ADD == it->action) {
-      DCHECK(typed_url.visits_size());
-      if (!typed_url.visits_size()) {
-        continue;
-      }
+    DCHECK(typed_url.visits_size());
+    if (!typed_url.visits_size()) {
+      continue;
+    }
 
-      // TODO(atwilson): Combine this with the UPDATE code below
-      // (http://crbug.com/101633).
-      if (!model_associator_->UpdateFromNewTypedUrl(
-              typed_url, &pending_new_visits_, &pending_updated_urls_,
-              &pending_new_urls_)) {
-        error_handler()->OnUnrecoverableError(
-            FROM_HERE, "Could not get existing url's visits.");
-        return;
-      }
+    if (!model_associator_->UpdateFromSyncDB(
+            typed_url, &pending_new_visits_, &pending_deleted_visits_,
+            &pending_updated_urls_, &pending_new_urls_)) {
+      error_handler()->OnUnrecoverableError(
+          FROM_HERE, "Could not get existing url's visits.");
+      return;
+    }
 
+    if (it->action == sync_api::ChangeRecord::ACTION_ADD) {
       model_associator_->Associate(&typed_url.url(), it->id);
-    } else {
-      DCHECK_EQ(sync_api::ChangeRecord::ACTION_UPDATE, it->action);
-      history::URLRow old_url;
-      if (!history_backend_->GetURL(url, &old_url)) {
-        LOG(ERROR) << "Could not fetch history row for " << url;
-        continue;
-      }
-
-      history::VisitVector visits;
-      if (!TypedUrlModelAssociator::FixupURLAndGetVisits(
-              history_backend_, &old_url, &visits)) {
-        error_handler()->OnUnrecoverableError(FROM_HERE,
-            "Could not get the url's visits.");
-        return;
-      }
-
-      history::URLRow new_url(old_url);
-      TypedUrlModelAssociator::UpdateURLRowFromTypedUrlSpecifics(
-          typed_url, &new_url);
-
-      pending_updated_urls_.push_back(
-        std::pair<history::URLID, history::URLRow>(old_url.id(), new_url));
-
-      if (old_url.title().compare(new_url.title()) != 0) {
-        pending_titles_.push_back(
-            std::pair<GURL, string16>(new_url.url(), new_url.title()));
-      }
-
-      std::vector<history::VisitInfo> added_visits;
-      history::VisitVector removed_visits;
-      TypedUrlModelAssociator::DiffVisits(visits, typed_url,
-                                          &added_visits, &removed_visits);
-      if (added_visits.size()) {
-        pending_new_visits_.push_back(
-            std::pair<GURL, std::vector<history::VisitInfo> >(
-                url, added_visits));
-      }
-      if (removed_visits.size()) {
-        pending_deleted_visits_.insert(pending_deleted_visits_.end(),
-                                       removed_visits.begin(),
-                                       removed_visits.end());
-      }
     }
   }
 }
@@ -313,8 +267,7 @@ void TypedUrlChangeProcessor::CommitChangesFromSyncModel() {
   if (!pending_deleted_urls_.empty())
     history_backend_->DeleteURLs(pending_deleted_urls_);
 
-  if (!model_associator_->WriteToHistoryBackend(&pending_titles_,
-                                                &pending_new_urls_,
+  if (!model_associator_->WriteToHistoryBackend(&pending_new_urls_,
                                                 &pending_updated_urls_,
                                                 &pending_new_visits_,
                                                 &pending_deleted_visits_)) {
@@ -323,7 +276,6 @@ void TypedUrlChangeProcessor::CommitChangesFromSyncModel() {
     return;
   }
 
-  pending_titles_.clear();
   pending_new_urls_.clear();
   pending_updated_urls_.clear();
   pending_new_visits_.clear();

chrome/browser/sync/glue/typed_url_change_processor.h

@@ -109,7 +109,6 @@ class TypedUrlChangeProcessor : public ChangeProcessor,
 
   // The set of pending changes that will be written out on the next
   // CommitChangesFromSyncModel() call.
-  TypedUrlModelAssociator::TypedUrlTitleVector pending_titles_;
   TypedUrlModelAssociator::TypedUrlVector pending_new_urls_;
   TypedUrlModelAssociator::TypedUrlUpdateVector pending_updated_urls_;
   std::vector<GURL> pending_deleted_urls_;

chrome/browser/sync/glue/typed_url_model_associator.cc

@@ -154,7 +154,6 @@ bool TypedUrlModelAssociator::AssociateModels(SyncError* error) {
       ++ix;
   }
 
-  TypedUrlTitleVector titles;
   TypedUrlVector new_urls;
   TypedUrlVisitVector new_visits;
   TypedUrlUpdateVector updated_urls;
@@ -231,10 +230,6 @@ bool TypedUrlModelAssociator::AssociateModels(SyncError* error) {
                     visits.back().visit_time.ToInternalValue());
           WriteToSyncNode(new_url, visits, &write_node);
         }
-        if (difference & DIFF_LOCAL_TITLE_CHANGED) {
-          titles.push_back(std::pair<GURL, string16>(new_url.url(),
-                                                     new_url.title()));
-        }
         if (difference & DIFF_LOCAL_ROW_CHANGED) {
           updated_urls.push_back(
               std::pair<history::URLID, history::URLRow>(ix->id(), new_url));
@@ -298,8 +293,11 @@ bool TypedUrlModelAssociator::AssociateModels(SyncError* error) {
       }
 
       if (current_urls.find(typed_url.url()) == current_urls.end()) {
-        if (!UpdateFromNewTypedUrl(typed_url, &new_visits, &updated_urls,
-                                   &new_urls)) {
+        // Update the local DB from the sync DB. Since we are doing our
+        // initial model association, we don't want to remove any of the
+        // existing visits (pass NULL as |visits_to_remove|).
+        if (!UpdateFromSyncDB(typed_url, &new_visits, NULL, &updated_urls,
+                              &new_urls)) {
             error->Reset(FROM_HERE, "Could not get existing url's visits.",
                          model_type());
             return false;
@@ -335,7 +333,7 @@ bool TypedUrlModelAssociator::AssociateModels(SyncError* error) {
   // this is the only thread that writes to the database.  We also don't have
   // to worry about the sync model getting out of sync, because changes are
   // propagated to the ChangeProcessor on this thread.
-  if (!WriteToHistoryBackend(&titles, &new_urls, &updated_urls,
+  if (!WriteToHistoryBackend(&new_urls, &updated_urls,
                              &new_visits, NULL)) {
     error->Reset(FROM_HERE,
                  "Failed to write to history backend",
@@ -345,9 +343,10 @@ bool TypedUrlModelAssociator::AssociateModels(SyncError* error) {
   return true;
 }
 
-bool TypedUrlModelAssociator::UpdateFromNewTypedUrl(
+bool TypedUrlModelAssociator::UpdateFromSyncDB(
     const sync_pb::TypedUrlSpecifics& typed_url,
     TypedUrlVisitVector* visits_to_add,
+    history::VisitVector* visits_to_remove,
     TypedUrlUpdateVector* updated_urls,
     TypedUrlVector* new_urls) {
   history::URLRow new_url(GURL(typed_url.url()));
@@ -364,12 +363,13 @@ bool TypedUrlModelAssociator::UpdateFromNewTypedUrl(
       return false;
     }
   }
+
   // Update the URL with information from the typed URL.
-  TypedUrlModelAssociator::UpdateURLRowFromTypedUrlSpecifics(
-      typed_url, &new_url);
+  UpdateURLRowFromTypedUrlSpecifics(typed_url, &new_url);
 
   // Figure out which visits we need to add.
-  DiffVisits(existing_visits, typed_url, &visits_to_add->back().second, NULL);
+  DiffVisits(existing_visits, typed_url, &visits_to_add->back().second,
+             visits_to_remove);
 
   if (existing_url) {
     updated_urls->push_back(
@@ -485,17 +485,10 @@ bool TypedUrlModelAssociator::GetSyncIdForTaggedNode(const std::string& tag,
 }
 
 bool TypedUrlModelAssociator::WriteToHistoryBackend(
-    const TypedUrlTitleVector* titles,
     const TypedUrlVector* new_urls,
     const TypedUrlUpdateVector* updated_urls,
     const TypedUrlVisitVector* new_visits,
     const history::VisitVector* deleted_visits) {
-  if (titles) {
-    for (TypedUrlTitleVector::const_iterator title = titles->begin();
-         title != titles->end(); ++title) {
-      history_backend_->SetPageTitle(title->first, title->second);
-    }
-  }
   if (new_urls) {
 #ifndef NDEBUG
     // All of these URLs should already have been associated.
@@ -525,6 +518,9 @@ bool TypedUrlModelAssociator::WriteToHistoryBackend(
     for (TypedUrlVisitVector::const_iterator visits = new_visits->begin();
          visits != new_visits->end(); ++visits) {
       DCHECK(IsAssociated(visits->first.spec()));
+      // If there are no visits to add, just skip this.
+      if (visits->second.empty())
+        continue;
       if (!history_backend_->AddVisits(visits->first, visits->second,
                                        history::SOURCE_SYNCED)) {
         LOG(ERROR) << "Could not add visits.";
@@ -577,11 +573,6 @@ TypedUrlModelAssociator::MergeResult TypedUrlModelAssociator::MergeUrls(
       new_url->set_title(node_title);
       new_url->set_hidden(node.hidden());
       different |= DIFF_LOCAL_ROW_CHANGED;
-
-      // If we're changing the local title, note this.
-      if (new_url->title().compare(url.title()) != 0) {
-        different |= DIFF_LOCAL_TITLE_CHANGED;
-      }
     } else {
       new_url->set_title(url.title());
       new_url->set_hidden(url.hidden());
@@ -816,8 +807,12 @@ void TypedUrlModelAssociator::UpdateURLRowFromTypedUrlSpecifics(
   CHECK_EQ(typed_url.visit_transitions_size(), typed_url.visits_size());
   new_url->set_title(UTF8ToUTF16(typed_url.title()));
   new_url->set_hidden(typed_url.hidden());
-  new_url->set_last_visit(base::Time::FromInternalValue(
-      typed_url.visits(typed_url.visits_size() - 1)));
+  // Only provide the initial value for the last_visit field - after that, let
+  // the history code update the last_visit field on its own.
+  if (new_url->last_visit().is_null()) {
+    new_url->set_last_visit(base::Time::FromInternalValue(
+        typed_url.visits(typed_url.visits_size() - 1)));
+  }
 }
 
 bool TypedUrlModelAssociator::CryptoReadyIfNecessary() {

chrome/browser/sync/glue/typed_url_model_associator.h

@@ -45,7 +45,6 @@ extern const char kTypedUrlTag[];
 class TypedUrlModelAssociator
   : public AbortablePerDataTypeAssociatorInterface<std::string, std::string> {
  public:
-  typedef std::vector<std::pair<GURL, string16> > TypedUrlTitleVector;
   typedef std::vector<history::URLRow> TypedUrlVector;
   typedef std::vector<std::pair<history::URLID, history::URLRow> >
       TypedUrlUpdateVector;
@@ -95,39 +94,37 @@ class TypedUrlModelAssociator
   // |sync_id| with that node's id.
   virtual bool GetSyncIdForTaggedNode(const std::string& tag, int64* sync_id);
 
-  bool WriteToHistoryBackend(const TypedUrlTitleVector* titles,
-                             const TypedUrlVector* new_urls,
+  bool WriteToHistoryBackend(const TypedUrlVector* new_urls,
                              const TypedUrlUpdateVector* updated_urls,
                              const TypedUrlVisitVector* new_visits,
                              const history::VisitVector* deleted_visits);
 
-  // Given a new typed URL in the sync DB, looks for an existing entry in the
+  // Given a typed URL in the sync DB, looks for an existing entry in the
   // local history DB and generates a list of visits to add to the
   // history DB to bring it up to date (avoiding duplicates).
-  // Updates the passed |visits_to_add| vector with the visits to add to the
-  // history DB, and adds a new entry to either |updated_urls| or |new_urls|
-  // depending on whether the URL already existed in the history DB.
+  // Updates the passed |visits_to_add| and |visits_to_remove| vectors with the
+  // visits to add to/remove from the history DB, and adds a new entry to either
+  // |updated_urls| or |new_urls| depending on whether the URL already existed
+  // in the history DB.
   // Returns false if we encountered an error trying to access the history DB.
-  bool UpdateFromNewTypedUrl(const sync_pb::TypedUrlSpecifics& typed_url,
-                             TypedUrlVisitVector* visits_to_add,
-                             TypedUrlUpdateVector* updated_urls,
-                             TypedUrlVector* new_urls);
+  bool UpdateFromSyncDB(const sync_pb::TypedUrlSpecifics& typed_url,
+                        TypedUrlVisitVector* visits_to_add,
+                        history::VisitVector* visits_to_remove,
+                        TypedUrlUpdateVector* updated_urls,
+                        TypedUrlVector* new_urls);
 
   // Bitfield returned from MergeUrls to specify the result of the merge.
   typedef uint32 MergeResult;
   static const MergeResult DIFF_NONE                = 0;
   static const MergeResult DIFF_UPDATE_NODE         = 1 << 0;
-  static const MergeResult DIFF_LOCAL_TITLE_CHANGED = 1 << 1;
-  static const MergeResult DIFF_LOCAL_ROW_CHANGED   = 1 << 2;
-  static const MergeResult DIFF_LOCAL_VISITS_ADDED  = 1 << 3;
+  static const MergeResult DIFF_LOCAL_ROW_CHANGED   = 1 << 1;
+  static const MergeResult DIFF_LOCAL_VISITS_ADDED  = 1 << 2;
 
   // Merges the URL information in |typed_url| with the URL information from the
   // history database in |url| and |visits|, and returns a bitmask with the
   // results of the merge:
   // DIFF_UPDATE_NODE - changes have been made to |new_url| and |visits| which
   //   should be persisted to the sync node.
-  // DIFF_LOCAL_TITLE_CHANGED - The title has changed, so the title in |new_url|
-  //   should be persisted to the history DB.
   // DIFF_LOCAL_ROW_CHANGED - The history data in |new_url| should be persisted
   //   to the history DB.
   // DIFF_LOCAL_VISITS_ADDED - |new_visits| contains a list of visits that

chrome/browser/sync/glue/typed_url_model_associator_unittest.cc

@@ -103,7 +103,7 @@ TEST_F(TypedUrlModelAssociatorTest, MergeUrls) {
   history::URLRow new_row3(GURL("http://pie.com/"));
   std::vector<history::VisitInfo> new_visits3;
   EXPECT_EQ(TypedUrlModelAssociator::DIFF_LOCAL_ROW_CHANGED |
-            TypedUrlModelAssociator::DIFF_LOCAL_TITLE_CHANGED,
+            TypedUrlModelAssociator::DIFF_NONE,
             TypedUrlModelAssociator::MergeUrls(specs3, row3, &visits3,
                                                &new_row3, &new_visits3));
   EXPECT_TRUE(URLsEqual(new_row3, expected3));
@@ -122,7 +122,6 @@ TEST_F(TypedUrlModelAssociatorTest, MergeUrls) {
   history::URLRow new_row4(GURL("http://pie.com/"));
   std::vector<history::VisitInfo> new_visits4;
   EXPECT_EQ(TypedUrlModelAssociator::DIFF_UPDATE_NODE |
-            TypedUrlModelAssociator::DIFF_LOCAL_TITLE_CHANGED |
             TypedUrlModelAssociator::DIFF_LOCAL_ROW_CHANGED |
             TypedUrlModelAssociator::DIFF_LOCAL_VISITS_ADDED,
             TypedUrlModelAssociator::MergeUrls(specs4, row4, &visits4,

chrome/browser/sync/test/integration/typed_urls_helper.cc

@@ -124,6 +124,7 @@ void AddToHistory(HistoryService* service,
                    history::RedirectList(),
                    source,
                    false);
+  service->SetPageTitle(url, ASCIIToUTF16(url.spec() + " - title"));
 }
 
 std::vector<history::URLRow> GetTypedUrlsFromHistoryService(