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;
+

[dhollowa, 2011/09/01 20:17:49]

nit: please remove extra line.

[GeorgeY, 2011/09/02 04:34:12]

Done.

+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.
+class AutofillProfileSyncableService
+    : public SyncableService,
+      public NotificationObserver,
+      public base::NonThreadSafe {
+ public:
+  AutofillProfileSyncableService(WebDatabase* web_database,
+                                 PersonalDataManager* data_manager);
+  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);

[dhollowa, 2011/09/01 20:17:49]

nit: OVERRIDE

[GeorgeY, 2011/09/02 04:34:12]

Done.

+
+  // Helper function that overwrites |merge_into| with data from proto-buffer
+  // |specifics|.
+  static bool OverwriteProfileWithServerData(

[dhollowa, 2011/09/01 20:17:49]

Since these are helpers for unit tests, please make them private and add the
unit tests as friends.

[GeorgeY, 2011/09/02 04:34:12]

Done.

+      AutofillProfile* merge_into,

[dhollowa, 2011/09/01 20:17:49]

nit: output parameters come after input parameters.  Also let's just call the
parameter |profile| instead of |merge_into| and get rid of the temporary |p|
variable in the implementation.

[GeorgeY, 2011/09/02 04:34:12]

Copied verbatim from the old file - changed, sure.

+      const sync_pb::AutofillProfileSpecifics& specifics);
+
+  // Writes |profile| data into supplied |profile_specifics|.
+  static void WriteAutofillProfile(const AutofillProfile& profile,
+                                   sync_pb::EntitySpecifics* profile_specifics);
+
+ protected:

[dhollowa, 2011/09/01 20:17:49]

It looks like too much is "protected".  Only |DataBundle|, |LoadAutofillData|
and |SaveChangesToWebData| need to be.  The rest can be private.

[GeorgeY, 2011/09/02 04:34:12]

Done.

+  typedef std::map<std::string, ScopedVector<AutofillProfile>::iterator>
+      GuidToProfileMap;

[dhollowa, 2011/09/01 20:17:49]

nit: To keep consistent with webdata and other autofill code please use all-caps
"GUID" throughout.

[GeorgeY, 2011/09/02 04:34:12]

Done.

+
+  // 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;
+
+  // 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);
+
+  // Creates SyncChange based on supplied |action| and |profile|.
+  SyncChange CreateChange(SyncChange::SyncChangeType action,

[dhollowa, 2011/09/01 20:17:49]

It appears that all of these "Create*" methods don't need object state.  So
they'd be better moved into the .cc file in an anonymous namespace as helper
methods.

[GeorgeY, 2011/09/02 04:34:12]

Done.

[dhollowa, 2011/09/07 01:48:15]

CreateGUIDToProfileMap and CreateData did not get moved.  It makes sense to
leave CreateOrUpdateProfile here.

[GeorgeY, 2011/09/07 20:55:07]

Please verify that you are looking at latest code.

+                          const AutofillProfile& profile);
+
+  // Creates ACTION_DELETE SyncChange based on supplied |guid|.
+  SyncChange CreateDeleteChange(const std::string& guid);
+
+  // Creates SyncData based on supplied |profile|.
+  SyncData CreateData(const AutofillProfile& profile);
+
+  // Creates |profile_map| from the supplied |profiles| vector. Necessary for
+  // fast processing of the changes.
+  void CreateGuidToProfileMap(ScopedVector<AutofillProfile>* profiles,
+                              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 |profiles| for similar profiles. if similar profile is
+  // found substitutes it for the new one, otherwise add a new profile. Returns
+  // true if addition happened, false otherwise.
+  bool CreateOrUpdateProfile(const SyncData& data,
+                             ScopedVector<AutofillProfile>* profiles,
+                             GuidToProfileMap* profile_map,
+                             DataBundle *bundle);

[dhollowa, 2011/09/01 20:17:49]

nit: s/ */* /

[GeorgeY, 2011/09/02 04:34:12]

Done.

+
+  // Syncs |change| to the cloud.
+  void ActOnChange(AutofillProfileChange* change);
+
+  // For unit-tests.
+  AutofillProfileSyncableService();
+  void set_sync_processor(SyncChangeProcessor* sync_processor) {
+    sync_processor_ = sync_processor;
+  }
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest,
+                           MergeDataAndStartSyncing);
+  FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest, GetAllSyncData);
+  FRIEND_TEST_ALL_PREFIXES(AutofillProfileSyncableServiceTest,
+                           ProcessSyncChanges);
+  // A convenience wrapper of a bunch of state we pass around while associating
+  // models, and send to the WebDatabase for persistence.
+  // struct DataBundle;

[dhollowa, 2011/09/01 20:17:49]

nit: Looks like this comment should go.

[GeorgeY, 2011/09/02 04:34:12]

Done. Remnants from the old file

+  static bool MergeField(FormGroup* f,

[dhollowa, 2011/09/01 20:17:49]

This could also be moved into the .cc file in an anonymous namespace.  Also,
please use more descriptive names: s/f/form_group/, s/t/field_type/.

[GeorgeY, 2011/09/02 04:34:12]

Again, most of supporting functions were copied verbatim from the old file. Sure
- changed the names

+                         AutofillFieldType t,
+                         const std::string& specifics_field);
+  WebDatabase* web_database_;
+  PersonalDataManager* personal_data_;
+  NotificationRegistrar notification_registrar_;
+
+  SyncChangeProcessor* sync_processor_;
+
+  int number_of_profiles_created_;

[dhollowa, 2011/09/01 20:17:49]

I don't see where this is used.

[GeorgeY, 2011/09/02 04:34:12]

Removed - was used in old unit-test.

+
+  DISALLOW_COPY_AND_ASSIGN(AutofillProfileSyncableService);
+};
+
+struct AutofillProfileSyncableService::DataBundle {

[dhollowa, 2011/09/01 20:17:49]

Move this to the .cc file.

[GeorgeY, 2011/09/02 04:34:12]

Done.

[dhollowa, 2011/09/07 01:48:15]

It appears from other comments (and code) that this is needed in the .h for use
in the unit tests.  Please add a comment at the top of the class declaration to
this effect.

[GeorgeY, 2011/09/07 20:55:07]

Done.

+  DataBundle();
+  ~DataBundle();
+
+  std::vector<std::string> profiles_to_delete;
+  std::vector<AutofillProfile*> updated_profiles;

[dhollowa, 2011/09/01 23:11:46]

profiles_to_update?

[dhollowa, 2011/09/07 01:48:15]

This didn't get changed.

[GeorgeY, 2011/09/07 20:55:07]

Done.

+  std::vector<AutofillProfile*> new_profiles;  // We own these pointers.

[dhollowa, 2011/09/01 20:17:49]

If these are owned, how about using a scoped vector?

[dhollowa, 2011/09/01 23:11:46]

profiles_to_add?

[GeorgeY, 2011/09/02 04:34:12]

Originall class from Model*
Not anymore :).

[dhollowa, 2011/09/07 01:48:15]

This didn't get changed.
 (1) s/new_profiles/profiles_to_add/
 (2) ScopedVector

[GeorgeY, 2011/09/07 20:55:07]

1. Done.
2. It is not owned by bundle anymore.

+};
+
+}  // namespace browser_sync
+
+#endif  // CHROME_BROWSER_SYNC_GLUE_AUTOFILL_PROFILE_SYNCABLE_SERVICE_H_
+