Password Login Database: report correct changes from AddLogin().

chrome/browser/password_manager/password_store_x.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/password_manager/password_store_x.h"
 
 #include <algorithm>
 #include <map>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "components/password_manager/core/browser/password_store_change.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 
 using autofill::PasswordForm;
 using content::BrowserThread;
 using password_manager::PasswordStoreChange;
 using password_manager::PasswordStoreChangeList;
 using password_manager::PasswordStoreDefault;
 using std::vector;
 
+namespace {
+
+bool AddLoginToBackend(const scoped_ptr<PasswordStoreX::NativeBackend>& backend,
+                       const PasswordForm& form,
+                       PasswordStoreChangeList* changes) {
+  *changes = backend->AddLogin(form);
+  return !changes->empty() &&

[Garrett Casto, 2014/05/15 20:48:52]

Nit: I prefer adding parens here "(!changes->empty()" so that the next line
should indent to line up instead of just 4 characters.

[vasilii, 2014/05/16 08:18:35]

Done.

+      changes->back().type() == PasswordStoreChange::ADD;
+}
+
+}  // namespace
+
 PasswordStoreX::PasswordStoreX(
     scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner,
     scoped_refptr<base::SingleThreadTaskRunner> db_thread_runner,
     password_manager::LoginDatabase* login_db,
     NativeBackend* backend)
     : PasswordStoreDefault(main_thread_runner, db_thread_runner, login_db),
       backend_(backend),
       migration_checked_(!backend),
       allow_fallback_(false) {}
 
 PasswordStoreX::~PasswordStoreX() {}
 
 PasswordStoreChangeList PasswordStoreX::AddLoginImpl(const PasswordForm& form) {
   CheckMigration();
   PasswordStoreChangeList changes;
-  if (use_native_backend() && backend_->AddLogin(form)) {
-    changes.push_back(PasswordStoreChange(PasswordStoreChange::ADD, form));
+  if (use_native_backend() && AddLoginToBackend(backend_, form, &changes)) {
     allow_fallback_ = false;
   } else if (allow_default_store()) {
     changes = PasswordStoreDefault::AddLoginImpl(form);
   }
   return changes;
 }
 
 PasswordStoreChangeList PasswordStoreX::UpdateLoginImpl(
     const PasswordForm& form) {
   CheckMigration();
@@ skipping 174 matching lines @@
   DCHECK(backend_.get());
   vector<PasswordForm*> forms;
   bool ok = PasswordStoreDefault::FillAutofillableLogins(&forms) &&
       PasswordStoreDefault::FillBlacklistLogins(&forms);
   if (ok) {
     // We add all the passwords (and blacklist entries) to the native backend
     // before attempting to remove any from the login database, to make sure we
     // don't somehow end up with some of the passwords in one store and some in
     // another. We'll always have at least one intact store this way.
     for (size_t i = 0; i < forms.size(); ++i) {
-      if (!backend_->AddLogin(*forms[i])) {
+      PasswordStoreChangeList changes;
+      if (!AddLoginToBackend(backend_, *forms[i], &changes)) {
         ok = false;
         break;
       }
     }
     if (ok) {
       for (size_t i = 0; i < forms.size(); ++i) {
         // If even one of these calls to RemoveLoginImpl() succeeds, then we
         // should prefer the native backend to the now-incomplete login
         // database. Thus we want to return a success status even in the case
         // where some fail. The only real problem with this is that we might
         // leave passwords in the login database and never come back to clean
         // them out if any of these calls do fail.
         PasswordStoreDefault::RemoveLoginImpl(*forms[i]);
       }
       // Finally, delete the database file itself. We remove the passwords from
       // it before deleting the file just in case there is some problem deleting
       // the file (e.g. directory is not writable, but file is), which would
       // otherwise cause passwords to re-migrate next (or maybe every) time.
       DeleteAndRecreateDatabaseFile();
     }
   }
   ssize_t result = ok ? forms.size() : -1;
   STLDeleteElements(&forms);
   return result;
 }