Implement a task scheduler for BrowsingDataRemover

chrome/browser/browsing_data/browsing_data_remover.h

Index: chrome/browser/browsing_data/browsing_data_remover.h
diff --git a/chrome/browser/browsing_data/browsing_data_remover.h b/chrome/browser/browsing_data/browsing_data_remover.h
index fc53dbe59d26d7885403c0bd5eaf98a905f891e1..996b043b98974e630b47adf52a1843886c29c7ce 100644
--- a/chrome/browser/browsing_data/browsing_data_remover.h
+++ b/chrome/browser/browsing_data/browsing_data_remover.h
@@ -63,8 +63,51 @@ class URLRequestContextGetter;
 class WebappRegistry;
 #endif
 
+////////////////////////////////////////////////////////////////////////////////
 // BrowsingDataRemover is responsible for removing data related to browsing:
 // visits in url database, downloads, cookies ...
+//
+//  USAGE:
+//
+//  0. Instantiation.
+//
+//       BrowsingDataRemover remover =
+//           BrowsingDataRemoverFactory::GetForBrowserContext(profile);
+//
+//  1. No observer.
+//
+//       remover->Remove(Unbounded(), REMOVE_COOKIES, ALL);
+//
+//  2. Using an observer.

[Bernhard Bauer, 2016/07/26 14:51:38]

This doesn't really make it clear what the difference to 3 is. Maybe "one-shot
deletion"?

[msramek, 2016/07/28 09:57:42]

Expanded the descriptions a bit. "One-shot deletion" is not exactly correct,
because this class removes the observer in its destructor, so it could run and
observe more deletions.

+//
+//       class CookiesDeleter : public BrowsingDataRemover::Observer {
+//         CookiesDeleter() { remover->AddObserver(this); }
+//         ~CookiesDeleter() { remover->RemoveObserver(this); }
+//
+//         void DeleteCookies() {
+//           remover->RemoveAndReply(Unbounded(), REMOVE_COOKIES, ALL, this);
+//         }
+//
+//         void OnBrowsingDataRemoverDone() {
+//           LOG(INFO) << "Cookies were deleted.";
+//         }
+//       }
+//
+//  3. Observing the status of deletions.
+//
+//       class BrowsingDataRemoverStatusObsever {
+//         BrowsingDataRemoverStatusObserver() { remover->AddObserver(this); }
+//         ~BrowsingDataRemoverStatusObserver() {
+//           remover->RemoveObserver(this);
+//         }
+//
+//         void OnBrowsingDataRemoving(bool removing) {
+//           LOG(INFO) << "Remover is now " << (removing ? "busy" : "idle");
+//         }
+//       }
+//
+////////////////////////////////////////////////////////////////////////////////
+
 class BrowsingDataRemover : public KeyedService
 #if defined(ENABLE_PLUGINS)
                             , public PepperFlashSettingsManager::Client
@@ -153,17 +196,18 @@ class BrowsingDataRemover : public KeyedService
   };
 
   // Observer is notified when the removal is active and when it's done.
+  // TODO(msramek): The semantics of the two methods are slightly different;
+  // one is called for every observer at the same time, while the other only
+  // for one observer at a time. Consider splitting the interface.

[Bernhard Bauer, 2016/07/26 14:51:38]

I have to agree: I do find it very weird to have the two parts of this interface
used in two completely different ways. Really, for one-shot deletions,
RemoveObserver() is purely used as a signal for cancellation, and then
AddObserver() is just used to allow RemoveObserver() to work. And all of that
just to require fewer changes to callsites, which doesn't seem like it's the
right tradeoff in the longer term.

So, I'd be okay with moving forward with this CL if we can strengthen the TODO
here to "split the interface", not just considering it :)

[msramek, 2016/07/28 09:57:42]

I strengthened the TODO, and I readded the deprecation message.
OnBrowsingDataRemoving was added mostly as a quick remedy while this task
queuing mechanism is finished; I'm currently considering that it might not even
be needed anymore. I'll have to talk to dbeam@ first about that though.

   class Observer {
    public:
-    // NOTE: DEPRECATED; talk to dbeam/msramek before using this.
-    //
     // Whether removal is active. Note that not having an active removal is not
     // same as completing a removal. That is why the removing status is separate
     // from the done message.
     virtual void OnBrowsingDataRemoving(bool is_removing) {}
 
-    // Done means keywords have been deleted, cache cleared and all other
-    // removal tasks are scheduled.
+    // Called when a removal task is finished. Note that every removal task can
+    // only have one observer attached to it, and only that one is called.
     virtual void OnBrowsingDataRemoverDone() {}
 
    protected:
@@ -186,6 +230,29 @@ class BrowsingDataRemover : public KeyedService
     virtual ~CompletionInhibitor() {}
   };
 
