Changed typed url sync to no longer modify typed/visit_count.

chrome/browser/sync/glue/typed_url_change_processor.cc

Index: chrome/browser/sync/glue/typed_url_change_processor.cc
diff --git a/chrome/browser/sync/glue/typed_url_change_processor.cc b/chrome/browser/sync/glue/typed_url_change_processor.cc
index 04c7ba0724bf4ef848b915136f25262cefc50117..b3b6f30abb3d0f1be50f9916ce3d35d97d39c6e5 100644
--- a/chrome/browser/sync/glue/typed_url_change_processor.cc
+++ b/chrome/browser/sync/glue/typed_url_change_processor.cc
@@ -169,10 +169,15 @@ void TypedUrlChangeProcessor::HandleURLsVisited(
   sync_api::WriteNode update_node(&trans);
   if (!update_node.InitByClientTagLookup(syncable::TYPED_URLS, tag)) {
     // If we don't know about it yet, it will be added later.
+    // TODO(atwilson): This can result in this URL not being synced until the
+    // next full association, as we may not get another notification for this
+    // URL if the navigation resulted in a redirect (http://crbug.com/85135).
     return;
   }
   sync_pb::TypedUrlSpecifics typed_url(update_node.GetTypedUrlSpecifics());
-  typed_url.add_visit(visits.back().visit_time.ToInternalValue());
+  typed_url.add_visits(visits.back().visit_time.ToInternalValue());
+  typed_url.add_visit_transitions(
+      visits.back().transition & PageTransition::CORE_MASK);
   update_node.SetTypedUrlSpecifics(typed_url);
 }
 
@@ -225,28 +230,32 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel(
     GURL url(typed_url.url());
 
     if (sync_api::SyncManager::ChangeRecord::ACTION_ADD == changes[i].action) {
-      DCHECK(typed_url.visit_size());
-      if (!typed_url.visit_size()) {
+      DCHECK(typed_url.visits_size());
+      if (!typed_url.visits_size()) {
         continue;
       }
 
-      history::URLRow new_url =
-          TypedUrlModelAssociator::TypedUrlSpecificsToURLRow(typed_url);
+      history::URLRow new_url(GURL(typed_url.url()));
+      TypedUrlModelAssociator::TypedUrlSpecificsToURLRow(typed_url, &new_url);

[Nicolas Zea, 2011/06/09 19:50:39]

It's kind of confusing that TypedUrlSpecificsToURLRow doesn't actually set all
the fields of the URLRow, and you in fact have to initialize it before hand. I
think I liked the previous method, where it returned a URLRow better. Or perhaps
rename to something else?

[Andrew T Wilson (Slow), 2011/06/09 22:11:36]

The problem is the old way was fragile - if there are fields in the URLRow that
aren't part of the sync data (like |typed_count| now), we would have no way of
correctly initializing them in the case that we are doing an update (we would
just clobber them with new default values).

I'll change the name to something more descriptive like
UpdateURLRowFromTypedUrlSpecifics().

 
       model_associator_->Associate(&new_url.url().spec(), changes[i].id);
       new_urls.push_back(new_url);
 
-      // The latest visit gets added automatically, so skip it.
-      std::vector<base::Time> added_visits;
-      for (int c = 0; c < typed_url.visit_size() - 1; ++c) {
-        DCHECK(typed_url.visit(c) < typed_url.visit(c + 1));
-        added_visits.push_back(
-            base::Time::FromInternalValue(typed_url.visit(c)));
+      std::vector<TypedUrlModelAssociator::TypedUrlVisitInfo> added_visits;
+      for (int c = 0; c < typed_url.visits_size(); ++c) {
+        DCHECK(c == 0 || typed_url.visits(c) > typed_url.visits(c - 1));
+        added_visits.push_back(TypedUrlModelAssociator::TypedUrlVisitInfo(
+            base::Time::FromInternalValue(typed_url.visits(c)),
+            typed_url.visit_transitions(c)));
       }
 
       new_visits.push_back(
-          std::pair<GURL, std::vector<base::Time> >(url, added_visits));
+          std::pair<GURL,
+          std::vector<TypedUrlModelAssociator::TypedUrlVisitInfo> >(
+              url, added_visits));
     } else {
+      DCHECK_EQ(sync_api::SyncManager::ChangeRecord::ACTION_UPDATE,
+                changes[i].action);
       history::URLRow old_url;
       if (!history_backend_->GetURL(url, &old_url)) {
         error_handler()->OnUnrecoverableError(FROM_HERE,
@@ -261,8 +270,8 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel(
         return;
       }
 
-      history::URLRow new_url =
-          TypedUrlModelAssociator::TypedUrlSpecificsToURLRow(typed_url);
+      history::URLRow new_url(old_url);
+      TypedUrlModelAssociator::TypedUrlSpecificsToURLRow(typed_url, &new_url);
 
       updated_urls.push_back(
         std::pair<history::URLID, history::URLRow>(old_url.id(), new_url));
@@ -272,13 +281,15 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel(
                                                    new_url.title()));
       }
 
-      std::vector<base::Time> added_visits;
+      std::vector<TypedUrlModelAssociator::TypedUrlVisitInfo> added_visits;
       history::VisitVector removed_visits;
       TypedUrlModelAssociator::DiffVisits(visits, typed_url,
                                           &added_visits, &removed_visits);
       if (added_visits.size()) {
         new_visits.push_back(
-          std::pair<GURL, std::vector<base::Time> >(url, added_visits));
+            std::pair<GURL,
+                std::vector<TypedUrlModelAssociator::TypedUrlVisitInfo> >(
+                    url, added_visits));
       }
       if (removed_visits.size()) {
         deleted_visits.insert(deleted_visits.end(), removed_visits.begin(),