Implement origin-based deletion for passwords in PasswordDefaultMac.

chrome/browser/password_manager/password_store_mac.cc

Index: chrome/browser/password_manager/password_store_mac.cc
diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index f067dd8f1afb92b3ee86dcf18889ec05df63799c..a31b9ef977be931768c6785fdd55185fa5334bbc 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -27,6 +27,7 @@
 #include "components/password_manager/core/browser/password_store_change.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/apple_keychain.h"
+#include "url/origin.h"
 
 using autofill::PasswordForm;
 using crypto::AppleKeychain;
@@ -1127,6 +1128,34 @@ PasswordStoreChangeList PasswordStoreMac::RemoveLoginImpl(
   return changes;
 }
 
+PasswordStoreChangeList PasswordStoreMac::RemoveLoginsByOriginAndTimeImpl(
+    const url::Origin& origin,
+    base::Time delete_begin,
+    base::Time delete_end) {
+  PasswordStoreChangeList changes;
+  ScopedVector<PasswordForm> forms_to_consider;
+  ScopedVector<PasswordForm> forms_to_remove;
+  if (login_metadata_db_ &&
+      login_metadata_db_->GetLoginsCreatedBetween(delete_begin, delete_end,
+                                                  &forms_to_consider)) {
+    MoveAllFormsOut(
+        &forms_to_consider,
+        [this, &origin, &forms_to_remove](
+            scoped_ptr<autofill::PasswordForm> form_to_consider) {
+          if (origin.IsSameOriginWith(url::Origin(form_to_consider->origin)) &&
+              login_metadata_db_->RemoveLogin(*form_to_consider))
+            forms_to_remove.push_back(form_to_consider.Pass());
+        });
+    if (!forms_to_remove.empty()) {
+      RemoveKeychainForms(forms_to_remove.get());
+      CleanOrphanedForms(&forms_to_remove);  // Add the orphaned forms.

[Timo Reimann, 2015/10/20 20:47:09]

Note that I did not implement tests to verify that keychain forms are removed /
orphaned forms cleaned during the execution of this method as I wasn't
completely sure about the right approach. Here are the available options I see:

1. Extend PasswordStoreMacTest.CheckRemoveLoginsBetween to cover origin-based
deletion too. The downside is that this would duplicate some functionality
(though not all) that PasswordStoreCommonTest.* (the new type-parameterized
tests) already cares about in a more specific way. Consequently, in the worst
case this could lead to two tests failing for the same regression, which is bad
from a test design perspective.
2. Extract the keychain and orphaned forms parts from
PasswordStoreMacTest.CheckRemoveLoginsBetween into a new test and share it by
all methods dealing with the two form types.
3. Make the keychain and orphaned forms injectable so that we can use mocks to
verify the behavior on.
4. Having read crbug.com/466638, I understand that we are dropping Mac keychain
support soon. This might turn it acceptable to skip writing tests for the
functionality at hand altogether and wait for the test gap to close naturally.

Happy to hear your thoughts.

[vasilii, 2015/10/21 16:47:56]

You can proceed with the option 4. I don't think we ship your feature soon
enough.

+      changes = FormsToRemoveChangeList(forms_to_remove.get());
+      LogStatsForBulkDeletion(changes.size());
+    }
+  }
+  return changes;
+}
+
 PasswordStoreChangeList PasswordStoreMac::RemoveLoginsCreatedBetweenImpl(
     base::Time delete_begin,
     base::Time delete_end) {