Migrate AutofillProfile sync to new API.

chrome/browser/sync/glue/autofill_profile_syncable_service.h

+// Copyright (c) 2011 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.
+
+#ifndef CHROME_BROWSER_SYNC_GLUE_AUTOFILL_PROFILE_SYNCABLE_SERVICE_H_
+#define CHROME_BROWSER_SYNC_GLUE_AUTOFILL_PROFILE_SYNCABLE_SERVICE_H_
+#pragma once
+
+#include <map>
+#include <set>
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/non_thread_safe.h"
+#include "chrome/browser/autofill/personal_data_manager.h"
+#include "chrome/browser/sync/api/sync_change.h"
+#include "chrome/browser/sync/api/sync_data.h"
+#include "chrome/browser/sync/api/sync_error.h"
+#include "chrome/browser/sync/api/syncable_service.h"
+#include "chrome/browser/sync/protocol/autofill_specifics.pb.h"
+#include "chrome/browser/webdata/autofill_change.h"
+#include "chrome/browser/webdata/autofill_entry.h"
+#include "content/common/content_notification_types.h"
+#include "content/common/notification_observer.h"
+#include "content/common/notification_registrar.h"
+
+class AutofillProfile;
+class Profile;
+class ProfileSyncServiceAutofillTest;
+
+class WebDatabase;
+
+namespace browser_sync {
+
+extern const char kAutofillProfileTag[];
+
+class UnrecoverableErrorHandler;
+
+// Contains all model association related logic:
+// * Algorithm to associate autofill model and sync model.
+// We do not check if we have local data before this run; we always
+// merge and sync.

[Ilya Sherman, 2011/09/02 21:49:50]

nit: This comment seems really out of context here.  Perhaps it should be
located with the implementation instead; and a replaced with a more general,
high-level comment here?

+class AutofillProfileSyncableService
+    : public SyncableService,
+      public NotificationObserver,
+      public base::NonThreadSafe {
+ public:
+  AutofillProfileSyncableService(WebDatabase* web_database,
+                                 PersonalDataManager* data_manager,
+                                 Profile* profile);
+  virtual ~AutofillProfileSyncableService();
+
+  static syncable::ModelType model_type() { return syncable::AUTOFILL_PROFILE; }
+
+  // SyncableService implementation.
+  virtual SyncError MergeDataAndStartSyncing(
+      syncable::ModelType type,
+      const SyncDataList& initial_sync_data,
+      SyncChangeProcessor* sync_processor) OVERRIDE;
+  virtual void StopSyncing(syncable::ModelType type) OVERRIDE;
+  virtual SyncDataList GetAllSyncData(syncable::ModelType type) const OVERRIDE;
+  virtual SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const SyncChangeList& change_list) OVERRIDE;
+
+  // NotificationObserver implementation.
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) OVERRIDE;
+
+ protected:
+  // The map of the guid to index in the |profiles_| vector.
+  typedef std::map<std::string, size_t> GUIDToProfileMap;

[Ilya Sherman, 2011/09/02 21:49:50]

nit: Can this be moved to "private:"?

[Ilya Sherman, 2011/09/02 21:49:50]

nit: Why map to an index rather than to an |AutofillProfile*| ?

[GeorgeY, 2011/09/03 00:01:29]

All the Load/Save accessors use vector<AutofillProfile*>, so doing that would've
required coping to and from before each operation. As for index instead of an
iterator - it is guaranteed to be immutable to addition operation of the vector
in all std:: implementations.

[GeorgeY, 2011/09/03 00:01:29]

Done.

[Ilya Sherman, 2011/09/03 00:10:57]

I don't follow what you mean here.  Storing the index means that you need to
index into |profiles_| to pull out the profile.  Storing the pointer lets you
skip the indexing, but is otherwise syntactically equivalent as far as I can
tell.  Storing the pointer also seems a bit semantically clearer to me.

+
+  // A convenience wrapper of a bunch of state we pass around while
+  // associating models, and send to the WebDatabase for persistence.
+  // We do this so we hold the write lock for only a small period.
+  // When storing the web db we are out of the write lock.
+  struct DataBundle;
+
+  // Helper to query WebDatabase for the current autofill state.
+  // Made virtual for ease of mocking in the unit-test.
+  virtual bool LoadAutofillData(std::vector<AutofillProfile*>* profiles) const;

[Ilya Sherman, 2011/09/02 21:49:50]

