Handle origin-specific password deletion in BrowsingDataRemover

chrome/browser/browsing_data/browsing_data_remover.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/browsing_data/browsing_data_remover.h"
 
 #include <map>
 #include <set>
 #include <string>
 
@@ skipping 680 matching lines @@
   }
 
   if (remove_mask & REMOVE_PASSWORDS) {
     content::RecordAction(UserMetricsAction("ClearBrowsingData_Passwords"));
     password_manager::PasswordStore* password_store =
         PasswordStoreFactory::GetForProfile(
             profile_, ServiceAccessType::EXPLICIT_ACCESS).get();
 
     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);
     }
   }
 
   if (remove_mask & REMOVE_HISTORY) {
     password_manager::PasswordStore* password_store =
         PasswordStoreFactory::GetForProfile(
             profile_, ServiceAccessType::EXPLICIT_ACCESS).get();
 
     if (password_store) {
       waiting_for_clear_passwords_stats_ = true;
@@ skipping 473 matching lines @@
   waiting_for_clear_domain_reliability_monitor_ = false;
   NotifyIfDone();
 }
 
 // static
 BrowsingDataRemover::CallbackSubscription
     BrowsingDataRemover::RegisterOnBrowsingDataRemovedCallback(
         const BrowsingDataRemover::Callback& callback) {
   return GetOnBrowsingDataRemovedCallbacks()->Add(callback);
 }