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

chrome/browser/password_manager/password_syncable_service.h

 // Copyright 2013 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_PASSWORD_MANAGER_PASSWORD_SYNCABLE_SERVICE_H__
 #define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_SYNCABLE_SERVICE_H__
 
 #include <string>
 #include <vector>
 
@@ skipping 42 matching lines @@
   // Returns the unique tag that will serve as the sync identifier for the
   // |password| entry.
   static std::string MakeTag(const autofill::PasswordForm& password);
   static std::string MakeTag(const sync_pb::PasswordSpecificsData& password);
   static std::string MakeTag(const std::string& origin_url,
                              const std::string& username_element,
                              const std::string& username_value,
                              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;
+
+  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:

 
   // Use the |PasswordStore| APIs to add and update entries.
-  void WriteToPasswordStore(PasswordForms* new_entries,
-                            PasswordForms* udpated_entries);
+  void WriteToPasswordStore(const PasswordForms& new_entries,
+                            const PasswordForms& updated_entries);
 
-  // Converts the |PasswordForm| to |SyncData| suitable for syncing.
-  syncer::SyncData CreateSyncData(const autofill::PasswordForm& password);
+  // 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.
   scoped_ptr<syncer::SyncErrorFactory> sync_error_factory_;
 
   // |SyncProcessor| will mirror the |PasswordStore| changes in the sync db.
   scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
 
   // The password store that adds/updates/deletes password entries.
   scoped_refptr<PasswordStore> password_store_;
 };
 
 #endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_SYNCABLE_SERVICE_H__