[iOS] Make CBD Manager Subscribe to Time Range changes

Currently, we set, but don't subscribe to changes of, Time Range in the
new CBD dialog. For this to work, it is implicitly assumed that Time
Range can only be changed through CBD.

Bug 961340 reveals a glitch that Time Range can actually be changed by
other entities. With this the least we can do is to have a more
defensive design and subscribe to changes on Time Range, so we can
keep the consistency between the interface and the browser state.

- CBDManager is chosen to be the Time Range observing entity, as
it is the creator of the Time Range item.
- Remove the definition (and use) of
TimeRangeSelectorTableViewControllerDelegate, which is no longer needed
since the observer is by itself actively observing changes on
Time Range.
- Update unit tests.

Bug: 961340
Change-Id: I2f692f770c8b533c1bf3730ba34c08fe2bc49269
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1602648Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Huanzhong Huang <huanzhong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660330}

ios/chrome/browser/ui/settings/clear_browsing_data/BUILD.gn

@@ -29,6 +29,7 @@ source_set("clear_browsing_data") {
     "//components/browsing_data/core",
     "//components/feature_engagement/public",
     "//components/history/core/browser",
+    "//components/prefs/ios",
     "//components/strings",
     "//ios/chrome/app/strings",
     "//ios/chrome/browser",

ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_collection_view_controller.mm

@@ -210,8 +210,7 @@
     case ItemTypeTimeRange: {
       UIViewController* controller =
           [[TimeRangeSelectorTableViewController alloc]
-              initWithPrefs:_browserState->GetPrefs()
-                   delegate:self.dataManager];
+              initWithPrefs:_browserState->GetPrefs()];
       [self.navigationController pushViewController:controller animated:YES];
       break;
     }

ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_manager.h

@@ -5,15 +5,18 @@
 #ifndef IOS_CHROME_BROWSER_UI_SETTINGS_CLEAR_BROWSING_DATA_CLEAR_BROWSING_DATA_MANAGER_H_
 #define IOS_CHROME_BROWSER_UI_SETTINGS_CLEAR_BROWSING_DATA_CLEAR_BROWSING_DATA_MANAGER_H_
 
-#import <Foundation/Foundation.h>
+#include "components/browsing_data/core/counters/browsing_data_counter.h"
+#import "ios/chrome/browser/ui/list_model/list_model.h"
 
-#import "ios/chrome/browser/browsing_data/browsing_data_remover_observer_bridge.h"
-#import "ios/chrome/browser/ui/settings/clear_browsing_data/browsing_data_counter_wrapper_producer.h"
-#import "ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_consumer.h"
-#import "ios/chrome/browser/ui/settings/clear_browsing_data/time_range_selector_table_view_controller.h"
+namespace ios {
+class ChromeBrowserState;
+}
+class BrowsingDataRemover;
+enum class BrowsingDataRemoveMask;
 
-@class ListModel;
 @class ActionSheetCoordinator;
+@class BrowsingDataCounterWrapperProducer;
+@protocol ClearBrowsingDataConsumer;
 @protocol CollectionViewFooterLinkDelegate;
 
 // Clear Browswing Data Section Identifiers.
