Notification scheduler: Fix an issue in Schedule API.

Currently the notification entry is inserted into stl container after
loading the icon, which causes the background task coordinator failed
to find the notification and won't trigger any background task.

This CL inserts the entry when all asynchronous opeartions are done,
and reply the result in a callback.

This behavior should be covered with end to end test.

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

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

@@ -72,10 +72,6 @@ class BackgroundTaskCoordinatorHelper {
         base::Time deliver_time_start =
             entry->schedule_params.deliver_time_start.value();
 
-        // Each background task has a minimum interval.
-        if (deliver_time_start < now + config_->background_task_min_interval)
-          deliver_time_start = now + config_->background_task_min_interval;
-
         // Consider suppression time.
         if (client_state->suppression_info.has_value() &&
             deliver_time_start <

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

@@ -121,8 +121,15 @@ class NotificationSchedulerImpl : public NotificationScheduler,
   void Schedule(
       std::unique_ptr<NotificationParams> notification_params) override {
     context_->notification_manager()->ScheduleNotification(
-        std::move(notification_params));
-    ScheduleBackgroundTask();
+        std::move(notification_params),
+        base::BindOnce(&NotificationSchedulerImpl::OnNotificationScheduled,
+                       weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  void OnNotificationScheduled(bool success) {
+    if (success) {
+      ScheduleBackgroundTask();
+    }
   }
 
   void DeleteAllNotifications(SchedulerClientType type) override {

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

@@ -204,13 +204,32 @@ TEST_F(NotificationSchedulerTest, InitScheduledNotificationManagerFailed) {
 // Test to schedule a notification.
 TEST_F(NotificationSchedulerTest, Schedule) {
   Init();
-
   auto param = std::unique_ptr<NotificationParams>();
-  EXPECT_CALL(*notification_manager(), ScheduleNotification(_));
+  EXPECT_CALL(*notification_manager(), ScheduleNotification(_, _))
+      .WillOnce(
+          Invoke([](std::unique_ptr<NotificationParams>,
+                    ScheduledNotificationManager::ScheduleCallback callback) {
+            std::move(callback).Run(true);
+          }));
   EXPECT_CALL(*task_coordinator(), ScheduleBackgroundTask(_, _));
   scheduler()->Schedule(std::move(param));
 }
 
+// When failed to add to the scheduled notification manager, no background task
+// is triggered.
+TEST_F(NotificationSchedulerTest, ScheduleFailed) {
+  Init();
+  auto param = std::unique_ptr<NotificationParams>();
+  EXPECT_CALL(*notification_manager(), ScheduleNotification(_, _))
+      .WillOnce(
+          Invoke([](std::unique_ptr<NotificationParams>,
+                    ScheduledNotificationManager::ScheduleCallback callback) {
+            std::move(callback).Run(false);
+          }));
+  EXPECT_CALL(*task_coordinator(), ScheduleBackgroundTask(_, _)).Times(0);
+  scheduler()->Schedule(std::move(param));
+}
+
 // Test to delete notifications.
 TEST_F(NotificationSchedulerTest, DeleteAllNotifications) {
   Init();

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

@@ -93,7 +93,8 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
   }
 
   void ScheduleNotification(
-      std::unique_ptr<NotificationParams> notification_params) override {
+      std::unique_ptr<NotificationParams> notification_params,
+      ScheduleCallback callback) override {
     DCHECK(notification_params);
     std::string guid = notification_params->guid;
     DCHECK(!guid.empty());
@@ -123,7 +124,8 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
     icon_store_->AddIcons(
         std::move(icon_bundles),
         base::BindOnce(&ScheduledNotificationManagerImpl::OnIconsAdded,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(entry)));
+                       weak_ptr_factory_.GetWeakPtr(), std::move(entry),
+                       std::move(callback)));
   }
 
   void DisplayNotification(const std::string& guid) override {
@@ -252,42 +254,45 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
                        weak_ptr_factory_.GetWeakPtr()));
   }
 
-  void OnNotificationAdded(SchedulerClientType type,
-                           std::string guid,
-                           bool success) {
-    stats::LogNotificationDbOperation(success);
-    auto* entry = FindNotificationEntry(type, guid);
-    if (!entry)
-      return;
-    // TODO(hesen): More Error handling.
-    if (!success) {
-      notifications_[type].erase(guid);
-      if (notifications_[type].empty())
-        notifications_.erase(type);
-      return;
-    }
-  }
-
   void OnIconsAdded(std::unique_ptr<NotificationEntry> entry,
+                    ScheduleCallback schedule_callback,
                     IconStore::IconTypeUuidMap icons_uuid_map,
                     bool success) {
-    if (!success)
+    if (!success) {
       // TODO(hesen): Log icon stats.
+      std::move(schedule_callback).Run(false);
       return;
+    }
 
     entry->icons_uuid = std::move(icons_uuid_map);
-    SaveNotificationEntry(std::move(entry));
+    const auto* entry_ptr = entry.get();
+    notification_store_->Add(
+        entry_ptr->guid, *entry_ptr,
+        base::BindOnce(&ScheduledNotificationManagerImpl::OnNotificationAdded,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(entry),
+                       std::move(schedule_callback)));
   }
 
-  void SaveNotificationEntry(std::unique_ptr<NotificationEntry> entry) {
+  void OnNotificationAdded(std::unique_ptr<NotificationEntry> entry,
+                           ScheduleCallback schedule_callback,
+                           bool success) {
+    stats::LogNotificationDbOperation(success);
+
+    // Delete the icons when failed to add to notification database.
+    if (!success) {
+      std::vector<std::string> icons_to_delete;
+      for (const auto& uuid : entry->icons_uuid) {
+        icons_to_delete.emplace_back(uuid.second);
+      }
+      icon_store_->DeleteIcons(std::move(icons_to_delete), base::DoNothing());
+      std::move(schedule_callback).Run(false);
+      return;
+    }
+
     auto type = entry->type;
     auto guid = entry->guid;
-    auto* entry_ptr = entry.get();
     notifications_[type][guid] = std::move(entry);
-    notification_store_->Add(
-        guid, *entry_ptr,
-        base::BindOnce(&ScheduledNotificationManagerImpl::OnNotificationAdded,
-                       weak_ptr_factory_.GetWeakPtr(), type, guid));
+    std::move(schedule_callback).Run(true);
   }
 
   void OnNotificationDeleted(bool success) {

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

@@ -29,6 +29,7 @@ class ScheduledNotificationManager {
   using Notifications =
       std::map<SchedulerClientType, std::vector<const NotificationEntry*>>;
   using InitCallback = base::OnceCallback<void(bool)>;
+  using ScheduleCallback = base::OnceCallback<void(bool)>;
 
   // Delegate that receives events from the manager.
   class Delegate {
@@ -56,7 +57,8 @@ class ScheduledNotificationManager {
 
   // Adds a new notification.
   virtual void ScheduleNotification(
-      std::unique_ptr<NotificationParams> notification_params) = 0;
+      std::unique_ptr<NotificationParams> notification_params,
+      ScheduleCallback callback) = 0;
 
   // Displays a notification, the scheduled notification will be removed from
   // storage, then Delegate::DisplayNotification() should be invoked.

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

@@ -157,6 +157,22 @@ class ScheduledNotificationManagerTest : public testing::Test {
     loop.Run();
   }
 
+  // Schedules a notification and wait until the callback is called.
+  void ScheduleNotification(std::unique_ptr<NotificationParams> params,
+                            bool expected_success) {
+    base::RunLoop loop;
+    auto schedule_callback = base::BindOnce(
+        [](base::RepeatingClosure quit_closure, bool expected_success,
+           bool success) {
+          EXPECT_EQ(success, expected_success);
+          quit_closure.Run();
+        },
+        loop.QuitClosure(), expected_success);
+    manager()->ScheduleNotification(std::move(params),
+                                    std::move(schedule_callback));
+    loop.Run();
+  }
+
  private:
   base::test::TaskEnvironment task_environment_;
   std::unique_ptr<MockDelegate> delegate_;
@@ -247,7 +263,7 @@ TEST_F(ScheduledNotificationManagerTest, ScheduleNotification) {
                               base::OnceCallback<void(bool)> cb) {
         std::move(cb).Run(true);
       }));
-  manager()->ScheduleNotification(std::move(params));
+  ScheduleNotification(std::move(params), true /*expected_success*/);
 
   // Verify in-memory data.
   ScheduledNotificationManager::Notifications notifications;
@@ -285,8 +301,12 @@ TEST_F(ScheduledNotificationManagerTest, ScheduleNotificationEmptyGuid) {
                           IconStore::AddCallback callback) {
         std::move(callback).Run({}, true);
       }));
-  EXPECT_CALL(*notification_store(), Add(_, _, _));
-  manager()->ScheduleNotification(std::move(params));
+  EXPECT_CALL(*notification_store(), Add(_, _, _))
+      .WillOnce(Invoke(
+          [&](const std::string&, const NotificationEntry&,
+              base::OnceCallback<void(bool)> cb) { std::move(cb).Run(true); }));
+
+  ScheduleNotification(std::move(params), true /*expected_success*/);
 
   // Verify in-memory data.
   ScheduledNotificationManager::Notifications notifications;
@@ -488,7 +508,7 @@ TEST_F(ScheduledNotificationManagerTest, ScheduleNotificationWithIcons) {
                               base::OnceCallback<void(bool)> cb) {
         std::move(cb).Run(true);
       }));
