[sync] Handle local changes in AutocompleteSyncBridge

components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc

Index: components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
diff --git a/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc b/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
index 304cfe1d09f19e282e97e1d43870393f2fc9728d..f5e5914e71fc903c2716d0d9fe8264becb87fc90 100644
--- a/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
+++ b/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
@@ -57,9 +57,16 @@ std::string BuildSerializedStorageKey(const std::string& name,
   return proto.SerializeAsString();
 }
 
-std::string GetStorageKeyFromModel(const AutofillKey& key) {
-  return BuildSerializedStorageKey(base::UTF16ToUTF8(key.name()),
-                                   base::UTF16ToUTF8(key.value()));
+void WriteAutofillEntry(const AutofillEntry& entry,

[maxbogue, 2017/01/12 01:33:35]

A more idiomatic name for this function would be PopulateAutofillSpecifics. We
discussed offline that this could potentially just return the specifics instead
of using an out pointer, but I think leaving this form lets you use it
efficiently above, which is desirable...

[Patrick Noland, 2017/01/13 23:31:52]

Actually I don't think this is needed at all

+                        sync_pb::AutofillSpecifics* autofill_specifics) {
+  autofill_specifics->set_name(base::UTF16ToUTF8(entry.key().name()));
+  autofill_specifics->set_value(base::UTF16ToUTF8(entry.key().value()));
+  autofill_specifics->add_usage_timestamp(
+      entry.date_created().ToInternalValue());
+  if (entry.date_created() != entry.date_last_used()) {
+    autofill_specifics->add_usage_timestamp(
+        entry.date_last_used().ToInternalValue());
+  }
 }
 
 }  // namespace
@@ -163,6 +170,39 @@ void AutocompleteSyncBridge::GetAllData(DataCallback callback) {
   callback.Run(std::move(batch));
 }
 