@@ -70,9 +73,7 @@ enum class ClearBrowsingDataListType {
 
 // Manager that serves as the bulk of the logic for
 // ClearBrowsingDataConsumer.
-@interface ClearBrowsingDataManager
-    : NSObject <BrowsingDataRemoverObserving,
-                TimeRangeSelectorTableViewControllerDelegate>
+@interface ClearBrowsingDataManager : NSObject
 
 // The manager's consumer.
 @property(nonatomic, weak) id<ClearBrowsingDataConsumer> consumer;

ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_manager.mm

@@ -14,6 +14,7 @@
 #include "components/feature_engagement/public/tracker.h"
 #include "components/google/core/common/google_util.h"
 #include "components/history/core/browser/web_history_service.h"
+#include "components/prefs/ios/pref_observer_bridge.h"
 #include "components/prefs/pref_service.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/sync/driver/sync_service.h"
@@ -24,6 +25,7 @@
 #include "ios/chrome/browser/browsing_data/browsing_data_remove_mask.h"
 #include "ios/chrome/browser/browsing_data/browsing_data_remover.h"
 #include "ios/chrome/browser/browsing_data/browsing_data_remover_factory.h"
+#import "ios/chrome/browser/browsing_data/browsing_data_remover_observer_bridge.h"
 #include "ios/chrome/browser/chrome_url_constants.h"
 #include "ios/chrome/browser/feature_engagement/tracker_factory.h"
 #include "ios/chrome/browser/history/web_history_service_factory.h"
@@ -40,7 +42,10 @@
 #import "ios/chrome/browser/ui/settings/cells/clear_browsing_data_item.h"
 #import "ios/chrome/browser/ui/settings/cells/legacy/legacy_settings_detail_item.h"
 #import "ios/chrome/browser/ui/settings/cells/table_view_clear_browsing_data_item.h"
+#import "ios/chrome/browser/ui/settings/clear_browsing_data/browsing_data_counter_wrapper_producer.h"
+#import "ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_consumer.h"
 #import "ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_ui_constants.h"
+#import "ios/chrome/browser/ui/settings/clear_browsing_data/time_range_selector_table_view_controller.h"
 #import "ios/chrome/browser/ui/table_view/cells/table_view_detail_icon_item.h"
 #import "ios/chrome/browser/ui/table_view/cells/table_view_text_button_item.h"
 #import "ios/chrome/browser/ui/table_view/cells/table_view_text_item.h"
@@ -93,9 +98,14 @@ static NSDictionary* _imageNamesByItemTypes = @{
       @"clear_browsing_data_autofill",
 };
 
-@interface ClearBrowsingDataManager () {
+@interface ClearBrowsingDataManager () <BrowsingDataRemoverObserving,
+                                        PrefObserverDelegate> {
   // Access to the kDeleteTimePeriod preference.
   IntegerPrefMember _timeRangePref;
+  // Pref observer to track changes to prefs.
+  std::unique_ptr<PrefObserverBridge> _prefObserverBridge;
+  // Registrar for pref changes notifications.
+  PrefChangeRegistrar _prefChangeRegistrar;
 
   // Observer for browsing data removal events and associated ScopedObserver
   // used to track registration with BrowsingDataRemover. They both may be
@@ -175,6 +185,11 @@ static NSDictionary* _imageNamesByItemTypes = @{
           ScopedObserver<BrowsingDataRemover, BrowsingDataRemoverObserver>>(
           observer_.get());
       scoped_observer_->Add(remover);
+
+      _prefChangeRegistrar.Init(_browserState->GetPrefs());
+      _prefObserverBridge.reset(new PrefObserverBridge(self));
+      _prefObserverBridge->ObserveChangesForPreference(
+          browsing_data::prefs::kDeleteTimePeriod, &_prefChangeRegistrar);
     }
   }
   return self;
@@ -815,11 +830,10 @@ static NSDictionary* _imageNamesByItemTypes = @{
   [self.consumer updateCellsForItem:footerItem];
 }
 
-#pragma mark TimeRangeSelectorTableViewControllerDelegate
+#pragma mark - PrefObserverDelegate
 
-- (void)timeRangeSelectorViewController:
-            (TimeRangeSelectorTableViewController*)tableViewController
-                    didSelectTimePeriod:(browsing_data::TimePeriod)timePeriod {
+- (void)onPreferenceChanged:(const std::string&)preferenceName {
+  DCHECK(preferenceName == browsing_data::prefs::kDeleteTimePeriod);
   NSString* detailText = [TimeRangeSelectorTableViewController
       timePeriodLabelForPrefs:self.browserState->GetPrefs()];
   if (self.listType == ClearBrowsingDataListType::kListTypeCollectionView) {

ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_manager_unittest.mm

@@ -20,6 +20,7 @@
 #include "ios/chrome/browser/signin/identity_test_environment_chrome_browser_state_adaptor.h"
 #include "ios/chrome/browser/sync/profile_sync_service_factory.h"
 #import "ios/chrome/browser/ui/settings/clear_browsing_data/fake_browsing_data_counter_wrapper_producer.h"
+#import "ios/chrome/browser/ui/table_view/cells/table_view_detail_icon_item.h"
 #import "ios/chrome/browser/ui/table_view/table_view_model.h"
 #include "ios/web/public/test/test_web_thread_bundle.h"
 #include "services/identity/public/cpp/identity_test_environment.h"
@@ -64,13 +65,16 @@ class ClearBrowsingDataManagerTest : public PlatformTest {
     manager_ = [[ClearBrowsingDataManager alloc]
                       initWithBrowserState:browser_state_.get()
                                   listType:ClearBrowsingDataListType::
-                                               kListTypeCollectionView
+                                               kListTypeTableView
                        browsingDataRemover:remover_.get()
         browsingDataCounterWrapperProducer:
             [[FakeBrowsingDataCounterWrapperProducer alloc] init]];
 
     test_sync_service_ = static_cast<syncer::TestSyncService*>(
         ProfileSyncServiceFactory::GetForBrowserState(browser_state_.get()));
+
+    time_range_pref_.Init(browsing_data::prefs::kDeleteTimePeriod,
+                          browser_state_->GetPrefs());
   }
 
   identity::IdentityTestEnvironment* identity_test_env() {
@@ -86,23 +90,24 @@ class ClearBrowsingDataManagerTest : public PlatformTest {
   ClearBrowsingDataManager* manager_;
   syncer::TestSyncService* test_sync_service_;
   web::TestWebThreadBundle thread_bundle_;
+  IntegerPrefMember time_range_pref_;
 };
 
 // Tests model is set up with all appropriate items and sections.
 TEST_F(ClearBrowsingDataManagerTest, TestModel) {
   [manager_ loadModel:model_];
 
-  int section_offset = 0;
+  EXPECT_EQ(3, [model_ numberOfSections]);
   if (IsNewClearBrowsingDataUIEnabled()) {
-    EXPECT_EQ(4, [model_ numberOfSections]);
+    // Time Range selector.
     EXPECT_EQ(1, [model_ numberOfItemsInSection:0]);
-    section_offset = 1;
+    EXPECT_EQ(5, [model_ numberOfItemsInSection:1]);
   } else {
-    EXPECT_EQ(3, [model_ numberOfSections]);
+    EXPECT_EQ(5, [model_ numberOfItemsInSection:0]);
+    // CBD button.
+    EXPECT_EQ(1, [model_ numberOfItemsInSection:1]);
   }
-  EXPECT_EQ(5, [model_ numberOfItemsInSection:0 + section_offset]);
-  EXPECT_EQ(1, [model_ numberOfItemsInSection:1 + section_offset]);
-  EXPECT_EQ(1, [model_ numberOfItemsInSection:2 + section_offset]);
+  EXPECT_EQ(1, [model_ numberOfItemsInSection:2]);
 }
 
 // Tests model is set up with correct number of items and sections if signed in
@@ -116,19 +121,18 @@ TEST_F(ClearBrowsingDataManagerTest, TestModelSignedInSyncOff) {
 
   [manager_ loadModel:model_];
 
-  int section_offset = 0;
+  EXPECT_EQ(4, [model_ numberOfSections]);
   if (IsNewClearBrowsingDataUIEnabled()) {
-    EXPECT_EQ(5, [model_ numberOfSections]);
+    // Time Range selector.
     EXPECT_EQ(1, [model_ numberOfItemsInSection:0]);
-    section_offset = 1;
+    EXPECT_EQ(5, [model_ numberOfItemsInSection:1]);
   } else {
-    EXPECT_EQ(4, [model_ numberOfSections]);
+    EXPECT_EQ(5, [model_ numberOfItemsInSection:0]);
+    // CBD button.
+    EXPECT_EQ(1, [model_ numberOfItemsInSection:1]);
   }
-
-  EXPECT_EQ(5, [model_ numberOfItemsInSection:0 + section_offset]);
-  EXPECT_EQ(1, [model_ numberOfItemsInSection:1 + section_offset]);
-  EXPECT_EQ(1, [model_ numberOfItemsInSection:2 + section_offset]);
-  EXPECT_EQ(1, [model_ numberOfItemsInSection:3 + section_offset]);
+  EXPECT_EQ(1, [model_ numberOfItemsInSection:2]);
+  EXPECT_EQ(1, [model_ numberOfItemsInSection:3]);
 }
 
 TEST_F(ClearBrowsingDataManagerTest, TestCacheCounterFormattingForAllTime) {
@@ -198,4 +202,25 @@ TEST_F(ClearBrowsingDataManagerTest,
   }
 }
 
+TEST_F(ClearBrowsingDataManagerTest, TestOnPreferenceChanged) {
+  // Only works with new UI
+  if (!IsNewClearBrowsingDataUIEnabled()) {
+    return;
+  }
+  ASSERT_EQ("en", GetApplicationContext()->GetApplicationLocale());
+  [manager_ loadModel:model_];
+  NSArray* timeRangeItems =
+      [model_ itemsInSectionWithIdentifier:SectionIdentifierTimeRange];
+  ASSERT_EQ(1UL, timeRangeItems.count);
+  TableViewDetailIconItem* timeRangeItem = timeRangeItems.firstObject;
+  ASSERT_TRUE([timeRangeItem isKindOfClass:[TableViewDetailIconItem class]]);
+
+  // Changes of Time Range should trigger updates on Time Range item's
+  // detailText.
+  time_range_pref_.SetValue(2);
+  EXPECT_NSEQ(@"Past Week", timeRangeItem.detailText);
+  time_range_pref_.SetValue(3);
+  EXPECT_NSEQ(@"Last 4 Weeks", timeRangeItem.detailText);
+}
+
 }  // namespace

ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_table_view_controller.mm

@@ -18,9 +18,11 @@
 #import "ios/chrome/browser/ui/elements/chrome_activity_overlay_coordinator.h"
 #include "ios/chrome/browser/ui/settings/cells/clear_browsing_data_constants.h"
 #import "ios/chrome/browser/ui/settings/cells/table_view_clear_browsing_data_item.h"
+#import "ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_consumer.h"
 #include "ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_local_commands.h"
 #import "ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_manager.h"
 #import "ios/chrome/browser/ui/settings/clear_browsing_data/clear_browsing_data_ui_constants.h"
+#import "ios/chrome/browser/ui/settings/clear_browsing_data/time_range_selector_table_view_controller.h"
 #import "ios/chrome/browser/ui/settings/settings_navigation_controller.h"
 #import "ios/chrome/browser/ui/table_view/cells/table_view_cells_constants.h"
 #import "ios/chrome/browser/ui/table_view/cells/table_view_text_button_item.h"
@@ -295,8 +297,7 @@
       case ItemTypeTimeRange: {
         UIViewController* controller =
             [[TimeRangeSelectorTableViewController alloc]
-                initWithPrefs:self.browserState->GetPrefs()
-                     delegate:self.dataManager];
+                initWithPrefs:self.browserState->GetPrefs()];
         [self.tableView deselectRowAtIndexPath:indexPath animated:YES];
         [self.navigationController pushViewController:controller animated:YES];
         break;

ios/chrome/browser/ui/settings/clear_browsing_data/time_range_selector_table_view_controller.h

@@ -5,28 +5,15 @@
 #ifndef IOS_CHROME_BROWSER_UI_SETTINGS_CLEAR_BROWSING_DATA_TIME_RANGE_SELECTOR_TABLE_VIEW_CONTROLLER_H_
 #define IOS_CHROME_BROWSER_UI_SETTINGS_CLEAR_BROWSING_DATA_TIME_RANGE_SELECTOR_TABLE_VIEW_CONTROLLER_H_
 
-#include "components/browsing_data/core/browsing_data_utils.h"
 #import "ios/chrome/browser/ui/settings/settings_root_table_view_controller.h"
 
 class PrefService;
-@class TimeRangeSelectorTableViewController;
-
-@protocol TimeRangeSelectorTableViewControllerDelegate <NSObject>
-
-// Informs the delegate that |timePeriod| was selected.
-- (void)timeRangeSelectorViewController:
-            (TimeRangeSelectorTableViewController*)tableViewController
-                    didSelectTimePeriod:(browsing_data::TimePeriod)timePeriod;
-
-@end
 
 // Table view for time range selection.
 @interface TimeRangeSelectorTableViewController
     : SettingsRootTableViewController
 
-- (instancetype)initWithPrefs:(PrefService*)prefs
-                     delegate:(id<TimeRangeSelectorTableViewControllerDelegate>)
-                                  delegate NS_DESIGNATED_INITIALIZER;
+- (instancetype)initWithPrefs:(PrefService*)prefs NS_DESIGNATED_INITIALIZER;
 
 - (instancetype)initWithTableViewStyle:(UITableViewStyle)style
                            appBarStyle:

ios/chrome/browser/ui/settings/clear_browsing_data/time_range_selector_table_view_controller.mm

@@ -6,12 +6,12 @@
 
 #import "base/mac/foundation_util.h"
 #include "base/stl_util.h"
+#include "components/browsing_data/core/browsing_data_utils.h"
 #include "components/browsing_data/core/pref_names.h"
 #include "components/prefs/pref_member.h"
 #include "components/prefs/pref_service.h"
 #import "ios/chrome/browser/ui/table_view/cells/table_view_detail_text_item.h"
 #import "ios/chrome/browser/ui/table_view/chrome_table_view_styler.h"
-#include "ios/chrome/browser/ui/ui_feature_flags.h"
 #include "ios/chrome/grit/ios_strings.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 
@@ -52,23 +52,17 @@ static_assert(
   IntegerPrefMember _timeRangePref;
 }
 
-@property(nonatomic, weak) id<TimeRangeSelectorTableViewControllerDelegate>
-    delegate;
-
 @end
 
 @implementation TimeRangeSelectorTableViewController
 
 #pragma mark Initialization
 
-- (instancetype)initWithPrefs:(PrefService*)prefs
-                     delegate:(id<TimeRangeSelectorTableViewControllerDelegate>)
-                                  delegate {
+- (instancetype)initWithPrefs:(PrefService*)prefs {
   UITableViewStyle style = UITableViewStylePlain;
   self = [super initWithTableViewStyle:style
                            appBarStyle:ChromeTableViewControllerStyleNoAppBar];
   if (self) {
-    _delegate = delegate;
     self.title = l10n_util::GetNSString(
         IDS_IOS_CLEAR_BROWSING_DATA_TIME_RANGE_SELECTOR_TITLE);
     _timeRangePref.Init(browsing_data::prefs::kDeleteTimePeriod, prefs);
@@ -156,10 +150,6 @@ static_assert(
   DCHECK_LE(timePeriod,
             static_cast<int>(browsing_data::TimePeriod::TIME_PERIOD_LAST));
   [self updatePrefValue:timePeriod];
-  [self.delegate
-      timeRangeSelectorViewController:self
-                  didSelectTimePeriod:static_cast<browsing_data::TimePeriod>(
-                                          timePeriod)];
   [self.tableView deselectRowAtIndexPath:indexPath animated:YES];
 }
 

ios/chrome/browser/ui/settings/clear_browsing_data/time_range_selector_table_view_controller_unittest.mm

@@ -39,17 +39,13 @@ class TimeRangeSelectorTableViewControllerTest
   void SetUp() override {
     ChromeTableViewControllerTest::SetUp();
     pref_service_ = CreateLocalState();
-    delegate_ = [OCMockObject
-        mockForProtocol:@protocol(
-                            TimeRangeSelectorTableViewControllerDelegate)];
     CreateController();
   }
 
   ChromeTableViewController* InstantiateController() override {
     time_range_selector_controller_ =
         [[TimeRangeSelectorTableViewController alloc]
-            initWithPrefs:pref_service_.get()
-                 delegate:delegate_];
+            initWithPrefs:pref_service_.get()];
     return time_range_selector_controller_;
   }
 
@@ -72,7 +68,6 @@ class TimeRangeSelectorTableViewControllerTest
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<PrefService> pref_service_;
-  id delegate_;
   TimeRangeSelectorTableViewController* time_range_selector_controller_;
 };
 
@@ -118,23 +113,4 @@ TEST_F(TimeRangeSelectorTableViewControllerTest, TestUpdateCheckedState) {
   }
 }
 
-TEST_F(TimeRangeSelectorTableViewControllerTest, TestUpdatePrefValue) {
-  CheckController();
-  UITableView* tableView = time_range_selector_controller_.tableView;
-  for (NSInteger checkedItem = 0; checkedItem < kNumberOfItems; ++checkedItem) {
-    NSIndexPath* indexPath = [NSIndexPath indexPathForRow:checkedItem
-                                                inSection:0];
-    [[delegate_ expect]
-        timeRangeSelectorViewController:time_range_selector_controller_
-                    didSelectTimePeriod:static_cast<browsing_data::TimePeriod>(
-                                            checkedItem)];
-    [time_range_selector_controller_ tableView:tableView
-                       didSelectRowAtIndexPath:indexPath];
-    EXPECT_EQ(
-        pref_service_->GetInteger(browsing_data::prefs::kDeleteTimePeriod),
-        checkedItem);
-    EXPECT_OCMOCK_VERIFY(delegate_);
-  }
-}
-
 }  // namespace