Wait for App Install before pausing app.

This CL corrects a bug in the logic where
AppTimeController notifies AppService to pause
an application before the AppService knows about
the application.

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

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

@@ -179,8 +179,11 @@ AppActivityRegistry::~AppActivityRegistry() {
 void AppActivityRegistry::OnAppInstalled(const AppId& app_id) {
   // App might be already present in registry, because we preserve info between
   // sessions and app service does not. Make sure not to override cached state.
-  if (!base::Contains(activity_registry_, app_id))
+  if (!base::Contains(activity_registry_, app_id)) {
     Add(app_id);
+  } else if (GetAppState(app_id) == AppState::kLimitReached) {
+    NotifyLimitReached(app_id, /* was_active */ false);
+  }
 }
 
 void AppActivityRegistry::OnAppUninstalled(const AppId& app_id) {
@@ -213,13 +216,7 @@ void AppActivityRegistry::OnAppActive(const AppId& app_id,
 
   // We are notified that a paused app is active. Notify observers to pause it.
   if (GetAppState(app_id) == AppState::kLimitReached) {
-    for (auto& observer : app_state_observers_) {
-      const base::Optional<AppLimit>& limit =
-          activity_registry_.at(app_id).limit;
-      DCHECK(limit->daily_limit());
-      observer.OnAppLimitReached(app_id, limit->daily_limit().value(),
-                                 /* was_active */ true);
-    }
+    NotifyLimitReached(app_id, /* was_active */ true);
     return;
   }
 
@@ -309,6 +306,12 @@ void AppActivityRegistry::RemoveAppStateObserver(
   app_state_observers_.RemoveObserver(observer);
 }
 
+void AppActivityRegistry::SetInstalledApps(
+    const std::vector<AppId>& installed_apps) {
+  for (const auto& app : installed_apps)
+    OnAppInstalled(app);
+}
+
 base::TimeDelta AppActivityRegistry::GetActiveTime(const AppId& app_id) const {
   DCHECK(base::Contains(activity_registry_, app_id));
   return activity_registry_.at(app_id).activity.RunningActiveTime();
@@ -337,23 +340,6 @@ base::Optional<base::TimeDelta> AppActivityRegistry::GetTimeLimit(
   return limit->daily_limit();
 }
 
-std::vector<PauseAppInfo> AppActivityRegistry::GetPausedApps(
-    bool show_pause_dialog) const {
-  std::vector<PauseAppInfo> paused_apps;
-  for (const auto& info : activity_registry_) {
-    const AppId& app_id = info.first;
-    const AppDetails& details = info.second;
-    if (GetAppState(app_id) == AppState::kLimitReached) {
-      DCHECK(details.limit.has_value());
-      DCHECK(details.limit->daily_limit().has_value());
-      paused_apps.push_back(PauseAppInfo(
-          app_id, details.limit->daily_limit().value(), show_pause_dialog));
-    }
-  }
-
-  return paused_apps;
-}
-
 AppActivityReportInterface::ReportParams
 AppActivityRegistry::GenerateAppActivityReport(
     enterprise_management::ChildStatusReportRequest* report) {
@@ -672,13 +658,7 @@ void AppActivityRegistry::SetAppState(const AppId& app_id, AppState app_state) {
       SetAppInactive(app_id, base::Time::Now());
     }
 
-    for (auto& observer : app_state_observers_) {
-      const base::Optional<AppLimit>& limit =
-          activity_registry_.at(app_id).limit;
-      DCHECK(limit->daily_limit());
-      observer.OnAppLimitReached(app_id, limit->daily_limit().value(),
-                                 was_active);
-    }
+    NotifyLimitReached(app_id, was_active);
     return;
   }
 
@@ -690,6 +670,19 @@ void AppActivityRegistry::SetAppState(const AppId& app_id, AppState app_state) {
   }
 }
 
+void AppActivityRegistry::NotifyLimitReached(const AppId& app_id,
+                                             bool was_active) {
+  DCHECK(base::Contains(activity_registry_, app_id));
+  DCHECK_EQ(GetAppState(app_id), AppState::kLimitReached);
+
+  const base::Optional<AppLimit>& limit = activity_registry_.at(app_id).limit;
+  DCHECK(limit->daily_limit());
+  for (auto& observer : app_state_observers_) {
+    observer.OnAppLimitReached(app_id, limit->daily_limit().value(),
+                               was_active);
+  }
+}
+
 void AppActivityRegistry::SetAppActive(const AppId& app_id,
                                        base::Time timestamp) {
   DCHECK(base::Contains(activity_registry_, app_id));

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

@@ -8,6 +8,7 @@
 #include <map>
 #include <memory>
 #include <set>
+#include <vector>
 
 #include "base/observer_list_types.h"
 #include "base/optional.h"
@@ -109,6 +110,10 @@ class AppActivityRegistry : public AppServiceWrapper::EventListener {
   void AddAppStateObserver(AppStateObserver* observer);
   void RemoveAppStateObserver(AppStateObserver* observer);
 
+  // Called from AppTimeController to notify AppActivityRegistry about installed
+  // apps which AppActivityRegistry may have missed.
+  void SetInstalledApps(const std::vector<AppId>& installed_apps);
+
   // Returns the total active time for the application since the last time limit
   // reset.
   base::TimeDelta GetActiveTime(const AppId& app_id) const;
@@ -123,9 +128,6 @@ class AppActivityRegistry : public AppServiceWrapper::EventListener {
   // Will return nullopt if there is no limit set.
   base::Optional<base::TimeDelta> GetTimeLimit(const AppId& app_id) const;
 
-  // Returns the vector of paused applications.
-  std::vector<PauseAppInfo> GetPausedApps(bool show_pause_dialog) const;
-
   // Populates |report| with collected app activity. Returns whether any data
   // were reported.
   AppActivityReportInterface::ReportParams GenerateAppActivityReport(
@@ -204,6 +206,10 @@ class AppActivityRegistry : public AppServiceWrapper::EventListener {
   // Should only be called if app exists in the registry.
   void SetAppState(const AppId& app_id, AppState app_state);
 
+  // Notifies state observers the application identified by |app_id| has reached
+  // its set time limit.
+  void NotifyLimitReached(const AppId& app_id, bool was_active);
+
   // Methods to set the application as active and inactive respectively.
   void SetAppActive(const AppId& app_id, base::Time timestamp);
   void SetAppInactive(const AppId& app_id, base::Time timestamp);

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

@@ -365,7 +365,8 @@ TEST_F(AppActivityRegistryTest, ResetTimeReached) {
   const AppLimit limit1(AppRestriction::kTimeLimit, kTenMinutes, start);
   const AppLimit limit2(AppRestriction::kTimeLimit,
                         base::TimeDelta::FromMinutes(20), start);
-  const std::map<AppId, AppLimit> limits{{kApp1, limit1}, {kApp2, limit2}};
+  const std::map<AppId, AppLimit> limits{{kApp1, limit1},
+                                         {GetChromeAppId(), limit2}};
   registry().UpdateAppLimits(limits);
 
   auto* app1_window = CreateWindowForApp(kApp1);
@@ -811,6 +812,22 @@ TEST_F(AppActivityRegistryTest, OverrideLimitReachedState) {
   ReInitializeRegistry();
   registry().AddAppStateObserver(&state_observer_mock);
   registry().UpdateAppLimits(app_limits);
+
+  // When OnAppInstalled is called for AppActivityRegistry, it will notify its
+  // app state observers that the app time limit has been reached.
+  EXPECT_CALL(state_observer_mock,
+              OnAppLimitReached(kApp1, base::TimeDelta::FromMinutes(30),
+                                /* was_active */ false))
+      .Times(1);
+  EXPECT_CALL(state_observer_mock,
+              OnAppLimitReached(kApp2, base::TimeDelta::FromMinutes(30),
+                                /* was_active */ false))
+      .Times(1);
+  EXPECT_CALL(
+      state_observer_mock,
+      OnAppLimitReached(GetChromeAppId(), base::TimeDelta::FromMinutes(30),
+                        /* was_active */ false))
+      .Times(1);
   InstallApps();
 
   EXPECT_EQ(registry().GetAppState(kApp1), AppState::kLimitReached);

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

@@ -253,13 +253,11 @@ AppTimeController::AppTimeController(Profile* profile)
   if (time_zone_settings)
     time_zone_settings->AddObserver(this);
 
-  // At this point application states should have been restored. Get the paused
-  // applications and notify app service. Don't show dialog for the paused apps.
-  app_service_wrapper_->PauseApps(
-      app_registry_->GetPausedApps(/* show_pause_dialog */ false));
-
   // Start observing |app_registry_|
   app_registry_->AddAppStateObserver(this);
+
+  // AppActivityRegistry may have missed |OnAppInstalled| calls. Notify it.
+  app_registry_->SetInstalledApps(app_service_wrapper_->GetInstalledApps());
 }
 
 AppTimeController::~AppTimeController() {