kiosk: Refactor KioskAppManagerBase::GetApps to return a vector

This change makes GetApp return the vector of apps instead of taking an
std::vector* output parameter.

Bug: b:306595222
Test: testing/xvfb.py tools/autotest.py -C out_/Default --run_all \
  chrome/browser/ash/app_mode/arc/arc_kiosk_app_manager_browsertest.cc \
  chrome/browser/ash/app_mode/kiosk_app_manager_browsertest.cc

Change-Id: I8638820756171a0b4de2230f1b16b72b174e6ef3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4984029
Reviewed-by: Irina Fedorova <irfedorova@google.com>
Commit-Queue: Edman Anjos <edman@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216905}

chrome/browser/ash/app_mode/arc/arc_kiosk_app_manager.cc

@@ -63,27 +63,28 @@ ArcKioskAppManager::~ArcKioskAppManager() {
 const ArcKioskAppData* ArcKioskAppManager::GetAppByAccountId(
     const AccountId& account_id) {
   for (auto& app : apps_) {
-    if (app->account_id() == account_id)
+    if (app->account_id() == account_id) {
       return app.get();
+    }
   }
   return nullptr;
 }
 
-void ArcKioskAppManager::GetApps(std::vector<App>* apps) const {
-  apps->clear();
-  apps->reserve(apps_.size());
-  for (auto& app : apps_) {
-    apps->emplace_back(*app.get());
+std::vector<ArcKioskAppManager::App> ArcKioskAppManager::GetApps() const {
+  std::vector<App> apps;
+  for (const auto& app : apps_) {
+    apps.emplace_back(*app);
   }
+  return apps;
 }
 
-void ArcKioskAppManager::GetAppsForTesting(
-    std::vector<const ArcKioskAppData*>* apps) const {
-  apps->clear();
-  apps->reserve(apps_.size());
-  for (auto& app : apps_) {
-    apps->push_back(app.get());
+std::vector<const ArcKioskAppData*> ArcKioskAppManager::GetAppsForTesting()
+    const {
+  std::vector<const ArcKioskAppData*> apps;
+  for (const auto& app : apps_) {
+    apps.push_back(app.get());
   }
+  return apps;
 }
 
 void ArcKioskAppManager::UpdateNameAndIcon(const AccountId& account_id,
@@ -133,8 +134,9 @@ void ArcKioskAppManager::UpdateAppsFromPolicy() {
   // Store current apps. We will compare old and new apps to determine which
   // apps are new, and which were deleted.
   std::map<std::string, std::unique_ptr<ArcKioskAppData>> old_apps;
-  for (auto& app : apps_)
+  for (auto& app : apps_) {
     old_apps[app->app_id()] = std::move(app);
+  }
   apps_.clear();
   auto_launch_account_id_.clear();
   auto_launched_with_zero_delay_ = false;
@@ -146,8 +148,9 @@ void ArcKioskAppManager::UpdateAppsFromPolicy() {
   const std::vector<policy::DeviceLocalAccount> device_local_accounts =
       policy::GetDeviceLocalAccounts(CrosSettings::Get());
   for (auto account : device_local_accounts) {
-    if (account.type != policy::DeviceLocalAccount::TYPE_ARC_KIOSK_APP)
+    if (account.type != policy::DeviceLocalAccount::TYPE_ARC_KIOSK_APP) {
       continue;
+    }
 
     const AccountId account_id(AccountId::FromUserEmail(account.user_id));
 
@@ -175,8 +178,9 @@ void ArcKioskAppManager::UpdateAppsFromPolicy() {
     } else {
       // Use package name when display name is not specified.
       std::string name = app_info.package_name();
-      if (!app_info.display_name().empty())
+      if (!app_info.display_name().empty()) {
         name = app_info.display_name();
+      }
       apps_.push_back(std::make_unique<ArcKioskAppData>(
           app_id, app_info.package_name(), app_info.class_name(),
           app_info.action(), account_id, name));
@@ -186,8 +190,9 @@ void ArcKioskAppManager::UpdateAppsFromPolicy() {
   }
 
   std::vector<KioskAppDataBase*> old_apps_to_remove;
-  for (auto& entry : old_apps)
+  for (auto& entry : old_apps) {
     old_apps_to_remove.emplace_back(entry.second.get());
+  }
   ClearRemovedApps(old_apps_to_remove);
 
   NotifyKioskAppsChanged();

chrome/browser/ash/app_mode/arc/arc_kiosk_app_manager.h

@@ -40,7 +40,7 @@ class ArcKioskAppManager : public KioskAppManagerBase {
   const ArcKioskAppData* GetAppByAccountId(const AccountId& account_id);
 
   // KioskAppManagerBase:
-  void GetApps(std::vector<App>* apps) const override;
+  std::vector<App> GetApps() const override;
 
   void UpdateNameAndIcon(const AccountId& account_id,
                          const std::string& name,
@@ -56,12 +56,10 @@ class ArcKioskAppManager : public KioskAppManagerBase {
   // returns empty account id.
   const AccountId& GetAutoLaunchAccountId() const;
 
- private:
-  friend class ArcKioskAppManagerTest;
   // Returns the list of all apps in their internal representation.
-  void GetAppsForTesting(
-      std::vector<const ArcKioskAppData*>* apps_internal) const;
+  std::vector<const ArcKioskAppData*> GetAppsForTesting() const;
 
+ private:
   // KioskAppmanagerBase:
   // Updates `apps_` based on CrosSettings.
   void UpdateAppsFromPolicy() override;

chrome/browser/ash/app_mode/arc/arc_kiosk_app_manager_browsertest.cc

@@ -123,10 +123,6 @@ class ArcKioskAppManagerTest : public InProcessBrowserTest {
                                  device_local_accounts);
   }
 
-  void GetApps(std::vector<const ArcKioskAppData*>* apps) const {
-    manager()->GetAppsForTesting(apps);
-  }
-
   ArcKioskAppManager* manager() const { return ArcKioskAppManager::Get(); }
 
  protected:
@@ -147,8 +143,7 @@ IN_PROC_BROWSER_TEST_F(ArcKioskAppManagerTest, Basic) {
     SetApps(init_apps, std::string());
     waiter.Wait(1);
 
-    std::vector<const ArcKioskAppData*> apps;
-    GetApps(&apps);
+    std::vector<const ArcKioskAppData*> apps = manager()->GetAppsForTesting();
     ASSERT_EQ(2u, apps.size());
     ASSERT_EQ(app1.package_name(), apps[0]->package_name());
     ASSERT_EQ(app2.package_name(), apps[1]->package_name());
@@ -169,8 +164,7 @@ IN_PROC_BROWSER_TEST_F(ArcKioskAppManagerTest, Basic) {
 
     EXPECT_TRUE(manager()->GetAutoLaunchAccountId().is_valid());
 
-    std::vector<const ArcKioskAppData*> apps;
-    GetApps(&apps);
+    std::vector<const ArcKioskAppData*> apps = manager()->GetAppsForTesting();
     ASSERT_EQ(2u, apps.size());
     ASSERT_EQ(app1.package_name(), apps[0]->package_name());
     ASSERT_EQ(app2.package_name(), apps[1]->package_name());
@@ -191,8 +185,7 @@ IN_PROC_BROWSER_TEST_F(ArcKioskAppManagerTest, Basic) {
     SetApps(new_apps, std::string());
     waiter.Wait(1);
 
-    std::vector<const ArcKioskAppData*> apps;
-    GetApps(&apps);
+    std::vector<const ArcKioskAppData*> apps = manager()->GetAppsForTesting();
     ASSERT_EQ(2u, apps.size());
     ASSERT_EQ(app1.package_name(), apps[0]->package_name());
     ASSERT_EQ(app3.package_name(), apps[1]->package_name());
@@ -210,9 +203,7 @@ IN_PROC_BROWSER_TEST_F(ArcKioskAppManagerTest, Basic) {
     CleanApps();
     waiter.Wait(1);
 
-    std::vector<const ArcKioskAppData*> apps;
-    GetApps(&apps);
-    ASSERT_EQ(0u, apps.size());
+    ASSERT_EQ(0u, manager()->GetAppsForTesting().size());
     EXPECT_FALSE(manager()->GetAutoLaunchAccountId().is_valid());
   }
 }
@@ -226,10 +217,9 @@ IN_PROC_BROWSER_TEST_F(ArcKioskAppManagerTest, GetAppByAccountId) {
   SetApps(init_apps, std::string());
 
   // Verify the app data searched by account id.
-  std::vector<const ArcKioskAppData*> apps;
-  GetApps(&apps);
+  std::vector<const ArcKioskAppData*> apps = manager()->GetAppsForTesting();
   ASSERT_EQ(1u, apps.size());
-  const ArcKioskAppData* app = apps.front();
+  const ArcKioskAppData* app = apps[0];
   const ArcKioskAppData* app_by_account_id =
       manager()->GetAppByAccountId(app->account_id());
   ASSERT_TRUE(app_by_account_id);
@@ -255,10 +245,9 @@ IN_PROC_BROWSER_TEST_F(ArcKioskAppManagerTest, UpdateNameAndIcon) {
   SetApps(init_apps, std::string());
 
   // Verify the initialized app data.
-  std::vector<const ArcKioskAppData*> apps;
-  GetApps(&apps);
+  std::vector<const ArcKioskAppData*> apps = manager()->GetAppsForTesting();
   ASSERT_EQ(1u, apps.size());
-  const ArcKioskAppData* app = apps.front();
+  const ArcKioskAppData* app = apps[0];
   ASSERT_EQ(app->name(), package_name);
   ASSERT_TRUE(app->icon().isNull());
 

chrome/browser/ash/app_mode/kiosk_app_manager.cc

@@ -564,14 +564,14 @@ void KioskAppManager::RemoveApp(const std::string& app_id,
   policy::SetDeviceLocalAccounts(service, device_local_accounts);
 }
 
-void KioskAppManager::GetApps(Apps* apps) const {
-  apps->clear();
+std::vector<KioskAppManager::App> KioskAppManager::GetApps() const {
+  std::vector<App> apps;
   for (const auto& app : apps_) {
-    const KioskAppData& app_data = *app;
-    if (app_data.status() != KioskAppData::Status::kError) {
-      apps->push_back(ConstructApp(app_data));
+    if (app->status() != KioskAppData::Status::kError) {
+      apps.push_back(ConstructApp(*app));
     }
   }
+  return apps;
 }
 
 KioskAppManager::App KioskAppManager::ConstructApp(

chrome/browser/ash/app_mode/kiosk_app_manager.h

@@ -184,7 +184,7 @@ class KioskAppManager : public KioskAppManagerBase,
 
   // KioskAppManagerBase:
   // Gets info of all apps that have no meta data load error.
-  void GetApps(Apps* apps) const override;
+  std::vector<App> GetApps() const override;
 
   // Gets app data for the given app id. Returns true if `app_id` is known and
   // `app` is populated. Otherwise, return false.

chrome/browser/ash/app_mode/kiosk_app_manager_base.h

@@ -56,7 +56,7 @@ class KioskAppManagerBase : public KioskAppDataDelegate {
 
   // Depends on the app internal representation for the particular type of
   // kiosk.
-  virtual void GetApps(AppList* apps) const = 0;
+  virtual AppList GetApps() const = 0;
 
   void AddObserver(KioskAppManagerObserver* observer);
   void RemoveObserver(KioskAppManagerObserver* observer);

chrome/browser/ash/app_mode/kiosk_app_manager_browsertest.cc

@@ -205,8 +205,7 @@ class KioskAppManagerTest : public InProcessBrowserTest {
   }
 
   std::string GetAppIds() const {
-    KioskAppManager::AppList apps;
-    manager()->GetApps(&apps);
+    KioskAppManager::AppList apps = manager()->GetApps();
 
     std::string str;
     for (size_t i = 0; i < apps.size(); ++i) {
@@ -285,8 +284,7 @@ class KioskAppManagerTest : public InProcessBrowserTest {
                     const std::string& expected_app_name,
                     const std::string& expected_required_platform_version) {
     // Check manifest data is cached correctly.
-    KioskAppManager::AppList apps;
-    manager()->GetApps(&apps);
+    KioskAppManager::AppList apps = manager()->GetApps();
     ASSERT_EQ(1u, apps.size());
     EXPECT_EQ(app_id, apps[0].app_id);
     EXPECT_EQ(expected_app_name, apps[0].name);
@@ -671,9 +669,7 @@ IN_PROC_BROWSER_TEST_F(KioskAppManagerTest, DownloadNewApp) {
 IN_PROC_BROWSER_TEST_F(KioskAppManagerTest, RemoveApp) {
   // Add a new app.
   RunAddNewAppTest(kTestLocalFsKioskApp, "1.0.0", kTestLocalFsKioskAppName, "");
-  KioskAppManager::AppList apps;
-  manager()->GetApps(&apps);
-  ASSERT_EQ(1u, apps.size());
+  ASSERT_EQ(1u, manager()->GetApps().size());
   base::FilePath crx_path;
   std::string version;
   EXPECT_TRUE(GetCachedCrx(kTestLocalFsKioskApp, &crx_path, &version));
@@ -686,8 +682,7 @@ IN_PROC_BROWSER_TEST_F(KioskAppManagerTest, RemoveApp) {
   // Remove the app now.
   manager()->RemoveApp(kTestLocalFsKioskApp, owner_settings_service_.get());
   content::RunAllTasksUntilIdle();
-  manager()->GetApps(&apps);
-  ASSERT_EQ(0u, apps.size());
+  ASSERT_EQ(0u, manager()->GetApps().size());
   {
     base::ScopedAllowBlockingForTesting allow_io;
     EXPECT_FALSE(base::PathExists(crx_path));
@@ -703,9 +698,7 @@ IN_PROC_BROWSER_TEST_F(KioskAppManagerTest, UpdateApp) {
 
   // Add a version 1 app first.
   RunAddNewAppTest(kTestLocalFsKioskApp, "1.0.0", kTestLocalFsKioskAppName, "");
-  KioskAppManager::AppList apps;
-  manager()->GetApps(&apps);
-  ASSERT_EQ(1u, apps.size());
+  ASSERT_EQ(1u, manager()->GetApps().size());
   base::FilePath crx_path;
   std::string version;
   EXPECT_TRUE(GetCachedCrx(kTestLocalFsKioskApp, &crx_path, &version));
@@ -726,8 +719,7 @@ IN_PROC_BROWSER_TEST_F(KioskAppManagerTest, UpdateApp) {
   EXPECT_EQ(waiter.data_load_failure_count(), 0);
 
   // Verify the app has been updated to v2.
-  manager()->GetApps(&apps);
-  ASSERT_EQ(1u, apps.size());
+  ASSERT_EQ(1u, manager()->GetApps().size());
   base::FilePath new_crx_path;
   std::string new_version;
   EXPECT_TRUE(GetCachedCrx(kTestLocalFsKioskApp, &new_crx_path, &new_version));
@@ -757,9 +749,7 @@ IN_PROC_BROWSER_TEST_F(KioskAppManagerTest, UpdateApp) {
 IN_PROC_BROWSER_TEST_F(KioskAppManagerTest, UpdateAndRemoveApp) {
   // Add a version 1 app first.
   RunAddNewAppTest(kTestLocalFsKioskApp, "1.0.0", kTestLocalFsKioskAppName, "");
-  KioskAppManager::AppList apps;
-  manager()->GetApps(&apps);
-  ASSERT_EQ(1u, apps.size());
+  ASSERT_EQ(1u, manager()->GetApps().size());
   base::FilePath v1_crx_path;
   std::string version;
   EXPECT_TRUE(GetCachedCrx(kTestLocalFsKioskApp, &v1_crx_path, &version));
@@ -780,8 +770,7 @@ IN_PROC_BROWSER_TEST_F(KioskAppManagerTest, UpdateAndRemoveApp) {
   EXPECT_EQ(waiter.data_load_failure_count(), 0);
 
   // Verify the app has been updated to v2.
-  manager()->GetApps(&apps);
-  ASSERT_EQ(1u, apps.size());
+  ASSERT_EQ(1u, manager()->GetApps().size());
   base::FilePath v2_crx_path;
   std::string new_version;
   EXPECT_TRUE(GetCachedCrx(kTestLocalFsKioskApp, &v2_crx_path, &new_version));
@@ -796,8 +785,7 @@ IN_PROC_BROWSER_TEST_F(KioskAppManagerTest, UpdateAndRemoveApp) {
   // Remove the app now.
   manager()->RemoveApp(kTestLocalFsKioskApp, owner_settings_service_.get());
   content::RunAllTasksUntilIdle();
-  manager()->GetApps(&apps);
-  ASSERT_EQ(0u, apps.size());
+  ASSERT_EQ(0u, manager()->GetApps().size());
   // Verify both v1 and v2 crx files are removed.
   {
     base::ScopedAllowBlockingForTesting allow_io;

chrome/browser/ash/app_mode/kiosk_controller.cc

@@ -72,24 +72,18 @@ KioskController::~KioskController() {
 }
 
 std::vector<KioskApp> KioskController::GetApps() const {
-  std::vector<KioskAppManagerBase::App> web_apps;
-  web_app_manager_->GetApps(&web_apps);
-  std::vector<KioskAppManagerBase::App> chrome_apps;
-  chrome_app_manager_->GetApps(&chrome_apps);
-  std::vector<KioskAppManagerBase::App> arc_apps;
-  arc_app_manager_->GetApps(&arc_apps);
-
   std::vector<KioskApp> apps;
-  for (const auto& web_app : web_apps) {
+  for (const KioskAppManagerBase::App& web_app : web_app_manager_->GetApps()) {
     apps.emplace_back(KioskAppId::ForWebApp(web_app.account_id), web_app.name,
                       web_app.icon);
   }
-  for (const auto& chrome_app : chrome_apps) {
+  for (const KioskAppManagerBase::App& chrome_app :
+       chrome_app_manager_->GetApps()) {
     apps.emplace_back(
         KioskAppId::ForChromeApp(chrome_app.app_id, chrome_app.account_id),
         chrome_app.name, chrome_app.icon);
   }
-  for (const auto& arc_app : arc_apps) {
+  for (const KioskAppManagerBase::App& arc_app : arc_app_manager_->GetApps()) {
     apps.emplace_back(KioskAppId::ForArcApp(arc_app.account_id), arc_app.name,
                       arc_app.icon);
   }

chrome/browser/ash/app_mode/web_app/web_kiosk_app_manager.cc

@@ -63,14 +63,15 @@ WebKioskAppManager::~WebKioskAppManager() {
   g_web_kiosk_app_manager = nullptr;
 }
 
-void WebKioskAppManager::GetApps(std::vector<App>* apps) const {
-  apps->clear();
-  apps->reserve(apps_.size());
-  for (auto& web_app : apps_) {
-    App app(*web_app);
-    app.url = web_app->install_url();
-    apps->push_back(std::move(app));
+std::vector<WebKioskAppManager::App> WebKioskAppManager::GetApps() const {
+  std::vector<App> apps;
+  apps.reserve(apps_.size());
+  for (const auto& manager_app : apps_) {
+    App app(*manager_app);
+    app.url = manager_app->install_url();
+    apps.push_back(std::move(app));
   }
+  return apps;
 }
 
 void WebKioskAppManager::LoadIcons() {

chrome/browser/ash/app_mode/web_app/web_kiosk_app_manager.h

@@ -47,7 +47,7 @@ class WebKioskAppManager : public KioskAppManagerBase {
   static KioskAppManagerBase::App CreateAppByData(const WebKioskAppData& data);
 
   // KioskAppManagerBase:
-  void GetApps(std::vector<App>* apps) const override;
+  std::vector<App> GetApps() const override;
 
   void LoadIcons();