Add the account id for OnAppUpdate.

When a component observes AppRegistryCaches for the multiple users
case (e.g. Ash), OnAppUpdate could be called for multiple
AppRegistryCaches. Add the account id in the AppRegistryCaches and
AppUpdate class, so that the AppRegistryCache observer can know the
current AppUpdate is for which account in the OnAppUpdate interface.

Unit tests are modified to test the account id in the OnAppUpdate
interface.

BUG=1068884

Change-Id: Id07b2e8480330667c45ff11fe1cf21d3ebe048ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2222244Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Nancy Wang <nancylingwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773991}

chrome/browser/apps/app_service/app_service_proxy.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_features.h"
+#include "components/account_id/account_id.h"
 #include "components/services/app_service/app_service_impl.h"
 #include "components/services/app_service/public/cpp/app_registry_cache_wrapper.h"
 #include "components/services/app_service/public/cpp/intent_filter_util.h"
@@ -32,7 +33,6 @@
 #include "chrome/browser/supervised_user/grit/supervised_user_unscaled_resources.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "components/user_manager/user.h"
-#include "components/user_manager/user_manager.h"
 #include "extensions/common/constants.h"
 #endif
 
@@ -130,11 +130,12 @@ void AppServiceProxy::Initialize() {
   }
 
 #if defined(OS_CHROMEOS)