-  manager()->ScheduleNotification(std::move(params));
+  ScheduleNotification(std::move(params), true /*expected_success*/);
 
   // Verify in-memory data.
   ScheduledNotificationManager::Notifications notifications;
@@ -528,7 +548,50 @@ TEST_F(ScheduledNotificationManagerTest, ScheduleNotificationWithIconsFailed) {
         std::move(callback).Run(std::move(icons_uuid_map), false);
       }));
 
-  manager()->ScheduleNotification(std::move(params));
+  ScheduleNotification(std::move(params), false /*expected_success*/);
+
+  // Verify in-memory data.
+  ScheduledNotificationManager::Notifications notifications;
+  manager()->GetAllNotifications(&notifications);
+  EXPECT_TRUE(notifications.empty());
+}
+
+// Verifies the case that failed to add to the notification store.
+TEST_F(ScheduledNotificationManagerTest, ScheduleAddNotificationFailed) {
+  InitWithData(std::vector<NotificationEntry>());
+  NotificationData notification_data;
+  notification_data.icons = CreateIcons();
+  ScheduleParams schedule_params;
+  auto params = std::make_unique<NotificationParams>(
+      SchedulerClientType::kTest1, notification_data, schedule_params);
+  params->schedule_params.deliver_time_start = base::Time::Now();
+  params->schedule_params.deliver_time_end =
+      base::Time::Now() + base::TimeDelta::FromDays(1);
+
+  // Succeeded to add icons.
+  EXPECT_CALL(*icon_store(), AddIcons(_, _))
+      .WillOnce(Invoke([](IconStore::IconTypeBundleMap icons,
+                          IconStore::AddCallback callback) {
+        IconStore::IconTypeUuidMap icons_uuid_map;
+        for (const auto& pair : icons) {
+          if (pair.first == IconType::kLargeIcon)
+            icons_uuid_map.emplace(pair.first, kLargeIconUuid);
+          else if (pair.first == IconType::kSmallIcon)
+            icons_uuid_map.emplace(pair.first, kSmallIconUuid);
+        }
+        std::move(callback).Run(std::move(icons_uuid_map), true);
+      }));
+
+  std::vector<std::string> icons_to_delete{kSmallIconUuid, kLargeIconUuid};
+  EXPECT_CALL(*icon_store(), DeleteIcons(icons_to_delete, _));
+
+  // Failed to add notifications.
+  EXPECT_CALL(*notification_store(), Add(_, _, _))
+      .WillOnce(Invoke(
+          [](const std::string&, const NotificationEntry&,
+             base::OnceCallback<void(bool)> cb) { std::move(cb).Run(false); }));
+
+  ScheduleNotification(std::move(params), false /*expected_success*/);
 
   // Verify in-memory data.
   ScheduledNotificationManager::Notifications notifications;

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

@@ -26,10 +26,6 @@ constexpr base::TimeDelta kDefaultDimissDuration = base::TimeDelta::FromDays(7);
 constexpr base::TimeDelta kDefaultBackgroundTaskWindowDuration =
     base::TimeDelta::FromHours(1);
 
-// Default randomized time window to distribute load from user actions.
-constexpr base::TimeDelta kDefaultBackgroundTaskMinInterval =
-    base::TimeDelta::FromMinutes(10);
-
 // static
 std::unique_ptr<SchedulerConfig> SchedulerConfig::Create() {
   return std::make_unique<SchedulerConfig>();
@@ -44,8 +40,7 @@ SchedulerConfig::SchedulerConfig()
       suppression_duration(kDefaultSuppressionDuration),
       dismiss_count(3),
       dismiss_duration(kDefaultDimissDuration),
-      background_task_window_duration(kDefaultBackgroundTaskWindowDuration),
-      background_task_min_interval(kDefaultBackgroundTaskMinInterval) {
+      background_task_window_duration(kDefaultBackgroundTaskWindowDuration) {
   // TODO(xingliu): Add constructor using finch data.
 }
 

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

@@ -52,9 +52,6 @@ struct SchedulerConfig {
   // The time window to launch the background task.
   base::TimeDelta background_task_window_duration;
 
-  // The minimum interval between background tasks.
-  base::TimeDelta background_task_min_interval;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(SchedulerConfig);
 };

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

@@ -18,8 +18,9 @@ class MockScheduledNotificationManager : public ScheduledNotificationManager {
   ~MockScheduledNotificationManager() override;
 
   MOCK_METHOD2(Init, void(Delegate*, base::OnceCallback<void(bool)>));
-  MOCK_METHOD1(ScheduleNotification,
-               void(std::unique_ptr<notifications::NotificationParams>));
+  MOCK_METHOD2(ScheduleNotification,
+               void(std::unique_ptr<notifications::NotificationParams>,
+                    ScheduleCallback));
   MOCK_METHOD1(DisplayNotification, void(const std::string&));
   MOCK_CONST_METHOD1(GetAllNotifications, void(Notifications*));
   MOCK_CONST_METHOD2(GetNotifications,

chrome/browser/ui/webui/notifications_internals/notifications_internals_ui_message_handler.cc

@@ -37,6 +37,9 @@ void NotificationsInternalsUIMessageHandler::HandleScheduleNotification(
     const base::ListValue* args) {
   CHECK_EQ(args->GetList().size(), 4u);
   notifications::ScheduleParams schedule_params;
+  schedule_params.deliver_time_start = base::Time::Now();
+  schedule_params.deliver_time_end =
+      base::Time::Now() + base::TimeDelta::FromMinutes(5);
   notifications::NotificationData data;
   data.custom_data.emplace("url", args->GetList()[1].GetString());
   data.title = base::UTF8ToUTF16(args->GetList()[2].GetString());