Formalize the restrictions of BrowsingDataRemover::RemoveWithFilter()

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 4b4325239d15ea3c876965f2ba3581260327f187..49705efcf60ecf5624a2d18a9ec20452a04efc21 100644
--- a/chrome/browser/browsing_data/browsing_data_remover.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover.cc
@@ -349,6 +349,7 @@ void BrowsingDataRemover::RemoveWithFilter(
     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);
@@ -360,6 +361,7 @@ void BrowsingDataRemover::RemoveWithFilterAndReply(
     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,
@@ -415,6 +417,16 @@ void BrowsingDataRemover::RemoveImpl(
     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;
 
@@ -1019,7 +1031,9 @@ void BrowsingDataRemover::RemoveImpl(
 #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());