Delay sending system notification until OnAppInstalled

This Cl ensures that system notifications for a particular
application are delayed until AppActivityRegistry receives
a signal from AppService that an application has been
installed.

Bug: 1056469
Change-Id: Ifb38093ee04f17ee88787e6f2824b6840822e4f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2101709
Commit-Queue: Yilkal Abe <yilkal@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750200}

chrome/browser/chromeos/child_accounts/time_limits/app_activity_registry.cc

@@ -98,6 +98,18 @@ void AppActivityRegistry::TestApi::SaveAppActivity() {
   registry_->SaveAppActivity();
 }
 
+AppActivityRegistry::SystemNotification::SystemNotification(
+    base::Optional<base::TimeDelta> app_time_limit,
+    AppNotification app_notification)
+    : time_limit(app_time_limit), notification(app_notification) {}
+
+AppActivityRegistry::SystemNotification::SystemNotification(
+    const SystemNotification&) = default;
+
+AppActivityRegistry::SystemNotification&
+AppActivityRegistry::SystemNotification::operator=(const SystemNotification&) =
+    default;
+
 AppActivityRegistry::AppDetails::AppDetails() = default;
 
 AppActivityRegistry::AppDetails::AppDetails(const AppActivity& activity)
@@ -181,8 +193,14 @@ void AppActivityRegistry::OnAppInstalled(const AppId& app_id) {
   // sessions and app service does not. Make sure not to override cached state.
   if (!base::Contains(activity_registry_, app_id)) {
     Add(app_id);
-  } else if (GetAppState(app_id) == AppState::kLimitReached) {
-    NotifyLimitReached(app_id, /* was_active */ false);
+  } else {
+    activity_registry_.at(app_id).received_app_installed_ = true;
+
+    // First send the system notifications for the application.
+    SendSystemNotificationsForApp(app_id);
+
+    if (GetAppState(app_id) == AppState::kLimitReached)
+      NotifyLimitReached(app_id, /* was_active */ false);
   }
 }
 
