Simplify BrowserStateDataRemover

Add a completionBlock parameter to ClearBrowsingDataCommand and
use it to wait for the completion of the browsing data removal
in BrowserStateDataRemover.

This allow removing the circular dependency between i/c/b/signin
and i/c/b/browsing_data and to simplify code.

Bug: none
Change-Id: I6ee62201dd321a453be8d7d6d588a8e713735d5f
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Reviewed-on: https://chromium-review.googlesource.com/916607
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537296}

ios/chrome/app/main_controller.mm

@@ -1645,7 +1645,7 @@ void MainControllerAuthenticationServiceDelegate::ClearBrowsingData(
       [self removeBrowsingDataFromBrowserState:browserState
                                           mask:mask
                                     timePeriod:timePeriod
-                             completionHandler:nil];
+                             completionHandler:[command completionBlock]];
 
       if (IsRemoveDataMaskSet(mask, BrowsingDataRemoveMask::REMOVE_COOKIES)) {
         base::Time beginDeleteTime =

ios/chrome/browser/browsing_data/BUILD.gn

@@ -51,7 +51,6 @@ source_set("browsing_data") {
     ":browsing_data_remove_mask",
     "//components/search_engines",
   ]
-  allow_circular_includes_from = [ "//ios/chrome/browser/signin" ]
 
   configs += [ "//build/config/compiler:enable_arc" ]
 }

ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.h

