Notification scheduler: Remove ImpressionHistoryTracker::Delegate.

This CL removed the unused delegate class for ImpressionHistoryTracker.

Bug: 965133
Change-Id: I0463a99f4dc7344c66533b4881fec5fe8fa6393a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1832711Reviewed-by: Hesen Zhang <hesen@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702481}

chrome/browser/notifications/scheduler/internal/impression_history_tracker.cc

@@ -43,15 +43,11 @@ ImpressionHistoryTrackerImpl::ImpressionHistoryTrackerImpl(
       config_(config),
       registered_clients_(std::move(registered_clients)),
       initialized_(false),
-      delegate_(nullptr),
       clock_(clock) {}
 
 ImpressionHistoryTrackerImpl::~ImpressionHistoryTrackerImpl() = default;
 
-void ImpressionHistoryTrackerImpl::Init(Delegate* delegate,
-                                        InitCallback callback) {
-  DCHECK(!delegate_ && delegate && !initialized_);
-  delegate_ = delegate;
+void ImpressionHistoryTrackerImpl::Init(InitCallback callback) {
   store_->InitAndLoad(
       base::BindOnce(&ImpressionHistoryTrackerImpl::OnStoreInitialized,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
@@ -77,15 +73,13 @@ void ImpressionHistoryTrackerImpl::AddImpression(
   impression_map_.emplace(guid, &it->second->impressions.back());
   SetNeedsUpdate(type, true /*needs_update*/);
   MaybeUpdateDb(type);
-  NotifyImpressionUpdate();
 }
 
 void ImpressionHistoryTrackerImpl::AnalyzeImpressionHistory() {
   DCHECK(initialized_);
   for (auto& client_state : client_states_)
     AnalyzeImpressionHistory(client_state.second.get());
-  if (MaybeUpdateAllDb())
-    NotifyImpressionUpdate();
+  MaybeUpdateAllDb();
 }
 
 void ImpressionHistoryTrackerImpl::GetClientStates(
@@ -443,11 +437,6 @@ bool ImpressionHistoryTrackerImpl::NeedsUpdate(SchedulerClientType type) const {
   return it == need_update_db_.end() ? false : it->second;
 }
 
-void ImpressionHistoryTrackerImpl::NotifyImpressionUpdate() {
-  if (delegate_)
-    delegate_->OnImpressionUpdated();
-}
-
 Impression* ImpressionHistoryTrackerImpl::FindImpressionNeedsUpdate(
     const std::string& notification_guid) {
   auto it = impression_map_.find(notification_guid);
@@ -477,8 +466,8 @@ void ImpressionHistoryTrackerImpl::OnClickInternal(
   SetNeedsUpdate(impression->type, true);
   UpdateThrottling(client_state, impression);
 
-  if (update_db && MaybeUpdateDb(client_state->type))
-    NotifyImpressionUpdate();
+  if (update_db)
+    MaybeUpdateDb(client_state->type);
 }
 
 void ImpressionHistoryTrackerImpl::OnButtonClickInternal(
@@ -509,8 +498,8 @@ void ImpressionHistoryTrackerImpl::OnButtonClickInternal(
   SetNeedsUpdate(impression->type, true);
   UpdateThrottling(client_state, impression);
 
-  if (update_db && MaybeUpdateDb(client_state->type))
-    NotifyImpressionUpdate();
+  if (update_db)
+    MaybeUpdateDb(client_state->type);
 }
 
 void ImpressionHistoryTrackerImpl::OnDismissInternal(
@@ -530,8 +519,7 @@ void ImpressionHistoryTrackerImpl::OnDismissInternal(
 
   // Check consecutive dismisses.
   AnalyzeImpressionHistory(client_state);
-  if (MaybeUpdateDb(impression->type))
-    NotifyImpressionUpdate();
+  MaybeUpdateDb(impression->type);
 }
 
 }  // namespace notifications

chrome/browser/notifications/scheduler/internal/impression_history_tracker.h

@@ -32,23 +32,8 @@ class ImpressionHistoryTracker : public UserActionHandler {
       std::map<SchedulerClientType, std::unique_ptr<ClientState>>;
   using InitCallback = base::OnceCallback<void(bool)>;
 
-  // TODO(xingliu): Delete this.
-  class Delegate {
-   public:
-    Delegate() = default;
-    virtual ~Delegate() = default;
-
-    // Called when the impression data is updated.
-    // TODO(xingliu): Rename this, only need to call this when the background
-    // task needs to reschedule to another time.
-    virtual void OnImpressionUpdated() = 0;
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(Delegate);
-  };
-
   // Initializes the impression tracker.
-  virtual void Init(Delegate* delegate, InitCallback callback) = 0;
+  virtual void Init(InitCallback callback) = 0;
 
   // Add a new impression, called after the notification is shown.
   virtual void AddImpression(
@@ -97,7 +82,7 @@ class ImpressionHistoryTrackerImpl : public ImpressionHistoryTracker {
 
  private:
   // ImpressionHistoryTracker implementation.
-  void Init(Delegate* delegate, InitCallback callback) override;
+  void Init(InitCallback callback) override;
   void AddImpression(SchedulerClientType type,
                      const std::string& guid,
                      const Impression::ImpressionResultMap& impression_mapping,
@@ -164,9 +149,6 @@ class ImpressionHistoryTrackerImpl : public ImpressionHistoryTracker {
   void SetNeedsUpdate(SchedulerClientType type, bool needs_update);
   bool NeedsUpdate(SchedulerClientType type) const;
 
-  // Notifies the delegate about impression data update.
-  void NotifyImpressionUpdate();
-
   // Finds an impression that needs to update based on notification id.
   Impression* FindImpressionNeedsUpdate(const std::string& notification_guid);
 
@@ -198,8 +180,6 @@ class ImpressionHistoryTrackerImpl : public ImpressionHistoryTracker {
   // If the database needs an update when any of the impression data is updated.
   std::map<SchedulerClientType, bool> need_update_db_;
 
-  Delegate* delegate_;
-
   // The clock to provide the current timestamp.
   base::Clock* clock_;
 

chrome/browser/notifications/scheduler/internal/impression_history_tracker_unittest.cc

@@ -84,20 +84,10 @@ class MockImpressionStore : public CollectionStore<ClientState> {
   DISALLOW_COPY_AND_ASSIGN(MockImpressionStore);
 };
 
-class MockDelegate : public ImpressionHistoryTracker::Delegate {
- public:
-  MockDelegate() = default;
-  ~MockDelegate() = default;
-  MOCK_METHOD0(OnImpressionUpdated, void());
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockDelegate);
-};
-
 // TODO(xingliu): Add more test cases following the test doc.
 class ImpressionHistoryTrackerTest : public ::testing::Test {
  public:
-  ImpressionHistoryTrackerTest() : store_(nullptr), delegate_(nullptr) {}
+  ImpressionHistoryTrackerTest() : store_(nullptr) {}
   ~ImpressionHistoryTrackerTest() override = default;
 
   void SetUp() override {
@@ -111,7 +101,6 @@ class ImpressionHistoryTrackerTest : public ::testing::Test {
   void CreateTracker(const TestCase& test_case) {
     auto store = std::make_unique<MockImpressionStore>();
     store_ = store.get();
-    delegate_ = std::make_unique<MockDelegate>();
 
     impression_trakcer_ = std::make_unique<ImpressionHistoryTrackerImpl>(
         config_, test_case.registered_clients, std::move(store), &clock_);
@@ -128,13 +117,12 @@ class ImpressionHistoryTrackerTest : public ::testing::Test {
               std::move(cb).Run(true, std::move(entries));
             }));
     base::RunLoop loop;
-    impression_trakcer_->Init(
-        delegate_.get(), base::BindOnce(
-                             [](base::RepeatingClosure closure, bool success) {
-                               EXPECT_TRUE(success);
-                               std::move(closure).Run();
-                             },
-                             loop.QuitClosure()));
+    impression_trakcer_->Init(base::BindOnce(
+        [](base::RepeatingClosure closure, bool success) {
+          EXPECT_TRUE(success);
+          std::move(closure).Run();
+        },
+        loop.QuitClosure()));
     loop.Run();
   }
 
@@ -161,7 +149,6 @@ class ImpressionHistoryTrackerTest : public ::testing::Test {
 
   const SchedulerConfig& config() const { return config_; }
   MockImpressionStore* store() { return store_; }
-  MockDelegate* delegate() { return delegate_.get(); }
   ImpressionHistoryTracker* tracker() { return impression_trakcer_.get(); }
   test::FakeClock* clock() { return &clock_; }
 
@@ -171,7 +158,6 @@ class ImpressionHistoryTrackerTest : public ::testing::Test {
   SchedulerConfig config_;
   std::unique_ptr<ImpressionHistoryTracker> impression_trakcer_;
   MockImpressionStore* store_;
-  std::unique_ptr<MockDelegate> delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(ImpressionHistoryTrackerTest);
 };
@@ -186,7 +172,6 @@ TEST_F(ImpressionHistoryTrackerTest, NewReigstedClient) {
 
   CreateTracker(test_case);
   EXPECT_CALL(*store(), Add(_, _, _));
-  EXPECT_CALL(*delegate(), OnImpressionUpdated()).Times(0);
   InitTrackerWithData(test_case);
   VerifyClientStates(test_case);
 }
@@ -199,7 +184,6 @@ TEST_F(ImpressionHistoryTrackerTest, DeprecateClient) {
 
   CreateTracker(test_case);
   EXPECT_CALL(*store(), Delete(_, _));
-  EXPECT_CALL(*delegate(), OnImpressionUpdated()).Times(0);
   InitTrackerWithData(test_case);
   VerifyClientStates(test_case);
 }
@@ -221,7 +205,6 @@ TEST_F(ImpressionHistoryTrackerTest, DeleteExpiredImpression) {
   CreateTracker(test_case);
   EXPECT_CALL(*store(), Update(_, _, _));
   InitTrackerWithData(test_case);
-  EXPECT_CALL(*delegate(), OnImpressionUpdated()).Times(0);
   VerifyClientStates(test_case);
 }
 
@@ -244,7 +227,6 @@ TEST_F(ImpressionHistoryTrackerTest, AddImpression) {
   Impression::CustomData custom_data = {{"url", "https://www.example.com"}};
   auto custom_suppression_duration = base::TimeDelta::FromDays(56);
   EXPECT_CALL(*store(), Update(_, _, _));
-  EXPECT_CALL(*delegate(), OnImpressionUpdated());
   tracker()->AddImpression(SchedulerClientType::kTest1, kGuid1,
                            impression_mapping, custom_data,
                            custom_suppression_duration);
@@ -274,7 +256,6 @@ TEST_F(ImpressionHistoryTrackerTest, ClickNoImpression) {
   CreateTracker(test_case);
   InitTrackerWithData(test_case);
   EXPECT_CALL(*store(), Update(_, _, _)).Times(0);
-  EXPECT_CALL(*delegate(), OnImpressionUpdated()).Times(0);
   UserActionData action_data(SchedulerClientType::kTest1,
                              UserActionType::kClick, kGuid1);
   tracker()->OnUserAction(action_data);
@@ -307,7 +288,6 @@ TEST_F(ImpressionHistoryTrackerTest, ConsecutiveDismisses) {
 
   CreateTracker(test_case);
   InitTrackerWithData(test_case);
-  EXPECT_CALL(*delegate(), OnImpressionUpdated());
   EXPECT_CALL(*store(), Update(_, _, _));
   UserActionData action_data(SchedulerClientType::kTest1,
                              UserActionType::kDismiss, "guid2");
@@ -407,7 +387,6 @@ TEST_P(ImpressionHistoryTrackerUserActionTest, UserAction) {
   CreateTracker(test_case);
   InitTrackerWithData(test_case);
   EXPECT_CALL(*store(), Update(_, _, _));
-  EXPECT_CALL(*delegate(), OnImpressionUpdated());
 
   // Trigger user action.
   if (GetParam().user_feedback == UserFeedback::kClick) {

chrome/browser/notifications/scheduler/internal/notification_scheduler.cc

@@ -39,25 +39,21 @@ class NotificationSchedulerImpl;
 class InitHelper {
  public:
   using InitCallback = base::OnceCallback<void(bool)>;
-  InitHelper() : context_(nullptr), impression_tracker_delegate_(nullptr) {}
+  InitHelper() : context_(nullptr) {}
 
   ~InitHelper() = default;
 
   // Initializes subsystems in notification scheduler, |callback| will be
   // invoked if all initializations finished or anyone of them failed. The
   // object should be destroyed along with the |callback|.
-  void Init(NotificationSchedulerContext* context,
-            ImpressionHistoryTracker::Delegate* impression_tracker_delegate,
-            InitCallback callback) {
+  void Init(NotificationSchedulerContext* context, InitCallback callback) {
     // TODO(xingliu): Initialize the databases in parallel, we currently
     // initialize one by one to work around a shared db issue. See
     // https://crbug.com/978680.
     context_ = context;
-    impression_tracker_delegate_ = impression_tracker_delegate;
     callback_ = std::move(callback);
 
     context_->impression_tracker()->Init(
-        impression_tracker_delegate_,
         base::BindOnce(&InitHelper::OnImpressionTrackerInitialized,
                        weak_ptr_factory_.GetWeakPtr()));
   }
@@ -79,7 +75,6 @@ class InitHelper {
   }
 
   NotificationSchedulerContext* context_;
-  ImpressionHistoryTracker::Delegate* impression_tracker_delegate_;
   InitCallback callback_;
 
   base::WeakPtrFactory<InitHelper> weak_ptr_factory_{this};
@@ -196,8 +191,7 @@ class DisplayHelper {
 };
 
 // Implementation of NotificationScheduler.
-class NotificationSchedulerImpl : public NotificationScheduler,
-                                  public ImpressionHistoryTracker::Delegate {
+class NotificationSchedulerImpl : public NotificationScheduler {
  public:
   NotificationSchedulerImpl(
       std::unique_ptr<NotificationSchedulerContext> context)
@@ -209,7 +203,7 @@ class NotificationSchedulerImpl : public NotificationScheduler,
   // NotificationScheduler implementation.
   void Init(InitCallback init_callback) override {
     init_helper_ = std::make_unique<InitHelper>();
-    init_helper_->Init(context_.get(), this,
+    init_helper_->Init(context_.get(),
                        base::BindOnce(&NotificationSchedulerImpl::OnInitialized,
                                       weak_ptr_factory_.GetWeakPtr(),
                                       std::move(init_callback)));
@@ -288,12 +282,6 @@ class NotificationSchedulerImpl : public NotificationScheduler,
     ScheduleBackgroundTask();
   }
 
-  // ImpressionHistoryTracker::Delegate implementation.
-  void OnImpressionUpdated() override {
-    // TODO(xingliu): Remove ImpressionHistoryTracker::Delegate, and add a
-    // browser test for user action hooks.
-  }
-
   void FindNotificationToShow(TaskFinishedCallback task_finish_callback) {
     DisplayDecider::Results results;
     ScheduledNotificationManager::Notifications notifications;

chrome/browser/notifications/scheduler/internal/notification_scheduler_unittest.cc

@@ -86,9 +86,8 @@ class NotificationSchedulerTest : public testing::Test {
 
  protected:
   void Init() {
-    EXPECT_CALL(*impression_tracker(), Init(_, _))
-        .WillOnce(Invoke([&](ImpressionHistoryTracker::Delegate* delegate,
-                             ImpressionHistoryTracker::InitCallback callback) {
+    EXPECT_CALL(*impression_tracker(), Init(_))
+        .WillOnce(Invoke([&](ImpressionHistoryTracker::InitCallback callback) {
           std::move(callback).Run(true);
         }));
 
@@ -160,9 +159,8 @@ TEST_F(NotificationSchedulerTest, InitSuccess) {
 
 // Tests the case when impression tracker failed to initialize.
 TEST_F(NotificationSchedulerTest, InitImpressionTrackerFailed) {
-  EXPECT_CALL(*impression_tracker(), Init(_, _))
-      .WillOnce(Invoke([](ImpressionHistoryTracker::Delegate* delegate,
-                          ImpressionHistoryTracker::InitCallback callback) {
+  EXPECT_CALL(*impression_tracker(), Init(_))
+      .WillOnce(Invoke([](ImpressionHistoryTracker::InitCallback callback) {
         // Impression tracker failed to load.
         std::move(callback).Run(false);
       }));
@@ -181,9 +179,8 @@ TEST_F(NotificationSchedulerTest, InitImpressionTrackerFailed) {
 
 // Tests the case when scheduled notification manager failed to initialize.
 TEST_F(NotificationSchedulerTest, InitScheduledNotificationManagerFailed) {
-  EXPECT_CALL(*impression_tracker(), Init(_, _))
-      .WillOnce(Invoke([](ImpressionHistoryTracker::Delegate* delegate,
-                          ImpressionHistoryTracker::InitCallback callback) {
+  EXPECT_CALL(*impression_tracker(), Init(_))
+      .WillOnce(Invoke([](ImpressionHistoryTracker::InitCallback callback) {
         std::move(callback).Run(true);
       }));
 

chrome/browser/notifications/scheduler/test/mock_impression_history_tracker.h

@@ -19,7 +19,7 @@ class MockImpressionHistoryTracker : public ImpressionHistoryTracker {
   MockImpressionHistoryTracker();
   ~MockImpressionHistoryTracker() override;
 
-  MOCK_METHOD2(Init, void(Delegate*, base::OnceCallback<void(bool)>));
+  MOCK_METHOD1(Init, void(ImpressionHistoryTracker::InitCallback));
   MOCK_METHOD5(AddImpression,
                void(SchedulerClientType,
                     const std::string&,