Support individual Keychain item deletion in PasswordStoreMac.

Rename AddLogin to AddPassword for consistency with other method names.
Remove a confusing out param from the form merge check function.

BUG=16486
TEST=Once the UI exists, deleting individual passwords should work on the Mac.

Review URL: http://codereview.chromium.org/155451

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@20573 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 180 insertions, 58 deletions

diff --git a/chrome/browser/keychain_mac.cc b/chrome/browser/keychain_mac.cc
index cbd7086..90d95a7 100644
--- a/chrome/browser/keychain_mac.cc
+++ b/chrome/browser/keychain_mac.cc
@@ -24,6 +24,10 @@ OSStatus MacKeychain::ItemFreeAttributesAndData(
   return SecKeychainItemFreeAttributesAndData(attrList, data);
 }
 
+OSStatus MacKeychain::ItemDelete(SecKeychainItemRef itemRef) const {
+  return SecKeychainItemDelete(itemRef);
+}
+
 OSStatus MacKeychain::SearchCreateFromAttributes(
     CFTypeRef keychainOrArray, SecItemClass itemClass,
     const SecKeychainAttributeList *attrList,
diff --git a/chrome/browser/keychain_mac.h b/chrome/browser/keychain_mac.h
index 65eeb37..68d7b30 100644
--- a/chrome/browser/keychain_mac.h
+++ b/chrome/browser/keychain_mac.h
@@ -34,6 +34,8 @@ class MacKeychain {
   virtual OSStatus ItemFreeAttributesAndData(SecKeychainAttributeList *attrList,
                                              void *data) const;
 
+  virtual OSStatus ItemDelete(SecKeychainItemRef itemRef) const;
+
   virtual OSStatus SearchCreateFromAttributes(
       CFTypeRef keychainOrArray, SecItemClass itemClass,
       const SecKeychainAttributeList *attrList,
diff --git a/chrome/browser/keychain_mock_mac.cc b/chrome/browser/keychain_mock_mac.cc
index a6bd5fd..879ffdc 100644
--- a/chrome/browser/keychain_mock_mac.cc
+++ b/chrome/browser/keychain_mock_mac.cc
@@ -229,6 +229,16 @@ OSStatus MockKeychain::ItemFreeAttributesAndData(
   return noErr;
 }
 
+OSStatus MockKeychain::ItemDelete(SecKeychainItemRef itemRef) const {
+  unsigned int item_index = reinterpret_cast<unsigned int>(itemRef) - 1;
+  // The mock only supports deleting the last item.
+  if (item_index != item_count_ - 1) {
+    NOTIMPLEMENTED();
+  }
+  --item_count_;
+  return noErr;
+}
+
 OSStatus MockKeychain::SearchCreateFromAttributes(
     CFTypeRef keychainOrArray, SecItemClass itemClass,
     const SecKeychainAttributeList *attrList,
diff --git a/chrome/browser/keychain_mock_mac.h b/chrome/browser/keychain_mock_mac.h
index 2c5303b..ba7a48eb 100644
--- a/chrome/browser/keychain_mock_mac.h
+++ b/chrome/browser/keychain_mock_mac.h
@@ -33,6 +33,7 @@ class MockKeychain : public MacKeychain {
       UInt32 length, const void *data) const;
   virtual OSStatus ItemFreeAttributesAndData(SecKeychainAttributeList *attrList,
                                              void *data) const;
+  virtual OSStatus ItemDelete(SecKeychainItemRef itemRef) const;
   virtual OSStatus SearchCreateFromAttributes(
       CFTypeRef keychainOrArray, SecItemClass itemClass,
       const SecKeychainAttributeList *attrList,
diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index 9234eaa..18b8057 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -34,7 +34,7 @@ class KeychainSearch {
   void Init(const char* server, const UInt32& port,
             const SecProtocolType& protocol,
             const SecAuthenticationType& auth_type, const char* security_domain,
-            const char* path, const char* username);
+            const char* path, const char* username, OSType creator);
 
   // Fills |items| with all Keychain items that match the Init'd search.
   // If the search fails for any reason, |items| will be unchanged.
@@ -62,9 +62,9 @@ void KeychainSearch::Init(const char* server, const UInt32& port,
                           const SecProtocolType& protocol,
                           const SecAuthenticationType& auth_type,
                           const char* security_domain, const char* path,
-                          const char* username) {
+                          const char* username, OSType creator) {
   // Allocate enough to hold everything we might use.
-  const unsigned int kMaxEntryCount = 7;
+  const unsigned int kMaxEntryCount = 8;
   search_attributes_.attr =
       static_cast<SecKeychainAttribute*>(calloc(kMaxEntryCount,
                                                 sizeof(SecKeychainAttribute)));
@@ -128,6 +128,14 @@ void KeychainSearch::Init(const char* server, const UInt32& port,
         const_cast<void*>(reinterpret_cast<const void*>(username));
     ++entries;
   }
+  if (creator != 0) {
+    DCHECK(entries <= kMaxEntryCount);
+    search_attributes_.attr[entries].tag = kSecCreatorItemAttr;
+    search_attributes_.attr[entries].length = sizeof(creator);
+    search_attributes_.attr[entries].data =
+        const_cast<void*>(reinterpret_cast<const void*>(&creator));
+    ++entries;
+  }
   search_attributes_.count = entries;
 }
 
@@ -328,19 +336,15 @@ bool FillPasswordFormFromKeychainItem(const MacKeychain& keychain,
   return true;
 }
 
-bool FormsMatchForMerge(const PasswordForm& form_a, const PasswordForm& form_b,
-                        bool* path_matches) {
+bool FormsMatchForMerge(const PasswordForm& form_a,
+                        const PasswordForm& form_b) {
   // We never merge blacklist entries between our store and the keychain.
   if (form_a.blacklisted_by_user || form_b.blacklisted_by_user) {
     return false;
   }
-  bool matches = form_a.scheme == form_b.scheme &&
-                 form_a.signon_realm == form_b.signon_realm &&
-                 form_a.username_value == form_b.username_value;
-  if (matches && path_matches) {
-    *path_matches = (form_a.origin == form_b.origin);
-  }
-  return matches;
+  return form_a.scheme == form_b.scheme &&
+         form_a.signon_realm == form_b.signon_realm &&
+         form_a.username_value == form_b.username_value;
 }
 
 // Returns an the best match for |form| from |keychain_forms|, or NULL if there
@@ -354,9 +358,8 @@ PasswordForm* BestKeychainFormForForm(
     // 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).
-    bool path_matches = false;
-    if (FormsMatchForMerge(base_form, *(*i), &path_matches)) {
-      if (path_matches) {
+    if (FormsMatchForMerge(base_form, *(*i))) {
+      if (base_form.origin == (*i)->origin) {
         return *i;
       } else if (!partial_match) {
         partial_match = *i;
@@ -416,7 +419,7 @@ void MergePasswordForms(std::vector<PasswordForm*>* keychain_forms,
 #pragma mark -
 
 MacKeychainPasswordFormAdapter::MacKeychainPasswordFormAdapter(
-    MacKeychain* keychain) : keychain_(keychain) {
+    MacKeychain* keychain) : keychain_(keychain), finds_only_owned_(false) {
 }
 
 std::vector<PasswordForm*>
@@ -448,7 +451,7 @@ PasswordForm* MacKeychainPasswordFormAdapter::PasswordExactlyMatchingForm(
   return NULL;
 }
 
-bool MacKeychainPasswordFormAdapter::AddLogin(const PasswordForm& form) {
+bool MacKeychainPasswordFormAdapter::AddPassword(const PasswordForm& form) {
   // We should never be trying to store a blacklist in the keychain.
   DCHECK(!form.blacklisted_by_user);
 
@@ -491,6 +494,20 @@ bool MacKeychainPasswordFormAdapter::AddLogin(const PasswordForm& form) {
   return result == noErr;
 }
 
+bool MacKeychainPasswordFormAdapter::RemovePassword(const PasswordForm& form) {
+  SecKeychainItemRef keychain_item = KeychainItemForForm(form);
+  if (keychain_item == NULL)
+    return false;
+  OSStatus result = keychain_->ItemDelete(keychain_item);
+  keychain_->Free(keychain_item);
+  return result == noErr;
+}
+
+void MacKeychainPasswordFormAdapter::SetFindsOnlyOwnedItems(
+    bool finds_only_owned) {
+  finds_only_owned_ = finds_only_owned;
+}
+
 std::vector<PasswordForm*>
     MacKeychainPasswordFormAdapter::CreateFormsFromKeychainItems(
         const std::vector<SecKeychainItemRef>& items) {
@@ -559,10 +576,11 @@ std::vector<SecKeychainItemRef>
   SecAuthenticationType auth_type = AuthTypeForScheme(scheme);
   const char* auth_domain = (scheme == PasswordForm::SCHEME_HTML) ?
       NULL : security_domain.c_str();
+  OSType creator = finds_only_owned_ ? kChromeKeychainCreatorCode : 0;
 
   KeychainSearch keychain_search(*keychain_);
   keychain_search.Init(server.c_str(), port, protocol, auth_type,
-                       auth_domain, path, username);
+                       auth_domain, path, username, creator);
   keychain_search.FindMatchingItems(&matches);
   return matches;
 }
@@ -640,8 +658,8 @@ void PasswordStoreMac::AddLoginImpl(const PasswordForm& form) {
 }
 
 void PasswordStoreMac::UpdateLoginImpl(const PasswordForm& form) {
-  // The keychain AddLogin will update if there is a collision and add if there
-  // isn't, which is the behavior we want, so there's no separate UpdateLogin.
+  // The keychain add will update if there is a collision and add if there
+  // isn't, which is the behavior we want, so there's no separate update call.
   if (AddToKeychainIfNecessary(form)) {
     int update_count = 0;
     login_metadata_db_->UpdateLogin(form, &update_count);
@@ -654,7 +672,25 @@ void PasswordStoreMac::UpdateLoginImpl(const PasswordForm& form) {
 }
 
 void PasswordStoreMac::RemoveLoginImpl(const PasswordForm& form) {
-  NOTIMPLEMENTED();
+  login_metadata_db_->RemoveLogin(form);
+
+  // See if we own a Keychain item associated with this item. We can do an exact
+  // search rather than messing around with trying to do fuzzy matching because
+  // passwords that we created will always have an exact-match database entry.
+  // (If a user does lose their profile but not their keychain we'll treat the
+  // entries we find like other imported entries anyway, so it's reasonable to
+  // handle deletes on them the way we would for an imported item.)
+  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_.get());
+  owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
+  PasswordForm* owned_password_form =
+      owned_keychain_adapter.PasswordExactlyMatchingForm(form);
+  if (owned_password_form) {
+    // If we don't have other forms using it (i.e., a form differing only by
+    // the names of the form elements), delete the keychain entry.
+    if (!DatabaseHasFormMatchingKeychainForm(form)) {
+      owned_keychain_adapter.RemovePassword(form);
+    }
+  }
 }
 
 void PasswordStoreMac::RemoveLoginsCreatedBetweenImpl(
@@ -664,9 +700,9 @@ void PasswordStoreMac::RemoveLoginsCreatedBetweenImpl(
 
 void PasswordStoreMac::GetLoginsImpl(GetLoginsRequest* request,
                                      const webkit_glue::PasswordForm& form) {
-  MacKeychainPasswordFormAdapter keychainAdapter(keychain_.get());
+  MacKeychainPasswordFormAdapter keychain_adapter(keychain_.get());
   std::vector<PasswordForm*> keychain_forms =
-      keychainAdapter.PasswordsMatchingForm(form);
+      keychain_adapter.PasswordsMatchingForm(form);
 
   std::vector<PasswordForm*> database_forms;
   login_metadata_db_->GetLogins(form, &database_forms);
@@ -701,5 +737,22 @@ bool PasswordStoreMac::AddToKeychainIfNecessary(const PasswordForm& form) {
     return true;
   }
   MacKeychainPasswordFormAdapter keychainAdapter(keychain_.get());
-  return keychainAdapter.AddLogin(form);
+  return keychainAdapter.AddPassword(form);
+}
+
+bool PasswordStoreMac::DatabaseHasFormMatchingKeychainForm(
+    const webkit_glue::PasswordForm& form) {
+  bool has_match = false;
+  std::vector<PasswordForm*> database_forms;
+  login_metadata_db_->GetLogins(form, &database_forms);
+  for (std::vector<PasswordForm*>::iterator i = database_forms.begin();
+       i != database_forms.end(); ++i) {
+    if (internal_keychain_helpers::FormsMatchForMerge(form, **i) &&
+        (*i)->origin == form.origin) {
+      has_match = true;
+      break;
+    }
+  }
+  STLDeleteElements(&database_forms);
+  return has_match;
 }
diff --git a/chrome/browser/password_manager/password_store_mac.h b/chrome/browser/password_manager/password_store_mac.h
index e99d7cc..f00fa76 100644
--- a/chrome/browser/password_manager/password_store_mac.h
+++ b/chrome/browser/password_manager/password_store_mac.h
@@ -34,6 +34,11 @@ class PasswordStoreMac : public PasswordStore {
   // succeeded (either we added successfully, or we didn't need to).
   bool AddToKeychainIfNecessary(const webkit_glue::PasswordForm& form);
 
+  // Returns true if our database contains a form that exactly matches the given
+  // keychain form.
+  bool DatabaseHasFormMatchingKeychainForm(
+    const webkit_glue::PasswordForm& form);
+
   scoped_ptr<MacKeychain> keychain_;
   scoped_ptr<LoginDatabaseMac> login_metadata_db_;
 
diff --git a/chrome/browser/password_manager/password_store_mac_internal.h b/chrome/browser/password_manager/password_store_mac_internal.h
index 35d0d8a..50a4088 100644
--- a/chrome/browser/password_manager/password_store_mac_internal.h
+++ b/chrome/browser/password_manager/password_store_mac_internal.h
@@ -37,7 +37,15 @@ class MacKeychainPasswordFormAdapter {
   // Creates a new keychain entry from |form|, or updates the password of an
   // existing keychain entry if there is a collision. Returns true if a keychain
   // entry was successfully added/updated.
-  bool AddLogin(const webkit_glue::PasswordForm& form);
+  bool AddPassword(const webkit_glue::PasswordForm& form);
+
+  // Removes the keychain password matching |form| if any. Returns true if a
+  // keychain item was found and successfully removed.
+  bool RemovePassword(const webkit_glue::PasswordForm& form);
+
+  // Controls whether or not Chrome will restrict Keychain searches to items
+  // that it created. Defaults to false.
+  void SetFindsOnlyOwnedItems(bool finds_only_owned);
 
  private:
   // Returns PasswordForms constructed from the given Keychain items.
@@ -52,14 +60,14 @@ class MacKeychainPasswordFormAdapter {
       const webkit_glue::PasswordForm& form);
 
   // Searches |keychain| for the specific keychain entry that corresponds to the
-  // given form, and returns it (or NULL if no match is found).  The caller is
+  // given form, and returns it (or NULL if no match is found). The caller is
   // responsible for calling MacKeychain::Free on on the returned item.
   SecKeychainItemRef KeychainItemForForm(
       const webkit_glue::PasswordForm& form);
 
   // Returns the Keychain items matching the given signon_realm, scheme, and
   // optionally path and username (either of both can be NULL).
-  // them. The caller is responsible for calling MacKeychain::Free on the
+  // The caller is responsible for calling MacKeychain::Free on the
   // returned items.
   std::vector<SecKeychainItemRef> MatchingKeychainItems(
       const std::string& signon_realm, webkit_glue::PasswordForm::Scheme scheme,
@@ -91,6 +99,9 @@ class MacKeychainPasswordFormAdapter {
 
   MacKeychain* keychain_;
 
+  // If true, Keychain searches are restricted to items created by Chrome.
+  bool finds_only_owned_;
+
   DISALLOW_COPY_AND_ASSIGN(MacKeychainPasswordFormAdapter);
 };
 
@@ -113,11 +124,8 @@ bool FillPasswordFormFromKeychainItem(const MacKeychain& keychain,
 
 // Returns true if the two given forms match based on signon_reaml, scheme, and
 // username_value, and are thus suitable for merging (see MergePasswordForms).
-// If this returns true, and path_matches is non-NULL, *path_matches will be set
-// based on whether the full origin matches as well.
 bool FormsMatchForMerge(const webkit_glue::PasswordForm& form_a,
-                        const webkit_glue::PasswordForm& form_b,
-                        bool* path_matches);
+                        const webkit_glue::PasswordForm& form_b);
 
 // Populates merged_forms by combining the password data from keychain_forms and
 // the metadata from database_forms, removing used entries from the two source
diff --git a/chrome/browser/password_manager/password_store_mac_unittest.cc b/chrome/browser/password_manager/password_store_mac_unittest.cc
index 2b85b9a..4962f19 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc
@@ -320,14 +320,22 @@ TEST_F(PasswordStoreMacTest, TestKeychainSearch) {
         NULL, NULL, NULL, NULL, NULL, NULL, NULL, false, false, 0 }, 0 },
   };
 
-  MacKeychainPasswordFormAdapter keychainAdapter(keychain_);
+  MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
+  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_);
+  owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
   for (unsigned int i = 0; i < ARRAYSIZE_UNSAFE(test_data); ++i) {
     scoped_ptr<PasswordForm> query_form(
         CreatePasswordFormFromData(test_data[i].data));
     std::vector<PasswordForm*> matching_items =
-        keychainAdapter.PasswordsMatchingForm(*query_form);
+        keychain_adapter.PasswordsMatchingForm(*query_form);
     EXPECT_EQ(test_data[i].expected_matches, matching_items.size());
     STLDeleteElements(&matching_items);
+
+    // None of the pre-seeded items are owned by us, so none should match an
+    // owned-passwords-only search.
+    matching_items = owned_keychain_adapter.PasswordsMatchingForm(*query_form);
+    EXPECT_EQ(0U, matching_items.size());
+    STLDeleteElements(&matching_items);
   }
 }
 
@@ -358,7 +366,7 @@ static void SetPasswordFormRealm(PasswordForm* form, const char* realm) {
 }
 
 TEST_F(PasswordStoreMacTest, TestKeychainExactSearch) {
-  MacKeychainPasswordFormAdapter keychainAdapter(keychain_);
+  MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
 
   PasswordFormData base_form_data[] = {
     { PasswordForm::SCHEME_HTML, "http://some.domain.com/",
@@ -377,7 +385,7 @@ TEST_F(PasswordStoreMacTest, TestKeychainExactSearch) {
     scoped_ptr<PasswordForm> base_form(CreatePasswordFormFromData(
         base_form_data[i]));
     PasswordForm* match =
-        keychainAdapter.PasswordExactlyMatchingForm(*base_form);
+        keychain_adapter.PasswordExactlyMatchingForm(*base_form);
     EXPECT_TRUE(match != NULL);
     if (match) {
       EXPECT_EQ(base_form->scheme, match->scheme);
@@ -413,7 +421,7 @@ TEST_F(PasswordStoreMacTest, TestKeychainExactSearch) {
 
     for (unsigned int j = 0; j < modified_forms.size(); ++j) {
       PasswordForm* match =
-          keychainAdapter.PasswordExactlyMatchingForm(*modified_forms[j]);
+          keychain_adapter.PasswordExactlyMatchingForm(*modified_forms[j]);
       EXPECT_EQ(NULL, match) << "In modified version " << j << " of base form "
                              << i;
     }
@@ -451,15 +459,16 @@ TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
         L"joe_user", L"fail_me", false, false, 0 }, false },
   };
 
-  MacKeychainPasswordFormAdapter keychainAdapter(keychain_);
+  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_);
+  owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
 
   for (unsigned int i = 0; i < ARRAYSIZE_UNSAFE(test_data); ++i) {
     PasswordForm* in_form = CreatePasswordFormFromData(test_data[i].data);
-    bool add_succeeded = keychainAdapter.AddLogin(*in_form);
+    bool add_succeeded = owned_keychain_adapter.AddPassword(*in_form);
     EXPECT_EQ(test_data[i].should_succeed, add_succeeded);
     if (add_succeeded) {
       scoped_ptr<PasswordForm> out_form(
-          keychainAdapter.PasswordExactlyMatchingForm(*in_form));
+          owned_keychain_adapter.PasswordExactlyMatchingForm(*in_form));
       EXPECT_TRUE(out_form.get() != NULL);
       EXPECT_EQ(out_form->scheme, in_form->scheme);
       EXPECT_EQ(out_form->signon_realm, in_form->signon_realm);
@@ -478,7 +487,8 @@ TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
       NULL, NULL, NULL, L"joe_user", L"updated_password", false, false, 0
     };
     PasswordForm* update_form = CreatePasswordFormFromData(data);
-    EXPECT_TRUE(keychainAdapter.AddLogin(*update_form));
+    MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
+    EXPECT_TRUE(keychain_adapter.AddPassword(*update_form));
     SecKeychainItemRef keychain_item = reinterpret_cast<SecKeychainItemRef>(2);
     PasswordForm stored_form;
     internal_keychain_helpers::FillPasswordFormFromKeychainItem(*keychain_,
@@ -489,6 +499,45 @@ TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
   }
 }
 
+TEST_F(PasswordStoreMacTest, TestKeychainRemove) {
+  struct TestDataAndExpectation {
+    PasswordFormData data;
+    bool should_succeed;
+  };
+  TestDataAndExpectation test_data[] = {
+    // Test deletion of an item that we add.
+    { { PasswordForm::SCHEME_HTML, "http://web.site.com/",
+        "http://web.site.com/path/to/page.html", NULL, NULL, NULL, NULL,
+        L"anonymous", L"knock-knock", false, false, 0 }, true },
+    // Make sure we don't delete items we don't own.
+    { { PasswordForm::SCHEME_HTML, "http://some.domain.com/",
+        "http://some.domain.com/insecure.html", NULL, NULL, NULL, NULL,
+        L"joe_user", NULL, true, false, 0 }, false },
+  };
+
+  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_);
+  owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
+
+  // Add our test item so that we can delete it.
+  PasswordForm* add_form = CreatePasswordFormFromData(test_data[0].data);
+  EXPECT_TRUE(owned_keychain_adapter.AddPassword(*add_form));
+  delete add_form;
+  
+  for (unsigned int i = 0; i < ARRAYSIZE_UNSAFE(test_data); ++i) {
+    scoped_ptr<PasswordForm> form(CreatePasswordFormFromData(
+        test_data[i].data));
+    EXPECT_EQ(test_data[i].should_succeed,
+              owned_keychain_adapter.RemovePassword(*form));
+
+    MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
+    PasswordForm* match = keychain_adapter.PasswordExactlyMatchingForm(*form);
+    EXPECT_EQ(test_data[i].should_succeed, match == NULL);
+    if (match) {
+      delete match;
+    }
+  }
+}
+
 TEST_F(PasswordStoreMacTest, TestFormMatch) {
   PasswordForm base_form;
   base_form.signon_realm = std::string("http://some.domain.com/");
@@ -506,18 +555,13 @@ TEST_F(PasswordStoreMacTest, TestFormMatch) {
     different_form.ssl_valid = true;
     different_form.preferred = true;
     different_form.date_created = base::Time::Now();
-    bool paths_match = false;
     EXPECT_TRUE(internal_keychain_helpers::FormsMatchForMerge(base_form,
-                                                              different_form,
-                                                              &paths_match));
-    EXPECT_TRUE(paths_match);
+                                                              different_form));
 
-    // Check that we detect path differences, but still match.
+    // Check that path differences don't prevent a match.
     base_form.origin = GURL("http://some.domain.com/other_page.html");
     EXPECT_TRUE(internal_keychain_helpers::FormsMatchForMerge(base_form,
-                                                              different_form,
-                                                              &paths_match));
-    EXPECT_FALSE(paths_match);
+                                                              different_form));
   }
 
   // Check that any one primary key changing is enough to prevent matching.
@@ -525,29 +569,25 @@ TEST_F(PasswordStoreMacTest, TestFormMatch) {
     PasswordForm different_form(base_form);
     different_form.scheme = PasswordForm::SCHEME_DIGEST;
     EXPECT_FALSE(internal_keychain_helpers::FormsMatchForMerge(base_form,
-                                                               different_form,
-                                                               NULL));
+                                                               different_form));
   }
   {
     PasswordForm different_form(base_form);
     different_form.signon_realm = std::string("http://some.domain.com:8080/");
     EXPECT_FALSE(internal_keychain_helpers::FormsMatchForMerge(base_form,
-                                                               different_form,
-                                                               NULL));
+                                                               different_form));
   }
   {
     PasswordForm different_form(base_form);
     different_form.username_value = std::wstring(L"john.doe");
     EXPECT_FALSE(internal_keychain_helpers::FormsMatchForMerge(base_form,
-                                                               different_form,
-                                                               NULL));
+                                                               different_form));
   }
   {
     PasswordForm different_form(base_form);
     different_form.blacklisted_by_user = true;
     EXPECT_FALSE(internal_keychain_helpers::FormsMatchForMerge(base_form,
-                                                               different_form,
-                                                               NULL));
+                                                               different_form));
   }
 
   // Blacklist forms should *never* match for merging, even when identical
@@ -556,8 +596,7 @@ TEST_F(PasswordStoreMacTest, TestFormMatch) {
     PasswordForm form_a(base_form);
     form_a.blacklisted_by_user = true;
     PasswordForm form_b(form_a);
-    EXPECT_FALSE(internal_keychain_helpers::FormsMatchForMerge(form_a, form_b,
-                                                               NULL));
+    EXPECT_FALSE(internal_keychain_helpers::FormsMatchForMerge(form_a, form_b));
   }
 }