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 b2b2ce795bc8c8b5c13de4ea38d8b9e42ec2441e..1c620ba730c4872c0cc955c05081eecc929e6c35 100644
--- a/chrome/browser/webdata/autocomplete_syncable_service.cc
+++ b/chrome/browser/webdata/autocomplete_syncable_service.cc
@@ -107,16 +107,44 @@ SyncError AutocompleteSyncableService::MergeDataAndStartSyncing(
     CreateOrUpdateEntry(*sync_iter, &new_db_entries, &new_synced_entries);
   }
 
+  // Check if newly received items need culling.
+  bool need_to_cull_data = false;
+  base::Time culling_time = base::Time::Now();
+  culling_time -=
+      base::TimeDelta::FromDays(AutofillEntry::kExpirationPeriodInDays);

[Ilya Sherman, 2012/03/06 08:48:35]

nit: might as well write these three lines as two lines by doing the calculation
immediately.

[GeorgeY, 2012/03/09 20:14:24]

Done.

+  for (size_t i = 0; i < new_synced_entries.size() && !need_to_cull_data; ++i)

[Ilya Sherman, 2012/03/06 08:48:35]

nit: The loop body is multiple lines, so please add curly braces.

[GeorgeY, 2012/03/09 20:14:24]

Done.

+    if (new_synced_entries[i].HasTimestampsOlder(culling_time))
+      need_to_cull_data = true;
+
   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))));
+    if (i->second.second->CullExpiredTimeStamps(culling_time))
+      need_to_cull_data = true;
+    // Sync back only the data that appeared after |culling_time|.
+    if (!i->second.second->timestamps().empty()) {
+      new_changes.push_back(
+          SyncChange(i->second.first, CreateSyncData(*(i->second.second))));
+    } else {
+      // Key is not on the server and is too old, it will not ever be synced -
+      // delete it locally.

[Ilya Sherman, 2012/03/06 08:48:35]

I don't follow how this tells us that the key is not on the server.  Don't we
also reach this case if the key is on the server and has different timestamps
than the local data?

[GeorgeY, 2012/03/09 20:14:24]

Then it would be SyncChange::ACTION_UPDATE

[Ilya Sherman, 2012/03/09 23:15:22]

Ah, makes sense -- thanks.  I think it would help to move the comment into the
inner if-stmt -- that was part of what threw me off, since at a glance it looks
like that applies to the whole else-stmt.

+      if (SyncChange::ACTION_ADD == i->second.first)
+        keys_to_ignore_.insert(i->first);
+    }
+  }
+
+  if (need_to_cull_data) {

[Ilya Sherman, 2012/03/06 08:48:35]

Why not just always make this call?  It should be a cheap no-op if nothing needs
to be culled, right?

[GeorgeY, 2012/03/09 20:14:24]

Yes, but the call *always* schedules extra task on the DB thread, this check
avoids it.

+    // 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_->RemoveFormElementsAddedBetween(base::Time(),
+                                                      culling_time);

[Ilya Sherman, 2012/03/06 08:48:35]

nit: WDS::RemoveFormElementsAddedBefore(culling_time) seems like a better public
interface for this.

[GeorgeY, 2012/03/09 20:14:24]

Why create a trivial interface (it would call RemoveFormElementsAddedBetween)
internally, when it is so easy to use this one? (RemoveFormElementsAddedBetween
is used in 11 other places).

   }
 
   SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes);
@@ -356,10 +384,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:
@@ -373,6 +403,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(