@@ -41,23 +41,6 @@ class IOSChromeBrowsingDataRemover {
   // removed once downstream code has been fixed to use BrowsingDataRemoveMask.
   const BrowsingDataRemoveMask REMOVE_ALL = BrowsingDataRemoveMask::REMOVE_ALL;
 
-  // When IOSChromeBrowsingDataRemover successfully removes data, a
-  // notification of type NOTIFICATION_BROWSING_DATA_REMOVED is triggered with
-  // a Details object of this type.
-  struct NotificationDetails {
-    NotificationDetails();
-    NotificationDetails(const NotificationDetails& details);
-    NotificationDetails(base::Time removal_begin,
-                        BrowsingDataRemoveMask removal_mask);
-    ~NotificationDetails();
-
-    // The beginning of the removal time range.
-    base::Time removal_begin;
-
-    // The removal mask (see the BrowsingDataRemoveMask enum for details).
-    BrowsingDataRemoveMask removal_mask;
-  };
-
   // Observer is notified when the removal is done. Done means keywords have
   // been deleted, cache cleared and all other tasks scheduled.
   class Observer {
@@ -68,10 +51,6 @@ class IOSChromeBrowsingDataRemover {
     virtual ~Observer() {}
   };
 
-  using Callback = base::Callback<void(const NotificationDetails&)>;
-  using CallbackSubscription = std::unique_ptr<
-      base::CallbackList<void(const NotificationDetails&)>::Subscription>;
-
   // Creates a IOSChromeBrowsingDataRemover bound to a specific period of time
   // (as defined via a TimePeriod). Returns a raw pointer, as
   // IOSChromeBrowsingDataRemover retains ownership of itself, and deletes
@@ -84,12 +63,6 @@ class IOSChromeBrowsingDataRemover {
   // data?
   static bool is_removing() { return is_removing_; }
 
-  // Add a callback to the list of callbacks to be called during a browsing data
-  // removal event. Returns a subscription object that can be used to
-  // un-register the callback.
-  static CallbackSubscription RegisterOnBrowsingDataRemovedCallback(
-      const Callback& callback);
-
   // Removes the specified items related to browsing for all origins.
   void Remove(BrowsingDataRemoveMask mask);
 

ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.mm

@@ -55,9 +55,6 @@
 
 namespace {
 
-using CallbackList = base::CallbackList<void(
-    const IOSChromeBrowsingDataRemover::NotificationDetails&)>;
-
 // A helper enum to report the deletion of cookies and/or cache. Do not
 // reorder the entries, as this enum is passed to UMA.
 enum CookieOrCacheDeletionChoice {
@@ -68,17 +65,6 @@ enum CookieOrCacheDeletionChoice {
   MAX_CHOICE_VALUE
 };
 
-// Contains all registered callbacks for browsing data removed notifications.
-CallbackList* g_on_browsing_data_removed_callbacks = nullptr;
-
-// Accessor for |*g_on_browsing_data_removed_callbacks|. Creates a new object
-// the first time so that it always returns a valid object.
-CallbackList* GetOnBrowsingDataRemovedCallbacks() {
-  if (!g_on_browsing_data_removed_callbacks)
-    g_on_browsing_data_removed_callbacks = new CallbackList();
-  return g_on_browsing_data_removed_callbacks;
-}
-
 bool AllDomainsPredicate(const std::string& domain) {
   return true;
 }
@@ -141,22 +127,6 @@ void ClearChannelIDsOnIOThread(
 
 bool IOSChromeBrowsingDataRemover::is_removing_ = false;
 
-IOSChromeBrowsingDataRemover::NotificationDetails::NotificationDetails()
-    : removal_begin(base::Time()),
-      removal_mask(BrowsingDataRemoveMask::REMOVE_NOTHING) {}
-
-IOSChromeBrowsingDataRemover::NotificationDetails::NotificationDetails(
-    const IOSChromeBrowsingDataRemover::NotificationDetails& details)
-    : removal_begin(details.removal_begin),
-      removal_mask(details.removal_mask) {}
-
-IOSChromeBrowsingDataRemover::NotificationDetails::NotificationDetails(
-    base::Time removal_begin,
-    BrowsingDataRemoveMask removal_mask)
-    : removal_begin(removal_begin), removal_mask(removal_mask) {}
-
-IOSChromeBrowsingDataRemover::NotificationDetails::~NotificationDetails() {}
-
 // Static.
 IOSChromeBrowsingDataRemover* IOSChromeBrowsingDataRemover::CreateForPeriod(
     ios::ChromeBrowserState* browser_state,
@@ -425,12 +395,6 @@ void IOSChromeBrowsingDataRemover::OnKeywordsLoaded(
 void IOSChromeBrowsingDataRemover::NotifyAndDelete() {
   set_removing(false);
 
-  // Notify observers.
-  IOSChromeBrowsingDataRemover::NotificationDetails details(delete_begin_,
-                                                            remove_mask_);
-
-  GetOnBrowsingDataRemovedCallbacks()->Notify(details);
-
   for (auto& observer : observer_list_)
     observer.OnIOSChromeBrowsingDataRemoverDone();
 
@@ -439,13 +403,6 @@ void IOSChromeBrowsingDataRemover::NotifyAndDelete() {
   base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
 }
 
-// static
-IOSChromeBrowsingDataRemover::CallbackSubscription
-IOSChromeBrowsingDataRemover::RegisterOnBrowsingDataRemovedCallback(
-    const IOSChromeBrowsingDataRemover::Callback& callback) {
-  return GetOnBrowsingDataRemovedCallbacks()->Add(callback);
-}
-
 void IOSChromeBrowsingDataRemover::OnTaskComplete() {
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
 

ios/chrome/browser/signin/browser_state_data_remover.h

@@ -5,8 +5,10 @@
 #ifndef IOS_CHROME_BROWSER_SIGNIN_BROWSER_STATE_DATA_REMOVER_H_
 #define IOS_CHROME_BROWSER_SIGNIN_BROWSER_STATE_DATA_REMOVER_H_
 
+#include <memory>
+
 #include "base/ios/block_types.h"
-#include "ios/chrome/browser/browsing_data/ios_chrome_browsing_data_remover.h"
+#include "base/macros.h"
 
 namespace ios {
 class ChromeBrowserState;
@@ -29,28 +31,21 @@ class BrowserStateDataRemover {
   static void ClearData(ios::ChromeBrowserState* browser_state,
                         ProceduralBlock completion);
 
-  // If set then the last username will be removed from the browser state prefs
-  // after the data has been wiped.
-  void SetForgetLastUsername();
-
+ private:
   // Wipes all the data in the browser state and invokes |callback| when done.
   // This can be called only once, and this object deletes itself after invoking
   // the callback.
   void RemoveBrowserStateData(ProceduralBlock callback);
 
- private:
-  void NotifyWithDetails(
-      const IOSChromeBrowsingDataRemover::NotificationDetails& details);
-  void ReadingListCleaned(
-      const IOSChromeBrowsingDataRemover::NotificationDetails& details,
-      bool reading_list_cleaned);
+  void BrowsingDataCleared();
+  void ReadingListCleaned(bool reading_list_cleaned);
 
   ios::ChromeBrowserState* browser_state_;
   ProceduralBlock callback_;
-  IOSChromeBrowsingDataRemover::CallbackSubscription callback_subscription_;
   std::unique_ptr<reading_list::ReadingListRemoverHelper>
       reading_list_remover_helper_;
-  bool forget_last_username_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserStateDataRemover);
 };
 
 #endif  // IOS_CHROME_BROWSER_SIGNIN_BROWSER_STATE_DATA_REMOVER_H_

ios/chrome/browser/signin/browser_state_data_remover.mm

@@ -22,47 +22,37 @@
 
 BrowserStateDataRemover::BrowserStateDataRemover(
     ios::ChromeBrowserState* browser_state)
-    : browser_state_(browser_state), forget_last_username_(false) {
-  callback_subscription_ =
-      IOSChromeBrowsingDataRemover::RegisterOnBrowsingDataRemovedCallback(
-          base::Bind(&BrowserStateDataRemover::NotifyWithDetails,
-                     base::Unretained(this)));
-}
+    : browser_state_(browser_state) {}
 
-BrowserStateDataRemover::~BrowserStateDataRemover() {
-}
+BrowserStateDataRemover::~BrowserStateDataRemover() {}
 
 // static
 void BrowserStateDataRemover::ClearData(ios::ChromeBrowserState* browser_state,
                                         ProceduralBlock completion) {
-  // The user just changed the account and chose to clear the previously
-  // existing data. As browsing data is being cleared, it is fine to clear the
-  // last username, as there will be no data to be merged.
   BrowserStateDataRemover* remover = new BrowserStateDataRemover(browser_state);
-  remover->SetForgetLastUsername();
   remover->RemoveBrowserStateData(completion);
 }
 
-void BrowserStateDataRemover::SetForgetLastUsername() {
-  forget_last_username_ = true;
-}
-
 void BrowserStateDataRemover::RemoveBrowserStateData(ProceduralBlock callback) {
   DCHECK(!callback_);
   callback_ = [callback copy];
 
+  // It is safe to use |this| in the block as the object manage its own lifetime
+  // and only call delete once the callback has been invoked.
   ClearBrowsingDataCommand* command = [[ClearBrowsingDataCommand alloc]
       initWithBrowserState:browser_state_
                       mask:BrowsingDataRemoveMask::REMOVE_ALL
-                timePeriod:browsing_data::TimePeriod::ALL_TIME];
+                timePeriod:browsing_data::TimePeriod::ALL_TIME
+           completionBlock:^{
+             this->BrowsingDataCleared();
+           }];
 
   UIWindow* mainWindow = [[UIApplication sharedApplication] keyWindow];
   DCHECK(mainWindow);
   [mainWindow chromeExecuteCommand:command];
 }
 
-void BrowserStateDataRemover::NotifyWithDetails(
-    const IOSChromeBrowsingDataRemover::NotificationDetails& details) {
+void BrowserStateDataRemover::BrowsingDataCleared() {
   // Remove bookmarks and Reading List entriesonce all browsing data was
   // removed.
   // Removal of browsing data waits for the bookmark model to be loaded, so
@@ -71,31 +61,19 @@ void BrowserStateDataRemover::NotifyWithDetails(
       << "Failed to remove all user bookmarks.";
   reading_list_remover_helper_ =
       std::make_unique<reading_list::ReadingListRemoverHelper>(browser_state_);
-  reading_list_remover_helper_->RemoveAllUserReadingListItemsIOS(
-      base::Bind(&BrowserStateDataRemover::ReadingListCleaned,
-                 base::Unretained(this), details));
+  reading_list_remover_helper_->RemoveAllUserReadingListItemsIOS(base::BindOnce(
+      &BrowserStateDataRemover::ReadingListCleaned, base::Unretained(this)));
 }
 
-void BrowserStateDataRemover::ReadingListCleaned(
-    const IOSChromeBrowsingDataRemover::NotificationDetails& details,
-    bool reading_list_cleaned) {
+void BrowserStateDataRemover::ReadingListCleaned(bool reading_list_cleaned) {
   CHECK(reading_list_cleaned)
       << "Failed to remove all user reading list items.";
 
-  if (details.removal_mask != BrowsingDataRemoveMask::REMOVE_ALL) {
-    NOTREACHED()
-        << "Unexpected partial remove browsing data request "
-        << "(removal mask = "
-        << static_cast<std::underlying_type<BrowsingDataRemoveMask>::type>(
-               details.removal_mask)
-        << ")";
-    return;
-  }
-
-  if (forget_last_username_) {
-    browser_state_->GetPrefs()->ClearPref(prefs::kGoogleServicesLastAccountId);
-    browser_state_->GetPrefs()->ClearPref(prefs::kGoogleServicesLastUsername);
-  }
+  // The user just changed the account and chose to clear the previously
+  // existing data. As browsing data is being cleared, it is fine to clear the
+  // last username, as there will be no data to be merged.
+  browser_state_->GetPrefs()->ClearPref(prefs::kGoogleServicesLastAccountId);
+  browser_state_->GetPrefs()->ClearPref(prefs::kGoogleServicesLastUsername);
 
   if (callback_)
     callback_();

ios/chrome/browser/ui/commands/clear_browsing_data_command.h

@@ -7,6 +7,7 @@
 
 #import <Foundation/Foundation.h>
 
+#import "base/ios/block_types.h"
 #import "components/browsing_data/core/browsing_data_utils.h"
 #include "ios/chrome/browser/browsing_data/browsing_data_remove_mask.h"
 #import "ios/chrome/browser/ui/commands/generic_chrome_command.h"
@@ -23,9 +24,11 @@ class ChromeBrowserState;
 
 // Initializes a command intented to clear browsing data for |browserState|
 // that corresponds to removal mask |mask| for the time period |timePeriod|.
+// |completionBlock| will be invoked when the data has been cleared.
 - (instancetype)initWithBrowserState:(ios::ChromeBrowserState*)browserState
                                 mask:(BrowsingDataRemoveMask)mask
                           timePeriod:(browsing_data::TimePeriod)timePeriod
+                     completionBlock:(ProceduralBlock)completionBlock
     NS_DESIGNATED_INITIALIZER;
 
 // When executed this command will remove browsing data for this BrowserState.
@@ -37,6 +40,9 @@ class ChromeBrowserState;
 // Time period for which the browsing data will be removed.
 @property(nonatomic, readonly) browsing_data::TimePeriod timePeriod;
 
+// Completion block invoked when the data has been cleared.
+@property(nonatomic, readonly) ProceduralBlock completionBlock;
+
 @end
 
 #endif  // IOS_CHROME_BROWSER_UI_COMMANDS_CLEAR_BROWSING_DATA_COMMAND_H_

ios/chrome/browser/ui/commands/clear_browsing_data_command.mm

@@ -16,21 +16,19 @@
 @synthesize browserState = _browserState;
 @synthesize mask = _mask;
 @synthesize timePeriod = _timePeriod;
-
-- (instancetype)initWithTag:(NSInteger)tag {
-  NOTREACHED();
-  return nil;
-}
+@synthesize completionBlock = _completionBlock;
 
 - (instancetype)initWithBrowserState:(ios::ChromeBrowserState*)browserState
                                 mask:(BrowsingDataRemoveMask)mask
-                          timePeriod:(browsing_data::TimePeriod)timePeriod {
+                          timePeriod:(browsing_data::TimePeriod)timePeriod
+                     completionBlock:(ProceduralBlock)completionBlock {
   self = [super initWithTag:IDC_CLEAR_BROWSING_DATA_IOS];
   if (self) {
     DCHECK(browserState);
     _browserState = browserState;
     _mask = mask;
     _timePeriod = timePeriod;
+    _completionBlock = completionBlock;
   }
   return self;
 }