Implement a task scheduler for BrowsingDataRemover

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 2b81ad069b34adb8aba597d68781fa09b9729f6f..c755ee53774bf3e1a63918fdff0bb05f7aeb804b 100644
--- a/chrome/browser/browsing_data/browsing_data_remover.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover.cc
@@ -252,6 +252,25 @@ void ClearChannelIDsOnIOThread(
 BrowsingDataRemover::CompletionInhibitor*
     BrowsingDataRemover::completion_inhibitor_ = nullptr;
 
+BrowsingDataRemover::RemovalTask::RemovalTask(
+    const TimeRange& time_range,
+    int remove_mask,
+    int origin_type_mask,
+    std::unique_ptr<BrowsingDataFilterBuilder> filter_builder,
+    Observer* observer)
+    : time_range(time_range),
+      remove_mask(remove_mask),
+      origin_type_mask(origin_type_mask),
+      filter_builder(std::move(filter_builder)),
+      observer(observer) {}
+
+BrowsingDataRemover::RemovalTask::~RemovalTask() {}
+
+BrowsingDataRemover::RemovalTask::RemovalTask(
+    RemovalTask&& removalTask) = default;
+BrowsingDataRemover::RemovalTask& BrowsingDataRemover::RemovalTask::operator=(
+    RemovalTask&& removalTask) = default;
+
 bool BrowsingDataRemover::TimeRange::operator==(
     const BrowsingDataRemover::TimeRange& other) const {
   return begin == other.begin && end == other.end;
@@ -297,12 +316,20 @@ BrowsingDataRemover::BrowsingDataRemover(
 }
 
 BrowsingDataRemover::~BrowsingDataRemover() {
-  // If we are still removing data, notify observers so they can remove
-  // themselves from the observer list.
+  if (!task_queue_.empty()) {
+    VLOG(1) << "BrowsingDataRemover shuts down with " << task_queue_.size()
+            << " pending tasks";
+  }
+
+  // If we are still removing data, notify observers that their task has been
+  // (albeit unsucessfuly) processed, so they can unregister themselves.
   // TODO(bauerb): If it becomes a problem that browsing data might not actually
   // be fully cleared when an observer is notified, add a success flag.
-  if (is_removing_)
-    Notify();
+  while (!task_queue_.empty()) {
+    if (observer_list_.HasObserver(task_queue_.front().observer))
+      task_queue_.front().observer->OnBrowsingDataRemoverDone();
+    task_queue_.pop();
+  }
 }
 
 void BrowsingDataRemover::Shutdown() {
@@ -320,20 +347,67 @@ void BrowsingDataRemover::SetRemoving(bool is_removing) {
 void BrowsingDataRemover::Remove(const TimeRange& time_range,
                                  int remove_mask,
                                  int origin_type_mask) {
+  RemoveAndReply(time_range, remove_mask, origin_type_mask, nullptr);
+}
+
+void BrowsingDataRemover::RemoveAndReply(
+    const TimeRange& time_range,
+    int remove_mask,
+    int origin_type_mask,
+    Observer* observer) {
   // Any instance of BrowsingDataFilterBuilder that |IsEmptyBlacklist()|
   // is OK to pass here.
-  RegistrableDomainFilterBuilder builder(
-      RegistrableDomainFilterBuilder::BLACKLIST);
-  DCHECK(builder.IsEmptyBlacklist());
-  RemoveImpl(time_range, remove_mask, builder, origin_type_mask);
+  std::unique_ptr<RegistrableDomainFilterBuilder> filter_builder =
+      base::WrapUnique(new RegistrableDomainFilterBuilder(
+          RegistrableDomainFilterBuilder::BLACKLIST));
+  DCHECK(filter_builder->IsEmptyBlacklist());
+  RemoveWithFilterAndReply(time_range, remove_mask, origin_type_mask,
+                           std::move(filter_builder), observer);
 }
 
 void BrowsingDataRemover::RemoveWithFilter(
     const TimeRange& time_range,
     int remove_mask,
     int origin_type_mask,
-    const BrowsingDataFilterBuilder& filter_builder) {
-  RemoveImpl(time_range, remove_mask, filter_builder, origin_type_mask);
+    std::unique_ptr<BrowsingDataFilterBuilder> filter_builder) {
+  RemoveWithFilterAndReply(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(!observer || observer_list_.HasObserver(observer))

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

If you allow the |observer| to be null, do you even still need the
Remove(WithFilter) methods without reply?

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

All methods can be reduced to the last one, as demonstrated in the test.

Since the usecase with no filter and no observer should be the most common one,
I think it makes sense to make these simpler versions of the method. It's a few
lines of code here and it lowers the footprint of this CL everywhere else, so it
seems preferable to me. But I'm happy to be convinced otherwise :)

[Bernhard Bauer, 2016/08/03 16:42:46]

Hm, could we make one method that takes no observer and one method that takes a
non-null observer (and have both forward to an internal method)? That way
existing callers for the no-observer method don't need to be changed, but we
would catch cases where someone accidentally passes in a null observer.

[msramek, 2016/08/03 21:16:44]

Yep, that makes more sense actually. Done.

+      << "Every observer must register itself (by calling AddObserver()) "
+      << "before observing a removal task.";
+
+  task_queue_.push(RemovalTask(

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

You could use emplace() for that.

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

Done. Nice :)

+      time_range,
+      remove_mask,
+      origin_type_mask,
+      std::move(filter_builder),
+      observer));
+
+  // If this is the only scheduled task, execute it immediately. Otherwise,
+  // it will be automatically executed when all tasks scheduled before it
+  // finish.
+  if (task_queue_.size() == 1) {
+    SetRemoving(true);
+    RunNextTask();
+  }
+}
+
+void BrowsingDataRemover::RunNextTask() {
+  DCHECK(!task_queue_.empty());
+  const RemovalTask& removal_task = task_queue_.front();
+
+  RemoveImpl(removal_task.time_range,
+             removal_task.remove_mask,
+             *removal_task.filter_builder,
+             removal_task.origin_type_mask);
 }
 
 void BrowsingDataRemover::RemoveImpl(
@@ -348,7 +422,6 @@ void BrowsingDataRemover::RemoveImpl(
   // delete_end, even though they should've used base::Time::Max().
   DCHECK_NE(base::Time(), time_range.end);
 
-  SetRemoving(true);
   delete_begin_ = time_range.begin;
   delete_end_ = time_range.end;
   remove_mask_ = remove_mask;
@@ -1106,8 +1179,40 @@ void BrowsingDataRemover::OnKeywordsLoaded() {
 }
 
 void BrowsingDataRemover::Notify() {
-  SetRemoving(false);
-  FOR_EACH_OBSERVER(Observer, observer_list_, OnBrowsingDataRemoverDone());
+  // Some tests call |RemoveImpl| directly, without using the task scheduler.
+  // TODO(msramek): Improve those tests so we don't have to do this. Tests
+  // relying on |RemoveImpl| do so because they need to pass in
+  // BrowsingDataFilterBuilder while still keeping ownership of it. Making
+  // BrowsingDataFilterBuilder copyable would solve this.
+  if (!is_removing_) {
+    DCHECK(task_queue_.empty());
+    return;
+  }
+
+  // Inform the observer of the current task unless it has unregistered
+  // itself in the meantime.
+  DCHECK(!task_queue_.empty());
+
+  if (task_queue_.front().observer != nullptr &&
+      observer_list_.HasObserver(task_queue_.front().observer)) {
+    task_queue_.front().observer->OnBrowsingDataRemoverDone();
+  }
+
+  task_queue_.pop();
+
+  if (task_queue_.empty()) {
+    // All removal tasks have finished. Inform the observers that we're idle.
+    SetRemoving(false);
+    return;
+  }
+
+  // Yield to the UI thread before executing the next removal task.
+  // TODO(msramek): Consider also adding a backoff if too many tasks
+  // are scheduled.
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&BrowsingDataRemover::RunNextTask,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BrowsingDataRemover::NotifyIfDone() {