+  // Represents a single removal task. Contains all parameters needed to execute
+  // it and a pointer to the observer that added it.
+  struct RemovalTask {

[Bernhard Bauer, 2016/07/26 14:51:38]

Does this need to be public?

[msramek, 2016/07/28 09:57:42]

Nope. Done.

+    RemovalTask(const TimeRange& time_range,
+                int remove_mask,
+                int origin_type_mask,
+                std::unique_ptr<BrowsingDataFilterBuilder> filter_builder,
+                Observer* observer);
+    ~RemovalTask();
+
+    // Move constructor and assignment operator so that RemovalTask can be used
+    // in a queue. Note that this class cannot be copied because of the
+    // contained unique_ptr.
+    RemovalTask(RemovalTask&& removalTask);
+    RemovalTask& operator=(RemovalTask&& removalTask);
+
+    TimeRange time_range;
+    int remove_mask;
+    int origin_type_mask;
+    std::unique_ptr<BrowsingDataFilterBuilder> filter_builder;
+    Observer* observer;
+  };
+
   static TimeRange Unbounded();
 
   static TimeRange Period(browsing_data::TimePeriod period);
@@ -202,22 +269,42 @@ class BrowsingDataRemover : public KeyedService
     completion_inhibitor_ = inhibitor;
   }
 
-  // Removes the specified items related to browsing for all origins that match
-  // the provided |origin_type_mask| (see BrowsingDataHelper::OriginTypeMask).
+  // Removes browsing data within the given |time_range|, with datatypes being
+  // specified by |remove_mask| and origin types by |origin_type_mask|.
   void Remove(const TimeRange& time_range,
               int remove_mask,
               int origin_type_mask);
 
-  // Removes the specified items related to browsing for all origins that match
-  // the provided |origin_type_mask| (see BrowsingDataHelper::OriginTypeMask).
-  // The |origin_filter| is used as a final filter for clearing operations.
+  // A version of the above that in addition informs the |observer| when the
+  // removal task is finished.
+  void RemoveAndReply(const TimeRange& time_range,
+                      int remove_mask,
+                      int origin_type_mask,
+                      Observer* observer);
+
+  // Like Remove(), but in case of URL-keyed only removes data whose URL match
+  // |filter_builder| (e.g. are on certain origin or domain).
   // TODO(dmurph): Support all backends with filter (crbug.com/113621).
   // DO NOT USE THIS METHOD UNLESS CALLER KNOWS WHAT THEY'RE DOING. NOT ALL
   // BACKENDS ARE SUPPORTED YET, AND MORE DATA THAN EXPECTED COULD BE DELETED.
-  virtual void RemoveWithFilter(const TimeRange& time_range,
-                                int remove_mask,
-                                int origin_type_mask,
-                                const BrowsingDataFilterBuilder& origin_filter);
+  virtual void RemoveWithFilter(
+      const TimeRange& time_range,
+      int remove_mask,
+      int origin_type_mask,
+      std::unique_ptr<BrowsingDataFilterBuilder> filter_builder);
+
+  // A version of the above that in addition informs the |observer| when the
+  // removal task is finished.
+  virtual void RemoveWithFilterAndReply(
+      const TimeRange& time_range,
+      int remove_mask,
+      int origin_type_mask,
+      std::unique_ptr<BrowsingDataFilterBuilder> filter_builder,
+      Observer* observer);
+
+  // Executes the next removal task. Called after the previous task was finished
+  // or directly from Remove() if the task queue was empty.
+  void RunNextTask();
 
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
@@ -307,7 +394,7 @@ class BrowsingDataRemover : public KeyedService
   // TODO(crbug.com/589586): Support all backends w/ origin filter.
   void RemoveImpl(const TimeRange& time_range,
                   int remove_mask,
-                  const BrowsingDataFilterBuilder& origin_filter,
+                  const BrowsingDataFilterBuilder& filter_builder,
                   int origin_type_mask);
 
   // Notifies observers and transitions to the idle state.
@@ -410,6 +497,9 @@ class BrowsingDataRemover : public KeyedService
   // True if Remove has been invoked.
   bool is_removing_;
 
+  // Removal tasks to be processed.
+  std::queue<RemovalTask> task_queue_;
+
   // If non-NULL, the |completion_inhibitor_| is notified each time an instance
   // is about to complete a browsing data removal process, and has the ability
   // to artificially delay completion. Used for testing.
@@ -465,6 +555,7 @@ class BrowsingDataRemover : public KeyedService
   // From which types of origins should we remove data?
   int origin_type_mask_ = 0;
 
+  // Observers of the global state and individual tasks.
   base::ObserverList<Observer, true> observer_list_;
 
   // Used if we need to clear history.