Implement a task scheduler for BrowsingDataRemover

chrome/browser/browsing_data/browsing_data_remover.cc

 // 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.
 
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 
 #include <map>
 #include <set>
 #include <string>
 
@@ skipping 281 matching lines @@
       remove_mask_(-1),
       origin_type_mask_(-1),
 #if BUILDFLAG(ANDROID_JAVA_UI)
       webapp_registry_(new WebappRegistry()),
 #endif
       weak_ptr_factory_(this) {
   DCHECK(browser_context);
 }
 
 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() {
   history_task_tracker_.TryCancelAll();
   template_url_sub_.reset();
 }
 
 void BrowsingDataRemover::SetRemoving(bool is_removing) {
   DCHECK_NE(is_removing_, is_removing);
   is_removing_ = is_removing;
   FOR_EACH_OBSERVER(Observer, observer_list_,
                     OnBrowsingDataRemoving(is_removing));
 }
 
 void BrowsingDataRemover::Remove(const TimeRange& time_range,
                                  int remove_mask,
                                  int origin_type_mask) {
-  // 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);
+  RemoveInternal(time_range, remove_mask, origin_type_mask,
+                 std::unique_ptr<RegistrableDomainFilterBuilder>(), nullptr);
+}
+
+void BrowsingDataRemover::RemoveAndReply(
+    const TimeRange& time_range,
+    int remove_mask,
+    int origin_type_mask,
+    Observer* observer) {
+  DCHECK(observer);
+  RemoveInternal(time_range, remove_mask, origin_type_mask,
+                 std::unique_ptr<RegistrableDomainFilterBuilder>(), 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) {
+  DCHECK(filter_builder);
+  RemoveInternal(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(filter_builder);
+  DCHECK(observer);
+  RemoveInternal(time_range, remove_mask, origin_type_mask,
+                 std::move(filter_builder), observer);
+}
+
+void BrowsingDataRemover::RemoveInternal(
+    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))
+      << "Every observer must register itself (by calling AddObserver()) "
+      << "before observing a removal task.";
+
+  // Remove() and RemoveAndReply() pass a null pointer to indicate no filter.
+  // No filter is equivalent to one that |IsEmptyBlacklist()|.
+  if (!filter_builder) {
+    filter_builder = base::WrapUnique(new RegistrableDomainFilterBuilder(
+        RegistrableDomainFilterBuilder::BLACKLIST));
+    DCHECK(filter_builder->IsEmptyBlacklist());
+  }
+
+  task_queue_.emplace(
+      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(
     const TimeRange& time_range,
     int remove_mask,
     const BrowsingDataFilterBuilder& filter_builder,
     int origin_type_mask) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   waiting_for_synchronous_clear_operations_ = true;
 
   // crbug.com/140910: Many places were calling this with base::Time() as
   // 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;
   origin_type_mask_ = origin_type_mask;
 
   base::Callback<bool(const GURL& url)> filter =
       filter_builder.BuildGeneralFilter();
   base::Callback<bool(const ContentSettingsPattern& url)> same_pattern_filter =
       filter_builder.BuildWebsiteSettingsPatternMatchesFilter();
 
@@ skipping 678 matching lines @@
 }
 
 int BrowsingDataRemover::GetLastUsedRemovalMask() {
   return remove_mask_;
 }
 
 int BrowsingDataRemover::GetLastUsedOriginTypeMask() {
   return origin_type_mask_;
 }
 
+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() {}
 
 void BrowsingDataRemover::ClearSettingsForOneTypeWithPredicate(
     HostContentSettingsMap* content_settings_map,
     ContentSettingsType content_type,
     const base::Callback<bool(const ContentSettingsPattern& primary_pattern,
                               const ContentSettingsPattern& secondary_pattern)>&
         predicate) {
   ContentSettingsForOneType settings;
   content_settings_map->GetSettingsForOneType(content_type, std::string(),
                                               &settings);
@@ skipping 39 matching lines @@
   // else notifies observers and deletes this BrowsingDataRemover.
   TemplateURLService* model =
       TemplateURLServiceFactory::GetForProfile(profile_);
   model->RemoveAutoGeneratedBetween(delete_begin_, delete_end_);
   waiting_for_clear_keyword_data_ = false;
   template_url_sub_.reset();
   NotifyIfDone();
 }
 
 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() {
   // TODO(brettw) http://crbug.com/305259: This should also observe session
   // clearing (what about other things such as passwords, etc.?) and wait for
   // them to complete before continuing.
 
   if (!AllDone())
     return;
 
@@ skipping 168 matching lines @@
   waiting_for_clear_offline_page_data_ = false;
   NotifyIfDone();
 }
 #endif
 
 void BrowsingDataRemover::OnClearedDomainReliabilityMonitor() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   waiting_for_clear_domain_reliability_monitor_ = false;
   NotifyIfDone();
 }