+void AutocompleteSyncBridge::ActOnChanges(const AutofillChangeList& changes) {
+  DCHECK(change_processor());

[skym, 2017/01/12 00:18:32]

I'd remove this. The bridge should always have a proc now.

[Patrick Noland, 2017/01/13 23:31:51]

Done.

+  std::unique_ptr<syncer::MetadataChangeList> change_list =

[skym, 2017/01/12 00:18:32]

Need to call TakeError() on this thing.

[Patrick Noland, 2017/01/13 23:31:52]

Done.

+      CreateMetadataChangeList();
+  for (const auto& change : changes) {
+    std::string storage_key = GetStorageKeyFromModel(change.key());
+    switch (change.type()) {
+      case AutofillChange::ADD:
+      case AutofillChange::UPDATE: {
+        base::Time date_created, date_last_used;
+        bool success = GetAutofillTable()->GetAutofillTimestamps(
+            change.key().name(), change.key().value(), &date_created,
+            &date_last_used);
+        DCHECK(success);

[skym, 2017/01/12 00:18:32]

Is this really impossible? How about something like the following, WDYT?

change_processor()->ReportError(FROM_HERE, "Failed reading from WebDatabase.");

[maxbogue, 2017/01/12 01:33:35]

+1, please handle your errors :)

[Patrick Noland, 2017/01/13 23:31:51]

Done.

+
+        AutofillEntry entry(change.key(), date_created, date_last_used);
+        sync_pb::AutofillSpecifics specifics;

[maxbogue, 2017/01/12 01:33:35]

You do not use this variable.

[Patrick Noland, 2017/01/13 23:31:51]

Done.

+
+        WriteAutofillEntry(entry, &specifics);

[skym, 2017/01/12 00:18:32]

This feels more awkward than it needed to be.

[maxbogue, 2017/01/12 01:33:35]

This line is populating a specifics variable that is never used, please remove
it.

[Patrick Noland, 2017/01/13 23:31:51]

Done.

+        std::unique_ptr<syncer::EntityData> entity_data =
+            CreateEntityData(entry);
+        change_processor()->Put(storage_key, std::move(entity_data),
+                                change_list.get());
+        break;
+      }
+      case AutofillChange::REMOVE: {
+        change_processor()->Delete(storage_key, change_list.get());
+        break;
+      }
+    }
+  }
+}
+
 std::string AutocompleteSyncBridge::GetClientTag(
     const syncer::EntityData& entity_data) {
   DCHECK(entity_data.specifics.has_autofill());
@@ -180,29 +220,31 @@ std::string AutocompleteSyncBridge::GetStorageKey(
   return BuildSerializedStorageKey(specifics.name(), specifics.value());
 }
 
+void AutocompleteSyncBridge::InjectStartSyncFlare(
+    const syncer::ModelTypeSyncBridge::StartSyncFlare& flare) {

[maxbogue, 2017/01/12 01:33:35]

omit "syncer::ModelTypeSyncBridge::"

[Patrick Noland, 2017/01/13 23:31:52]

n/a now that flare is gone

+  flare_ = flare;
+}
+
 // AutofillWebDataServiceObserverOnDBThread implementation.
 void AutocompleteSyncBridge::AutofillEntriesChanged(
     const AutofillChangeList& changes) {
   DCHECK(thread_checker_.CalledOnValidThread());
-}
-
-// static
-AutofillEntry AutocompleteSyncBridge::CreateAutofillEntry(
-    const sync_pb::AutofillSpecifics& autofill_specifics) {
-  AutofillKey key(base::UTF8ToUTF16(autofill_specifics.name()),
-                  base::UTF8ToUTF16(autofill_specifics.value()));
-  base::Time date_created, date_last_used;
-  const google::protobuf::RepeatedField<int64_t>& timestamps =
-      autofill_specifics.usage_timestamp();
-  if (!timestamps.empty()) {
-    date_created = base::Time::FromInternalValue(*timestamps.begin());
-    date_last_used = base::Time::FromInternalValue(*timestamps.rbegin());
+  if (change_processor()->IsTrackingMetadata()) {

[maxbogue, 2017/01/12 01:33:35]

I would prefer this just be at the top of ActOnLocalChanges, like:

if (!change_processor()->IsTrackingMetadata())
  return;

[skym, 2017/01/12 18:49:16]

So, this only works if we're not tracking because sync is disabled or we haven't
finished the initial download yet. If we're not tracking because we haven't
given the metadata to the proc, then we need to do this ourselves now, and then
act on the changes.

[Patrick Noland, 2017/01/13 23:31:51]

Done.

+    ActOnChanges(changes);
+  } else if (!flare_.is_null()) {

[maxbogue, 2017/01/12 01:33:35]

The flare is simply an optimization right? We're asking sync to start up faster?
But in USS world our ability to track changes isn't blocked by sync starting up,
it's blocked on the model loading metadata, so I'm not certain why a flare is
needed... Let's chat tomorrow morning about this.

[skym, 2017/01/12 18:49:16]

Agreed, lets remove the flare.

+    flare_.Run(syncer::AUTOFILL);

[skym, 2017/01/12 00:18:32]

It looks like we end up dropping the changes here, because if this isn't a first
time, we no longer merge.

[maxbogue, 2017/01/12 01:33:35]

I don't think this comment is accurate; maybe Sky can clarify?

[skym, 2017/01/12 18:49:16]

So I think in the Directory world, we were more concerned about consistency
because metadata wasn't stored with our data. If there were many local changes
before sync started up, we *hand wave hand wave* thought this would increase our
chances of having data not line up. For a straight forward data type like
AUTOFILL, I don't think this fear really lines up, so maybe my speculation is
incorrect.

However, in this USS world, we don't need to start up sync. Flaring here doesn't
do any good. The proc is going to be able to track local changes just fine for
us w/o sync being enabled. If we haven't done our first time sync yet, and we're
going to merge, that's fine, we'll merge and life will be okay. You can probably
remove the concept of flaring from this class. The only scenario I see flaring
as useful going forward is if you want to do a round trip to the server really
fast. Things like showing foreign session tabs on the NTP on startup :)

However, this kind of exposes a different problem with our approach to
implementing this class. No one is calling OnMetadataLoaded(). You're not
technically allowed to Put()/Delete() to the proc before you do this. Which this
method should probably worry about. I've created crbug.com/680603 for this. I'm
fine with you fixing it as part of this CL or just leaving a TODO. But we'll
want to do it soon.

[Patrick Noland, 2017/01/13 23:31:51]

I've gone ahead and added metadata loading 

+    flare_.Reset();
   }
-  return AutofillEntry(key, date_created, date_last_used);
 }
 
 AutofillTable* AutocompleteSyncBridge::GetAutofillTable() const {
   return AutofillTable::FromWebDatabase(web_data_backend_->GetDatabase());
 }
 
+std::string AutocompleteSyncBridge::GetStorageKeyFromModel(

[maxbogue, 2017/01/12 01:33:35]

"// static" on a line above

[Patrick Noland, 2017/01/13 23:31:52]

n/a now that unit tests don't use this

+    const AutofillKey& key) {
+  return BuildSerializedStorageKey(base::UTF16ToUTF8(key.name()),
+                                   base::UTF16ToUTF8(key.value()));
+}
+
 }  // namespace autofill