Handle origin-specific password deletion in BrowsingDataRemover

chrome/browser/browsing_data/browsing_data_remover.cc

Index: chrome/browser/browsing_data/browsing_data_remover.cc
diff --git a/chrome/browser/browsing_data/browsing_data_remover.cc b/chrome/browser/browsing_data/browsing_data_remover.cc
index d8350d970077cd728a41973a0c22cc18204d4d52..c22f3a207631a7a7de9c4c401813973f3c085cdf 100644
--- a/chrome/browser/browsing_data/browsing_data_remover.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover.cc
@@ -698,10 +698,15 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
 
     if (password_store) {
       waiting_for_clear_passwords_ = true;
-      password_store->RemoveLoginsCreatedBetween(
-          delete_begin_, delete_end_,
+      auto on_cleared_passwords =
           base::Bind(&BrowsingDataRemover::OnClearedPasswords,
-                     weak_ptr_factory_.GetWeakPtr()));
+                     weak_ptr_factory_.GetWeakPtr());
+      if (remove_origin.unique())

[Mike West, 2016/02/04 18:25:11]

Why "unique"? What if I pass in a `data:` URL, for example? I think this should
probably be checking something like `remove_url.empty()` or something similar,
if the goal is to clear out origin-based data iff an origin is provided.

Nit: Braces around multi-line blocks.

[Timo Reimann, 2016/02/05 01:12:38]

The "unique" check was meant as a temporary solution and supposed to be replaced
at some point in the future. See our discussion we had back in July [1] when I
added the functionality to remove downloads by origins. It also resulted in a
TODO for you placed at [2].

Checking the URL for emptiness does sound more reasonable though. Ideally, I'd
want a property of |remove_origin| as opposed to |remove_url| to be checked to
indicate origin-based deletion since the two variables could theoretically start
to deviate in the course of the (fairly biggish) RemoveImpl implementation
(e.g., |remove_url| getting reassigned). OTOH, |remove_url| is const, so we
should be fairly safe.
Thus, I replaced remove_origin.unique() with remove_url.is_empty(), constified
|remove_origin| for increased safety, and updated the header file comment.

Hope this matches what you had in mind.
 
Done.

[1] https://codereview.chromium.org/1251243003/patch/1/10003
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

+        password_store->RemoveLoginsCreatedBetween(delete_begin_, delete_end_,
+                                                   on_cleared_passwords);
+      else

[Mike West, 2016/02/04 18:25:11]

Nit: Ditto.

[Timo Reimann, 2016/02/05 01:12:38]

Done.

+        password_store->RemoveLoginsByOriginAndTime(
+            remove_origin, delete_begin_, delete_end_, on_cleared_passwords);
     }
   }