@@ -672,6 +690,7 @@ void AppActivityRegistry::CleanRegistry(base::Time timestamp) {
 
 void AppActivityRegistry::Add(const AppId& app_id) {
   activity_registry_[app_id].activity = AppActivity(AppState::kAvailable);
+  activity_registry_[app_id].received_app_installed_ = true;
   newly_installed_apps_.push_back(app_id);
   for (auto& observer : app_state_observers_)
     observer.OnAppInstalled(app_id);
@@ -859,34 +878,31 @@ void AppActivityRegistry::CheckTimeLimitForApp(const AppId& app_id) {
 
   if (time_left <= kFiveMinutes && time_left > kOneMinute &&
       last_notification != AppNotification::kFiveMinutes) {
-    notification_delegate_->ShowAppTimeLimitNotification(
-        app_id, time_limit, AppNotification::kFiveMinutes);
-    details.activity.set_last_notification(AppNotification::kFiveMinutes);
+    MaybeShowSystemNotification(
+        app_id, SystemNotification(time_limit, AppNotification::kFiveMinutes));
     ScheduleTimeLimitCheckForApp(app_id);
     return;
   }
 
   if (time_left <= kOneMinute && time_left > kZeroMinutes &&
       last_notification != AppNotification::kOneMinute) {
-    notification_delegate_->ShowAppTimeLimitNotification(
-        app_id, time_limit, AppNotification::kOneMinute);
-    details.activity.set_last_notification(AppNotification::kOneMinute);
+    MaybeShowSystemNotification(
+        app_id, SystemNotification(time_limit, AppNotification::kOneMinute));
     ScheduleTimeLimitCheckForApp(app_id);
     return;
   }
 
   if (time_left == kZeroMinutes &&
       last_notification != AppNotification::kTimeLimitReached) {
-    details.activity.set_last_notification(AppNotification::kTimeLimitReached);
+    MaybeShowSystemNotification(
+        app_id,
+        SystemNotification(time_limit, AppNotification::kTimeLimitReached));
 
     if (ContributesToWebTimeLimit(app_id, GetAppState(app_id))) {
       WebTimeLimitReached(base::Time::Now());
     } else {
       SetAppState(app_id, AppState::kLimitReached);
     }
-
-    notification_delegate_->ShowAppTimeLimitNotification(
-        app_id, time_limit, AppNotification::kTimeLimitReached);
   }
 }
 
@@ -910,16 +926,18 @@ bool AppActivityRegistry::ShowLimitUpdatedNotificationIfNeeded(
 
   // Time limit was removed.
   if (!has_time_limit && had_time_limit) {
-    notification_delegate_->ShowAppTimeLimitNotification(
-        app_id, base::nullopt, AppNotification::kTimeLimitChanged);
+    MaybeShowSystemNotification(
+        app_id,
+        SystemNotification(base::nullopt, AppNotification::kTimeLimitChanged));
     return true;
   }
 
   // Time limit was set or value changed.
   if (has_time_limit && (!had_time_limit || old_limit->daily_limit() !=
                                                 new_limit->daily_limit())) {
-    notification_delegate_->ShowAppTimeLimitNotification(
-        app_id, new_limit->daily_limit(), AppNotification::kTimeLimitChanged);
+    MaybeShowSystemNotification(
+        app_id, SystemNotification(new_limit->daily_limit(),
+                                   AppNotification::kTimeLimitChanged));
     return true;
   }
 
@@ -1025,5 +1043,41 @@ bool AppActivityRegistry::ShouldCleanUpStoredPref() {
   return time < base::Time::Now() - base::TimeDelta::FromDays(30);
 }
 
+void AppActivityRegistry::SendSystemNotificationsForApp(const AppId& app_id) {
+  DCHECK(base::Contains(activity_registry_, app_id));
+
+  AppDetails& app_details = activity_registry_.at(app_id);
+  DCHECK(app_details.received_app_installed_);
+
+  // TODO(yilkal): Filter out the notifications to show. For example don't show
+  // 5 min and 1 min left notifications at the same time here. However, time
+  // limit changed and 1 min left notifications can be shown at the same time.
+  for (const auto& elem : app_details.pending_notifications_) {
+    notification_delegate_->ShowAppTimeLimitNotification(
+        app_id, elem.time_limit, elem.notification);
+  }
+  app_details.pending_notifications_.clear();
+}
+
+void AppActivityRegistry::MaybeShowSystemNotification(
+    const AppId& app_id,
+    const SystemNotification& notification) {
+  DCHECK(base::Contains(activity_registry_, app_id));
+
+  AppDetails& app_details = activity_registry_.at(app_id);
+  app_details.activity.set_last_notification(notification.notification);
+
+  // AppActivityRegistry has not yet received OnAppInstalled call from
+  // AppService. Add notification to |AppDetails::pending_notifications_|.
+  if (!app_details.received_app_installed_) {
+    app_details.pending_notifications_.push_back(notification);
+    return;
+  }
+
+  // Otherwise, just show the notification.
+  notification_delegate_->ShowAppTimeLimitNotification(
+      app_id, notification.time_limit, notification.notification);
+}
+
 }  // namespace app_time
 }  // namespace chromeos

chrome/browser/chromeos/child_accounts/time_limits/app_activity_registry.h

@@ -170,6 +170,15 @@ class AppActivityRegistry : public AppServiceWrapper::EventListener {
       AppRestriction restriction) const;
 
  private:
+  struct SystemNotification {
+    SystemNotification(base::Optional<base::TimeDelta> app_time_limit,
+                       AppNotification app_notification);
+    SystemNotification(const SystemNotification&);
+    SystemNotification& operator=(const SystemNotification&);
+    base::Optional<base::TimeDelta> time_limit = base::nullopt;
+    AppNotification notification = AppNotification::kUnknown;
+  };
+
   // Bundles detailed data stored for a specific app.
   struct AppDetails {
     AppDetails();
@@ -204,6 +213,16 @@ class AppActivityRegistry : public AppServiceWrapper::EventListener {
     // Timer set up for when the app time limit is expected to be reached and
     // preceding notifications.
     std::unique_ptr<base::OneShotTimer> app_limit_timer;
+
+    // Boolean to specify if OnAppInstalled call has been received for this
+    // particular application.
+    bool received_app_installed_ = false;
+
+    // At the beginning of a session, we may want to send system notifications
+    // for applications. This may happen if there is an update in
+    // PerAppTimeLimits policy while the user was logged out. In these
+    // scenarios, we have to wait until the application is installed.
+    std::vector<SystemNotification> pending_notifications_;
   };
 
   // Removes data older than |timestamp| from the registry.
@@ -261,6 +280,13 @@ class AppActivityRegistry : public AppServiceWrapper::EventListener {
   // from base::Time::Now();
   bool ShouldCleanUpStoredPref();
 
+  // Sends system notification for the application.
+  void SendSystemNotificationsForApp(const AppId& app_id);
+
+  // Shows notification or queues it to be shown later.
+  void MaybeShowSystemNotification(const AppId& app_id,
+                                   const SystemNotification& notification);
+
   Profile* const profile_;
 
   // Owned by AppTimeController.

chrome/browser/chromeos/child_accounts/time_limits/app_activity_registry_unittest.cc

@@ -874,6 +874,9 @@ TEST_F(AppActivityRegistryTest, AvoidReduntantNotifications) {
   // Reinitialized the registry. We don't expect redundant time limit updatese
   // will result in notifications.
   ReInitializeRegistry();
+  registry().OnAppInstalled(GetChromeAppId());
+  registry().OnAppInstalled(kApp1);
+  registry().OnAppInstalled(kApp2);
 
   EXPECT_CALL(notification_delegate_mock(),
               ShowAppTimeLimitNotification(
@@ -908,6 +911,38 @@ TEST_F(AppActivityRegistryTest, AvoidReduntantNotifications) {
   registry().UpdateAppLimits(app_limits);
 }
 
+TEST_F(AppActivityRegistryTest, NoNotification) {
+  AppLimit app1_limit(AppRestriction::kTimeLimit,
+                      base::TimeDelta::FromMinutes(30), base::Time::Now());
+  std::map<AppId, AppLimit> app_limits = {{kApp1, app1_limit}};
+
+  EXPECT_CALL(notification_delegate_mock(),
+              ShowAppTimeLimitNotification(
+                  kApp1, app1_limit.daily_limit(),
+                  chromeos::app_time::AppNotification::kTimeLimitChanged))
+      .Times(0);
+  registry().SaveAppActivity();
+  ReInitializeRegistry();
+  registry().UpdateAppLimits(app_limits);
+}
+
+TEST_F(AppActivityRegistryTest, NotificationAfterAppInstall) {
+  AppLimit app1_limit(AppRestriction::kTimeLimit,
+                      base::TimeDelta::FromMinutes(30), base::Time::Now());
+  std::map<AppId, AppLimit> app_limits = {{kApp1, app1_limit}};
+
+  EXPECT_CALL(notification_delegate_mock(),
+              ShowAppTimeLimitNotification(
+                  kApp1, app1_limit.daily_limit(),
+                  chromeos::app_time::AppNotification::kTimeLimitChanged))
+      .Times(1);
+
+  registry().SaveAppActivity();
+  ReInitializeRegistry();
+  registry().UpdateAppLimits(app_limits);
+  registry().OnAppInstalled(kApp1);
+}
+
 TEST_F(AppActivityRegistryTest, AvoidRedundantCallsToPauseApp) {
   AppStateObserverMock state_observer_mock;
   registry().AddAppStateObserver(&state_observer_mock);