Notification scheduler: Use impression mapping proto.

This CL we use the impression mapping data to determine the impression
result. Also did minor refactor on the code and add unit tests.

Bug: 965133
Change-Id: I00695a366156fb99df9cf0622caae0ee351c7b1a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1705517Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680229}

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

@@ -201,8 +201,6 @@ void ImpressionHistoryTrackerImpl::AnalyzeImpressionHistory(
       ++it;
     }
 
-    // TODO(xingliu): Use scheduling params to determine ImpressionResult.
-    // https://crbug.com/968880
     switch (impression->feedback) {
       case UserFeedback::kDismiss:
         dismisses.emplace_back(impression);
@@ -210,8 +208,8 @@ void ImpressionHistoryTrackerImpl::AnalyzeImpressionHistory(
             &dismisses, impression->create_time - config_.dismiss_duration);
 
         // Three consecutive dismisses will result in suppression.
-        ApplyNegativeImpressions(client_state, &dismisses,
-                                 config_.dismiss_count);
+        CheckConsecutiveUserAction(client_state, &dismisses,
+                                   config_.dismiss_count);
         break;
       case UserFeedback::kClick:
         OnClickInternal(impression->guid, false /*update_db*/);
@@ -254,6 +252,75 @@ void ImpressionHistoryTrackerImpl::PruneImpressionByCreateTime(
   }
 }
 
