PasswordStore: Clean up expectations about rewriting vectors of forms

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>
 
@@ skipping 136 matching lines @@
     // until we perform a write operation, or until a read returns actual data.
     if (matched_forms.size() > 0)
       allow_fallback_ = false;
   } else if (allow_default_store()) {
     DCHECK(matched_forms.empty());
     return PasswordStoreDefault::FillMatchingLogins(form, prompt_policy);
   }
   return matched_forms.Pass();
 }
 
 void PasswordStoreX::GetAutofillableLoginsImpl(

[engedy, 2015/02/25 15:17:48]

Do we override the base class implementation for this and below only so that the
results get sorted?

Given that the Fill[...]() methods are only called (once?) by the Sync backend,
I think we could add the sorting there without much performance impact, and use
the base class implementations for these
Get{Autofillable|Blacklisted}LoginsImpl?

[vabr (Chromium), 2015/03/09 10:56:19]

This sounds very reasonable.
I propose to do it in a separate CL, though. If there are issues I'm not seeing
yet, or the performance hits us or something, it would be easier to revert. The
CL is at https://codereview.chromium.org/986983002/.

[engedy, 2015/03/09 13:33:17]

Acknowledged.

     scoped_ptr<PasswordStore::GetLoginsRequest> request) {
   CheckMigration();
   ScopedVector<autofill::PasswordForm> obtained_forms;
   if (use_native_backend() &&
       backend_->GetAutofillableLogins(&obtained_forms)) {
     SortLoginsByOrigin(&obtained_forms.get());
     // See GetLoginsImpl() for why we disallow fallback conditionally here.
     if (!obtained_forms.empty())
       allow_fallback_ = false;
     request->NotifyConsumerWithResults(obtained_forms.Pass());
@@ skipping 20 matching lines @@
   } else if (allow_default_store()) {
     PasswordStoreDefault::GetBlacklistLoginsImpl(request.Pass());
     return;
   }
   // The consumer will be left hanging unless we reply.
   request->NotifyConsumerWithResults(ScopedVector<autofill::PasswordForm>());
 }
 
 bool PasswordStoreX::FillAutofillableLogins(
     ScopedVector<autofill::PasswordForm>* forms) {
   CheckMigration();

[engedy, 2015/02/25 15:17:48]

Need to clear |forms| here.

[vabr (Chromium), 2015/03/09 10:56:19]

Done.

   if (use_native_backend() && backend_->GetAutofillableLogins(forms)) {
     // See GetLoginsImpl() for why we disallow fallback conditionally here.
     if (forms->size() > 0)
       allow_fallback_ = false;
     return true;
   }
   if (allow_default_store())
     return PasswordStoreDefault::FillAutofillableLogins(forms);
   return false;
 }
 
 bool PasswordStoreX::FillBlacklistLogins(
     ScopedVector<autofill::PasswordForm>* forms) {
   CheckMigration();

[engedy, 2015/02/25 15:17:48]

Need to clear |forms| here.

[vabr (Chromium), 2015/03/09 10:56:19]

Done.

   if (use_native_backend() && backend_->GetBlacklistLogins(forms)) {
     // See GetLoginsImpl() for why we disallow fallback conditionally here.
     if (forms->size() > 0)
       allow_fallback_ = false;
     return true;
   }
   if (allow_default_store())
     return PasswordStoreDefault::FillBlacklistLogins(forms);
   return false;
 }
@@ skipping 27 matching lines @@
     backend_.reset();
     // Don't warn again. We'll use the default store because backend_ is NULL.
     allow_fallback_ = false;
   }
   return !backend_.get();
 }
 
 ssize_t PasswordStoreX::MigrateLogins() {
   DCHECK(backend_.get());
   ScopedVector<autofill::PasswordForm> forms;
+  ScopedVector<autofill::PasswordForm> blacklist_forms;
   bool ok = PasswordStoreDefault::FillAutofillableLogins(&forms) &&
-      PasswordStoreDefault::FillBlacklistLogins(&forms);
+            PasswordStoreDefault::FillBlacklistLogins(&blacklist_forms);
+  forms.reserve(forms.size() + blacklist_forms.size());
+  forms.insert(forms.end(), blacklist_forms.begin(), blacklist_forms.end());
+  blacklist_forms.weak_clear();
   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) {
       PasswordStoreChangeList changes;
       if (!AddLoginToBackend(backend_, *forms[i], &changes)) {
         ok = false;
         break;
@@ skipping 12 matching lines @@
       // 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;
   return result;
 }