Implement a task scheduler for BrowsingDataRemover

chrome/browser/browsing_data/browsing_data_remover.h

 // 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.
 
 #ifndef CHROME_BROWSER_BROWSING_DATA_BROWSING_DATA_REMOVER_H_
 #define CHROME_BROWSER_BROWSING_DATA_BROWSING_DATA_REMOVER_H_
 
 #include <stdint.h>
 
 #include <set>
@@ skipping 45 matching lines @@
 }
 
 namespace net {
 class URLRequestContextGetter;
 }
 
 #if BUILDFLAG(ANDROID_JAVA_UI)
 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
 #endif
 {
  public:
   // Mask used for Remove.
   enum RemoveDataMask {
     REMOVE_APPCACHE = 1 << 0,
     REMOVE_CACHE = 1 << 1,
@@ skipping 68 matching lines @@
 
   struct TimeRange {
     TimeRange(base::Time begin, base::Time end) : begin(begin), end(end) {}
     bool operator==(const TimeRange& other) const;
 
     base::Time begin;
     base::Time end;
   };
 
   // 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:
     virtual ~Observer() {}
   };
 
   // The completion inhibitor can artificially delay completion of the browsing
   // data removal process. It is used during testing to simulate scenarios in
   // which the deletion stalls or takes a very long time.
   class CompletionInhibitor {
    public:
     // Invoked when a |remover| is just about to complete clearing browser data,
     // and will be prevented from completing until after the callback
     // |continue_to_completion| is run.
     virtual void OnBrowsingDataRemoverWouldComplete(
         BrowsingDataRemover* remover,
         const base::Closure& continue_to_completion) = 0;
 
    protected:
     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);
 
   // Is the BrowsingDataRemover currently in the process of removing data?
   bool is_removing() { return is_removing_; }
 
   // Sets a CompletionInhibitor, which will be notified each time an instance is
   // about to complete a browsing data removal process, and will be able to
   // artificially delay the completion.
   // TODO(crbug.com/483528): Make this non-static.
   static void set_completion_inhibitor_for_testing(
       CompletionInhibitor* inhibitor) {
     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);
 
   // Used for testing.
   void OverrideStoragePartitionForTesting(
       content::StoragePartition* storage_partition);
 
 #if BUILDFLAG(ANDROID_JAVA_UI)
   void OverrideWebappRegistryForTesting(
@@ skipping 69 matching lines @@
   // Removes the specified items related to browsing for a specific host. If the
   // provided |remove_url| is empty, data is removed for all origins; otherwise,
   // it is restricted by the origin filter origin (where implemented yet). The
   // |origin_type_mask| parameter defines the set of origins from which data
   // should be removed (protected, unprotected, or both).
   // TODO(ttr314): Remove "(where implemented yet)" constraint above once
   // crbug.com/113621 is done.
   // 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.
   void Notify();
 
   // Checks if we are all done, and if so, calls Notify().
   void NotifyIfDone();
 
   // Called when history deletion is done.
   void OnHistoryDeletionDone();
@@ skipping 82 matching lines @@
 
   // Start time to delete from.
   base::Time delete_begin_;
 
   // End time to delete to.
   base::Time delete_end_;
 
   // 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.
   static CompletionInhibitor* completion_inhibitor_;
 
 #if defined(ENABLE_PLUGINS)
   // Used to delete plugin data.
   std::unique_ptr<content::PluginDataRemover> plugin_data_remover_;
   base::WaitableEventWatcher watcher_;
 
@@ skipping 35 matching lines @@
   bool waiting_for_clear_webrtc_logs_ = false;
 #endif
   bool waiting_for_clear_auto_sign_in_ = false;
 
   // The removal mask for the current removal operation.
   int remove_mask_ = 0;
 
   // 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.
   base::CancelableTaskTracker history_task_tracker_;
 
   std::unique_ptr<TemplateURLService::Subscription> template_url_sub_;
 
   // We do not own this.
   content::StoragePartition* storage_partition_for_testing_ = nullptr;
 
 #if BUILDFLAG(ANDROID_JAVA_UI)
   // WebappRegistry makes calls across the JNI. In unit tests, the Java side is
   // not initialised, so the registry must be mocked out.
   std::unique_ptr<WebappRegistry> webapp_registry_;
 #endif
 
   base::WeakPtrFactory<BrowsingDataRemover> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowsingDataRemover);
 };
 
 #endif  // CHROME_BROWSER_BROWSING_DATA_BROWSING_DATA_REMOVER_H_