Notification scheduler: Minor refactor DisplayDecider for testing.

This CL moves static input from FindNotificationsToShow to the
constructor.

This can help to improve testing and make it easier to add more
features in the future.

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

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

@@ -25,7 +25,7 @@ class DecisionHelper {
  public:
   DecisionHelper(const SchedulerConfig* config,
                  const std::vector<SchedulerClientType>& clients,
-                 std::unique_ptr<DistributionPolicy> distribution_policy,
+                 DistributionPolicy* distribution_policy,
                  SchedulerTaskTime task_start_time,
                  Notifications notifications,
                  ClientStates client_states)
@@ -34,7 +34,7 @@ class DecisionHelper {
         client_states_(std::move(client_states)),
         config_(config),
         clients_(clients),
-        policy_(std::move(distribution_policy)),
+        policy_(distribution_policy),
         daily_max_to_show_all_types_(0),
         last_shown_type_(SchedulerClientType::kUnknown),
         shown_(0) {}
@@ -146,7 +146,7 @@ class DecisionHelper {
   const ClientStates client_states_;
   const SchedulerConfig* config_;
   const std::vector<SchedulerClientType> clients_;
-  std::unique_ptr<DistributionPolicy> policy_;
+  DistributionPolicy* policy_;
   int daily_max_to_show_all_types_;
 
   SchedulerClientType last_shown_type_;
@@ -158,33 +158,43 @@ class DecisionHelper {
 
 class DisplayDeciderImpl : public DisplayDecider {
  public:
-  DisplayDeciderImpl() = default;
+  DisplayDeciderImpl(const SchedulerConfig* config,
+                     std::vector<SchedulerClientType> clients,
+                     std::unique_ptr<DistributionPolicy> distribution_policy)
+      : config_(config),
+        clients_(std::move(clients)),
+        distribution_policy_(std::move(distribution_policy)) {}
   ~DisplayDeciderImpl() override = default;
 
  private:
   // DisplayDecider implementation.
   void FindNotificationsToShow(
-      const SchedulerConfig* config,
-      std::vector<SchedulerClientType> clients,
-      std::unique_ptr<DistributionPolicy> distribution_policy,
       SchedulerTaskTime task_start_time,
       Notifications notifications,
       ClientStates client_states,
       Results* results) override {
     auto helper = std::make_unique<DecisionHelper>(
-        config, std::move(clients), std::move(distribution_policy),
-        task_start_time, std::move(notifications), std::move(client_states));
+        config_, clients_, distribution_policy_.get(), task_start_time,
+        std::move(notifications), std::move(client_states));
     helper->DecideNotificationToShow(results);
   }
 
+  const SchedulerConfig* config_;
+  const std::vector<SchedulerClientType> clients_;
+  std::unique_ptr<DistributionPolicy> distribution_policy_;
+
   DISALLOW_COPY_AND_ASSIGN(DisplayDeciderImpl);
 };
 
 }  // namespace
 
 // static
-std::unique_ptr<DisplayDecider> DisplayDecider::Create() {
-  return std::make_unique<DisplayDeciderImpl>();
+std::unique_ptr<DisplayDecider> DisplayDecider::Create(
+    const SchedulerConfig* config,
+    std::vector<SchedulerClientType> clients,
+    std::unique_ptr<DistributionPolicy> distribution_policy) {
+  return std::make_unique<DisplayDeciderImpl>(config, std::move(clients),
+                                              std::move(distribution_policy));
 }
 
 }  // namespace notifications

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

@@ -34,16 +34,16 @@ class DisplayDecider {
   using Results = std::set<std::string>;
 
   // Creates the decider to determine notifications to show.
-  static std::unique_ptr<DisplayDecider> Create();
+  static std::unique_ptr<DisplayDecider> Create(
+      const SchedulerConfig* config,
+      std::vector<SchedulerClientType> clients,
+      std::unique_ptr<DistributionPolicy> distribution_policy);
 
   DisplayDecider() = default;
   virtual ~DisplayDecider() = default;
 
   // Finds notifications to show. Returns a list of notification guids.
   virtual void FindNotificationsToShow(
-      const SchedulerConfig* config,
-      std::vector<SchedulerClientType> clients,
-      std::unique_ptr<DistributionPolicy> distribution_policy,
       SchedulerTaskTime task_start_time,
       Notifications notifications,
       ClientStates client_states,

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

@@ -100,9 +100,9 @@ class DisplayDeciderTest : public testing::Test {
     }
 
     // Copy test inputs into |decider_|.
-    decider_ = DisplayDecider::Create();
+    decider_ =
+        DisplayDecider::Create(&config_, clients, DistributionPolicy::Create());
     decider_->FindNotificationsToShow(
-        &config_, std::move(clients), DistributionPolicy::Create(),
         test_data_.task_start_time, std::move(notifications),
         std::move(client_states), &results_);
 

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

@@ -16,7 +16,6 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/notifications/scheduler/internal/background_task_coordinator.h"
 #include "chrome/browser/notifications/scheduler/internal/display_decider.h"
-#include "chrome/browser/notifications/scheduler/internal/distribution_policy.h"
 #include "chrome/browser/notifications/scheduler/internal/impression_history_tracker.h"
 #include "chrome/browser/notifications/scheduler/internal/notification_entry.h"
 #include "chrome/browser/notifications/scheduler/internal/notification_scheduler_context.h"
@@ -251,7 +250,6 @@ class NotificationSchedulerImpl : public NotificationScheduler,
     context_->client_registrar()->GetRegisteredClients(&clients);
 
     context_->display_decider()->FindNotificationsToShow(
-        context_->config(), std::move(clients), DistributionPolicy::Create(),
         task_start_time, std::move(notifications), std::move(client_states),
         &results);
 

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

@@ -255,8 +255,8 @@ TEST_F(NotificationSchedulerTest, BackgroundTaskStartShowNothing) {
 
   // No notification picked to show.
   DisplayDecider::Results result;
-  EXPECT_CALL(*display_decider(), FindNotificationsToShow(_, _, _, _, _, _, _))
-      .WillOnce(SetArgPointee<6>(result));
+  EXPECT_CALL(*display_decider(), FindNotificationsToShow(_, _, _, _))
+      .WillOnce(SetArgPointee<3>(result));
 
   EXPECT_CALL(*display_agent(), ShowNotification(_, _)).Times(0);
   EXPECT_CALL(*notification_manager(), DisplayNotification(_)).Times(0);
@@ -289,8 +289,8 @@ TEST_F(NotificationSchedulerTest, BackgroundTaskStartShowNotification) {
       ShowNotification(NotifcationDataEq(kTitle),
                        SystemDataEq(SchedulerClientType::kTest1, kGuid)));
   DisplayDecider::Results result({kGuid});
-  EXPECT_CALL(*display_decider(), FindNotificationsToShow(_, _, _, _, _, _, _))
-      .WillOnce(SetArgPointee<6>(result));
+  EXPECT_CALL(*display_decider(), FindNotificationsToShow(_, _, _, _))
+      .WillOnce(SetArgPointee<3>(result));
   EXPECT_CALL(*impression_tracker(), AddImpression(_, _));
 
   EXPECT_CALL(*client(), BeforeShowNotification(_, _))

chrome/browser/notifications/scheduler/schedule_service_factory_helper.cc

@@ -12,6 +12,7 @@
 #include "base/time/default_clock.h"
 #include "chrome/browser/notifications/scheduler/internal/background_task_coordinator.h"
 #include "chrome/browser/notifications/scheduler/internal/display_decider.h"
+#include "chrome/browser/notifications/scheduler/internal/distribution_policy.h"
 #include "chrome/browser/notifications/scheduler/internal/icon_store.h"
 #include "chrome/browser/notifications/scheduler/internal/impression_history_tracker.h"
 #include "chrome/browser/notifications/scheduler/internal/impression_store.h"
@@ -96,10 +97,12 @@ KeyedService* CreateNotificationScheduleService(
                           config->background_task_random_time_window),
       base::DefaultClock::GetInstance());
 
+  auto display_decider = DisplayDecider::Create(
+      config.get(), registered_clients, DistributionPolicy::Create());
   auto context = std::make_unique<NotificationSchedulerContext>(
       std::move(client_registrar), std::move(background_task_coordinator),
       std::move(impression_tracker), std::move(notification_manager),
-      std::move(display_agent), DisplayDecider::Create(), std::move(config));
+      std::move(display_agent), std::move(display_decider), std::move(config));
 
   auto scheduler = NotificationScheduler::Create(std::move(context));
   auto init_aware_scheduler =

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

@@ -16,14 +16,8 @@ class MockDisplayDecider : public DisplayDecider {
  public:
   MockDisplayDecider();
   ~MockDisplayDecider() override;
-  MOCK_METHOD7(FindNotificationsToShow,
-               void(const SchedulerConfig*,
-                    std::vector<SchedulerClientType>,
-                    std::unique_ptr<DistributionPolicy>,
-                    SchedulerTaskTime,
-                    Notifications,
-                    ClientStates,
-                    Results*));
+  MOCK_METHOD4(FindNotificationsToShow,
+               void(SchedulerTaskTime, Notifications, ClientStates, Results*));
 };
 
 }  // namespace test