[sync] Significantly speed up password model association on mac

chrome/browser/password_manager/password_store_mac_internal.h

Index: chrome/browser/password_manager/password_store_mac_internal.h
diff --git a/chrome/browser/password_manager/password_store_mac_internal.h b/chrome/browser/password_manager/password_store_mac_internal.h
index 7121657c180392ba6a9d57efe89043b8a3c8e7a0..8431f804e0f13778527c5c76d19ed50263634ef4 100644
--- a/chrome/browser/password_manager/password_store_mac_internal.h
+++ b/chrome/browser/password_manager/password_store_mac_internal.h
@@ -18,6 +18,9 @@ using crypto::AppleKeychain;
 // PasswordForms instead of Keychain items.
 class MacKeychainPasswordFormAdapter {
  public:
+  // Pair of pointers to a SecKeychainItemRef and a corresponding PasswordForm.
+  typedef std::pair<SecKeychainItemRef*, autofill::PasswordForm*> ItemFormPair;
+
   // Creates an adapter for |keychain|. This class does not take ownership of
   // |keychain|, so the caller must make sure that the keychain outlives the
   // created object.
@@ -28,10 +31,11 @@ class MacKeychainPasswordFormAdapter {
   std::vector<autofill::PasswordForm*> PasswordsFillingForm(
       const autofill::PasswordForm& query_form);
 
-  // Returns PasswordForms for each keychain entry that could be merged with
-  // |form|. Differs from PasswordsFillingForm in that the username must match.
+  // Returns PasswordForms populated with password data for each keychain entry
+  // in |item_form_pairs| that could be merged with |query_form|.
   // Caller is responsible for deleting the returned forms.
-  std::vector<autofill::PasswordForm*> PasswordsMergeableWithForm(
+  std::vector<autofill::PasswordForm*> ExtractPasswordsMergeableWithForm(
+      const std::vector<ItemFormPair>& item_form_pairs,
       const autofill::PasswordForm& query_form);
 
   // Returns the PasswordForm for the Keychain entry that matches |form| on all
@@ -41,14 +45,19 @@ class MacKeychainPasswordFormAdapter {
   autofill::PasswordForm* PasswordExactlyMatchingForm(
       const autofill::PasswordForm& query_form);
 
-  // Returns true if PasswordsMergeableWithForm would return any items. This is
-  // a separate method because calling PasswordsMergeableWithForm and checking
-  // the return count would require reading the passwords from the keychain,
-  // thus potentially triggering authorizaiton UI, whereas this won't.
+  // Returns true if the keychain contains any items that are mergeable with
+  // |query_form|. This is different form actually extracting the passwords
+  // and checking the return count, since it would require reading the passwords
+  // from the keychain, thus potentially triggering authorizaiton UI, whereas
+  // this won't.
   bool HasPasswordsMergeableWithForm(
       const autofill::PasswordForm& query_form);
 
   // Returns all keychain items of types corresponding to password forms.
+  std::vector<SecKeychainItemRef> GetAllPasswordFormKeychainItems();
+
+  // Returns password data from all keychain items of types corresponding to
+  // password forms. Caller is responsible for deleting the returned forms.
   std::vector<autofill::PasswordForm*> GetAllPasswordFormPasswords();
 
   // Creates a new keychain entry from |form|, or updates the password of an
@@ -87,6 +96,12 @@ class MacKeychainPasswordFormAdapter {
       const char* path,
       const char* username);
 
+  // Returns true if the signon_realm of |query_form| can be successfully parsed
+  // by ExtractSignonRealmComponents, and if |query_form| matches |other_form|.
+  bool FormIsValidAndMatchesOtherForm(
+      const autofill::PasswordForm& query_form,
+      const autofill::PasswordForm& other_form);
+
   // Takes a PasswordForm's signon_realm and parses it into its component parts,
   // which are returned though the appropriate out parameters.
   // Returns true if it can be successfully parsed, in which case all out params
@@ -129,19 +144,23 @@ class MacKeychainPasswordFormAdapter {
 namespace internal_keychain_helpers {
 
 // Sets the fields of |form| based on the keychain data from |keychain_item|.
-// Fields that can't be determined from |keychain_item| will be unchanged.
+// Fields that can't be determined from |keychain_item| will be unchanged. If
+// |extract_password_data| is true, the password data will be copied from
+// |keychain_item| in addition to its attributes. If it is false, only the
+// password attributes will be copied.

[stuartmorgan, 2013/09/30 22:39:12]

See comment below; there's a critical caveat you've added to the behavior here
that's not covered in the comment.

[Raghu Simha, 2013/10/02 02:09:50]

Comment updated.

 //
-// IMPORTANT: This function can cause the OS to trigger UI (to allow access to
-// the keychain item if we aren't trusted for the item), and block until the UI
-// is dismissed.
+// IMPORTANT: If |extract_password_data| is true, this function can cause the OS
+// to trigger UI (to allow access to the keychain item if we aren't trusted for
+// the item), and block until the UI is dismissed.
 //
 // If excessive prompting for access to other applications' keychain items
-// becomes an issue, the password storage API will need to be refactored to
-// allow the password to be retrieved later (accessing other fields doesn't
-// require authorization).
+// becomes an issue, the password storage API will need to intially call this
+// function with |extract_password_data| set to false, and retrieve the password
+// later (accessing other fields doesn't require authorization).
 bool FillPasswordFormFromKeychainItem(const AppleKeychain& keychain,
                                       const SecKeychainItemRef& keychain_item,
-                                      autofill::PasswordForm* form);
+                                      autofill::PasswordForm* form,
+                                      bool extract_password_data);
 
 // Returns true if the two given forms match based on signon_reaml, scheme, and
 // username_value, and are thus suitable for merging (see MergePasswordForms).
@@ -169,6 +188,16 @@ std::vector<autofill::PasswordForm*> GetPasswordsForForms(
     const AppleKeychain& keychain,
     std::vector<autofill::PasswordForm*>* database_forms);
 
+// Loads all items in the system keychain into |keychain_items|, and copies all
+// their attributes into a container of pairs of pointers to PasswordForms and
+// SecKeychainItemRefs without copying any password data. Caller owns the
+// SecKeychainItemRefs and PasswordForms that are returned. This operation does
+// not require OS authorization.

[stuartmorgan, 2013/09/30 22:39:12]

You should mention that the resulting PasswordForms essentially lie about
whether or not they are blacklist entries, always claiming not to be.

[Raghu Simha, 2013/10/02 02:09:50]

I've rewritten this comment and mentioned blacklist entries.

+std::vector<MacKeychainPasswordFormAdapter::ItemFormPair>
+    GetAllKeychainItemAttributesAsPasswordForms(

[stuartmorgan, 2013/09/30 22:39:12]

I don't understand the use of the word "attributes" in this method name.
Keychain items correspond to password forms; keychain item attributes correspond
to password form fields. What is "attributes" intended to convey here?

[Raghu Simha, 2013/10/02 02:09:50]

Good point :)

I've renamed this ExtractAllKeychainItemAttributesIntoPasswordForms. It's a
little wordy, but I couldn't think of a name that more accurately describes
what's happening here. Let me know what you think.

+        std::vector<SecKeychainItemRef>* keychain_items,
+        const AppleKeychain& keychain);
+
 }  // namespace internal_keychain_helpers
 
 #endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_MAC_INTERNAL_H_