PasswordStoreMac: Use ScopedVector::weak_erase to improve performance

chrome/browser/password_manager/password_store_mac.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_mac.h"
 #include "chrome/browser/password_manager/password_store_mac_internal.h"
 
 #include <CoreServices/CoreServices.h>
 #include <set>
 #include <string>
@@ skipping 177 matching lines @@
   return changes;
 }
 
 // Moves the content of |second| to the end of |first|.
 void AppendSecondToFirst(ScopedVector<autofill::PasswordForm>* first,
                          ScopedVector<autofill::PasswordForm>* second) {
   first->insert(first->end(), second->begin(), second->end());
   second->weak_clear();
 }
 
+// Returns the best match for |base_form| from |keychain_forms|, or nullptr if
+// there is no suitable match.
+const PasswordForm* BestKeychainFormForForm(
+    const PasswordForm& base_form,
+    const std::vector<PasswordForm*>& keychain_forms) {
+  const PasswordForm* partial_match = nullptr;
+  for (const auto* keychain_form : keychain_forms) {
+    // TODO(stuartmorgan): We should really be scoring path matches and picking
+    // the best, rather than just checking exact-or-not (although in practice
+    // keychain items with paths probably came from us).
+    if (internal_keychain_helpers::FormsMatchForMerge(
+            base_form, *keychain_form,
+            internal_keychain_helpers::FUZZY_FORM_MATCH)) {
+      if (base_form.origin == keychain_form->origin) {
+        return keychain_form;
+      } else if (!partial_match) {
+        partial_match = keychain_form;
+      }
+    }
+  }
+  return partial_match;
+}
+
+// Iterates over all elements in |forms|, passes the pointed to objects to
+// |move_form|, and clears |forms| efficiently.

[vasilii, 2015/01/30 14:43:34]

Expand the comment saying something about FormMover (that it is a callable and
so on).

[vabr (Chromium), 2015/01/30 14:49:51]

Done.

+template <typename FormMover>
+inline void MoveAllFormsOut(ScopedVector<autofill::PasswordForm>* forms,
+                            FormMover mover) {
+  for (autofill::PasswordForm* form_ptr : *forms) {
+    scoped_ptr<autofill::PasswordForm> form(form_ptr);
+    mover(form.Pass());

[vasilii, 2015/01/30 14:43:34]

mover(scoped_ptr<autofill::PasswordForm>(form_ptr));

[vabr (Chromium), 2015/01/30 14:49:51]

Done.

[vasilii, 2015/01/30 15:29:08]

Just copy-paste my comment :-)

[vabr (Chromium), 2015/01/30 16:47:51]

Hopefully done this time. :)

+  }
+  // We moved the ownership of every form out of |forms|. For performance
+  // reasons, we can just weak_clear it, instead of nullptr-ing the respective
+  // elements and letting the vector's destructor to go through the list once
+  // more. This was tested on a banchmark, and seemed to make a difference on

[vasilii, 2015/01/30 14:43:34]

bEnchmark

[vabr (Chromium), 2015/01/30 14:49:51]

Done.

+  // Mac.
+  forms->weak_clear();
+}
+
 }  // namespace
 
 #pragma mark -
 
 // TODO(stuartmorgan): Convert most of this to private helpers in
 // MacKeychainPasswordFormAdapter once it has sufficient higher-level public
 // methods to provide test coverage.
 namespace internal_keychain_helpers {
 
 // Returns a URL built from the given components. To create a URL without a
@@ skipping 187 matching lines @@
   }
   bool equal_realm = form_a.signon_realm == form_b.signon_realm;
   if (strictness == FUZZY_FORM_MATCH) {
     equal_realm |= (!form_a.original_signon_realm.empty()) &&
                    form_a.original_signon_realm == form_b.signon_realm;
   }
   return form_a.scheme == form_b.scheme && equal_realm &&
          form_a.username_value == form_b.username_value;
 }
 
