[notification scheduler]: Move IconStore to

ScheduledNotificationManager.

Keep this cl simple to just move the ownership of
IconStore for future change.

-TODO in next cl: implement the logic to read/write/delete
icon asynchronously.

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

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

@@ -16,7 +16,6 @@
 #include "chrome/browser/notifications/scheduler/internal/background_task_coordinator.h"
 #include "chrome/browser/notifications/scheduler/internal/display_decider.h"
 #include "chrome/browser/notifications/scheduler/internal/distribution_policy.h"
-#include "chrome/browser/notifications/scheduler/internal/icon_store.h"
 #include "chrome/browser/notifications/scheduler/internal/impression_history_tracker.h"
 #include "chrome/browser/notifications/scheduler/internal/notification_entry.h"
 #include "chrome/browser/notifications/scheduler/internal/notification_scheduler_context.h"
@@ -62,23 +61,13 @@ class InitHelper {
     impression_tracker_delegate_ = impression_tracker_delegate;
     callback_ = std::move(callback);
 
-    context->icon_store()->Init(base::BindOnce(
-        &InitHelper::OnIconStoreInitialized, weak_ptr_factory_.GetWeakPtr()));
-  }
-
- private:
-  void OnIconStoreInitialized(bool success) {
-    if (!success) {
-      std::move(callback_).Run(false /*success*/);
-      return;
-    }
-
     context_->impression_tracker()->Init(
         impression_tracker_delegate_,
         base::BindOnce(&InitHelper::OnImpressionTrackerInitialized,
                        weak_ptr_factory_.GetWeakPtr()));
   }
 
