Formalize the restrictions of BrowsingDataRemover::RemoveWithFilter()

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 331 matching lines @@
   DCHECK(observer);
   RemoveInternal(time_range, remove_mask, origin_type_mask,
                  std::unique_ptr<RegistrableDomainFilterBuilder>(), observer);
 }
 
 void BrowsingDataRemover::RemoveWithFilter(
     const TimeRange& time_range,
     int remove_mask,
     int origin_type_mask,
     std::unique_ptr<BrowsingDataFilterBuilder> filter_builder) {
+  DCHECK_EQ(FILTERABLE_DATATYPES, remove_mask | FILTERABLE_DATATYPES);

[Bernhard Bauer, 2016/08/10 14:22:13]

Nit: If you changed this to DCHECK_EQ(0, remove_mask & ~FILTERABLE_DATATYPES),
you would immediately see the remaining bits in case of a failure (right?).

[msramek, 2016/08/11 13:11:01]

Done. You will see the remaining bits as a decimal number that you still have to
convert and compare with enum, so it's still not super helpful, but it's
definitely better.

   DCHECK(filter_builder);
   RemoveInternal(time_range, remove_mask, origin_type_mask,
                  std::move(filter_builder), nullptr);
 }
 
 void BrowsingDataRemover::RemoveWithFilterAndReply(
     const TimeRange& time_range,
     int remove_mask,
     int origin_type_mask,
     std::unique_ptr<BrowsingDataFilterBuilder> filter_builder,
     Observer* observer) {
+  DCHECK_EQ(FILTERABLE_DATATYPES, remove_mask | FILTERABLE_DATATYPES);
   DCHECK(filter_builder);
   DCHECK(observer);
   RemoveInternal(time_range, remove_mask, origin_type_mask,
                  std::move(filter_builder), observer);
 }
 
 void BrowsingDataRemover::RemoveInternal(
     const TimeRange& time_range,
     int remove_mask,
     int origin_type_mask,
@@ skipping 35 matching lines @@
              removal_task.remove_mask,
              *removal_task.filter_builder,
              removal_task.origin_type_mask);
 }
 
 void BrowsingDataRemover::RemoveImpl(
     const TimeRange& time_range,
     int remove_mask,
     const BrowsingDataFilterBuilder& filter_builder,
     int origin_type_mask) {
+  // =============== README before adding more storage backends ===============
+  //
+  // If you're adding a data storage backend that is included among
+  // RemoveDataMask::FILTERABLE_DATATYPES, you must do one of the following:
+  // 1. Support one of the filters generated by |filter_builder|.
+  // 2. Add a comment explaining why is it acceptable in your case to delete all
+  //    data without filtering URLs / origins / domains.
+  // 3. Do not support partial deletion, i.e. only delete your data if
+  //    |filter_builder.IsEmptyBlacklist()|. Add a comment explaining why this
+  //    is acceptable.
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   waiting_for_synchronous_clear_operations_ = true;
 
   // crbug.com/140910: Many places were calling this with base::Time() as
   // delete_end, even though they should've used base::Time::Max().
   DCHECK_NE(base::Time(), time_range.end);
 
   delete_begin_ = time_range.begin;
   delete_end_ = time_range.end;
   remove_mask_ = remove_mask;
@@ skipping 584 matching lines @@
               cryptohome::Identification(user->GetAccountId()),
               chromeos::attestation::kContentProtectionKeyPrefix,
               base::Bind(&BrowsingDataRemover::OnClearPlatformKeys,
                          weak_ptr_factory_.GetWeakPtr()));
       waiting_for_clear_platform_keys_ = true;
     }
 #endif  // defined(OS_CHROMEOS)
 #endif  // defined(ENABLE_PLUGINS)
   }
 
-  // Remove omnibox zero-suggest cache results.
+  // Remove omnibox zero-suggest cache results. Filtering is not supported.
+  // This is not a problem, as deleting more data than necessary will just cause
+  // another server round-trip; no data are actually lost.

[Bernhard Bauer, 2016/08/10 14:22:14]

Nit: "no data is actually lost" :)

[msramek, 2016/08/11 13:11:01]

*grumble* *grumble* Done. :) But yeah, I remember reading somewhere that
singular is preferred, either it was in our code, or generally in en_US.

[Bernhard Bauer, 2016/08/11 13:25:42]

Yes, I think in this case you are not talking about the plural of "datum"
(http://www.merriam-webster.com/dictionary/datum), but rather the uncountable
noun "data" :)

   if ((remove_mask & (REMOVE_CACHE | REMOVE_COOKIES)))
     prefs->SetString(omnibox::kZeroSuggestCachedResults, std::string());
 
   if (remove_mask & (REMOVE_COOKIES | REMOVE_HISTORY)) {
     domain_reliability::DomainReliabilityService* service =
       domain_reliability::DomainReliabilityServiceFactory::
           GetForBrowserContext(profile_);
     if (service) {
       domain_reliability::DomainReliabilityClearMode mode;
       if (remove_mask & REMOVE_COOKIES)
@@ skipping 375 matching lines @@
   waiting_for_clear_offline_page_data_ = false;
   NotifyIfDone();
 }
 #endif
 
 void BrowsingDataRemover::OnClearedDomainReliabilityMonitor() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   waiting_for_clear_domain_reliability_monitor_ = false;
   NotifyIfDone();
 }