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 <queue>
 #include <set>
 
 #include "base/callback_forward.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/synchronization/waitable_event_watcher.h"
@@ skipping 36 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 to report when one's own removal task is finished.
+//
+//       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. Using an observer to report when any removal task started or all removal
+//     tasks finished (i.e. when BrowsingDataRemover changes state from idle
+//     to busy and vice versa).
+//
+//       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. Split the interface or deprecate
+  // OnBrowsingDataRemoving().
   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 {
@@ skipping 18 matching lines @@
 
   // 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);

[Bernhard Bauer, 2016/08/04 12:30:51]

You might want to make these non-virtual anymore.

[msramek, 2016/08/04 12:48:57]

It's actually "not yet" rather than "anymore". This part was landed in
https://codereview.chromium.org/2161583002 which was extracted from
https://codereview.chromium.org/2025683003/. In the latter CL, I'm mocking
RemoveWithFilter() so I need it to be virtual.

Should I perhaps instead declare all four as virtual, so that readers of this
code don't have to wonder why the first two aren't and the last two are?

[Bernhard Bauer, 2016/08/04 12:57:41]

Could you just declare the Internal method as virtual? That's where it all ends
up anyway.

[Bernhard Bauer, 2016/08/04 12:57:41]

Could you just declare the Internal method as virtual? That's where all calls
end up anyway.

[msramek, 2016/08/04 13:20:27]

...yes :) Apparently I'm a bit slow today. Done. Also made it protected.

 
   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 14 matching lines @@
   // this class. The constructor is protected so that the class is mockable.
   BrowsingDataRemover(content::BrowserContext* browser_context);
   ~BrowsingDataRemover() override;
 
  private:
   // The clear API needs to be able to toggle removing_ in order to test that
   // only one BrowsingDataRemover instance can be called at a time.
   FRIEND_TEST_ALL_PREFIXES(ExtensionBrowsingDataTest, OneAtATime);
   // Testing our static method, ClearSettingsForOneTypeWithPredicate.
   FRIEND_TEST_ALL_PREFIXES(BrowsingDataRemoverTest, ClearWithPredicate);
+  // Testing the private RemovalTask.
+  FRIEND_TEST_ALL_PREFIXES(BrowsingDataRemoverTest, MultipleTasks);
 
   // The BrowsingDataRemover tests need to be able to access the implementation
   // of Remove(), as it exposes details that aren't yet available in the public
   // API. As soon as those details are exposed via new methods, this should be
   // removed.
   //
   // TODO(mkwst): See http://crbug.com/113621
   friend class BrowsingDataRemoverTest;
 
   friend class BrowsingDataRemoverFactory;
 
+  // Represents a single removal task. Contains all parameters needed to execute
+  // it and a pointer to the observer that added it.
+  struct RemovalTask {
+    RemovalTask(const TimeRange& time_range,
+                int remove_mask,
+                int origin_type_mask,
+                std::unique_ptr<BrowsingDataFilterBuilder> filter_builder,
+                Observer* observer);
+    ~RemovalTask();
+
+    TimeRange time_range;
+    int remove_mask;
+    int origin_type_mask;
+    std::unique_ptr<BrowsingDataFilterBuilder> filter_builder;
+    Observer* observer;
+  };
+
   // Clears all host-specific settings for one content type that satisfy the
   // given predicate.
   //
   // This should only be called on the UI thread.
   static void ClearSettingsForOneTypeWithPredicate(
       HostContentSettingsMap* content_settings_map,
       ContentSettingsType content_type,
       const base::Callback<
           bool(const ContentSettingsPattern& primary_pattern,
                const ContentSettingsPattern& secondary_pattern)>& predicate);
@@ skipping 15 matching lines @@
   // PepperFlashSettingsManager::Client implementation.
   void OnDeauthorizeFlashContentLicensesCompleted(uint32_t request_id,
                                                   bool success) override;
 #endif
 
 #if defined (OS_CHROMEOS)
   void OnClearPlatformKeys(chromeos::DBusMethodCallStatus call_status,
                            bool result);
 #endif
 
+  // A common reduction of all public Remove[WithFilter][AndReply] methods.
+  virtual void RemoveInternal(
+      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();
+
   // 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_