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

chrome/browser/sync/glue/typed_url_model_associator.cc

Index: chrome/browser/sync/glue/typed_url_model_associator.cc
diff --git a/chrome/browser/sync/glue/typed_url_model_associator.cc b/chrome/browser/sync/glue/typed_url_model_associator.cc
index 9411712dc251e492a8d9232dc10eb73bef738c74..c5d4e854704a856c31cb249a8ec8a40d7a83292c 100644
--- a/chrome/browser/sync/glue/typed_url_model_associator.cc
+++ b/chrome/browser/sync/glue/typed_url_model_associator.cc
@@ -85,16 +85,23 @@ bool TypedUrlModelAssociator::AssociateModels() {
 
       sync_api::ReadNode node(&trans);
       if (node.InitByClientTagLookup(syncable::TYPED_URLS, tag)) {
+        // Same URL exists in sync data and in history data - compare the
+        // entries to see if there's any difference.
         const sync_pb::TypedUrlSpecifics& typed_url(
             node.GetTypedUrlSpecifics());
         DCHECK_EQ(tag, typed_url.url());
 
-        history::URLRow new_url(ix->url());
+        // Initialize fields in |new_url| to the same values as the fields in
+        // the existing URLRow in the history DB. This is needed because we
+        // overwrite the existing value below in WriteToHistoryBackend(), but
+        // some of the values in that structure are not synced (like
+        // typed_count).
+        history::URLRow new_url(*ix);
 
-        std::vector<base::Time> added_visits;
+        std::vector<TypedUrlVisitInfo> added_visits;
         int difference = MergeUrls(typed_url, *ix, &visits, &new_url,
                                    &added_visits);
-        if (difference & DIFF_NODE_CHANGED) {
+        if (difference & DIFF_UPDATE_NODE) {
           sync_api::WriteNode write_node(&trans);
           if (!write_node.InitByClientTagLookup(syncable::TYPED_URLS, tag)) {
             LOG(ERROR) << "Failed to edit typed_url sync node.";
@@ -103,9 +110,9 @@ bool TypedUrlModelAssociator::AssociateModels() {
           // We don't want to resurrect old visits that have been aged out by
           // other clients, so remove all visits that are older than the
           // earliest existing visit in the sync node.
-          if (typed_url.visit_size() > 0) {
+          if (typed_url.visits_size() > 0) {
             base::Time earliest_visit =
-                base::Time::FromInternalValue(typed_url.visit(0));
+                base::Time::FromInternalValue(typed_url.visits(0));
             for (history::VisitVector::iterator it = visits.begin();
                  it != visits.end() && it->visit_time < earliest_visit; ) {
               it = visits.erase(it);
@@ -119,18 +126,17 @@ bool TypedUrlModelAssociator::AssociateModels() {
           }
           WriteToSyncNode(new_url, visits, &write_node);
         }
-        if (difference & DIFF_TITLE_CHANGED) {
+        if (difference & DIFF_LOCAL_TITLE_CHANGED) {
           titles.push_back(std::pair<GURL, string16>(new_url.url(),
                                                      new_url.title()));
         }
-        if (difference & DIFF_ROW_CHANGED) {
+        if (difference & DIFF_LOCAL_ROW_CHANGED) {
           updated_urls.push_back(
               std::pair<history::URLID, history::URLRow>(ix->id(), new_url));
         }
-        if (difference & DIFF_VISITS_ADDED) {
-          new_visits.push_back(
-              std::pair<GURL, std::vector<base::Time> >(ix->url(),
-                                                        added_visits));
+        if (difference & DIFF_LOCAL_VISITS_ADDED) {
+          new_visits.push_back(std::pair<GURL, std::vector<TypedUrlVisitInfo> >(
+              ix->url(), added_visits));
         }
 
         Associate(&tag, node.GetId());
@@ -152,6 +158,8 @@ bool TypedUrlModelAssociator::AssociateModels() {
       current_urls.insert(tag);
     }
 
+    // Now walk the sync nodes and detect any URLs that exist there, but not in
+    // the history DB, so we can add them to our local history DB.
     int64 sync_child_id = typed_url_root.GetFirstChildId();
     while (sync_child_id != sync_api::kInvalidId) {
       sync_api::ReadNode sync_child_node(&trans);
@@ -164,16 +172,21 @@ bool TypedUrlModelAssociator::AssociateModels() {
 
       if (current_urls.find(typed_url.url()) == current_urls.end()) {
         new_visits.push_back(
-            std::pair<GURL, std::vector<base::Time> >(
+            std::pair<GURL, std::vector<TypedUrlVisitInfo> >(
                 GURL(typed_url.url()),
-                std::vector<base::Time>()));
-        std::vector<base::Time>& visits = new_visits.back().second;
-        history::URLRow new_url = TypedUrlSpecificsToURLRow(typed_url);
-
-        // The latest visit gets added automatically, so skip it.
-        for (int c = 0; c < typed_url.visit_size() - 1; ++c) {
-          DCHECK(typed_url.visit(c) < typed_url.visit(c + 1));
-          visits.push_back(base::Time::FromInternalValue(typed_url.visit(c)));
+                std::vector<TypedUrlVisitInfo>()));
+        std::vector<TypedUrlVisitInfo>& visits = new_visits.back().second;
+        history::URLRow new_url(GURL(typed_url.url()));
+        TypedUrlModelAssociator::TypedUrlSpecificsToURLRow(typed_url, &new_url);
+
+        for (int c = 0; c < typed_url.visits_size(); ++c) {
+          DCHECK(c == 0 || typed_url.visits(c) > typed_url.visits(c - 1));
+          DCHECK(static_cast<PageTransition::Type>(
+              typed_url.visit_transitions(c)) <= PageTransition::LAST_CORE);

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

DCHECK_LT

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

Done.

+          visits.push_back(TypedUrlVisitInfo(
+              base::Time::FromInternalValue(typed_url.visits(c)),
+              static_cast<PageTransition::Type>(
+                  typed_url.visit_transitions(c))));
         }
 
         Associate(&typed_url.url(), sync_child_node.GetId());
@@ -324,6 +337,11 @@ bool TypedUrlModelAssociator::WriteToHistoryBackend(
   if (updated_urls) {
     for (TypedUrlUpdateVector::const_iterator url = updated_urls->begin();
          url != updated_urls->end(); ++url) {
+      // Caller should have already initialized the visit and typed counts
+      // appropriately to match what the values should have been before adding
+      // visits.
+      DCHECK(url->second.visit_count());
+      DCHECK(url->second.typed_count());
       DCHECK(IsAssociated(url->second.url().spec()));
       if (!history_backend_->UpdateURL(url->first, url->second)) {
         LOG(ERROR) << "Could not update page: " << url->second.url().spec();
@@ -353,46 +371,49 @@ bool TypedUrlModelAssociator::WriteToHistoryBackend(
 
 // static
 int TypedUrlModelAssociator::MergeUrls(
-    const sync_pb::TypedUrlSpecifics& typed_url,
+    const sync_pb::TypedUrlSpecifics& node,
     const history::URLRow& url,
     history::VisitVector* visits,
     history::URLRow* new_url,
-    std::vector<base::Time>* new_visits) {
+    std::vector<TypedUrlVisitInfo>* new_visits) {
   DCHECK(new_url);
-  DCHECK(!typed_url.url().compare(url.url().spec()));
-  DCHECK(!typed_url.url().compare(new_url->url().spec()));
+  DCHECK(!node.url().compare(url.url().spec()));
+  DCHECK(!node.url().compare(new_url->url().spec()));
   DCHECK(visits->size());
+  DCHECK(node.visits_size() == node.visit_transitions_size());

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

DCHECK_EQ

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

Done.

 
-  int original_visit_count = visits->size();
+  // If we have an old-format node (before we added the visits and
+  // visit_transitions arrays to the protobuf), just overwrite
+  // it with our local history data.
+  if (node.visits_size() == 0)
+    return DIFF_UPDATE_NODE;
 
   // Convert these values only once.
-  string16 typed_title(UTF8ToUTF16(typed_url.title()));
-  base::Time typed_visit =
-      base::Time::FromInternalValue(
-          typed_url.visit(typed_url.visit_size() - 1));
-
+  string16 node_title(UTF8ToUTF16(node.title()));
+  base::Time node_last_visit = base::Time::FromInternalValue(
+      node.visits(node.visits_size() - 1));
 
   // This is a bitfield representing what we'll need to update with the output
   // value.
   int different = DIFF_NONE;
 
   // Check if the non-incremented values changed.
-  if ((typed_title.compare(url.title()) != 0) ||
-      (typed_url.hidden() != url.hidden())) {
+  if ((node_title.compare(url.title()) != 0) ||
+      (node.hidden() != url.hidden())) {
     // Use the values from the most recent visit.
-    if (typed_visit >= url.last_visit()) {
-      new_url->set_title(typed_title);
-      new_url->set_hidden(typed_url.hidden());
-      different |= DIFF_ROW_CHANGED;
+    if (node_last_visit >= url.last_visit()) {
+      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_TITLE_CHANGED;
+        different |= DIFF_LOCAL_TITLE_CHANGED;
       }
     } else {
       new_url->set_title(url.title());
       new_url->set_hidden(url.hidden());
-      different |= DIFF_NODE_CHANGED;
+      different |= DIFF_UPDATE_NODE;
     }
   } else {
     // No difference.
@@ -400,93 +421,62 @@ int TypedUrlModelAssociator::MergeUrls(
     new_url->set_hidden(url.hidden());
   }
 
-  // For typed count, we just select the maximum value. This is not technically
-  // correct since it undercounts URLs that have been typed on multiple systems
-  // between syncs, but it's the best we can do.
-  if (typed_url.typed_count() > url.typed_count()) {
-    new_url->set_typed_count(typed_url.typed_count());
-    different |= DIFF_ROW_CHANGED;
-  } else if (typed_url.typed_count() < url.typed_count()) {
-    new_url->set_typed_count(url.typed_count());
-    different |= DIFF_NODE_CHANGED;
-  } else {
-    // No difference.
-    new_url->set_typed_count(typed_url.typed_count());
-  }
-
-  size_t left_num_visits = typed_url.visit_size();
-  size_t right_num_visits = visits->size();
-  size_t left = 0;
-  size_t right = 0;
+  size_t node_num_visits = node.visits_size();
+  size_t history_num_visits = visits->size();
+  size_t node_visit_index = 0;
+  size_t history_visit_index = 0;
   // Walk through the two sets of visits and figure out if any new visits were
   // added on either side (left = sync node, right = history db entry).

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

left/right naming not used anymore.

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

Done.

-  while (left < left_num_visits || right < right_num_visits) {
+  while (node_visit_index < node_num_visits ||
+         history_visit_index < history_num_visits) {
     // Time objects are initialized to "earliest possible time".
-    base::Time left_time, right_time;
-    if (left < left_num_visits)
-      left_time = base::Time::FromInternalValue(typed_url.visit(left));
-    if (right < right_num_visits)
-      right_time = (*visits)[right].visit_time;
-    if (left >= left_num_visits ||
-        (right < right_num_visits && left_time > right_time)) {
+    base::Time node_time, history_time;
+    if (node_visit_index < node_num_visits)
+      node_time = base::Time::FromInternalValue(node.visits(node_visit_index));
+    if (history_visit_index < history_num_visits)
+      history_time = (*visits)[history_visit_index].visit_time;
+    if (node_visit_index >= node_num_visits ||
+        (history_visit_index < history_num_visits &&
+         node_time > history_time)) {
       // We found a visit in the history DB that doesn't exist in the sync DB,
       // so mark the node as modified so the caller will update the sync node.
-      different |= DIFF_NODE_CHANGED;
-      ++right;
-    } else if (right >= right_num_visits || left_time < right_time) {
+      different |= DIFF_UPDATE_NODE;
+      ++history_visit_index;
+    } else if (history_visit_index >= history_num_visits ||
+               node_time < history_time) {
       // Found a visit in the sync node that doesn't exist in the history DB, so
       // add it to our list of new visits and set the appropriate flag so the
       // caller will update the history DB.
-      different |= DIFF_VISITS_ADDED;
-      new_visits->push_back(left_time);
+      different |= DIFF_LOCAL_VISITS_ADDED;
+      new_visits->push_back(TypedUrlVisitInfo(
+          node_time, node.visit_transitions(node_visit_index)));
       // This visit is added to visits below.
-      ++left;
+      ++node_visit_index;
     } else {
       // Same (already synced) entry found in both DBs - no need to do anything.
-      ++left;
-      ++right;
+      ++node_visit_index;
+      ++history_visit_index;
     }
   }
 
-  if (different & DIFF_VISITS_ADDED) {
+  if (different & DIFF_LOCAL_VISITS_ADDED) {
     // Insert new visits into the apropriate place in the visits vector.
     history::VisitVector::iterator visit_ix = visits->begin();
-    for (std::vector<base::Time>::iterator new_visit = new_visits->begin();
+    for (std::vector<TypedUrlVisitInfo>::iterator new_visit =
+             new_visits->begin();
          new_visit != new_visits->end(); ++new_visit) {
-      while (visit_ix != visits->end() && *new_visit > visit_ix->visit_time) {
+      while (visit_ix != visits->end() &&
+             new_visit->first > visit_ix->visit_time) {
         ++visit_ix;
       }
       visit_ix = visits->insert(visit_ix,
-                                history::VisitRow(url.id(), *new_visit,
-                                                  0, 0, 0));
+                                history::VisitRow(url.id(), new_visit->first,
+                                                  0, new_visit->second, 0));
       ++visit_ix;
     }
   }
 
   new_url->set_last_visit(visits->back().visit_time);
-
-  // visit_count is a value that is only loosely correlated with the visit
-  // array (for example, reloading a page does not increment the visit_count
-  // but does register a visit in the visit vector). To determine the merged
-  // visit_count, we'll calculate the largest diff between visit_count and the
-  // sizes of the visit arrays and then apply that to the final visit array.
-  // This is inexact, but the exact value isn't particularly important (it's
-  // just used to help prioritize between various autocomplete suggestions, so
-  // in general just the magnitude is important).
-  int typed_url_visit_delta =
-      typed_url.visit_size() - typed_url.visited_count();
-  int history_url_visit_delta =
-      original_visit_count - url.visit_count();
-  if (std::abs(typed_url_visit_delta) > std::abs(history_url_visit_delta))
-    new_url->set_visit_count(visits->size() - typed_url_visit_delta);
-  else
-    new_url->set_visit_count(visits->size() - history_url_visit_delta);
-
-  if (new_url->visit_count() != url.visit_count())
-    different |= DIFF_ROW_CHANGED;
-  if (new_url->visit_count() != typed_url.visited_count())
-    different |= DIFF_NODE_CHANGED;
-
   return different;
 }
 
@@ -502,13 +492,13 @@ void TypedUrlModelAssociator::WriteToSyncNode(
   sync_pb::TypedUrlSpecifics typed_url;
   typed_url.set_url(url.url().spec());
   typed_url.set_title(UTF16ToUTF8(url.title()));
-  typed_url.set_typed_count(url.typed_count());
   typed_url.set_hidden(url.hidden());
-  typed_url.set_visited_count(url.visit_count());
 
   for (history::VisitVector::const_iterator visit = visits.begin();
        visit != visits.end(); ++visit) {
-    typed_url.add_visit(visit->visit_time.ToInternalValue());
+    typed_url.add_visits(visit->visit_time.ToInternalValue());
+    typed_url.add_visit_transitions(
+        visit->transition & PageTransition::CORE_MASK);
   }
 
   node->SetTypedUrlSpecifics(typed_url);
@@ -518,48 +508,49 @@ void TypedUrlModelAssociator::WriteToSyncNode(
 void TypedUrlModelAssociator::DiffVisits(
     const history::VisitVector& old_visits,
     const sync_pb::TypedUrlSpecifics& new_url,
-    std::vector<base::Time>* new_visits,
+    std::vector<TypedUrlVisitInfo>* new_visits,
     history::VisitVector* removed_visits) {
-  size_t left_visit_count = old_visits.size();
-  size_t right_visit_count = new_url.visit_size();
-  size_t left = 0;
-  size_t right = 0;
-  while (left < left_visit_count && right < right_visit_count) {
-    base::Time right_time = base::Time::FromInternalValue(new_url.visit(right));
-    if (old_visits[left].visit_time < right_time) {
-      removed_visits->push_back(old_visits[left]);
-      ++left;
-    } else if (old_visits[left].visit_time > right_time) {
-      new_visits->push_back(right_time);
-      ++right;
+  size_t old_visit_count = old_visits.size();
+  size_t new_visit_count = new_url.visits_size();
+  size_t old_index = 0;
+  size_t new_index = 0;
+  while (old_index < old_visit_count && new_index < new_visit_count) {
+    base::Time new_visit_time =
+        base::Time::FromInternalValue(new_url.visits(new_index));
+    if (old_visits[old_index].visit_time < new_visit_time) {
+      removed_visits->push_back(old_visits[old_index]);
+      ++old_index;
+    } else if (old_visits[old_index].visit_time > new_visit_time) {
+      new_visits->push_back(TypedUrlVisitInfo(
+          new_visit_time, new_url.visit_transitions(new_index)));
+      ++new_index;
     } else {
-      ++left;
-      ++right;
+      ++old_index;
+      ++new_index;
     }
   }
 
-  for ( ; left < left_visit_count; ++left) {
-    removed_visits->push_back(old_visits[left]);
+  for ( ; old_index < old_visit_count; ++old_index) {
+    removed_visits->push_back(old_visits[old_index]);
   }
 
-  for ( ; right < right_visit_count; ++right) {
-    new_visits->push_back(base::Time::FromInternalValue(new_url.visit(right)));
+  for ( ; new_index < new_visit_count; ++new_index) {
+    new_visits->push_back(TypedUrlVisitInfo(
+        base::Time::FromInternalValue(new_url.visits(new_index)),
+        new_url.visit_transitions(new_index)));
   }
 }
 
 
 // static
-history::URLRow TypedUrlModelAssociator::TypedUrlSpecificsToURLRow(
-    const sync_pb::TypedUrlSpecifics& typed_url) {
-  DCHECK(typed_url.visit_size() > 0);
-  history::URLRow new_url(GURL(typed_url.url()));
-  new_url.set_title(UTF8ToUTF16(typed_url.title()));
-  new_url.set_visit_count(typed_url.visited_count());
-  new_url.set_typed_count(typed_url.typed_count());
-  new_url.set_hidden(typed_url.hidden());
-  new_url.set_last_visit(base::Time::FromInternalValue(
-      typed_url.visit(typed_url.visit_size() - 1)));
-  return new_url;
+void TypedUrlModelAssociator::TypedUrlSpecificsToURLRow(
+    const sync_pb::TypedUrlSpecifics& typed_url, history::URLRow* new_url) {
+  DCHECK(typed_url.visits_size() > 0);

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

DCHECK_GT

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

Done.

+  DCHECK(typed_url.visit_transitions_size() == typed_url.visits_size());

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

DCHECK_EQ

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

Done.

+  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)));
 }
 
 bool TypedUrlModelAssociator::CryptoReadyIfNecessary() {