[Notification scheduler]: Complete adding icons.

- Changed AddIcons with a map of IconType and UUID as input.
- Complete ScheduleNotification logic.
- Added unit tests for adding icons.

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

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

@@ -11,6 +11,7 @@
 #include "base/stl_util.h"
 #include "chrome/browser/notifications/scheduler/internal/icon_entry.h"
 #include "chrome/browser/notifications/scheduler/internal/proto_conversion.h"
+#include "chrome/browser/notifications/scheduler/public/notification_scheduler_types.h"
 
 namespace leveldb_proto {
 
@@ -51,18 +52,29 @@ void IconProtoDbStore::Init(InitCallback callback) {
                            std::move(callback)));
 }
 
-void IconProtoDbStore::AddIcons(std::vector<SkBitmap> icons,
-                                AddCallback callback) {
+void IconProtoDbStore::AddIcons(IconTypeBundleMap icons, AddCallback callback) {
+  if (icons.empty()) {
+    std::move(callback).Run({} /*IconTypeUuidMap*/, true);
+    return;
+  }
+
   std::vector<std::string> icons_uuid;
   for (size_t i = 0; i < icons.size(); i++) {
     icons_uuid.emplace_back(base::GenerateGUID());
   }
 
+  std::vector<IconType> icons_type;
+  std::vector<SkBitmap> icons_bitmap;
+  for (auto&& it = icons.begin(); it != icons.end(); ++it) {
+    icons_type.emplace_back(it->first);
+    icons_bitmap.emplace_back(std::move(it->second.bitmap));
+  }
+
   icon_converter_->ConvertIconToString(
-      std::move(icons),
+      std::move(icons_bitmap),
       base::BindOnce(&IconProtoDbStore::OnIconsEncoded,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback),
-                     std::move(icons_uuid)));
+                     std::move(icons_type), std::move(icons_uuid)));
 }
 
 void IconProtoDbStore::LoadIcons(const std::vector<std::string>& keys,
@@ -115,10 +127,12 @@ void IconProtoDbStore::OnIconEntriesLoaded(
 
 void IconProtoDbStore::OnIconsEncoded(
     AddCallback callback,
+    std::vector<IconType> icons_type,
     std::vector<std::string> icons_uuid,
     std::unique_ptr<EncodeResult> encode_result) {
+  IconTypeUuidMap icons_uuid_map;
   if (!encode_result->success) {
-    std::move(callback).Run({} /*icons_uuid*/, false);
+    std::move(callback).Run(std::move(icons_uuid_map), false);
     return;
   }
 
@@ -128,9 +142,10 @@ void IconProtoDbStore::OnIconsEncoded(
     IconEntry icon_entry;
     icon_entry.data = std::move(encoded_data[i]);
     entries_to_save->emplace_back(icons_uuid[i], std::move(icon_entry));
+    icons_uuid_map.emplace(icons_type[i], std::move(icons_uuid[i]));
   }
   auto add_callback =
-      base::BindOnce(std::move(callback), std::move(icons_uuid));
+      base::BindOnce(std::move(callback), std::move(icons_uuid_map));
   db_->UpdateEntries(std::move(entries_to_save),
                      std::make_unique<KeyVector>() /*entries_to_delete*/,
                      std::move(add_callback));
@@ -145,7 +160,7 @@ void IconProtoDbStore::OnIconsDecoded(
     return;
   }
 
-  IconsMap icons_map;
+  LoadedIconsMap icons_map;
   auto icons = std::move(decoded_result->decoded_icons);
   for (size_t i = 0; i < icons_uuid.size(); i++) {
     icons_map.emplace(std::move(icons_uuid[i]), std::move(icons[i]));

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

@@ -36,12 +36,14 @@ namespace notifications {
 // be loaded into memory.
 class IconStore {
  public:
-  using IconsMap = std::map<std::string /*icons_uuid*/, SkBitmap>;
-  using LoadIconsCallback = base::OnceCallback<void(bool, IconsMap)>;
+  using LoadedIconsMap = std::map<std::string /*icons_uuid*/, SkBitmap>;
+  using IconTypeUuidMap = std::map<IconType, std::string>;
+  using IconTypeBundleMap = std::map<IconType, IconBundle>;
+
   using InitCallback = base::OnceCallback<void(bool)>;
+  using LoadIconsCallback = base::OnceCallback<void(bool, LoadedIconsMap)>;
+  using AddCallback = base::OnceCallback<void(IconTypeUuidMap, bool)>;
   using UpdateCallback = base::OnceCallback<void(bool)>;
-  using AddCallback =
-      base::OnceCallback<void(std::vector<std::string> /*icons_uuid*/, bool)>;
 
   // Initializes the storage.
   virtual void Init(InitCallback callback) = 0;
@@ -51,7 +53,7 @@ class IconStore {
                          LoadIconsCallback callback) = 0;
 
   // Adds multiple icons to storage.
-  virtual void AddIcons(std::vector<SkBitmap>, AddCallback callback) = 0;
+  virtual void AddIcons(IconTypeBundleMap icons, AddCallback callback) = 0;
 
   // Deletes multiple icons.
   virtual void DeleteIcons(const std::vector<std::string>& keys,
@@ -77,7 +79,7 @@ class IconProtoDbStore : public IconStore {
   void Init(InitCallback callback) override;
   void LoadIcons(const std::vector<std::string>& keys,
                  LoadIconsCallback callback) override;
-  void AddIcons(std::vector<SkBitmap> icons, AddCallback callback) override;
+  void AddIcons(IconTypeBundleMap icons, AddCallback callback) override;
   void DeleteIcons(const std::vector<std::string>& keys,
                    UpdateCallback callback) override;
 
@@ -93,6 +95,7 @@ class IconProtoDbStore : public IconStore {
 
   // Called when the icons are encoded.
   void OnIconsEncoded(AddCallback callback,
+                      std::vector<IconType> icons_type,
                       std::vector<std::string> icons_uuid,
                       std::unique_ptr<EncodeResult> encode_result);
 

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

@@ -61,13 +61,18 @@ class IconStoreTest : public testing::Test {
     db()->InitStatusCallback(leveldb_proto::Enums::InitStatus::kOK);
   }
 
-  std::vector<SkBitmap> CreateIcons(size_t size) {
-    std::vector<SkBitmap> result;
-    for (size_t i = 0; i < size; i++) {
-      SkBitmap icon;
-      icon.allocN32Pixels(1, 1);
-      result.emplace_back(std::move(icon));
-    }
+  IconStore::IconTypeBundleMap CreateIcons() {
+    IconStore::IconTypeBundleMap result;
+    SkBitmap large_icon;
+    large_icon.allocN32Pixels(1, 1);
+    IconBundle large_icon_bundle;
+    large_icon_bundle.bitmap = std::move(large_icon);
+    result.emplace(IconType::kLargeIcon, std::move(large_icon_bundle));
+    SkBitmap small_icon;
+    small_icon.allocN32Pixels(1, 1);
+    IconBundle small_icon_bundle;
+    small_icon_bundle.bitmap = std::move(small_icon);
+    result.emplace(IconType::kSmallIcon, std::move(small_icon_bundle));
     return result;
   }
 
@@ -84,7 +89,7 @@ class IconStoreTest : public testing::Test {
         base::BindOnce(&IconStoreTest::OnIconsLoaded, base::Unretained(this)));
   }
 
-  void AddIcons(std::vector<SkBitmap> input) {
+  void AddIcons(IconStore::IconTypeBundleMap input) {
     auto add_icons_callback =
         base::BindOnce(&IconStoreTest::OnIconsAdded, base::Unretained(this));
     EXPECT_CALL(*icon_converter(), ConvertIconToString(_, _))
@@ -97,12 +102,12 @@ class IconStoreTest : public testing::Test {
     store()->AddIcons(std::move(input), std::move(add_icons_callback));
   }
 
-  void OnIconsAdded(std::vector<std::string> icons_uuid, bool success) {
-    icons_uuid_ = std::move(icons_uuid);
+  void OnIconsAdded(IconStore::IconTypeUuidMap icons_uuid, bool success) {
+    icons_uuid_map_ = std::move(icons_uuid);
     add_result_ = success;
   }
 
-  void OnIconsLoaded(bool success, IconStore::IconsMap icons) {
+  void OnIconsLoaded(bool success, IconStore::LoadedIconsMap icons) {
     loaded_icons_ = std::move(icons);
     load_result_ = success;
   }
@@ -113,15 +118,23 @@ class IconStoreTest : public testing::Test {
   leveldb_proto::test::FakeDB<proto::Icon, IconEntry>* db() { return db_; }
   bool load_result() const { return load_result_; }
   bool add_result() const { return add_result_; }
-  const std::vector<std::string>& icons_uuid() const { return icons_uuid_; }
-  const IconStore::IconsMap& loaded_icons() const { return loaded_icons_; }
+  std::vector<std::string> icons_uuid() const {
+    std::vector<std::string> result;
+    for (const auto& pair : icons_uuid_map_) {
+      result.emplace_back(pair.second);
+    }
+    return result;
+  }
+  const IconStore::LoadedIconsMap& loaded_icons() const {
+    return loaded_icons_;
+  }
 
  private:
   base::test::TaskEnvironment task_environment_;
   bool add_result_;
   bool load_result_;
-  IconStore::IconsMap loaded_icons_;
-  std::vector<std::string> icons_uuid_;
+  IconStore::LoadedIconsMap loaded_icons_;
+  IconStore::IconTypeUuidMap icons_uuid_map_;
   std::unique_ptr<IconStore> store_;
   std::map<std::string, proto::Icon> db_entries_;
   leveldb_proto::test::FakeDB<proto::Icon, IconEntry>* db_;
@@ -144,7 +157,7 @@ TEST_F(IconStoreTest, InitFailed) {
 
 TEST_F(IconStoreTest, AddIconsFailed) {
   InitDb();
-  std::vector<SkBitmap> input = CreateIcons(2);
+  auto input = CreateIcons();
 
   // Save two icons into store.
   AddIcons(input);
@@ -155,7 +168,7 @@ TEST_F(IconStoreTest, AddIconsFailed) {
 
 TEST_F(IconStoreTest, AddAndLoadIcons) {
   InitDb();
-  std::vector<SkBitmap> input = CreateIcons(2);
+  auto input = CreateIcons();
 
   // Save two icons into store.
   AddIcons(input);
@@ -172,7 +185,7 @@ TEST_F(IconStoreTest, AddAndLoadIcons) {
 
 TEST_F(IconStoreTest, LoadIconsFailed) {
   InitDb();
-  std::vector<SkBitmap> input = CreateIcons(2);
+  auto input = CreateIcons();
 
   // Save two icons into store.
   AddIcons(input);
@@ -190,7 +203,7 @@ TEST_F(IconStoreTest, LoadIconsFailed) {
 
 TEST_F(IconStoreTest, DeleteIcons) {
   InitDb();
-  std::vector<SkBitmap> input = CreateIcons(2);
+  auto input = CreateIcons();
 
   // Save two icons into store.
   AddIcons(input);
@@ -212,7 +225,7 @@ TEST_F(IconStoreTest, DeleteIcons) {
 
 TEST_F(IconStoreTest, DeleteIconsFailed) {
   InitDb();
-  std::vector<SkBitmap> input = CreateIcons(2);
+  auto input = CreateIcons();
 
   // Save two icons into store.
   AddIcons(input);

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

@@ -97,16 +97,14 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
 
     auto entry =
         std::make_unique<NotificationEntry>(notification_params->type, guid);
+    auto icon_bundles = std::move(notification_params->notification_data.icons);
     entry->notification_data =
         std::move(notification_params->notification_data);
-
     entry->schedule_params = std::move(notification_params->schedule_params);
-    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));
+    icon_store_->AddIcons(
+        std::move(icon_bundles),
+        base::BindOnce(&ScheduledNotificationManagerImpl::OnIconsAdded,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(entry)));
   }
 
   void DisplayNotification(const std::string& guid) override {
@@ -244,11 +242,26 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
     }
   }
 
-  void OnIconAdded(SchedulerClientType type,
-                   std::string guid,
-                   std::string icon_uuid,
-                   bool success) {
-    NOTIMPLEMENTED();
+  void OnIconsAdded(std::unique_ptr<NotificationEntry> entry,
+                    IconStore::IconTypeUuidMap icons_uuid_map,
+                    bool success) {
+    if (!success)
+      // TODO(hesen): Log icon stats.
+      return;
+
+    entry->icons_uuid = std::move(icons_uuid_map);
+    SaveNotificationEntry(std::move(entry));
+  }
+
+  void SaveNotificationEntry(std::unique_ptr<NotificationEntry> entry) {
+    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));
   }
 
   void OnNotificationDeleted(bool success) {

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

@@ -30,11 +30,20 @@ namespace {
 
 const char kGuid[] = "test_guid_1234";
 const char kTitle[] = "test_title";
+const char kSmallIconUuid[] = "test_small_icon_uuid";
+const char kLargeIconUuid[] = "test_large_icon_uuid";
 
 NotificationEntry CreateNotificationEntry(SchedulerClientType type) {
   return NotificationEntry(type, base::GenerateGUID());
 }
 
+IconStore::IconTypeBundleMap CreateIcons() {
+  IconStore::IconTypeBundleMap result;
+  result.emplace(IconType::kLargeIcon, IconBundle());
+  result.emplace(IconType::kSmallIcon, IconBundle());
+  return result;
+}
+
 class MockDelegate : public ScheduledNotificationManager::Delegate {
  public:
   MockDelegate() = default;
@@ -73,7 +82,8 @@ class MockIconStore : public IconStore {
   MOCK_METHOD2(LoadIcons,
                void(const std::vector<std::string>&,
                     IconStore::LoadIconsCallback));
-  MOCK_METHOD2(AddIcons, void(std::vector<SkBitmap>, IconStore::AddCallback));
+  MOCK_METHOD2(AddIcons,
+               void(IconStore::IconTypeBundleMap, IconStore::AddCallback));
   MOCK_METHOD2(DeleteIcons,
                void(const std::vector<std::string>&,
                     IconStore::UpdateCallback));
@@ -188,6 +198,11 @@ TEST_F(ScheduledNotificationManagerTest, ScheduleNotification) {
   EXPECT_FALSE(guid.empty());
 
   // Verify call contract.
+  EXPECT_CALL(*icon_store(), AddIcons(_, _))
+      .WillOnce(Invoke([](IconStore::IconTypeBundleMap icons,
+                          IconStore::AddCallback callback) {
+        std::move(callback).Run({}, true);
+      }));
   EXPECT_CALL(*notification_store(), Add(guid, _, _))
       .WillOnce(Invoke([guid](const std::string&, const NotificationEntry&,
                               base::OnceCallback<void(bool)> cb) {
@@ -226,6 +241,11 @@ TEST_F(ScheduledNotificationManagerTest, ScheduleNotificationEmptyGuid) {
       base::Time::Now() + base::TimeDelta::FromDays(1);
 
   // Verify call contract.
+  EXPECT_CALL(*icon_store(), AddIcons(_, _))
+      .WillOnce(Invoke([](IconStore::IconTypeBundleMap icons,
+                          IconStore::AddCallback callback) {
+        std::move(callback).Run({}, true);
+      }));
   EXPECT_CALL(*notification_store(), Add(_, _, _));
   manager()->ScheduleNotification(std::move(params));
 
@@ -376,5 +396,87 @@ TEST_F(ScheduledNotificationManagerTest, PruneExpiredNotifications) {
   EXPECT_EQ(notifications[SchedulerClientType::kTest2].size(), 2u);
 }
 
+// Test to schedule a notification with two icons in notification data.
+TEST_F(ScheduledNotificationManagerTest, ScheduleNotificationWithIcons) {
+  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);
+
+  std::string guid = params->guid;
+  EXPECT_FALSE(guid.empty());
+
+  // Verify call contract.
+  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);
+      }));
+
+  EXPECT_CALL(*notification_store(), Add(guid, _, _))
+      .WillOnce(Invoke([guid](const std::string&, const NotificationEntry&,
+                              base::OnceCallback<void(bool)> cb) {
+        std::move(cb).Run(true);
+      }));
+  manager()->ScheduleNotification(std::move(params));
+
+  // Verify in-memory data.
+  ScheduledNotificationManager::Notifications notifications;
+  manager()->GetAllNotifications(&notifications);
+  EXPECT_EQ(notifications.size(), 1u);
+  const NotificationEntry* entry = *(notifications.begin()->second.begin());
+  EXPECT_EQ(entry->guid, guid);
+  EXPECT_NE(entry->create_time, base::Time());
+  EXPECT_EQ(entry->icons_uuid.size(), 2u);
+  EXPECT_EQ(entry->icons_uuid.at(IconType::kLargeIcon), kLargeIconUuid);
+  EXPECT_EQ(entry->icons_uuid.at(IconType::kSmallIcon), kSmallIconUuid);
+}
+
+// Test to schedule a notification failed on saving icons.
+TEST_F(ScheduledNotificationManagerTest, ScheduleNotificationWithIconsFailed) {
+  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);
+
+  // Verify call contract.
+  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), false);
+      }));
+
+  manager()->ScheduleNotification(std::move(params));
+
+  // Verify in-memory data.
+  ScheduledNotificationManager::Notifications notifications;
+  manager()->GetAllNotifications(&notifications);
+  EXPECT_TRUE(notifications.empty());
+}
+
 }  // namespace
 }  // namespace notifications