[Sync] Implementation of model association for passwords using sync API

chrome/browser/password_manager/password_syncable_service.h

Index: chrome/browser/password_manager/password_syncable_service.h
diff --git a/chrome/browser/password_manager/password_syncable_service.h b/chrome/browser/password_manager/password_syncable_service.h
index dd32b2c323ab774c14fe583990b7c3724055809e..ef3e246a67b6791dbf0025ea8327829eab39cedb 100644
--- a/chrome/browser/password_manager/password_syncable_service.h
+++ b/chrome/browser/password_manager/password_syncable_service.h
@@ -60,15 +60,51 @@ class PasswordSyncableService : public syncer::SyncableService {
                              const std::string& password_element,
                              const std::string& signon_realm);
 
+ protected:
+  // Notifies password store of a change that was performed by sync.
+  // Virtual so tests can override.
+  virtual void NotifyPasswordStoreOfLoginChanges();
+
  private:
   typedef std::vector<autofill::PasswordForm*> PasswordForms;
+  typedef std::map<std::string, autofill::PasswordForm*>
+      PasswordEntryMap;
 
-  // Use the |PasswordStore| APIs to add and update entries.
-  void WriteToPasswordStore(PasswordForms* new_entries,
-                            PasswordForms* udpated_entries);
+  friend class PasswordStoreDataVerifier;
+  friend class TestSyncChangeProcessor;

[Ilya Sherman, 2013/11/19 22:49:04]

Please do not add friend classes.  Instead, use protected methods that tests can
override.

[lipalani1, 2013/11/26 23:31:35]

These classes need to use private helpers to reduce their complexity. The
inheritance relationship does not make sense as they test something else.(i.e,.
we would have to do multiple inheritance).
On 2013/11/19 22:49:04, Ilya Sherman wrote:

[Ilya Sherman, 2013/12/05 07:01:54]

My understanding is that you just want to be able to use the static methods
CreateSyncData and ExtractPasswordFromSpecifics.  If so, you can do this like
so:

class TestPasswordSyncableService : public PasswordSyncableService {
 public:
  TestPasswordSyncableService() {}
  virtual ~TestPasswordSyncableService() {}

  using PasswordSyncableService::CreateSyncData;
  using PasswordSyncableService::ExtractPasswordFromSpecifics;

 private:
  DISALLOW_COPY_AND_ASSIGN(TestPasswordSyncableService);
};

Or, even better, you could do something like:

# In password_syncable_service.cc

namespace internal {

syncer::SyncData CreateSyncData(const autofill::PasswordForm& password) {
  // impl
}

void ExtractPasswordFromSpecifics(...) {
  // impl
}

}  // namespace internal

# In password_syncable_service_unittest.cc

// Forward-declare internal functions shared with tests:

namespace internal {

syncer::SyncData CreateSyncData(const autofill::PasswordForm& password);
void ExtractPasswordFromSpecifics(...);

}

You can then call these free functions from the test code, but they are not
exported as part of the header.  You can then also tuck functions that need to
call these into an anonymous namespace, if those functions themselves don't need
to be exported.  This isn't just a weird idea that I invented -- this was *the*
pattern recommended to me by my C++ readability reviewer.

On 2013/11/26 23:31:35, lipalani1 wrote:

 
-  // Converts the |PasswordForm| to |SyncData| suitable for syncing.
-  syncer::SyncData CreateSyncData(const autofill::PasswordForm& password);
+  // Use the |PasswordStore| APIs to add and update entries.
+  void WriteToPasswordStore(const PasswordForms& new_entries,
+                            const PasswordForms& updated_entries);
+
+  // Checks if |data|, the entry in sync db, needs to be created or updated
+  // in the passwords db. Depending on what action needs to be performed the
+  // entry is added to |new_entries| or |updated_entries|. If the item is

[Ilya Sherman, 2013/11/19 22:49:04]

nit: "is added to" -> "may be added to", since it's possible that this method is
a no-op (if the entry already exists in the passwords db).

[lipalani1, 2013/11/26 23:31:35]

Done.

+  // identical to an entry in passwords db, no action is performed. If an
+  // item needs to be updated in the sync database then the item is also
+  // added to |updated_local_entries| list.If |data| is identical to an entry

[Ilya Sherman, 2013/11/19 22:49:04]

nit: "|updated_local_entries| list.If" -> "|updated_local_entries|. If"

[lipalani1, 2013/11/26 23:31:35]

Done.

+  // in |loaded_data| then that entry will be removed from |loaded_data|.
+  void CreateOrUpdateEntry(
+      const syncer::SyncData& data,
+      PasswordEntryMap* loaded_data,

[Ilya Sherman, 2013/11/19 22:49:04]

nit: Perhaps rename |loaded_data| to something more like
|tentative_unsynced_data|?

[lipalani1, 2013/11/26 23:31:35]

Done.

+      ScopedVector<autofill::PasswordForm>* new_entries,
+      ScopedVector<autofill::PasswordForm>* updated_entries,

[Ilya Sherman, 2013/11/19 22:49:04]

nit: Where is ScopedVector fwd-declared?  It seems like you might be pulling
this declaration in as an implicit dependency from other header files.

[lipalani1, 2013/11/26 23:31:35]

Done.

+      syncer::SyncChangeList* updated_local_entries);
+
+  // Converts the |password| into a SyncData object.
+  static syncer::SyncData CreateSyncData(
+     const autofill::PasswordForm& password);

[Ilya Sherman, 2013/11/19 22:49:04]

nit: Please leave a blank line after this one.

[lipalani1, 2013/11/26 23:31:35]

Done.

+  static void ExtractPasswordFromSpecifics(

[Ilya Sherman, 2013/11/19 22:49:04]

nit: Please document.

[lipalani1, 2013/11/26 23:31:35]

Done.

+     const sync_pb::PasswordSpecificsData& password,
+     autofill::PasswordForm* new_password);
+
+  // Merges the local and sync passwords and outputs the entry into
+  // |new_password_form|. Returns true if the local and the sync
+  // passwords differ. Returns false if they are identical.
+  bool MergeLocalAndSyncPasswords(
+      const sync_pb::PasswordSpecificsData& password_specifics,
+      const autofill::PasswordForm& password_form,
+      autofill::PasswordForm* new_password_form);
 
   // The factory that creates sync errors. |SyncError| has rich data
   // suitable for debugging.