Cull autofill entries older than 60 days.

chrome/browser/webdata/autocomplete_syncable_service.cc

Index: chrome/browser/webdata/autocomplete_syncable_service.cc
diff --git a/chrome/browser/webdata/autocomplete_syncable_service.cc b/chrome/browser/webdata/autocomplete_syncable_service.cc
index 3b3ecb1b2cb64812d766542376557fc7bcef1cc1..7284102a4a3c6d8ee5291867a543f88ab6dca537 100644
--- a/chrome/browser/webdata/autocomplete_syncable_service.cc
+++ b/chrome/browser/webdata/autocomplete_syncable_service.cc
@@ -29,30 +29,51 @@ const char kAutofillEntryNamespaceTag[] = "autofill_entry|";
 // Merges timestamps from the |autofill| entry and |timestamps|. Returns
 // true if they were different, false if they were the same.
 // All of the timestamp vectors are assummed to be sorted, resulting vector is
-// sorted as well.
+// sorted as well. Only two timestamps - the earliest and the latest are stored.
 bool MergeTimestamps(const sync_pb::AutofillSpecifics& autofill,
                      const std::vector<base::Time>& timestamps,
                      std::vector<base::Time>* new_timestamps) {
   DCHECK(new_timestamps);
-  std::set<base::Time> timestamp_union(timestamps.begin(),
-                                       timestamps.end());
 
+  new_timestamps->clear();
   size_t timestamps_count = autofill.usage_timestamp_size();
-
-  bool different = timestamps.size() != timestamps_count;
-  for (size_t i = 0; i < timestamps_count; ++i) {
-    if (timestamp_union.insert(base::Time::FromInternalValue(
-            autofill.usage_timestamp(i))).second) {
+  if (timestamps_count == 0 && timestamps.empty()) {
+    return false;
+  } else if (timestamps_count == 0) {
+    new_timestamps->insert(new_timestamps->begin(),
+                           timestamps.begin(),
+                           timestamps.end());
+    return true;
+  } else if (timestamps.empty()) {
+    new_timestamps->reserve(2);
+    new_timestamps->push_back(base::Time::FromInternalValue(
+        autofill.usage_timestamp(0));
+    if (timestamps_count > 1) {
+      new_timestamps->push_back(base::Time::FromInternalValue(
+          autofill.usage_timestamp(timestamps_count - 1));
+    }
+    return true;
+  } else {
+    base::Time time_begin = base::Time::FromInternalValue(
+        autofill.usage_timestamp(0);
+    base::Time time_end = base::Time::FromInternalValue(
+        autofill.usage_timestamp(timestamps_count - 1);
+    bool different = (timestamps_count != timestamps.size());
+    if (timestamps.front() > time_begin) {

[Nicolas Zea, 2012/03/26 20:58:22]

I don't think this is quite right. You're not setting different = true if
timestamps.front() < time_begin. Also, operator> returns true if lhs is more
recent than rhs (it compares microseconds since epoch). So you're updating
timestamps[0] only if it's already more recent, when I believe the more recent
one is the one you want to keep, right? Also you're modifying timestamps instead
of new_timestamps here.

In addition, I'd prefer that the count of timestamps not be a consideration when
comparing with sync data, given that other clients will continue to track all
the timestamps (and the fact that previously this code unioned means they'll
continue to add the timestamps back in when they restart sync).

how about something like:
if (timestamps.front() != sync_time_begin || timestamps.back() != sync_time_end)
{
  new_timestamps.push_back(timestamps.front() > sync_time_begin ?
timestamps.front() : sync_time_begin);
  if (timestamps.size() > 1 || sync_timestamps_count > 1)
    new_timestamps.push_back(timestamps.back() < sync_time_end ?
timestamps.back() : sync_time_end);
  return true;
}
return false;

This way you only care about the first/last sync timestamps and you return
different if they're not already set properly.

[GeorgeY, 2012/03/26 21:19:47]

sure. works as well, though slightly modified (your code does not work if both
have only one different timestamp, for example)

+      if (timestamps.size() == 1)
+        timestamps.push_back(timestamps[0]);
+      timestamps[0] = time_begin;
       different = true;
     }
+    if (timestamps.back() < time_end) {
+      if (timestamps.size() == 1)
+        timestamps.push_back(time_end);
+      else
+        timestamps[1] = time_end;
+      different = true;
+    }
+    return different;
   }
-
-  if (different) {
-    new_timestamps->insert(new_timestamps->begin(),
-                           timestamp_union.begin(),
-                           timestamp_union.end());
-  }
-  return different;
 }
 
 }  // namespace
@@ -101,24 +122,58 @@ SyncError AutocompleteSyncableService::MergeDataAndStartSyncing(
   sync_processor_ = sync_processor.Pass();
 
   std::vector<AutofillEntry> new_synced_entries;
+  std::vector<AutofillEntry> synced_expired_entries;
   // Go through and check for all the entries that sync already knows about.
   // CreateOrUpdateEntry() will remove entries that are same with the synced
   // ones from |new_db_entries|.
   for (SyncDataList::const_iterator sync_iter = initial_sync_data.begin();
        sync_iter != initial_sync_data.end(); ++sync_iter) {
-    CreateOrUpdateEntry(*sync_iter, &new_db_entries, &new_synced_entries);
+    CreateOrUpdateEntry(*sync_iter, &new_db_entries,
+                        &new_synced_entries, &synced_expired_entries);
   }
 
+  // Check if newly received items need culling.
+  bool need_to_cull_data = !synced_expired_entries.empty();
+
   if (!SaveChangesToWebData(new_synced_entries))
     return SyncError(FROM_HERE, "Failed to update webdata.", model_type());
 
   WebDataService::NotifyOfMultipleAutofillChanges(web_data_service_);
+  keys_to_ignore_.clear();
 
   SyncChangeList new_changes;
   for (AutocompleteEntryMap::iterator i = new_db_entries.begin();
        i != new_db_entries.end(); ++i) {
-    new_changes.push_back(
-        SyncChange(i->second.first, CreateSyncData(*(i->second.second))));
+    // Sync back only the data that appeared after
+    // |AutofillEntry::ExpirationTime()|.
+    if (!i->second.second->IsExpired()) {
+      new_changes.push_back(
+          SyncChange(i->second.first, CreateSyncData(*(i->second.second))));
+    } else {
+      need_to_cull_data = true;
+      // Key is not on the server and is too old, it will not ever be synced -
+      // delete it locally.
+      if (i->second.first == SyncChange::ACTION_ADD)
+        keys_to_ignore_.insert(i->first);
+    }
+  }
+
+  // Delete only the changes never synced to the db as they are too old.
+  for (size_t i = 0; i < synced_expired_entries.size(); ++i) {
+    // Key is on the server and not local and is too old, we need to notify
+    // sync that it has expired.
+    if (keys_to_ignore_.find(synced_expired_entries[i].key()) ==
+        keys_to_ignore_.end()) {
+      new_changes.push_back(SyncChange(SyncChange::ACTION_DELETE,
+          CreateSyncData(synced_expired_entries[i])));
+    }
+  }
+
+  if (need_to_cull_data) {
+    // This will schedule deletion operation later on DB thread and we will
+    // be notified on the results of the deletion and deletes will be synced to
+    // the sync.
+    web_data_service_->RemoveExpiredFormElements();
   }
 
   SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes);
@@ -169,6 +224,7 @@ SyncError AutocompleteSyncableService::ProcessSyncChanges(
   std::vector<AutofillEntry> entries;
   scoped_ptr<AutocompleteEntryMap> db_entries;
   std::vector<AutofillEntry> new_entries;
+  std::vector<AutofillEntry> ignored_entries;
 
   SyncError list_processing_error;
 
@@ -192,7 +248,8 @@ SyncError AutocompleteSyncableService::ProcessSyncChanges(
                 std::make_pair(SyncChange::ACTION_ADD, it);
           }
         }
-        CreateOrUpdateEntry(i->sync_data(), db_entries.get(), &new_entries);
+        CreateOrUpdateEntry(i->sync_data(), db_entries.get(),
+                            &new_entries, &ignored_entries);
         break;
       case SyncChange::ACTION_DELETE: {
         DCHECK(i->sync_data().GetSpecifics().has_autofill())
@@ -217,6 +274,17 @@ SyncError AutocompleteSyncableService::ProcessSyncChanges(
   if (!SaveChangesToWebData(new_entries))
     return SyncError(FROM_HERE, "Failed to update webdata.", model_type());
 
+  // Remove already expired data.
+  for (size_t i = 0; i < ignored_entries.size(); ++i) {
+    if (db_entries.get() &&
+        db_entries->find(ignored_entries[i].key()) != db_entries->end()) {
+      bool success = web_data_service_->GetDatabase()->GetAutofillTable()->
+              RemoveFormElement(ignored_entries[i].key().name(),
+                                ignored_entries[i].key().value());
+      DCHECK(success);
+    }
+  }
+
   WebDataService::NotifyOfMultipleAutofillChanges(web_data_service_);
 
   return list_processing_error;
@@ -264,7 +332,8 @@ bool AutocompleteSyncableService::SaveChangesToWebData(
 void AutocompleteSyncableService::CreateOrUpdateEntry(
     const SyncData& data,
     AutocompleteEntryMap* loaded_data,
-    std::vector<AutofillEntry>* new_entries) {
+    std::vector<AutofillEntry>* new_entries,
+    std::vector<AutofillEntry>* ignored_entries) {
   const sync_pb::EntitySpecifics& specifics = data.GetSpecifics();
   const sync_pb::AutofillSpecifics& autofill_specifics(
       specifics.autofill());
@@ -282,12 +351,20 @@ void AutocompleteSyncableService::CreateOrUpdateEntry(
     // New entry.
     std::vector<base::Time> timestamps;
     size_t timestamps_count = autofill_specifics.usage_timestamp_size();
-    timestamps.resize(timestamps_count);
-    for (size_t ts = 0; ts < timestamps_count; ++ts) {
-      timestamps[ts] = base::Time::FromInternalValue(
-            autofill_specifics.usage_timestamp(ts));
+    timestamps.reserve(2);
+    if (timestamps_count) {
+      timestamps.push_back(base::Time::FromInternalValue(
+          autofill_specifics.usage_timestamp(0)))
     }
-    new_entries->push_back(AutofillEntry(key, timestamps));
+    if (timestamps_count > 1) {
+      timestamps.push_back(base::Time::FromInternalValue(
+          autofill_specifics.usage_timestamp(timestamps_count - 1)))
+    }
+    AutofillEntry new_entry(key, timestamps);
+    if (new_entry.IsExpired())
+      ignored_entries->push_back(new_entry);
+    else
+      new_entries->push_back(new_entry);
   } else {
     // Entry already present - merge if necessary.
     std::vector<base::Time> timestamps;
@@ -295,7 +372,10 @@ void AutocompleteSyncableService::CreateOrUpdateEntry(
         autofill_specifics, it->second.second->timestamps(), &timestamps);
     if (different) {
       AutofillEntry new_entry(it->second.second->key(), timestamps);
-      new_entries->push_back(new_entry);
+      if (new_entry.IsExpired())
+        ignored_entries->push_back(new_entry);
+      else
+        new_entries->push_back(new_entry);
 
       // Update the sync db if the list of timestamps have changed.
       *(it->second.second) = new_entry;

[Nicolas Zea, 2012/03/26 20:58:22]

This code has no effect if the entry is expired right? Perhaps move into the
else case of the if statement above?

[GeorgeY, 2012/03/26 21:19:47]

Sure. It will get deleted in any case.

@@ -358,10 +438,12 @@ void AutocompleteSyncableService::ActOnChanges(
         break;
       }
       case AutofillChange::REMOVE: {
-        std::vector<base::Time> timestamps;
-        AutofillEntry entry(change->key(), timestamps);
-        new_changes.push_back(SyncChange(SyncChange::ACTION_DELETE,
-                                         CreateSyncData(entry)));
+        if (keys_to_ignore_.find(change->key()) == keys_to_ignore_.end()) {
+          std::vector<base::Time> timestamps;
+          AutofillEntry entry(change->key(), timestamps);
+          new_changes.push_back(SyncChange(SyncChange::ACTION_DELETE,
+                                           CreateSyncData(entry)));
+        }
         break;
       }
       default:
@@ -375,6 +457,8 @@ void AutocompleteSyncableService::ActOnChanges(
                   << " Failed processing change:"
                   << " Error:" << error.message();
   }
+  // |keys_to_ignore_| are only needed for the very first notification.
+  keys_to_ignore_.clear();
 }
 
 SyncData AutocompleteSyncableService::CreateSyncData(