+ private:
   void OnImpressionTrackerInitialized(bool success) {
     if (!success) {
       std::move(callback_).Run(false /*success*/);

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

@@ -10,7 +10,6 @@
 #include "base/time/default_clock.h"
 #include "chrome/browser/notifications/scheduler/internal/background_task_coordinator.h"
 #include "chrome/browser/notifications/scheduler/internal/display_decider.h"
-#include "chrome/browser/notifications/scheduler/internal/icon_store.h"
 #include "chrome/browser/notifications/scheduler/internal/impression_history_tracker.h"
 #include "chrome/browser/notifications/scheduler/internal/scheduled_notification_manager.h"
 #include "chrome/browser/notifications/scheduler/internal/scheduler_config.h"
@@ -24,14 +23,12 @@ namespace notifications {
 NotificationSchedulerContext::NotificationSchedulerContext(
     std::unique_ptr<NotificationSchedulerClientRegistrar> client_registrar,
     std::unique_ptr<NotificationBackgroundTaskScheduler> background_task,
-    std::unique_ptr<IconStore> icon_store,
     std::unique_ptr<ImpressionHistoryTracker> impression_tracker,
     std::unique_ptr<ScheduledNotificationManager> notification_manager,
     std::unique_ptr<DisplayAgent> display_agent,
     std::unique_ptr<DisplayDecider> display_decider,
     std::unique_ptr<SchedulerConfig> config)
     : client_registrar_(std::move(client_registrar)),
-      icon_store_(std::move(icon_store)),
       impression_tracker_(std::move(impression_tracker)),
       notification_manager_(std::move(notification_manager)),
       display_agent_(std::move(display_agent)),

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

@@ -16,7 +16,6 @@ namespace notifications {
 class BackgroundTaskCoordinator;
 class DisplayAgent;
 class DisplayDecider;
-class IconStore;
 class ImpressionHistoryTracker;
 class NotificationBackgroundTaskScheduler;
 class NotificationSchedulerClientRegistrar;
@@ -30,7 +29,6 @@ class NotificationSchedulerContext {
   NotificationSchedulerContext(
       std::unique_ptr<NotificationSchedulerClientRegistrar> client_registrar,
       std::unique_ptr<NotificationBackgroundTaskScheduler> background_task,
-      std::unique_ptr<IconStore> icon_store,
       std::unique_ptr<ImpressionHistoryTracker> impression_tracker,
       std::unique_ptr<ScheduledNotificationManager> notification_manager,
       std::unique_ptr<DisplayAgent> display_agent,
@@ -46,8 +44,6 @@ class NotificationSchedulerContext {
     return background_task_coordinator_.get();
   }
 
-  IconStore* icon_store() { return icon_store_.get(); }
-
   ImpressionHistoryTracker* impression_tracker() {
     return impression_tracker_.get();
   }
@@ -66,9 +62,6 @@ class NotificationSchedulerContext {
   // Holds a list of clients using the notification scheduler system.
   std::unique_ptr<NotificationSchedulerClientRegistrar> client_registrar_;
 
-  // Stores notification icons.
-  std::unique_ptr<IconStore> icon_store_;
-
   // Tracks user impressions towards specific notification type.
   std::unique_ptr<ImpressionHistoryTracker> impression_tracker_;
 

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

@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "base/guid.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/notifications/scheduler/internal/icon_store.h"
 #include "chrome/browser/notifications/scheduler/internal/notification_entry.h"
 #include "chrome/browser/notifications/scheduler/internal/scheduler_config.h"
 #include "chrome/browser/notifications/scheduler/public/notification_params.h"
@@ -27,13 +28,15 @@ bool CreateTimeCompare(const NotificationEntry* lhs,
 
 class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
  public:
-  using Store = std::unique_ptr<CollectionStore<NotificationEntry>>;
+  using NotificationStore = std::unique_ptr<CollectionStore<NotificationEntry>>;
 
   ScheduledNotificationManagerImpl(
-      Store store,
+      NotificationStore notification_store,
+      std::unique_ptr<IconStore> icon_store,
       const std::vector<SchedulerClientType>& clients,
       const SchedulerConfig& config)
-      : store_(std::move(store)),
+      : notification_store_(std::move(notification_store)),
+        icon_store_(std::move(icon_store)),
         clients_(clients.begin(), clients.end()),
         delegate_(nullptr),
         config_(config),
@@ -43,9 +46,10 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
   void Init(Delegate* delegate, InitCallback callback) override {
     DCHECK(!delegate_);
     delegate_ = delegate;
-    store_->InitAndLoad(
-        base::BindOnce(&ScheduledNotificationManagerImpl::OnStoreInitialized,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+
+    notification_store_->InitAndLoad(base::BindOnce(
+        &ScheduledNotificationManagerImpl::OnNotificationStoreInitialized,
+        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
   // NotificationManager implementation.
@@ -69,7 +73,7 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
     entry->schedule_params = std::move(notification_params->schedule_params);
     auto* entry_ptr = entry.get();
     notifications_[type][guid] = std::move(entry);
-    store_->Add(
+    notification_store_->Add(
         guid, *entry_ptr,
         base::BindOnce(&ScheduledNotificationManagerImpl::OnNotificationAdded,
                        weak_ptr_factory_.GetWeakPtr()));
@@ -88,7 +92,7 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
     if (notifications_[entry->type].empty())
       notifications_.erase(entry->type);
 
-    store_->Delete(
+    notification_store_->Delete(
         guid,
         base::BindOnce(&ScheduledNotificationManagerImpl::OnNotificationDeleted,
                        weak_ptr_factory_.GetWeakPtr()));
@@ -133,7 +137,7 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
     while (it != notifications_[type].end()) {
       const auto& entry = *it->second;
       ++it;
-      store_->Delete(
+      notification_store_->Delete(
           entry.guid,
           base::BindOnce(
               &ScheduledNotificationManagerImpl::OnNotificationDeleted,
@@ -155,9 +159,10 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
     }
   }
 
-  void OnStoreInitialized(InitCallback callback,
-                          bool success,
-                          CollectionStore<NotificationEntry>::Entries entries) {
+  void OnNotificationStoreInitialized(
+      InitCallback callback,
+      bool success,
+      CollectionStore<NotificationEntry>::Entries entries) {
     if (!success) {
       std::move(callback).Run(false);
       return;
@@ -169,7 +174,7 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
       bool expired = entry->create_time + config_.notification_expiration <=
                      base::Time::Now();
       if (expired) {
-        store_->Delete(
+        notification_store_->Delete(
             entry->guid,
             base::BindOnce(
                 &ScheduledNotificationManagerImpl::OnNotificationDeleted,
@@ -179,14 +184,26 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
       }
     }
     SyncRegisteredClients();
-    std::move(callback).Run(true);
+
+    icon_store_->Init(base::BindOnce(
+        &ScheduledNotificationManagerImpl::OnIconStoreInitialized,
+        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  }
+
+  void OnIconStoreInitialized(InitCallback callback, bool success) {
+    std::move(callback).Run(success);
   }
 
   void OnNotificationAdded(bool success) { NOTIMPLEMENTED(); }
 
   void OnNotificationDeleted(bool success) { NOTIMPLEMENTED(); }
 
-  Store store_;
+  void OnIconAdded(bool success) { NOTIMPLEMENTED(); }
+
+  void OnIconDeleted(bool success) { NOTIMPLEMENTED(); }
+
+  NotificationStore notification_store_;
+  std::unique_ptr<IconStore> icon_store_;
   const std::unordered_set<SchedulerClientType> clients_;
   Delegate* delegate_;
   std::map<SchedulerClientType,
@@ -201,11 +218,12 @@ class ScheduledNotificationManagerImpl : public ScheduledNotificationManager {
 // static
 std::unique_ptr<ScheduledNotificationManager>
 ScheduledNotificationManager::Create(
-    std::unique_ptr<CollectionStore<NotificationEntry>> store,
+    std::unique_ptr<CollectionStore<NotificationEntry>> notification_store,
+    std::unique_ptr<IconStore> icon_store,
     const std::vector<SchedulerClientType>& clients,
     const SchedulerConfig& config) {
-  return std::make_unique<ScheduledNotificationManagerImpl>(std::move(store),
-                                                            clients, config);
+  return std::make_unique<ScheduledNotificationManagerImpl>(
+      std::move(notification_store), std::move(icon_store), clients, config);
 }
 
 ScheduledNotificationManager::ScheduledNotificationManager() = default;

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

@@ -19,6 +19,7 @@ namespace notifications {
 struct NotificationEntry;
 struct NotificationParams;
 struct SchedulerConfig;
+class IconStore;
 
 // Class to manage in-memory scheduled notifications loaded from the storage.
 class ScheduledNotificationManager {
@@ -43,7 +44,8 @@ class ScheduledNotificationManager {
 
   // Creates the instance.
   static std::unique_ptr<ScheduledNotificationManager> Create(
-      std::unique_ptr<CollectionStore<NotificationEntry>> store,
+      std::unique_ptr<CollectionStore<NotificationEntry>> notification_store,
+      std::unique_ptr<IconStore> icon_store,
       const std::vector<SchedulerClientType>& clients,
       const SchedulerConfig& config);
 

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

@@ -10,6 +10,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_task_environment.h"
 #include "chrome/browser/notifications/scheduler/internal/collection_store.h"
+#include "chrome/browser/notifications/scheduler/internal/icon_store.h"
 #include "chrome/browser/notifications/scheduler/internal/notification_entry.h"
 #include "chrome/browser/notifications/scheduler/internal/scheduler_config.h"
 #include "chrome/browser/notifications/scheduler/public/notification_params.h"
@@ -60,18 +61,37 @@ class MockNotificationStore : public CollectionStore<NotificationEntry> {
   DISALLOW_COPY_AND_ASSIGN(MockNotificationStore);
 };
 
+class MockIconStore : public IconStore {
+ public:
+  MockIconStore() {}
+
+  MOCK_METHOD1(Init, void(IconStore::InitCallback));
+  MOCK_METHOD2(Load, void(const std::string&, IconStore::LoadCallback));
+  MOCK_METHOD3(Add,
+               void(const std::string&,
+                    std::unique_ptr<IconEntry>,
+                    IconStore::UpdateCallback));
+  MOCK_METHOD2(Delete, void(const std::string&, IconStore::UpdateCallback));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockIconStore);
+};
+
 class ScheduledNotificationManagerTest : public testing::Test {
  public:
-  ScheduledNotificationManagerTest() : store_(nullptr) {}
+  ScheduledNotificationManagerTest()
+      : notification_store_(nullptr), icon_store_(nullptr) {}
   ~ScheduledNotificationManagerTest() override = default;
 
   void SetUp() override {
     delegate_ = std::make_unique<MockDelegate>();
-    auto store = std::make_unique<MockNotificationStore>();
-    store_ = store.get();
+    auto notification_store = std::make_unique<MockNotificationStore>();
+    auto icon_store = std::make_unique<MockIconStore>();
+    notification_store_ = notification_store.get();
+    icon_store_ = icon_store.get();
     config_.notification_expiration = base::TimeDelta::FromDays(1);
     manager_ = ScheduledNotificationManager::Create(
-        std::move(store),
+        std::move(notification_store), std::move(icon_store),
         {SchedulerClientType::kTest1, SchedulerClientType::kTest2,
          SchedulerClientType::kTest3},
         config_);
@@ -79,7 +99,8 @@ class ScheduledNotificationManagerTest : public testing::Test {
 
  protected:
   ScheduledNotificationManager* manager() { return manager_.get(); }
-  MockNotificationStore* store() { return store_; }
+  MockNotificationStore* notification_store() { return notification_store_; }
+  MockIconStore* icon_store() { return icon_store_; }
   MockDelegate* delegate() { return delegate_.get(); }
   const SchedulerConfig& config() const { return config_; }
   // Initializes the manager with predefined data in the store.
@@ -92,11 +113,16 @@ class ScheduledNotificationManagerTest : public testing::Test {
     }
 
     // Initialize the store and call the callback.
-    EXPECT_CALL(*store(), InitAndLoad(_))
+    EXPECT_CALL(*notification_store(), InitAndLoad(_))
         .WillOnce(
             Invoke([&entries](base::OnceCallback<void(bool, Entries)> cb) {
               std::move(cb).Run(true, std::move(entries));
             }));
+    EXPECT_CALL(*icon_store(), Init(_))
+        .WillOnce(Invoke([](base::OnceCallback<void(bool)> cb) {
+          std::move(cb).Run(true);
+        }));
+
     base::RunLoop loop;
     manager()->Init(delegate(),
                     base::BindOnce(
@@ -111,7 +137,8 @@ class ScheduledNotificationManagerTest : public testing::Test {
  private:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<MockDelegate> delegate_;
-  MockNotificationStore* store_;
+  MockNotificationStore* notification_store_;
+  MockIconStore* icon_store_;
   std::vector<SchedulerClientType> clients_;
   std::unique_ptr<ScheduledNotificationManager> manager_;
   SchedulerConfig config_;
@@ -121,7 +148,7 @@ class ScheduledNotificationManagerTest : public testing::Test {
 
 // Verify that error is received when initialization failed.
 TEST_F(ScheduledNotificationManagerTest, InitFailed) {
-  EXPECT_CALL(*store(), InitAndLoad(_))
+  EXPECT_CALL(*notification_store(), InitAndLoad(_))
       .WillOnce(Invoke([](base::OnceCallback<void(bool, Entries)> cb) {
         std::move(cb).Run(false, Entries());
       }));
@@ -151,7 +178,7 @@ TEST_F(ScheduledNotificationManagerTest, ScheduleNotification) {
   EXPECT_FALSE(guid.empty());
 
   // Verify call contract.
-  EXPECT_CALL(*store(), Add(guid, _, _));
+  EXPECT_CALL(*notification_store(), Add(guid, _, _));
   manager()->ScheduleNotification(std::move(params));
 
   // Verify in-memory data.
@@ -174,7 +201,7 @@ TEST_F(ScheduledNotificationManagerTest, ScheduleNotificationEmptyGuid) {
       SchedulerClientType::kTest1, NotificationData(), ScheduleParams());
 
   // Verify call contract.
-  EXPECT_CALL(*store(), Add(_, _, _));
+  EXPECT_CALL(*notification_store(), Add(_, _, _));
   manager()->ScheduleNotification(std::move(params));
 
   // Verify in-memory data.
@@ -198,7 +225,7 @@ TEST_F(ScheduledNotificationManagerTest, DisplayNotification) {
   InitWithData(std::vector<NotificationEntry>({entry}));
 
   // Verify delegate and dependency call contract.
-  EXPECT_CALL(*store(), Delete(kGuid, _));
+  EXPECT_CALL(*notification_store(), Delete(kGuid, _));
   EXPECT_CALL(*delegate(), DisplayNotification(NotificationEntryIs(entry)));
   manager()->DisplayNotification(kGuid);
 
@@ -266,24 +293,28 @@ TEST_F(ScheduledNotificationManagerTest, DeleteNotifications) {
   manager()->GetAllNotifications(&notifications);
   EXPECT_EQ(notifications.size(), 3u);
 
-  EXPECT_CALL(*store(), Delete(_, _)).Times(2).RetiresOnSaturation();
+  EXPECT_CALL(*notification_store(), Delete(_, _))
+      .Times(2)
+      .RetiresOnSaturation();
   manager()->DeleteNotifications(SchedulerClientType::kTest2);
   manager()->GetAllNotifications(&notifications);
   EXPECT_EQ(notifications.size(), 2u);
 
   // Ensure deleting non-existing key will not crash, and store will not call
   // Delete.
-  EXPECT_CALL(*store(), Delete(_, _)).Times(0).RetiresOnSaturation();
+  EXPECT_CALL(*notification_store(), Delete(_, _))
+      .Times(0)
+      .RetiresOnSaturation();
   manager()->DeleteNotifications(SchedulerClientType::kTest2);
   manager()->GetAllNotifications(&notifications);
   EXPECT_EQ(notifications.size(), 2u);
 
-  EXPECT_CALL(*store(), Delete(_, _)).RetiresOnSaturation();
+  EXPECT_CALL(*notification_store(), Delete(_, _)).RetiresOnSaturation();
   manager()->DeleteNotifications(SchedulerClientType::kTest1);
   manager()->GetAllNotifications(&notifications);
   EXPECT_EQ(notifications.size(), 1u);
 
-  EXPECT_CALL(*store(), Delete(_, _)).RetiresOnSaturation();
+  EXPECT_CALL(*notification_store(), Delete(_, _)).RetiresOnSaturation();
   manager()->DeleteNotifications(SchedulerClientType::kTest3);
   manager()->GetAllNotifications(&notifications);
   EXPECT_EQ(notifications.size(), 0u);
@@ -308,7 +339,9 @@ TEST_F(ScheduledNotificationManagerTest, PruneExpiredNotifications) {
   entry5.create_time =
       now - base::TimeDelta::FromDays(1) - base::TimeDelta::FromMicroseconds(1);
 
-  EXPECT_CALL(*store(), Delete(_, _)).Times(3).RetiresOnSaturation();
+  EXPECT_CALL(*notification_store(), Delete(_, _))
+      .Times(3)
+      .RetiresOnSaturation();
   InitWithData(std::vector<NotificationEntry>(
       {entry0, entry1, entry2, entry3, entry4, entry5}));
   ScheduledNotificationManager::Notifications notifications;

chrome/browser/notifications/scheduler/schedule_service_factory_helper.cc

@@ -79,13 +79,13 @@ KeyedService* CreateNotificationScheduleService(
   auto notification_store =
       std::make_unique<NotificationStore>(std::move(notification_db));
   auto notification_manager = ScheduledNotificationManager::Create(
-      std::move(notification_store), registered_clients, *config.get());
+      std::move(notification_store), std::move(icon_store), registered_clients,
+      *config.get());
 
   auto context = std::make_unique<NotificationSchedulerContext>(
       std::move(client_registrar), std::move(background_task_scheduler),
-      std::move(icon_store), std::move(impression_tracker),
-      std::move(notification_manager), std::move(display_agent),
-      DisplayDecider::Create(), std::move(config));
+      std::move(impression_tracker), std::move(notification_manager),
+      std::move(display_agent), DisplayDecider::Create(), std::move(config));
 
   auto scheduler = NotificationScheduler::Create(std::move(context));
   auto init_aware_scheduler =