-// Returns an the best match for |base_form| from |keychain_forms|, or NULL if
-// there is no suitable match.
-PasswordForm* BestKeychainFormForForm(
-    const PasswordForm& base_form,
-    const std::vector<PasswordForm*>* keychain_forms) {
-  PasswordForm* partial_match = NULL;
-  for (std::vector<PasswordForm*>::const_iterator i = keychain_forms->begin();
-       i != keychain_forms->end(); ++i) {
-    // TODO(stuartmorgan): We should really be scoring path matches and picking
-    // the best, rather than just checking exact-or-not (although in practice
-    // keychain items with paths probably came from us).
-    if (FormsMatchForMerge(base_form, *(*i), FUZZY_FORM_MATCH)) {
-      if (base_form.origin == (*i)->origin) {
-        return *i;
-      } else if (!partial_match) {
-        partial_match = *i;
-      }
-    }
-  }
-  return partial_match;
-}
-
 // Moves entries from |forms| that are blacklist entries into |blacklist|.
 void ExtractBlacklistForms(ScopedVector<autofill::PasswordForm>* forms,
                            ScopedVector<autofill::PasswordForm>* blacklist) {
   blacklist->reserve(blacklist->size() + forms->size());
   ScopedVector<autofill::PasswordForm> non_blacklist;
-  for (auto& form : *forms) {
+  // Move forms in either |non_blacklist| or |blacklist|, depending on whether
+  // they are blacklisted by the user.
+  MoveAllFormsOut(forms, [&non_blacklist, blacklist](
+                             scoped_ptr<autofill::PasswordForm> form) {
     if (form->blacklisted_by_user)
-      blacklist->push_back(form);
+      blacklist->push_back(form.release());
     else
-      non_blacklist.push_back(form);
-    form = nullptr;
-  }
+      non_blacklist.push_back(form.release());
+  });
   forms->swap(non_blacklist);
 }
 
 // Takes |keychain_forms| and |database_forms| and moves the following 2 types
 // of forms to |merged_forms|: (1) blacklisted |database_forms|, (2)
 // |database_forms| which have a corresponding entry in |keychain_forms|. The
 // database forms of type (2) have their password value updated from the
 // corresponding keychain form, and all the keychain forms corresponding to some
 // database form are removed from |keychain_forms| and deleted.
 void MergePasswordForms(ScopedVector<autofill::PasswordForm>* keychain_forms,
                         ScopedVector<autofill::PasswordForm>* database_forms,
                         ScopedVector<autofill::PasswordForm>* merged_forms) {
   // Pull out the database blacklist items, since they are used as-is rather
   // than being merged with keychain forms.
   ExtractBlacklistForms(database_forms, merged_forms);
 
   // Merge the normal entries.
   ScopedVector<autofill::PasswordForm> unused_database_forms;
   unused_database_forms.reserve(database_forms->size());
-  std::set<autofill::PasswordForm*> used_keychain_forms;
-  for (auto& db_form : *database_forms) {
-    PasswordForm* best_match =
-        BestKeychainFormForForm(*db_form, &keychain_forms->get());
+  std::set<const autofill::PasswordForm*> used_keychain_forms;
+  // Move all database forms to either |merged_forms| or
+  // |unused_database_forms|, based on whether they have a match in the keychain
+  // forms or not. If there is a match, add its password to the DB form and
+  // mark the keychain form as used.
+  MoveAllFormsOut(database_forms, [keychain_forms, &used_keychain_forms,
+                                   merged_forms, &unused_database_forms](
+                                      scoped_ptr<autofill::PasswordForm> form) {
+    const PasswordForm* best_match =
+        BestKeychainFormForForm(*form, keychain_forms->get());
     if (best_match) {
       used_keychain_forms.insert(best_match);
-      db_form->password_value = best_match->password_value;
-      merged_forms->push_back(db_form);
+      form->password_value = best_match->password_value;
+      merged_forms->push_back(form.release());
     } else {
-      unused_database_forms.push_back(db_form);
+      unused_database_forms.push_back(form.release());
     }
-    db_form = nullptr;
-  }
+  });
   database_forms->swap(unused_database_forms);
 
   // Clear out all the Keychain entries we used.
   ScopedVector<autofill::PasswordForm> unused_keychain_forms;
   unused_keychain_forms.reserve(keychain_forms->size());
   for (auto& keychain_form : *keychain_forms) {
     if (!ContainsKey(used_keychain_forms, keychain_form)) {
       unused_keychain_forms.push_back(keychain_form);
       keychain_form = nullptr;
     }
@@ skipping 34 matching lines @@
   std::vector<SecKeychainItemRef> keychain_items;
   std::vector<ItemFormPair> item_form_pairs =
       ExtractAllKeychainItemAttributesIntoPasswordForms(&keychain_items,
                                                         keychain);
 
   // Next, compare the attributes of the PasswordForms in |database_forms|
   // against those in |item_form_pairs|, and extract password data for each
   // matching PasswordForm using its corresponding SecKeychainItemRef.
   ScopedVector<autofill::PasswordForm> unused_db_forms;
   unused_db_forms.reserve(database_forms->size());
-  for (auto& db_form : *database_forms) {
+  // Move database forms with a password stored in |keychain| to |passwords|,
+  // including the password. The rest is moved to |unused_db_forms|.
+  MoveAllFormsOut(database_forms,
+                  [&keychain, &item_form_pairs, passwords, &unused_db_forms](
+                      scoped_ptr<autofill::PasswordForm> form) {
     ScopedVector<autofill::PasswordForm> keychain_matches =
-        ExtractPasswordsMergeableWithForm(keychain, item_form_pairs, *db_form);
+        ExtractPasswordsMergeableWithForm(keychain, item_form_pairs, *form);
 
     ScopedVector<autofill::PasswordForm> db_form_container;
-    db_form_container.push_back(db_form);
-    db_form = nullptr;
+    db_form_container.push_back(form.release());
     MergePasswordForms(&keychain_matches, &db_form_container, passwords);
-    unused_db_forms.insert(unused_db_forms.end(), db_form_container.begin(),
-                           db_form_container.end());
-    db_form_container.weak_clear();
-  }
+    AppendSecondToFirst(&unused_db_forms, &db_form_container);
+  });
   database_forms->swap(unused_db_forms);
 
   STLDeleteContainerPairSecondPointers(item_form_pairs.begin(),
                                        item_form_pairs.end());
   for (SecKeychainItemRef item : keychain_items) {
     keychain.Free(item);
   }
 }
 
 // TODO(stuartmorgan): signon_realm for proxies is not yet supported.
@@ skipping 28 matching lines @@
 bool FormIsValidAndMatchesOtherForm(const PasswordForm& query_form,
                                     const PasswordForm& other_form) {
   std::string server;
   std::string security_domain;
   UInt32 port;
   bool is_secure;
   if (!ExtractSignonRealmComponents(query_form.signon_realm, &server, &port,
                                     &is_secure, &security_domain)) {
     return false;
   }
-  return internal_keychain_helpers::FormsMatchForMerge(
-      query_form, other_form, STRICT_FORM_MATCH);
+  return FormsMatchForMerge(query_form, other_form, STRICT_FORM_MATCH);
 }
 
 ScopedVector<autofill::PasswordForm> ExtractPasswordsMergeableWithForm(
     const AppleKeychain& keychain,
     const std::vector<ItemFormPair>& item_form_pairs,
     const PasswordForm& query_form) {
   ScopedVector<autofill::PasswordForm> matches;
   for (std::vector<ItemFormPair>::const_iterator i = item_form_pairs.begin();
        i != item_form_pairs.end(); ++i) {
     if (FormIsValidAndMatchesOtherForm(query_form, *(i->second))) {
       // Create a new object, since the caller is responsible for deleting the
       // returned forms.
       scoped_ptr<PasswordForm> form_with_password(new PasswordForm());
-      internal_keychain_helpers::FillPasswordFormFromKeychainItem(
+      FillPasswordFormFromKeychainItem(
           keychain, *(i->first), form_with_password.get(),
           true);  // Load password attributes and data.
       // Do not include blacklisted items found in the keychain.
       if (!form_with_password->blacklisted_by_user)
         matches.push_back(form_with_password.release());
     }
   }
   return matches.Pass();
 }
 
@@ skipping 434 matching lines @@
 
   ScopedVector<autofill::PasswordForm> matched_forms;
   internal_keychain_helpers::MergePasswordForms(
       &keychain_forms, &database_forms, &matched_forms);
 
   // Strip any blacklist entries out of the unused Keychain array, then take
   // all the entries that are left (which we can use as imported passwords).
   ScopedVector<PasswordForm> keychain_blacklist_forms;
   internal_keychain_helpers::ExtractBlacklistForms(&keychain_forms,
                                                    &keychain_blacklist_forms);
-  matched_forms.insert(matched_forms.end(),
-                       keychain_forms.begin(),
-                       keychain_forms.end());
-  keychain_forms.weak_clear();
+  AppendSecondToFirst(&matched_forms, &keychain_forms);
 
   if (!database_forms.empty()) {
     RemoveDatabaseForms(database_forms.get());
     NotifyLoginsChanged(FormsToRemoveChangeList(database_forms.get()));
   }
 
   callback_runner.Run(matched_forms.Pass());
 }
 
 void PasswordStoreMac::GetBlacklistLoginsImpl(GetLoginsRequest* request) {
@@ skipping 93 matching lines @@
 
   // Filter forms with corresponding Keychain entry out of |database_forms|.
   ScopedVector<PasswordForm> forms_with_keychain_entry;
   internal_keychain_helpers::GetPasswordsForForms(*keychain_, &database_forms,
                                                   &forms_with_keychain_entry);
 
   // Clean up any orphaned database entries.
   RemoveDatabaseForms(database_forms.get());
 
   // Move the orphaned DB forms to the output parameter.
-  orphaned_forms->insert(orphaned_forms->end(), database_forms.begin(),
-                         database_forms.end());
-  database_forms.weak_clear();
+  AppendSecondToFirst(orphaned_forms, &database_forms);
 }