+void ImpressionHistoryTrackerImpl::GenerateImpressionResult(
+    Impression* impression) {
+  DCHECK(impression);
+  auto it = impression->impression_mapping.find(impression->feedback);
+  if (it != impression->impression_mapping.end()) {
+    // Use client defined impression mapping.
+    impression->impression = it->second;
+  } else {
+    // Use default mapping from user feedback to impression result.
+    switch (impression->feedback) {
+      case UserFeedback::kClick:
+      case UserFeedback::kHelpful:
+        impression->impression = ImpressionResult::kPositive;
+        break;
+      case UserFeedback::kDismiss:
+      case UserFeedback::kIgnore:
+        impression->impression = ImpressionResult::kNeutral;
+        break;
+      case UserFeedback::kNotHelpful:
+        impression->impression = ImpressionResult::kNegative;
+        break;
+      case UserFeedback::kNoFeedback:
+        NOTREACHED();
+        break;
+    }
+  }
+}
+
+void ImpressionHistoryTrackerImpl::UpdateThrottling(ClientState* client_state,
+                                                    Impression* impression) {
+  DCHECK(client_state);
+  DCHECK(impression);
+
+  // Affect the notification throttling.
+  switch (impression->impression) {
+    case ImpressionResult::kPositive:
+      ApplyPositiveImpression(client_state, impression);
+      break;
+    case ImpressionResult::kNegative:
+      ApplyNegativeImpression(client_state, impression);
+      break;
+    case ImpressionResult::kNeutral:
+      break;
+    case ImpressionResult::kInvalid:
+      NOTREACHED();
+      break;
+  }
+}
+
+void ImpressionHistoryTrackerImpl::CheckConsecutiveUserAction(
+    ClientState* client_state,
+    std::deque<Impression*>* impressions,
+    size_t num_actions) {
+  if (impressions->size() < num_actions)
+    return;
+
+  // Suppress the notification if the user performed consecutive operations that
+  // generates negative impressions.
+  for (size_t i = 0, size = impressions->size(); i < size; ++i) {
+    Impression* impression = (*impressions)[i];
+    if (impression->integrated)
+      continue;
+
+    impression->integrated = true;
+    SetNeedsUpdate(client_state->type, true);
+    GenerateImpressionResult(impression);
+  }
+}
+
 void ImpressionHistoryTrackerImpl::ApplyPositiveImpression(
     ClientState* client_state,
     Impression* impression) {
@@ -261,9 +328,9 @@ void ImpressionHistoryTrackerImpl::ApplyPositiveImpression(
   if (impression->integrated)
     return;
 
+  DCHECK_EQ(impression->impression, ImpressionResult::kPositive);
   SetNeedsUpdate(client_state->type, true);
   impression->integrated = true;
-  impression->impression = ImpressionResult::kPositive;
 
   // A positive impression directly releases the suppression.
   if (client_state->suppression_info.has_value()) {
@@ -279,37 +346,15 @@ void ImpressionHistoryTrackerImpl::ApplyPositiveImpression(
                          config_.max_daily_shown_per_type);
 }
 
-void ImpressionHistoryTrackerImpl::ApplyNegativeImpressions(
-    ClientState* client_state,
-    std::deque<Impression*>* impressions,
-    size_t num_actions) {
-  if (impressions->size() < num_actions)
-    return;
-
-  // Suppress the notification if the user performed consecutive operations that
-  // generates negative impressions.
-  for (size_t i = 0, size = impressions->size(); i < size; ++i) {
-    if ((*impressions)[i]->integrated)
-      continue;
-
-    (*impressions)[i]->integrated = true;
-
-    // Each user feedback after |num_action| will apply a new negative
-    // impression.
-    if (i + 1 >= num_actions)
-      ApplyNegativeImpression(client_state, (*impressions)[i]);
-  }
-}
-
 void ImpressionHistoryTrackerImpl::ApplyNegativeImpression(
     ClientState* client_state,
     Impression* impression) {
   if (impression->integrated)
     return;
 
+  DCHECK_EQ(impression->impression, ImpressionResult::kNegative);
   SetNeedsUpdate(client_state->type, true);
   impression->integrated = true;
-  impression->impression = ImpressionResult::kNegative;
 
   // Suppress the notification, the user will not see this type of notification
   // for a while.
@@ -403,8 +448,10 @@ void ImpressionHistoryTrackerImpl::OnClickInternal(
   if (it == client_states_.end())
     return;
   ClientState* client_state = it->second.get();
-  ApplyPositiveImpression(client_state, impression);
   impression->feedback = UserFeedback::kClick;
+  GenerateImpressionResult(impression);
+  SetNeedsUpdate(impression->type, true);
+  UpdateThrottling(client_state, impression);
 
   if (update_db && MaybeUpdateDb(client_state->type))
     NotifyImpressionUpdate();
@@ -424,11 +471,9 @@ void ImpressionHistoryTrackerImpl::OnButtonClickInternal(
   ClientState* client_state = it->second.get();
   switch (button_type) {
     case ActionButtonType::kHelpful:
-      ApplyPositiveImpression(client_state, impression);
       impression->feedback = UserFeedback::kHelpful;
       break;
     case ActionButtonType::kUnhelpful:
-      ApplyNegativeImpression(client_state, impression);
       impression->feedback = UserFeedback::kNotHelpful;
       break;
     case ActionButtonType::kUnknownAction:
@@ -436,6 +481,10 @@ void ImpressionHistoryTrackerImpl::OnButtonClickInternal(
       break;
   }
 
+  GenerateImpressionResult(impression);
+  SetNeedsUpdate(impression->type, true);
+  UpdateThrottling(client_state, impression);
+
   if (update_db && MaybeUpdateDb(client_state->type))
     NotifyImpressionUpdate();
 }
@@ -452,9 +501,12 @@ void ImpressionHistoryTrackerImpl::OnDismissInternal(
     return;
   ClientState* client_state = it->second.get();
 
-  AnalyzeImpressionHistory(client_state);
+  impression->feedback = UserFeedback::kDismiss;
+  SetNeedsUpdate(impression->type, true);
 
-  if (update_db && MaybeUpdateDb(client_state->type))
+  // Check consecutive dismisses.
+  AnalyzeImpressionHistory(client_state);
+  if (MaybeUpdateDb(impression->type))
     NotifyImpressionUpdate();
 }
 

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

@@ -118,16 +118,22 @@ class ImpressionHistoryTrackerImpl : public ImpressionHistoryTracker {
   // Analyzes the impression history for a particular client.
   void AnalyzeImpressionHistory(ClientState* client_state);
 
+  // Check consecutive user actions, and generate impression result if no less
+  // than |num_actions| count of user actions.
+  void CheckConsecutiveUserAction(ClientState* client_state,
+                                  std::deque<Impression*>* impressions,
+                                  size_t num_actions);
+
+  // Generates user impression result.
+  void GenerateImpressionResult(Impression* impression);
+
+  // Updates notification throttling based on the impression result.
+  void UpdateThrottling(ClientState* client_state, Impression* impression);
+
   // Applies a positive impression result to this notification type.
   void ApplyPositiveImpression(ClientState* client_state,
                                Impression* impression);
 
-  // Applies negative impression on this notification type when |num_actions|
-  // consecutive negative impression result are generated.
-  void ApplyNegativeImpressions(ClientState* client_state,
-                                std::deque<Impression*>* impressions,
-                                size_t num_actions);
-
   // Applies one negative impression.
   void ApplyNegativeImpression(ClientState* client_state,
                                Impression* impression);

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

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/test/scoped_task_environment.h"
+#include "base/time/time.h"
 #include "chrome/browser/notifications/scheduler/internal/impression_history_tracker.h"
 #include "chrome/browser/notifications/scheduler/test/fake_clock.h"
 #include "chrome/browser/notifications/scheduler/test/test_utils.h"
@@ -23,6 +24,7 @@ namespace notifications {
 namespace {
 
 const char kGuid1[] = "guid1";
+const char kTimeStr[] = "04/25/20 01:00:00 AM";
 
 struct TestCase {
   // Input data that will be pushed to the target class.
@@ -233,7 +235,7 @@ TEST_F(ImpressionHistoryTrackerTest, AddImpression) {
   tracker()->AddImpression(SchedulerClientType::kTest2, kGuid1);
   VerifyClientStates(test_case);
 
-  SetNow("04/25/20 01:00:00 AM");
+  SetNow(kTimeStr);
 
   EXPECT_CALL(*store(), Update(_, _, _));
   EXPECT_CALL(*delegate(), OnImpressionUpdated());
@@ -254,12 +256,15 @@ TEST_F(ImpressionHistoryTrackerTest, ClickNoImpression) {
   VerifyClientStates(test_case);
 }
 
+// Defines the expected state of impression data after certain user action.
 struct UserActionTestParam {
   ImpressionResult impression_result = ImpressionResult::kInvalid;
   UserFeedback user_feedback = UserFeedback::kNoFeedback;
   int current_max_daily_show = 0;
   base::Optional<ActionButtonType> button_type;
-  base::Optional<SuppressionInfo> suppression_info;
+  bool integrated = false;
+  bool has_suppression = false;
+  std::map<UserFeedback, ImpressionResult> impression_mapping;
 };
 
 class ImpressionHistoryTrackerUserActionTest
@@ -273,28 +278,51 @@ class ImpressionHistoryTrackerUserActionTest
   DISALLOW_COPY_AND_ASSIGN(ImpressionHistoryTrackerUserActionTest);
 };
 
-// TODO(xingliu): Add test for unhelpful/dismiss.
 const UserActionTestParam kUserActionTestParams[] = {
+    // Click.
     {ImpressionResult::kPositive, UserFeedback::kClick, 3, base::nullopt,
-     base::nullopt},
+     true /*integrated*/, false /*has_suppression*/},
+    // Helpful button.
     {ImpressionResult::kPositive, UserFeedback::kHelpful, 3,
-     ActionButtonType::kHelpful, base::nullopt}};
+     ActionButtonType::kHelpful, true /*integrated*/,
+     false /*has_suppression*/},
+    // Unhelpful button.
+    {ImpressionResult::kNegative, UserFeedback::kNotHelpful, 0,
+     ActionButtonType::kUnhelpful, true /*integrated*/,
+     true /*has_suppression*/},
+    // One dismiss.
+    {ImpressionResult::kInvalid, UserFeedback::kDismiss, 2, base::nullopt,
+     false /*integrated*/, false /*has_suppression*/},
+    // Click with negative impression result from impression mapping.
+    {ImpressionResult::kNegative,
+     UserFeedback::kClick,
+     0,
+     base::nullopt,
+     true /*integrated*/,
+     true /*has_suppression*/,
+     {{UserFeedback::kClick,
+       ImpressionResult::kNegative}} /*impression_mapping*/}};
 
 // User actions like clicks should update the ClientState data accordingly.
 TEST_P(ImpressionHistoryTrackerUserActionTest, UserAction) {
+  clock()->SetNow(base::Time::UnixEpoch());
   TestCase test_case = CreateDefaultTestCase();
   Impression impression = CreateImpression(base::Time::Now(), kGuid1);
   DCHECK(!test_case.input.empty());
+  impression.impression_mapping = GetParam().impression_mapping;
   test_case.input.front().impressions.emplace_back(impression);
 
   impression.impression = GetParam().impression_result;
-  impression.integrated = true;
+  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);
-  test_case.expected.front().suppression_info = GetParam().suppression_info;
+  if (GetParam().has_suppression) {
+    test_case.expected.front().suppression_info =
+        SuppressionInfo(base::Time::UnixEpoch(), config().suppression_duration);
+  }
 
   CreateTracker(test_case);
   InitTrackerWithData(test_case);
@@ -302,11 +330,14 @@ TEST_P(ImpressionHistoryTrackerUserActionTest, UserAction) {
   EXPECT_CALL(*delegate(), OnImpressionUpdated());
 
   // Trigger user action.
-  if (GetParam().user_feedback == UserFeedback::kClick)
+  if (GetParam().user_feedback == UserFeedback::kClick) {
     tracker()->OnClick(SchedulerClientType::kTest1, kGuid1);
-  else if (GetParam().button_type.has_value())
+  } else if (GetParam().button_type.has_value()) {
     tracker()->OnActionClick(SchedulerClientType::kTest1, kGuid1,
                              GetParam().button_type.value());
+  } else if (GetParam().user_feedback == UserFeedback::kDismiss) {
+    tracker()->OnDismiss(SchedulerClientType::kTest1, kGuid1);
+  }
 
   VerifyClientStates(test_case);
 }