[sync] Handle local changes in AutocompleteSyncBridge

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h"
 
 #include <algorithm>
 #include <set>
 #include <unordered_set>
 #include <utility>
@@ skipping 275 matching lines @@
 
 AutocompleteSyncBridge::AutocompleteSyncBridge(
     AutofillWebDataBackend* backend,
     const ChangeProcessorFactory& change_processor_factory)
     : ModelTypeSyncBridge(change_processor_factory, syncer::AUTOFILL),
       web_data_backend_(backend),
       scoped_observer_(this) {
   DCHECK(web_data_backend_);
 
   scoped_observer_.Add(web_data_backend_);
+
+  LoadMetadata();

[skym, 2017/01/14 00:04:33]

As far as I can tell, no unit tests check that this was called.

[Patrick Noland, 2017/01/17 22:59:55]

Done.

 }
 
 AutocompleteSyncBridge::~AutocompleteSyncBridge() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 std::unique_ptr<MetadataChangeList>
 AutocompleteSyncBridge::CreateMetadataChangeList() {
   DCHECK(thread_checker_.CalledOnValidThread());
   return base::MakeUnique<AutofillMetadataChangeList>(GetAutofillTable(),
@@ skipping 77 matching lines @@
     return;
   }
 
   auto batch = base::MakeUnique<MutableDataBatch>();
   for (const AutofillEntry& entry : entries) {
     batch->Put(GetStorageKeyFromModel(entry.key()), CreateEntityData(entry));
   }
   callback.Run(std::move(batch));
 }
 
+void AutocompleteSyncBridge::ActOnLocalChanges(
+    const AutofillChangeList& changes) {
+  if (!change_processor()->IsTrackingMetadata()) {
+    return;
+  }
+
+  std::unique_ptr<AutofillMetadataChangeList> change_list =

[maxbogue, 2017/01/14 00:39:02]

auto mcl = base::MakeUnique...

auto because MakeUnique makes the type clear, and mcl because to me it's
actually more descriptive than change_list (less confuse-able with non-metadata
changes)

[skym, 2017/01/17 17:57:21]

I'm not a huge fan of abbreviations. I'd prefer metadata_change_list or
metadata_changes, both of which we have precedent using. But I'm okay with
whatever else is chosen.

[Patrick Noland, 2017/01/17 22:59:55]

Done. I also don't love abbreviations whose meanings aren't obvious, so I went
with metadata_change_list.

+      base::MakeUnique<AutofillMetadataChangeList>(GetAutofillTable(),
+                                                   syncer::AUTOFILL);
+  for (const auto& change : changes) {
+    const 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);
+        if (!success) {
+          change_processor()->ReportError(
+              FROM_HERE, "Failed reading autofill entry from WebDatabase.");
+          return;
+        }
+
+        AutofillEntry entry(change.key(), date_created, date_last_used);

[skym, 2017/01/14 00:04:33]

If you're going to const the storage_key, might as well const this too, right?

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+        std::unique_ptr<syncer::EntityData> entity_data =

[skym, 2017/01/14 00:04:33]

I'd personally inline this, you're only saving a single character switching from
"CreateEntityData(entry)" to "std::move(entity_data)"

[maxbogue, 2017/01/14 00:39:02]

+1

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+            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;
+      }
+    }
+  }
+
+  Optional<ModelError> error = change_list->TakeError();
+  if (error) {
+    change_processor()->ReportError(

[skym, 2017/01/14 00:04:33]

You should give |error| (via error.value()) to ReportError() here. That way we
preserve the original FROM_HERE and hopefully a more useful message.

[maxbogue, 2017/01/14 00:39:02]

+1

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+        FROM_HERE, "Failed writing autofill metadata to WebDatabase.");
+  }
+}
+
+void AutocompleteSyncBridge::LoadMetadata() {
+  std::unique_ptr<syncer::MetadataBatch> batch =

[skym, 2017/01/14 00:04:33]

auto!

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+      base::MakeUnique<syncer::MetadataBatch>();
+  if (!GetAutofillTable()->GetAllSyncMetadata(syncer::AUTOFILL, batch.get())) {
+    change_processor()->ReportError(
+        FROM_HERE, "Failed reading autofill metadata from WebDatabase.");
+    return;
+  }
+  change_processor()->OnMetadataLoaded(std::move(batch));
+}
+
 std::string AutocompleteSyncBridge::GetClientTag(
     const EntityData& entity_data) {
   DCHECK(entity_data.specifics.has_autofill());
   const AutofillSpecifics specifics = entity_data.specifics.autofill();
   return std::string(kAutocompleteEntryNamespaceTag) +
          net::EscapePath(specifics.name()) +
          std::string(kAutocompleteTagDelimiter) +
          net::EscapePath(specifics.value());
 }
 
 std::string AutocompleteSyncBridge::GetStorageKey(
     const EntityData& entity_data) {
   DCHECK(entity_data.specifics.has_autofill());
   const AutofillSpecifics specifics = entity_data.specifics.autofill();
   return BuildSerializedStorageKey(specifics.name(), specifics.value());
 }
 
 // AutofillWebDataServiceObserverOnDBThread implementation.
 void AutocompleteSyncBridge::AutofillEntriesChanged(
     const AutofillChangeList& changes) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  ActOnLocalChanges(changes);
 }
 
 AutofillTable* AutocompleteSyncBridge::GetAutofillTable() const {
   return AutofillTable::FromWebDatabase(web_data_backend_->GetDatabase());
 }
 
 }  // namespace autofill