-  if (user_manager::UserManager::Get() &&
-      user_manager::UserManager::Get()->GetActiveUser()) {
-    AppRegistryCacheWrapper::Get().AddAppRegistryCache(
-        user_manager::UserManager::Get()->GetActiveUser()->GetAccountId(),
-        &cache_);
+  const user_manager::User* user =
+      chromeos::ProfileHelper::Get()->GetUserByProfile(profile_);
+  if (user) {
+    cache_.SetAccountId(user->GetAccountId());
+    AppRegistryCacheWrapper::Get().AddAppRegistryCache(user->GetAccountId(),
+                                                       &cache_);
   }
 #endif
 

chrome/browser/ui/webui/chromeos/add_supervision/add_supervision_handler_utils_unittest.cc

@@ -13,12 +13,13 @@ TEST_F(AddSupervisionHandlerUtilsTest, TestShouldIncludeAppUpdate) {
   // Return ARC apps.
   apps::mojom::App arc_state;
   arc_state.app_type = apps::mojom::AppType::kArc;
-  apps::AppUpdate arc_update(&arc_state, nullptr /* delta */);
+  apps::AppUpdate arc_update(&arc_state, nullptr /* delta */, EmptyAccountId());
   EXPECT_TRUE(ShouldIncludeAppUpdate(arc_update));
 
   // Don't return non-ARC apps.
   apps::mojom::App non_arc_state;
   non_arc_state.app_type = apps::mojom::AppType::kBuiltIn;
-  apps::AppUpdate non_arc_update(&non_arc_state, nullptr /* delta */);
+  apps::AppUpdate non_arc_update(&non_arc_state, nullptr /* delta */,
+                                 EmptyAccountId());
   EXPECT_FALSE(ShouldIncludeAppUpdate(non_arc_update));
 }

components/services/app_service/DEPS

@@ -8,6 +8,12 @@ specific_include_rules = {
   "app_registry_cache_wrapper\.cc": [
     "+components/account_id/account_id.h",
   ],
+  "app_registry_cache\.h": [
+    "+components/account_id/account_id.h",
+  ],
+  "app_update\.h": [
+    "+components/account_id/account_id.h",
+  ],
   "instance_registry\.h": [
     "+ash/public/cpp/shelf_types.h",
   ],

components/services/app_service/public/cpp/app_registry_cache.cc

@@ -34,7 +34,7 @@ void AppRegistryCache::Observer::Observe(AppRegistryCache* cache) {
   }
 }
 
-AppRegistryCache::AppRegistryCache() = default;
+AppRegistryCache::AppRegistryCache() : account_id_(EmptyAccountId()) {}
 
 AppRegistryCache::~AppRegistryCache() {
   for (auto& obs : observers_) {
@@ -92,7 +92,7 @@ void AppRegistryCache::DoOnApps(std::vector<apps::mojom::AppPtr> deltas) {
     apps::mojom::App* delta = d_iter.second;
 
     for (auto& obs : observers_) {
-      obs.OnAppUpdate(AppUpdate(state, delta));
+      obs.OnAppUpdate(AppUpdate(state, delta, account_id_));
     }
   }
 
@@ -126,4 +126,8 @@ apps::mojom::AppType AppRegistryCache::GetAppType(const std::string& app_id) {
   return apps::mojom::AppType::kUnknown;
 }
 
+void AppRegistryCache::SetAccountId(const AccountId& account_id) {
+  account_id_ = account_id;
+}
+
 }  // namespace apps

components/services/app_service/public/cpp/app_registry_cache.h

@@ -12,6 +12,7 @@
 #include "base/observer_list.h"
 #include "base/observer_list_types.h"
 #include "base/sequence_checker.h"
+#include "components/account_id/account_id.h"
 #include "components/services/app_service/public/cpp/app_update.h"
 
 namespace apps {
@@ -95,6 +96,8 @@ class AppRegistryCache {
 
   apps::mojom::AppType GetAppType(const std::string& app_id);
 
+  void SetAccountId(const AccountId& account_id);
+
   // Calls f, a void-returning function whose arguments are (const
   // apps::AppUpdate&), on each app in the cache.
   //
@@ -118,7 +121,7 @@ class AppRegistryCache {
       const apps::mojom::App* delta =
           (d_iter != deltas_in_progress_.end()) ? d_iter->second : nullptr;
 
-      f(apps::AppUpdate(state, delta));
+      f(apps::AppUpdate(state, delta, account_id_));
     }
 
     for (const auto& d_iter : deltas_in_progress_) {
@@ -129,7 +132,7 @@ class AppRegistryCache {
         continue;
       }
 
-      f(apps::AppUpdate(nullptr, delta));
+      f(apps::AppUpdate(nullptr, delta, account_id_));
     }
   }
 
@@ -154,7 +157,7 @@ class AppRegistryCache {
         (d_iter != deltas_in_progress_.end()) ? d_iter->second : nullptr;
 
     if (state || delta) {
-      f(apps::AppUpdate(state, delta));
+      f(apps::AppUpdate(state, delta, account_id_));
       return true;
     }
     return false;
@@ -186,6 +189,8 @@ class AppRegistryCache {
   std::map<std::string, apps::mojom::App*> deltas_in_progress_;
   std::vector<apps::mojom::AppPtr> deltas_pending_;
 
+  AccountId account_id_;
+
   SEQUENCE_CHECKER(my_sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(AppRegistryCache);

components/services/app_service/public/cpp/app_registry_cache_unittest.cc

@@ -38,6 +38,7 @@ class AppRegistryCacheTest : public testing::Test,
 
   // apps::AppRegistryCache::Observer overrides.
   void OnAppUpdate(const apps::AppUpdate& update) override {
+    EXPECT_EQ(account_id_, update.AccountId());
     EXPECT_NE("", update.Name());
     if (update.ReadinessChanged() &&
         (update.Readiness() == apps::mojom::Readiness::kReady)) {
@@ -53,9 +54,12 @@ class AppRegistryCacheTest : public testing::Test,
     NOTREACHED();
   }
 
+  const AccountId& account_id() const { return account_id_; }
+
   int num_freshly_installed_ = 0;
   std::set<std::string> updated_ids_;
   std::set<std::string> updated_names_;
+  AccountId account_id_ = AccountId::FromUserEmail("test@gmail.com");
 };
 
 // Responds to a cache's OnAppUpdate to call back into the cache, checking that
@@ -96,6 +100,7 @@ class RecursiveObserver : public apps::AppRegistryCache::Observer {
  protected:
   // apps::AppRegistryCache::Observer overrides.
   void OnAppUpdate(const apps::AppUpdate& outer) override {
+    EXPECT_EQ(account_id_, outer.AccountId());
     int num_apps = 0;
     cache_->ForEachApp([this, &outer, &num_apps](const apps::AppUpdate& inner) {
       if (check_names_snapshot_) {
@@ -180,6 +185,7 @@ class RecursiveObserver : public apps::AppRegistryCache::Observer {
   std::string expected_name_for_p_;
   int expected_num_apps_;
   int num_apps_seen_on_app_update_;
+  AccountId account_id_ = AccountId::FromUserEmail("test@gmail.com");
 
   // Records previously seen app names, keyed by app_id's, so we can check
   // that, for these tests, a given app's name is always increasing (in string
@@ -206,6 +212,7 @@ class RecursiveObserver : public apps::AppRegistryCache::Observer {
 TEST_F(AppRegistryCacheTest, ForEachApp) {
   std::vector<apps::mojom::AppPtr> deltas;
   apps::AppRegistryCache cache;
+  cache.SetAccountId(account_id());
 
   updated_names_.clear();
   CallForEachApp(cache);
@@ -260,6 +267,7 @@ TEST_F(AppRegistryCacheTest, ForEachApp) {
 TEST_F(AppRegistryCacheTest, Observer) {
   std::vector<apps::mojom::AppPtr> deltas;
   apps::AppRegistryCache cache;
+  cache.SetAccountId(account_id());
 
   cache.AddObserver(this);
 
@@ -304,6 +312,7 @@ TEST_F(AppRegistryCacheTest, Observer) {
 TEST_F(AppRegistryCacheTest, Recursive) {
   std::vector<apps::mojom::AppPtr> deltas;
   apps::AppRegistryCache cache;
+  cache.SetAccountId(account_id());
   RecursiveObserver observer(&cache);
 
   observer.PrepareForOnApps(2, "peach");
@@ -332,6 +341,7 @@ TEST_F(AppRegistryCacheTest, Recursive) {
 TEST_F(AppRegistryCacheTest, SuperRecursive) {
   std::vector<apps::mojom::AppPtr> deltas;
   apps::AppRegistryCache cache;
+  cache.SetAccountId(account_id());
   RecursiveObserver observer(&cache);
 
   // Set up a series of OnApps to be called during observer.OnAppUpdate:

components/services/app_service/public/cpp/app_update.cc

@@ -128,8 +128,9 @@ void AppUpdate::Merge(apps::mojom::App* state, const apps::mojom::App* delta) {
 }
 
 AppUpdate::AppUpdate(const apps::mojom::App* state,
-                     const apps::mojom::App* delta)
-    : state_(state), delta_(delta) {
+                     const apps::mojom::App* delta,
+                     const ::AccountId& account_id)
+    : state_(state), delta_(delta), account_id_(account_id) {
   DCHECK(state_ || delta_);
   if (state_ && delta_) {
     DCHECK(state_->app_type == delta->app_type);
@@ -499,4 +500,8 @@ bool AppUpdate::IntentFiltersChanged() const {
          (!state_ || (delta_->intent_filters != state_->intent_filters));
 }
 
+const ::AccountId& AppUpdate::AccountId() const {
+  return account_id_;
+}
+
 }  // namespace apps

components/services/app_service/public/cpp/app_update.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "components/account_id/account_id.h"
 #include "components/services/app_service/public/mojom/types.mojom.h"
 
 namespace apps {
@@ -48,7 +49,9 @@ class AppUpdate {
   static void Merge(apps::mojom::App* state, const apps::mojom::App* delta);
 
   // At most one of |state| or |delta| may be nullptr.
-  AppUpdate(const apps::mojom::App* state, const apps::mojom::App* delta);
+  AppUpdate(const apps::mojom::App* state,
+            const apps::mojom::App* delta,
+            const AccountId& account_id);
 
   // Returns whether this is the first update for the given AppId.
   // Equivalently, there are no previous deltas for the AppId.
@@ -126,10 +129,14 @@ class AppUpdate {
   std::vector<apps::mojom::IntentFilterPtr> IntentFilters() const;
   bool IntentFiltersChanged() const;
 
+  const ::AccountId& AccountId() const;
+
  private:
   const apps::mojom::App* state_;
   const apps::mojom::App* delta_;
 
+  const ::AccountId& account_id_;
+
   DISALLOW_COPY_AND_ASSIGN(AppUpdate);
 };
 

components/services/app_service/public/cpp/app_update_unittest.cc

@@ -78,6 +78,8 @@ class AppUpdateTest : public testing::Test {
   std::vector<apps::mojom::IntentFilterPtr> expect_intent_filters_;
   bool expect_intent_filters_changed_;
 
+  AccountId account_id_ = AccountId::FromUserEmail("test@gmail.com");
+
   static constexpr uint32_t kPermissionTypeLocation = 100;
   static constexpr uint32_t kPermissionTypeNotification = 200;
 
@@ -178,10 +180,12 @@ class AppUpdateTest : public testing::Test {
 
     EXPECT_EQ(expect_intent_filters_, u.IntentFilters());
     EXPECT_EQ(expect_intent_filters_changed_, u.IntentFiltersChanged());
+
+    EXPECT_EQ(account_id_, u.AccountId());
   }
 
   void TestAppUpdate(apps::mojom::App* state, apps::mojom::App* delta) {
-    apps::AppUpdate u(state, delta);
+    apps::AppUpdate u(state, delta, account_id_);
 
     EXPECT_EQ(app_type, u.AppType());
     EXPECT_EQ(app_id, u.AppId());