Address readability comments in PasswordSyncableService.

components/password_manager/core/browser/password_syncable_service.h

Index: components/password_manager/core/browser/password_syncable_service.h
diff --git a/components/password_manager/core/browser/password_syncable_service.h b/components/password_manager/core/browser/password_syncable_service.h
index b70ea1f0c4edab6a2a124fdd422a2af8e5e74862..f8589d8ccd17fb6f8a8f8027c8d1fe242e11acea 100644
--- a/components/password_manager/core/browser/password_syncable_service.h
+++ b/components/password_manager/core/browser/password_syncable_service.h
@@ -34,14 +34,16 @@ namespace password_manager {
 
 class PasswordStoreSync;
 
+// The implementation of Syncable Service API for the passwords.
 class PasswordSyncableService : public syncer::SyncableService,
                                 public base::NonThreadSafe {
  public:
-  // |PasswordSyncableService| is owned by |PasswordStore|.
+  // |PasswordSyncableService| is typically owned by |password_store|. The
+  // constructor doesn't take the ownership of |password_store|.
   explicit PasswordSyncableService(PasswordStoreSync* password_store);
   virtual ~PasswordSyncableService();
 
-  // syncer::SyncableServiceImplementations
+  // syncer::SyncableService:
   virtual syncer::SyncMergeResult MergeDataAndStartSyncing(
       syncer::ModelType type,
       const syncer::SyncDataList& initial_sync_data,
@@ -54,22 +56,42 @@ class PasswordSyncableService : public syncer::SyncableService,
       const tracked_objects::Location& from_here,
       const syncer::SyncChangeList& change_list) OVERRIDE;
 
-  // Notifies sync of changes to the password database.
+  // Notifies the Sync engine of changes to the password database.
   void ActOnPasswordStoreChanges(const PasswordStoreChangeList& changes);
 
   // Provides a StartSyncFlare to the SyncableService. See
-  // sync_start_util for more.
+  // chrome/browser/sync/glue/sync_start_util.h for more.
   void InjectStartSyncFlare(
       const syncer::SyncableService::StartSyncFlare& flare);
 
+  // Only exposed for testing:
+  // Converts the |password| into a SyncData object.
+  static syncer::SyncData SyncDataFromPassword(
+      const autofill::PasswordForm& password);
+
+  // Extracts the |PasswordForm| data from sync's protobuffer format.
+  static void PasswordFromSpecifics(
+      const sync_pb::PasswordSpecificsData& password,
+      autofill::PasswordForm* new_password);
+
+  // Returns the unique tag that will serve as the sync identifier for the
+  // |password| entry.
+  static std::string MakePasswordSyncTag(
+      const sync_pb::PasswordSpecificsData& password);
+
+ protected:
+  // Notifies password store of a change that was performed by sync.
+  // Virtual so tests can override.
+  virtual void NotifyPasswordStoreOfLoginChanges(
+      const PasswordStoreChangeList& changes);
+
  private:
   typedef std::vector<autofill::PasswordForm*> PasswordForms;
   // Map from password sync tag to password form.
   typedef std::map<std::string, autofill::PasswordForm*> PasswordEntryMap;
 
-  // Helper function to retrieve the entries from password db and fill both
-  // |password_entries| and |passwords_entry_map|. |passwords_entry_map| can be
-  // NULL.
+  // Retrieves the entries from password db and fills both |password_entries|
+  // and |passwords_entry_map|. |passwords_entry_map| can be NULL.
   bool ReadFromPasswordStore(
       ScopedVector<autofill::PasswordForm>* password_entries,
       PasswordEntryMap* passwords_entry_map) const;
@@ -79,19 +101,10 @@ class PasswordSyncableService : public syncer::SyncableService,
                             const PasswordForms& updated_entries,
                             const PasswordForms& deleted_entries);
 
-  // Notifies password store of a change that was performed by sync.
-  // Virtual so tests can override.
-  virtual void NotifyPasswordStoreOfLoginChanges(
-      const PasswordStoreChangeList& changes);
-
-  // 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 may be added to |new_sync_entries| or to |updated_sync_entries|. If
-  // the item is identical to an entry in the passwords db, no action is
-  // performed. If an item needs to be updated in the sync db, then the item is
-  // also added to |updated_db_entries| list. If |data|'s tag is identical to
-  // an entry's tag in |umatched_data_from_password_db| then that entry will be
-  // removed from |umatched_data_from_password_db|.
+  // Checks if |data|, an entry in sync db, and updates |new_sync_entries| or
+  // |updated_sync_entries| or |updated_db_entries| accordingly. An element is
+  // removed from |unmatched_data_from_password_db| if its tag is identical to
+  // |data|'s.
   void CreateOrUpdateEntry(
       const syncer::SyncData& data,
       PasswordEntryMap* umatched_data_from_password_db,
@@ -99,17 +112,26 @@ class PasswordSyncableService : public syncer::SyncableService,
       ScopedVector<autofill::PasswordForm>* updated_sync_entries,
       syncer::SyncChangeList* updated_db_entries);
 
+  // Calls |modify| for each element in |entries| and appends the changes to
+  // |all_changes|.
+  void AppendChanges(
+      PasswordStoreChangeList (password_manager::PasswordStoreSync::*modify)(
+          const autofill::PasswordForm& form),
+      const PasswordForms& entries,
+      PasswordStoreChangeList* all_changes);
+
   // 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.
+  // |sync_processor_| will mirror the |PasswordStore| changes in the sync db.
   scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
 
-  // The password store that adds/updates/deletes password entries.
+  // The password store that adds/updates/deletes password entries. Not owned by
+  // this class.
   PasswordStoreSync* const password_store_;
 
-  // A signal to start sync as soon as possible.
+  // A signal activated by this class to start sync as soon as possible.
   syncer::SyncableService::StartSyncFlare flare_;
 
   // True if processing sync changes is in progress.
@@ -118,17 +140,6 @@ class PasswordSyncableService : public syncer::SyncableService,
   DISALLOW_COPY_AND_ASSIGN(PasswordSyncableService);
 };
 
-// Converts the |password| into a SyncData object.
-syncer::SyncData SyncDataFromPassword(const autofill::PasswordForm& password);
-
-// Extracts the |PasswordForm| data from sync's protobuffer format.
-void PasswordFromSpecifics(const sync_pb::PasswordSpecificsData& password,
-                           autofill::PasswordForm* new_password);
-
-// Returns the unique tag that will serve as the sync identifier for the
-// |password| entry.
-std::string MakePasswordSyncTag(const sync_pb::PasswordSpecificsData& password);
-
 }  // namespace password_manager
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_SYNCABLE_SERVICE_H__