nit: Probably worth mentioning in the function comment that the caller owns the
returned |AutofillProfile|'s.

[GeorgeY, 2011/09/03 00:01:29]

Done.

+
+  // Helper to persist any changes that occured during model association to
+  // the WebDatabase.
+  // Made virtual for ease of mocking in the unit-test.
+  virtual bool SaveChangesToWebData(const DataBundle& bundle);
+
+ private:
+  friend class ::ProfileSyncServiceAutofillTest;
+  friend class MockAutofillProfileSyncableService;
+  FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest,
+                           MergeDataAndStartSyncing);
+  FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest, GetAllSyncData);
+  FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest,
+                           ProcessSyncChanges);
+
+  // Helper function that overwrites |profile| with data from proto-buffer
+  // |specifics|.
+  static bool OverwriteProfileWithServerData(
+      const sync_pb::AutofillProfileSpecifics& specifics,
+      AutofillProfile* profile);
+
+  // Writes |profile| data into supplied |profile_specifics|.
+  static void WriteAutofillProfile(const AutofillProfile& profile,
+                                   sync_pb::EntitySpecifics* profile_specifics);
+
+  // Creates |profile_map| from the supplied |profiles| vector. Necessary for
+  // fast processing of the changes.
+  void CreateGUIDToProfileMap(const ScopedVector<AutofillProfile>& profiles,

[Ilya Sherman, 2011/09/02 21:49:50]

nit: Passing around ScopedVectors, even by reference, makes me a little bit
anxious.  Can you change this to const std::vector<AutofillProfile>& instead?

[GeorgeY, 2011/09/03 00:01:29]

Done.

+                              GUIDToProfileMap* profile_map);
+
+  // Creates or updates a profile based on |data|. Looks at the guid of the data
+  // if a profile with such guid is present in |profile_map| updates it. If not
+  // searches through it for similar profiles. if similar profile is
+  // found substitutes it for the new one, otherwise add a new profile. Returns
+  // iterator pointing to added/updated profile.

[Ilya Sherman, 2011/09/02 21:49:50]

nit: There are several sentence breaks in this comment that are missing
punctuation and/or capitalization.

[GeorgeY, 2011/09/03 00:01:29]

Done.

+  GUIDToProfileMap::iterator CreateOrUpdateProfile(
+      const SyncData& data, GUIDToProfileMap* profile_map, DataBundle* bundle);
+
+  // Syncs |change| to the cloud.
+  void ActOnChange(AutofillProfileChange* change);

[Ilya Sherman, 2011/09/02 21:49:50]

nit: Should |change| be passed by const-ref here?

[GeorgeY, 2011/09/03 00:01:29]

Done.

+
+  // Creates SyncData based on supplied |profile|.
+  static SyncData CreateData(const AutofillProfile& profile);
+
+  // For unit-tests.
+  AutofillProfileSyncableService();
+  void set_sync_processor(SyncChangeProcessor* sync_processor) {
+    sync_processor_ = sync_processor;
+  }
+
+  WebDatabase* web_database_;
+  PersonalDataManager* personal_data_;
+  NotificationRegistrar notification_registrar_;
+
+  // Cached Autofill profiles. *Warning* deleted profiles are still in the
+  // vector - use the |profiles_map_| to iterate through actual profiles.
+  ScopedVector<AutofillProfile> profiles_;
+  GUIDToProfileMap profiles_map_;
+
+  SyncChangeProcessor* sync_processor_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutofillProfileSyncableService);
+};
+
+struct AutofillProfileSyncableService::DataBundle {

[Ilya Sherman, 2011/09/02 21:49:50]

nit: Quoth David, "Move this to the .cc file".  Quoth you, "Done."  But it's
still here ;)

[GeorgeY, 2011/09/03 00:01:29]

Quoth me :): it is used in unit-tests, so it needs to be in header.
"Done" meant checked that it needed here and so left it :)

[Ilya Sherman, 2011/09/03 00:10:57]

Ok :)

+  DataBundle();
+  ~DataBundle();
+
+  std::vector<std::string> profiles_to_delete;
+  std::vector<AutofillProfile*> updated_profiles;
+  std::vector<AutofillProfile*> new_profiles;
+};
+
+}  // namespace browser_sync
+
+#endif  // CHROME_BROWSER_SYNC_GLUE_AUTOFILL_PROFILE_SYNCABLE_SERVICE_H_
+