[Notification scheduler]: Fix unsorted NotificationsShownToday.

- The input was assumed sorted, and the implementation was
based on that assumption. Now it is not guaranteed, and this CL
is about to fix it.

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

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

@@ -4,9 +4,7 @@
 
 #include "chrome/browser/notifications/scheduler/internal/scheduler_utils.h"
 
-#include <algorithm>
 #include <utility>
-#include <vector>
 
 #include "base/containers/circular_deque.h"
 #include "base/task/post_task.h"
@@ -16,35 +14,6 @@
 #include "ui/gfx/codec/png_codec.h"
 
 namespace notifications {
-namespace {
-
-using FirstAndLastIters =
-    std::pair<base::circular_deque<Impression>::const_iterator,
-              base::circular_deque<Impression>::const_iterator>;
-
-base::Optional<FirstAndLastIters> FindFirstAndLastNotificationShownToday(
-    const base::circular_deque<Impression>& impressions,
-    const base::Time& now,
-    const base::Time& beginning_of_today) {
-  if (impressions.empty() || impressions.cbegin()->create_time > now ||
-      impressions.crbegin()->create_time < beginning_of_today)
-    return base::nullopt;
-
-  auto last =
-      std::upper_bound(impressions.cbegin(), impressions.cend(), now,
-                       [](const base::Time& lhs, const Impression& rhs) {
-                         return lhs < rhs.create_time;
-                       });
-
-  auto first =
-      std::lower_bound(impressions.cbegin(), last, beginning_of_today,
-                       [](const Impression& lhs, const base::Time& rhs) {
-                         return lhs.create_time < rhs;
-                       });
-  return base::make_optional<FirstAndLastIters>(first, last - 1);
-}
-
-}  // namespace
 
 bool ToLocalHour(int hour,
                  const base::Time& today,
@@ -84,30 +53,24 @@ void NotificationsShownToday(
     int* shown_total,
     SchedulerClientType* last_shown_type,
     base::Clock* clock) {
-  base::Time last_shown_time;
   base::Time now = clock->Now();
   base::Time beginning_of_today;
   bool success = ToLocalHour(0, now, 0, &beginning_of_today);
+  base::Time last_shown_time = beginning_of_today;
   DCHECK(success);
   for (const auto& state : client_states) {
     auto* client_state = state.second;
-
-    auto iter_pair = FindFirstAndLastNotificationShownToday(
-        client_state->impressions, now, beginning_of_today);
-
-    if (!iter_pair)
-      continue;
-
-    if (iter_pair->second != client_state->impressions.cend())
-      DLOG(ERROR) << "Wrong format: time stamped to the future! "
-                  << iter_pair->second->create_time;
-
-    if (iter_pair->second->create_time > last_shown_time) {
-      last_shown_time = iter_pair->second->create_time;
-      *last_shown_type = client_state->type;
+    int count = 0;
+    for (const auto& impression : client_state->impressions) {
+      if (impression.create_time >= beginning_of_today &&
+          impression.create_time <= now) {
+        count++;
+        if (impression.create_time >= last_shown_time) {
+          last_shown_time = impression.create_time;
+          *last_shown_type = client_state->type;
+        }
+      }
     }
-
-    int count = std::distance(iter_pair->first, iter_pair->second) + 1;
     (*shown_per_type)[client_state->type] = count;
     (*shown_total) += count;
   }

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

@@ -5,6 +5,7 @@
 #include "chrome/browser/notifications/scheduler/internal/scheduler_utils.h"
 
 #include <algorithm>
+#include <vector>
 
 #include "base/containers/circular_deque.h"
 #include "base/guid.h"
@@ -31,17 +32,25 @@ class SchedulerUtilsTest : public testing::Test {
     ToLocalHour(0, clock_.Now(), 0, &beginning_of_today_);
   }
 
-  void CreateFakeImpressions(std::vector<base::Time> times,
-                             base::circular_deque<Impression>& impressions) {
-    impressions.clear();
+  void CreateFakeImpressions(ClientState* client_state,
+                             const std::vector<base::Time>& times) {
+    DCHECK(client_state);
+    client_state->impressions.clear();
+    auto type = client_state->type;
     for (const auto& time : times) {
-      impressions.emplace_back(SchedulerClientType::kTest1,
-                               base::GenerateGUID(), time);
+      client_state->impressions.emplace_back(type, base::GenerateGUID(), time);
     }
-    std::sort(impressions.begin(), impressions.end(),
-              [](const Impression& lhs, const Impression& rhs) {
-                return lhs.create_time < rhs.create_time;
-              });
+  }
+
+  std::unique_ptr<ClientState> CreateFakeClientStateWithImpression(
+      SchedulerClientType type,
+      const SchedulerConfig& config,
+      const std::vector<base::Time>& times) {
+    auto client_state = std::make_unique<ClientState>();
+    client_state->type = type;
+    client_state->current_max_daily_show = config.initial_daily_shown_per_type;
+    CreateFakeImpressions(client_state.get(), times);
+    return client_state;
   }
 
  protected:
@@ -85,6 +94,56 @@ TEST_F(SchedulerUtilsTest, ToLocalHour) {
   EXPECT_EQ(expected, another_day);
 }
 
+TEST_F(SchedulerUtilsTest, NotificationsShownTodayMultipleClients) {
+  InitFakeClock();
+  base::Time now = clock()->Now();
+  // Create fake clients.
+  std::map<SchedulerClientType, const ClientState*> client_states;
+  //            begin_of_today         now                  end_of_today
+  // client1  * |                   *  *                    | *
+  // client2  * *   *                  |  *                 | *
+  // client3  * |                      |                    | *
+
+  std::vector<base::Time> create_times = {
+      now - base::TimeDelta::FromSeconds(2) /*today*/,
+      now - base::TimeDelta::FromSeconds(1) /*today*/,
+      beginning_of_today() - base::TimeDelta::FromSeconds(1) /*yesterday*/,
+      beginning_of_today() + base::TimeDelta::FromDays(1) /*tomorrow*/};
+  auto new_client1 = CreateFakeClientStateWithImpression(
+      SchedulerClientType::kTest1, config(), create_times);
+
+  create_times = {
+      now /*today*/,
+      beginning_of_today() + base::TimeDelta::FromDays(1) /*tomorrow*/,
+      beginning_of_today() - base::TimeDelta::FromSeconds(1) /*yesterday*/,
+      beginning_of_today() + base::TimeDelta::FromSeconds(1) /*today*/,
+      beginning_of_today() /*today*/};
+  auto new_client2 = CreateFakeClientStateWithImpression(
+      SchedulerClientType::kTest2, config(), create_times);
+
+  create_times = {
+      beginning_of_today() - base::TimeDelta::FromSeconds(2), /*yesterday*/
+      beginning_of_today() + base::TimeDelta::FromDays(1)     /*tomorrow*/
+  };
+  auto new_client3 = CreateFakeClientStateWithImpression(
+      SchedulerClientType::kTest3, config(), create_times);
+
+  client_states[SchedulerClientType::kTest1] = new_client1.get();
+  client_states[SchedulerClientType::kTest2] = new_client2.get();
+  client_states[SchedulerClientType::kTest3] = new_client3.get();
+
+  std::map<SchedulerClientType, int> shown_per_type;
+  int shown_total = 0;
+  SchedulerClientType last_shown_type = SchedulerClientType::kUnknown;
+  NotificationsShownToday(client_states, &shown_per_type, &shown_total,
+                          &last_shown_type, clock());
+  EXPECT_EQ(shown_total, 5);
+  EXPECT_EQ(last_shown_type, SchedulerClientType::kTest2);
+  EXPECT_EQ(shown_per_type.at(SchedulerClientType::kTest1), 2);
+  EXPECT_EQ(shown_per_type.at(SchedulerClientType::kTest2), 3);
+  EXPECT_EQ(shown_per_type.at(SchedulerClientType::kTest3), 0);
+}
+
 TEST_F(SchedulerUtilsTest, NotificationsShownToday) {
   // Create fake client.
   auto new_client = CreateNewClientState(SchedulerClientType::kTest1, config());
@@ -102,16 +161,16 @@ TEST_F(SchedulerUtilsTest, NotificationsShownToday) {
       beginning_of_today() + base::TimeDelta::FromSeconds(1) /*today*/,
       beginning_of_today() /*today*/};
 
-  CreateFakeImpressions(create_times, new_client->impressions);
+  CreateFakeImpressions(new_client.get(), create_times);
   count = NotificationsShownToday(new_client.get(), clock());
   EXPECT_EQ(count, 3);
 
   // Test case 3:
   create_times = {
       beginning_of_today() - base::TimeDelta::FromSeconds(2), /*yesterday*/
-      beginning_of_today() - base::TimeDelta::FromDays(2),    /*tomorrow*/
+      beginning_of_today() + base::TimeDelta::FromDays(1),    /*tomorrow*/
   };
-  CreateFakeImpressions(create_times, new_client->impressions);
+  CreateFakeImpressions(new_client.get(), create_times);
   count = NotificationsShownToday(new_client.get(), clock());
   EXPECT_EQ(count, 0);
 
@@ -120,7 +179,7 @@ TEST_F(SchedulerUtilsTest, NotificationsShownToday) {
       now /*today*/, now - base::TimeDelta::FromSeconds(1) /*today*/,
       beginning_of_today() - base::TimeDelta::FromSeconds(1) /*yesterday*/,
       beginning_of_today() + base::TimeDelta::FromDays(1) /*tomorrow*/};
-  CreateFakeImpressions(create_times, new_client->impressions);
+  CreateFakeImpressions(new_client.get(), create_times);
   count = NotificationsShownToday(new_client.get(), clock());
   EXPECT_EQ(count, 2);
 }