[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);
 
+  // Converts the |password| into a SyncData object.
+  static syncer::SyncData CreateSyncData(
+     const autofill::PasswordForm& password);

[Ilya Sherman, 2013/11/05 08:39:21]

Why is this public?  If it's just for testing, please do something more akin to
the treatment of GetFingerprintInternal here:
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

[lipalani1, 2013/11/19 00:46:54]

Done.

+  static void ExtractPasswordFromSpecifics(
+     const sync_pb::PasswordSpecificsData& password,
+     autofill::PasswordForm* new_password);

[Ilya Sherman, 2013/11/05 08:39:21]

Why is this public?

[lipalani1, 2013/11/19 00:46:54]

made it private.
On 2013/11/05 08:39:21, Ilya Sherman wrote:

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

[Ilya Sherman, 2013/11/05 08:39:21]

nit: "db" -> "database" (throughout)

[lipalani1, 2013/11/19 00:46:54]

I borrowed the abbreviation db from db thread. So I think it is an accepted
convention. let me know if you disagree.

On 2013/11/05 08:39:21, Ilya Sherman wrote:

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

I won't block the review on this, but I do think that "database" is marginally
clearer than "db", and not so lengthy as to be overly verbose.

On 2013/11/19 00:46:54, lipalani1 wrote:

+  // 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
+  // identical to an entry in passwords db no action is performed. If an

[Ilya Sherman, 2013/11/05 08:39:21]

nit: "an entry in passwords db no action" -> "an entry in the passwords
database, no action"

[lipalani1, 2013/11/19 00:46:54]

Done.

+  // item needs to be updated in the sync database then the item is also
+  // added to |updated_db_entries| list.

[Ilya Sherman, 2013/11/05 08:39:21]

Please document that this method has a side-effect of removing entries from
|loaded_data|.

[lipalani1, 2013/11/19 00:46:54]

Done.

+  void CreateOrUpdateEntry(
+      const syncer::SyncData& data,
+      PasswordEntryMap* loaded_data,
+      ScopedVector<autofill::PasswordForm>* new_entries,
+      ScopedVector<autofill::PasswordForm>* updated_entries,
+      syncer::SyncChangeList* updated_db_entries);

[Ilya Sherman, 2013/11/05 08:39:21]

nit: "db" -> "database" in variable names as well.  Something like
"updated_local_entries" would also work.

[lipalani1, 2013/11/19 00:46:54]

Done.

+
+  // Virtual so tests can override.

[Ilya Sherman, 2013/11/05 08:39:21]

Please document what this method does.

[Ilya Sherman, 2013/11/05 08:39:21]

Subclasses should not override private methods... please raise the visibility of
this to be protected if it's meant to be overridden.

[lipalani1, 2013/11/19 00:46:54]

Done.

[lipalani1, 2013/11/19 00:46:54]

Done.

+  virtual void NotifyPasswordStore();

[Ilya Sherman, 2013/11/05 08:39:21]

This method name is somewhat unclear -- what is the password store being
notified of?  Please try to find a more descriptive name.

[lipalani1, 2013/11/19 00:46:54]

Done.

 
   // 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__