[Notification scheduler]: Support custom suppression duration.

- Clients are able to specify a custom suppression duration for
 each notification entry overwriting the default one in config.
-TODO: Proto change.

Bug: 1005954
Change-Id: Idcdf018f9f32dc5b468aba9782b93d0c6a9df5ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1815217
Auto-Submit: Hesen Zhang <hesen@chromium.org>
Commit-Queue: Hesen Zhang <hesen@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701738}

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

@@ -61,7 +61,8 @@ void ImpressionHistoryTrackerImpl::AddImpression(
     SchedulerClientType type,
     const std::string& guid,
     const Impression::ImpressionResultMap& impression_mapping,
-    const Impression::CustomData& custom_data) {
+    const Impression::CustomData& custom_data,
+    const base::Optional<base::TimeDelta>& custom_suppression_duration) {
   DCHECK(initialized_);
   auto it = client_states_.find(type);
   if (it == client_states_.end())
@@ -70,6 +71,7 @@ void ImpressionHistoryTrackerImpl::AddImpression(
   Impression impression(type, guid, clock_->Now());
   impression.impression_mapping = impression_mapping;
   impression.custom_data = custom_data;
+  impression.custom_suppression_duration = custom_suppression_duration;
   it->second->impressions.emplace_back(std::move(impression));
 
   impression_map_.emplace(guid, &it->second->impressions.back());
@@ -373,7 +375,10 @@ void ImpressionHistoryTrackerImpl::ApplyNegativeImpression(
 
   // Suppress the notification, the user will not see this type of notification
   // for a while.
-  SuppressionInfo supression_info(clock_->Now(), config_.suppression_duration);
+  SuppressionInfo supression_info(
+      clock_->Now(), impression->custom_suppression_duration.has_value()
+                         ? impression->custom_suppression_duration.value()
+                         : config_.suppression_duration);
   client_state->suppression_info = std::move(supression_info);
   client_state->current_max_daily_show = 0;
   stats::LogImpressionEvent(stats::ImpressionEvent::kNewSuppression);

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

@@ -54,7 +54,8 @@ class ImpressionHistoryTracker : public UserActionHandler {
       SchedulerClientType type,
       const std::string& guid,
       const Impression::ImpressionResultMap& impression_map,
-      const Impression::CustomData& custom_data) = 0;
+      const Impression::CustomData& custom_data,
+      const base::Optional<base::TimeDelta>& custom_suppression_duration) = 0;
 
   // Analyzes the impression history for all notification clients, and adjusts
   // the |current_max_daily_show|.
@@ -99,7 +100,9 @@ class ImpressionHistoryTrackerImpl : public ImpressionHistoryTracker {
   void AddImpression(SchedulerClientType type,
                      const std::string& guid,
                      const Impression::ImpressionResultMap& impression_mapping,
-                     const Impression::CustomData& custom_data) override;
+                     const Impression::CustomData& custom_data,
+                     const base::Optional<base::TimeDelta>&
+                         custom_suppression_duration) override;
   void AnalyzeImpressionHistory() override;
   void GetClientStates(std::map<SchedulerClientType, const ClientState*>*
                            client_states) const override;

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

@@ -232,9 +232,9 @@ TEST_F(ImpressionHistoryTrackerTest, AddImpression) {
   InitTrackerWithData(test_case);
 
   // No-op for unregistered client.
-  tracker()->AddImpression(SchedulerClientType::kTest2, kGuid2,
-                           Impression::ImpressionResultMap(),
-                           Impression::CustomData());
+  tracker()->AddImpression(
+      SchedulerClientType::kTest2, kGuid2, Impression::ImpressionResultMap(),
+      Impression::CustomData(), base::nullopt /*custom_suppression_duration*/);
   VerifyClientStates(test_case);
 
   clock()->SetNow(kTimeStr);
@@ -242,14 +242,17 @@ TEST_F(ImpressionHistoryTrackerTest, AddImpression) {
   Impression::ImpressionResultMap impression_mapping = {
       {UserFeedback::kDismiss, ImpressionResult::kNegative}};
   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);
+                           impression_mapping, custom_data,
+                           custom_suppression_duration);
   Impression expected_impression(SchedulerClientType::kTest1, kGuid1,
                                  clock()->Now());
   expected_impression.impression_mapping = impression_mapping;
   expected_impression.custom_data = custom_data;
+  expected_impression.custom_suppression_duration = custom_suppression_duration;
   test_case.expected.back().impressions.emplace_back(expected_impression);
   VerifyClientStates(test_case);
   EXPECT_EQ(*tracker()->GetImpression(kGuid1), expected_impression);
@@ -320,6 +323,7 @@ struct UserActionTestParam {
   base::Optional<ActionButtonType> button_type;
   bool integrated = false;
   bool has_suppression = false;
+  base::Optional<base::TimeDelta> custom_suppression_duration;
   std::map<UserFeedback, ImpressionResult> impression_mapping;
 };
 
@@ -335,27 +339,43 @@ class ImpressionHistoryTrackerUserActionTest
 };
 
 const UserActionTestParam kUserActionTestParams[] = {
-    // Click.
+    // Suite 0: Click.
     {ImpressionResult::kPositive, UserFeedback::kClick, 3, base::nullopt,
      true /*integrated*/, false /*has_suppression*/},
-    // Helpful button.
+
+    // Suite 1: Helpful button.
     {ImpressionResult::kPositive, UserFeedback::kHelpful, 3,
      ActionButtonType::kHelpful, true /*integrated*/,
      false /*has_suppression*/},
-    // Unhelpful button.
+
+    // Suite 2: Unhelpful button.
     {ImpressionResult::kNegative, UserFeedback::kNotHelpful, 0,
      ActionButtonType::kUnhelpful, true /*integrated*/,
      true /*has_suppression*/},
-    // One dismiss.
+
+    // Suite 3: One dismiss.
     {ImpressionResult::kInvalid, UserFeedback::kDismiss, 2, base::nullopt,
      false /*integrated*/, false /*has_suppression*/},
-    // Click with negative impression result from impression mapping.
+
+    // Suite 4: Click with negative impression result from impression mapping.
     {ImpressionResult::kNegative,
      UserFeedback::kClick,
      0,
      base::nullopt,
      true /*integrated*/,
      true /*has_suppression*/,
+     base::nullopt /*custom_suppression_duration*/,
+     {{UserFeedback::kClick,
+       ImpressionResult::kNegative}} /*impression_mapping*/},
+
+    // Suite 5: Click with negative impression result from impression mapping.
+    {ImpressionResult::kNegative,
+     UserFeedback::kClick,
+     0,
+     base::nullopt,
+     true /*integrated*/,
+     true /*has_suppression*/,
+     base::TimeDelta::FromDays(2) /*custom_suppression_duration*/,
      {{UserFeedback::kClick,
        ImpressionResult::kNegative}} /*impression_mapping*/}};
 
@@ -366,18 +386,22 @@ TEST_P(ImpressionHistoryTrackerUserActionTest, UserAction) {
   Impression impression = CreateImpression(base::Time::Now(), kGuid1);
   DCHECK(!test_case.input.empty());
   impression.impression_mapping = GetParam().impression_mapping;
+  impression.custom_suppression_duration =
+      GetParam().custom_suppression_duration;
   test_case.input.front().impressions.emplace_back(impression);
 
   impression.impression = GetParam().impression_result;
   impression.integrated = GetParam().integrated;
   impression.feedback = GetParam().user_feedback;
-
   test_case.expected.front().current_max_daily_show =
       GetParam().current_max_daily_show;
   test_case.expected.front().impressions.emplace_back(impression);
   if (GetParam().has_suppression) {
     test_case.expected.front().suppression_info =
-        SuppressionInfo(base::Time::UnixEpoch(), config().suppression_duration);
+        SuppressionInfo(base::Time::UnixEpoch(),
+                        GetParam().custom_suppression_duration.has_value()
+                            ? GetParam().custom_suppression_duration.value()
+                            : config().suppression_duration);
   }
 
   CreateTracker(test_case);

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

@@ -69,6 +69,10 @@ struct Impression {
   // Custom data associated with a notification. Send back to the client when
   // the user interacts with the notification.
   CustomData custom_data;
+
+  // Custom suppresion duration in days. It will override |suppression_duration|
+  // in config.
+  base::Optional<base::TimeDelta> custom_suppression_duration;
 };
 
 // Contains details about supression and recovery after suppression expired.

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

@@ -39,19 +39,16 @@ class NotificationSchedulerImpl;
 class InitHelper {
  public:
   using InitCallback = base::OnceCallback<void(bool)>;
-  InitHelper()
-      : context_(nullptr),
-        impression_tracker_delegate_(nullptr) {}
+  InitHelper() : context_(nullptr), impression_tracker_delegate_(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,
+            ImpressionHistoryTracker::Delegate* impression_tracker_delegate,
+            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.
@@ -162,7 +159,8 @@ class DisplayHelper {
     // Tracks user impression on the notification to be shown.
     context_->impression_tracker()->AddImpression(
         entry->type, entry->guid, entry->schedule_params.impression_mapping,
-        updated_notification_data->custom_data);
+        updated_notification_data->custom_data,
+        entry->schedule_params.custom_suppression_duration);
 
     stats::LogNotificationShow(*updated_notification_data, entry->type);
 

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

@@ -6,6 +6,7 @@
 
 #include <map>
 #include <memory>
+#include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ -149,7 +150,6 @@ class NotificationSchedulerTest : public testing::Test {
   test::MockDisplayAgent* display_agent_;
   test::MockDisplayDecider* display_decider_;
 
-
   std::unique_ptr<NotificationScheduler> notification_scheduler_;
   DISALLOW_COPY_AND_ASSIGN(NotificationSchedulerTest);
 };
@@ -314,7 +314,7 @@ TEST_F(NotificationSchedulerTest, BackgroundTaskStartShowNotification) {
       .WillOnce(SetArgPointee<2>(result));
 
   EXPECT_CALL(*task_coordinator(), ScheduleBackgroundTask(_, _));
-  EXPECT_CALL(*impression_tracker(), AddImpression(_, _, _, _));
+  EXPECT_CALL(*impression_tracker(), AddImpression(_, _, _, _, _));
   EXPECT_CALL(*notification_manager(), DisplayNotification(_, _))
       .WillOnce(
           Invoke([&](const std::string& guid,
@@ -344,7 +344,7 @@ TEST_F(NotificationSchedulerTest, BackgroundTaskStartNoEntry) {
       .WillOnce(SetArgPointee<2>(result));
 
   EXPECT_CALL(*task_coordinator(), ScheduleBackgroundTask(_, _));
-  EXPECT_CALL(*impression_tracker(), AddImpression(_, _, _, _)).Times(0);
+  EXPECT_CALL(*impression_tracker(), AddImpression(_, _, _, _, _)).Times(0);
   EXPECT_CALL(*display_agent(), ShowNotification(_, _)).Times(0);
   EXPECT_CALL(*client(), BeforeShowNotification(_, _)).Times(0);
   EXPECT_CALL(*notification_manager(), DisplayNotification(_, _))
@@ -371,7 +371,7 @@ TEST_F(NotificationSchedulerTest, BackgroundTaskStartNoClient) {
       .WillOnce(SetArgPointee<2>(result));
 
   EXPECT_CALL(*task_coordinator(), ScheduleBackgroundTask(_, _));
-  EXPECT_CALL(*impression_tracker(), AddImpression(_, _, _, _)).Times(0);
+  EXPECT_CALL(*impression_tracker(), AddImpression(_, _, _, _, _)).Times(0);
   EXPECT_CALL(*display_agent(), ShowNotification(_, _)).Times(0);
   EXPECT_CALL(*client(), BeforeShowNotification(_, _)).Times(0);
   EXPECT_CALL(*notification_manager(), DisplayNotification(_, _))
@@ -410,7 +410,7 @@ TEST_F(NotificationSchedulerTest, ClientDropNotification) {
           }));
 
   EXPECT_CALL(*task_coordinator(), ScheduleBackgroundTask(_, _));
-  EXPECT_CALL(*impression_tracker(), AddImpression(_, _, _, _)).Times(0);
+  EXPECT_CALL(*impression_tracker(), AddImpression(_, _, _, _, _)).Times(0);
   EXPECT_CALL(*display_agent(), ShowNotification(_, _)).Times(0);
 
   OnStartTask();

chrome/browser/notifications/scheduler/public/schedule_params.cc

@@ -14,7 +14,8 @@ bool ScheduleParams::operator==(const ScheduleParams& other) const {
   return priority == other.priority &&
          impression_mapping == other.impression_mapping &&
          deliver_time_start == other.deliver_time_start &&
-         deliver_time_end == other.deliver_time_end;
+         deliver_time_end == other.deliver_time_end &&
+         custom_suppression_duration == other.custom_suppression_duration;
 }
 
 ScheduleParams::~ScheduleParams() = default;

chrome/browser/notifications/scheduler/public/schedule_params.h

@@ -45,6 +45,11 @@ struct ScheduleParams {
   // The end time of the deliver time window of the notification. Use in pair
   // with |deliver_time_start|.
   base::Optional<base::Time> deliver_time_end;
+
+  // Support a custom suppression duration(in days) for the notification.
+  // If client sets this field, it will override |suppression_duration| in
+  // config.
+  base::Optional<base::TimeDelta> custom_suppression_duration;
 };
 
 }  // namespace notifications

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

@@ -20,11 +20,12 @@ class MockImpressionHistoryTracker : public ImpressionHistoryTracker {
   ~MockImpressionHistoryTracker() override;
 
   MOCK_METHOD2(Init, void(Delegate*, base::OnceCallback<void(bool)>));
-  MOCK_METHOD4(AddImpression,
+  MOCK_METHOD5(AddImpression,
                void(SchedulerClientType,
                     const std::string&,
                     const Impression::ImpressionResultMap&,
-                    const Impression::CustomData&));
+                    const Impression::CustomData&,
+                    const base::Optional<base::TimeDelta>&));
   MOCK_METHOD0(AnalyzeImpressionHistory, void());
   MOCK_CONST_METHOD1(GetClientStates,
                      void(std::map<SchedulerClientType, const ClientState*>*));

chrome/browser/notifications/scheduler/test/test_utils.cc

@@ -154,7 +154,7 @@ std::string DebugString(const ClientState* client_state) {
 
   if (client_state->suppression_info.has_value()) {
     std::ostringstream stream;
-    stream << "Suppression info, last_trigger_time:"
+    stream << "\n Suppression info, last_trigger_time: "
            << client_state->suppression_info->last_trigger_time << "\n"
            << "duration:" << client_state->suppression_info->duration << "\n"
            << "recover_goal:" << client_state->suppression_info->recover_goal;