Update UI Shelf to use AppService to verify app_id.

For Crostini, Extension, built-in apps, the AppService is used to
valid the app id. For ARC apps, ArcAppListPrefs is used, as ARC apps
have some very special cases, e.g. PlayStore. ARC tests also assume
synchronous behaviour, and will need more refactoring so the AppService
can be used.

Related unit tests has been fixed. Extension apps in test cases use
OnExtensionLoaded to add extension apps, so set the extension name
so that we can use AppService to dump the apps name.

BUG=1002351

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

chrome/browser/apps/app_service/extension_apps.cc

@@ -445,6 +445,7 @@ void ExtensionApps::OnExtensionLoaded(content::BrowserContext* browser_context,
   app->app_type = app_type_;
   app->app_id = extension->id();
   app->readiness = apps::mojom::Readiness::kReady;
+  app->name = extension->name();
   Publish(std::move(app));
 }
 

chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc

@@ -312,11 +312,10 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
     manifest.SetInteger(extensions::manifest_keys::kManifestVersion, 2);
     manifest.SetString(extensions::manifest_keys::kDescription,
                        "for testing pinned apps");
-
-    // AppService checks the app's type, so sets the type in manifest for
-    // extensions.
-    if (base::FeatureList::IsEnabled(features::kAppServiceShelf))
-      manifest.SetString(extensions::manifest_keys::kLaunchWebURL, kWebAppUrl);
+    // AppService checks the app's type. So set the
+    // manifest_keys::kLaunchWebURL, so that the extension can get the type
+    // from manifest value, and then AppService can get the extension's type.
+    manifest.SetString(extensions::manifest_keys::kLaunchWebURL, kWebAppUrl);
 
     base::DictionaryValue manifest_platform_app;
     manifest_platform_app.SetString(extensions::manifest_keys::kName,
@@ -431,6 +430,11 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
     manifest_web_app.SetInteger(extensions::manifest_keys::kManifestVersion, 2);
     manifest_web_app.SetString(extensions::manifest_keys::kDescription,
                                "For testing");
+    // AppService checks the app's type. So set the
+    // manifest_keys::kLaunchWebURL, so that the extension can get the type
+    // from manifest value, and then AppService can get the extension's type.
+    manifest_web_app.SetString(extensions::manifest_keys::kLaunchWebURL,
+                               kWebAppUrl);
     web_app_ = Extension::Create(base::FilePath(), Manifest::UNPACKED,
                                  manifest_web_app, Extension::FROM_BOOKMARK,
                                  kWebAppId, &error);
@@ -526,6 +530,7 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
   // Create and initialize the controller, owned by the test shell delegate.
   void InitLauncherController() {
     CreateLauncherController()->Init();
+    FlushMojoCallsForAppService();
   }
 
   // Create and initialize the controller; create a tab and show the browser.
@@ -693,6 +698,7 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
       app_list_syncable_service_->ProcessSyncChanges(FROM_HERE,
                                                      combined_sync_list);
     }
+    FlushMojoCallsForAppService();
   }
 
   // Set the index at which the chrome icon should be.
@@ -765,15 +771,7 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
           } else if (app == extensionYoutubeApp_->id()) {
             result += "youtube";
           } else {
-            const auto* extension = extension_registry_->GetExtensionById(
-                app, extensions::ExtensionRegistry::COMPATIBILITY);
-            if (extension && !extension->name().empty()) {
-              std::string name = extension->name();
-              name[0] = std::tolower(name[0]);
-              result += name;
-            } else {
-              result += "unknown";
-            }
+            result += GetAppNameFromAppService(app);
           }
           break;
         }
@@ -818,15 +816,7 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
               }
             }
             if (!arc_app_found) {
-              const auto* extension = extension_registry_->GetExtensionById(
-                  app, extensions::ExtensionRegistry::COMPATIBILITY);
-              if (extension && !extension->name().empty()) {
-                std::string name = extension->name();
-                name[0] = std::toupper(name[0]);
-                result += name;
-              } else {
-                result += "Unknown";
-              }
+              result += GetAppNameFromAppService(app);
             }
           }
           break;
@@ -959,6 +949,12 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
   // Allow AppService async callbacks to run.
   void WaitForAppService() { base::RunLoop().RunUntilIdle(); }
 
+  // Flush mojo calls to allow AppService async callbacks to run.
+  void FlushMojoCallsForAppService() {
+    if (app_service_proxy_)
+      app_service_proxy_->FlushMojoCallsForTesting();
+  }
+
   // Add extension and allow AppService async callbacks to run.
   void AddExtension(const Extension* extension) {
     extension_service_->AddExtension(extension);
@@ -972,6 +968,16 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
     WaitForAppService();
   }
 
+  const std::string GetAppNameFromAppService(const std::string& app_id) {
+    std::string name = "Unknown";
+    if (!app_service_proxy_)
+      return name;
+    app_service_proxy_->AppRegistryCache().ForOneApp(
+        app_id,
+        [&name](const apps::AppUpdate& update) { name = update.Name(); });
+    return name;
+  }
+
   // Needed for extension service & friends to work.
   scoped_refptr<Extension> extension_chrome_;
   scoped_refptr<Extension> extension1_;
@@ -1064,13 +1070,16 @@ class ChromeLauncherControllerExtendedShelfTest
     manifest.SetInteger(extensions::manifest_keys::kManifestVersion, 2);
     manifest.SetString(extensions::manifest_keys::kDescription,
                        "for testing pinned apps");
+    // AppService checks the app's type. So set the
+    // manifest_keys::kLaunchWebURL, so that the extension can get the type
+    // from manifest value, and then AppService can get the extension's type.
+    manifest.SetString(extensions::manifest_keys::kLaunchWebURL, kWebAppUrl);
 
     const std::vector<std::pair<std::string, std::string>> extra_extensions = {
         {extension_misc::kCalendarAppId, "Calendar"},
         {extension_misc::kGoogleSheetsAppId, "Sheets"},
         {extension_misc::kGoogleSlidesAppId, "Slides"},
         {extension_misc::kFilesManagerAppId, "Files"},
-        {app_list::kInternalAppIdCamera, "Camera"},
         {extension_misc::kGooglePhotosAppId, "Photos"},
     };
 
@@ -3728,7 +3737,7 @@ TEST_F(MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerTest,
       multi_user_util::GetAccountIdFromProfile(profile2));
 
   const std::string app_id = extension1_->id();
-  extension_service_->AddExtension(extension1_.get());
+  AddExtension(extension1_.get());
 
   EXPECT_EQ(1, model_->item_count());
   EXPECT_FALSE(
@@ -3751,6 +3760,7 @@ TEST_F(MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerTest,
 
   // Switch to a new profile
   SwitchActiveUser(account_id2);
+  FlushMojoCallsForAppService();
   EXPECT_FALSE(launcher_controller_->IsAppPinned(app_id));
   EXPECT_EQ(1, model_->item_count());
   EXPECT_FALSE(
@@ -3758,6 +3768,7 @@ TEST_F(MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerTest,
 
   // Switch back
   SwitchActiveUser(account_id);
+  FlushMojoCallsForAppService();
   EXPECT_TRUE(launcher_controller_->IsAppPinned(app_id));
   EXPECT_EQ(2, model_->item_count());
   EXPECT_TRUE(
@@ -4284,6 +4295,8 @@ class ChromeLauncherControllerPlayStoreAvailabilityTest
   void SetUp() override {
     if (GetParam())
       arc::SetArcAlwaysStartWithoutPlayStoreForTesting();
+    // To prevent crash on test exit and pending decode request.
+    ArcAppIcon::DisableSafeDecodingForTesting();
     ArcDefaultAppList::UseTestAppsDirectory();
     ChromeLauncherControllerTest::SetUp();
   }
@@ -4633,6 +4646,8 @@ TEST_F(ChromeLauncherControllerDemoModeTest, PinnedAppsOnline) {
   profile()->GetTestingPrefService()->SetManagedPref(
       prefs::kPolicyPinnedLauncherApps, policy_value.CreateDeepCopy());
 
+  FlushMojoCallsForAppService();
+
   // Since the device is online, all policy pinned apps are pinned.
   EXPECT_TRUE(launcher_controller_->IsAppPinned(extension1_->id()));
   EXPECT_EQ(AppListControllerDelegate::PIN_FIXED,
@@ -4682,6 +4697,7 @@ TEST_F(ChromeLauncherControllerDemoModeTest, PinnedAppsOffline) {
 
   profile()->GetTestingPrefService()->SetManagedPref(
       prefs::kPolicyPinnedLauncherApps, policy_value.CreateDeepCopy());
+  FlushMojoCallsForAppService();
 
   // Since the device is online, the policy pinned apps that shouldn't be pinned
   // in Demo Mode are unpinned.
@@ -4727,6 +4743,7 @@ TEST_F(ChromeLauncherControllerTest, CrostiniTerminalPinUnpin) {
 
   // Reload after allowing Crostini UI
   crostini::CrostiniTestHelper test_helper(profile());
+  test_helper.ReInitializeAppServiceIntegration();
   // TODO(crubug.com/918739): Fix pins are not refreshed on enabling Crostini.
   // As a workaround add any app that triggers pin update.
   AddExtension(extension1_.get());

chrome/browser/ui/ash/launcher/launcher_controller_helper.cc

@@ -170,34 +170,28 @@ std::string LauncherControllerHelper::GetAppID(content::WebContents* tab) {
 }
 
 bool LauncherControllerHelper::IsValidIDForCurrentUser(
-    const std::string& id) const {
-  const ArcAppListPrefs* arc_prefs = GetArcAppListPrefs();
-  if (arc_prefs && arc_prefs->IsRegistered(id))
+    const std::string& app_id) const {
+  if (IsValidIDForArcApp(app_id)) {
     return true;
+  }
+
+  if (base::FeatureList::IsEnabled(features::kAppServiceShelf)) {
+    return IsValidIDFromAppService(app_id);
+  }
 
   crostini::CrostiniRegistryService* registry_service =
       crostini::CrostiniRegistryServiceFactory::GetForProfile(profile_);
-  if (registry_service && registry_service->IsCrostiniShelfAppId(id)) {
+  if (registry_service && registry_service->IsCrostiniShelfAppId(app_id)) {
     return crostini::IsCrostiniUIAllowedForProfile(profile_) &&
-           registry_service->GetRegistration(id).has_value();
+           registry_service->GetRegistration(app_id).has_value();
   }
 
-  if (app_list::IsInternalApp(id))
+  if (app_list::IsInternalApp(app_id)) {
     return true;
+  }
 
-  if (!GetExtensionByID(profile_, id))
+  if (!GetExtensionByID(profile_, app_id)) {
     return false;
-  if (id == arc::kPlayStoreAppId) {
-    if (!arc::IsArcAllowedForProfile(profile()) || !arc::IsPlayStoreAvailable())
-      return false;
-    const arc::ArcSessionManager* arc_session_manager =
-        arc::ArcSessionManager::Get();
-    DCHECK(arc_session_manager);
-    if (!arc_session_manager->IsAllowed())
-      return false;
-    if (!arc::IsArcPlayStoreEnabledForProfile(profile()) &&
-        arc::IsArcPlayStoreEnabledPreferenceManagedForProfile(profile()))
-      return false;
   }
 
   return true;
@@ -295,3 +289,47 @@ void LauncherControllerHelper::ExtensionEnableFlowFinished() {
 void LauncherControllerHelper::ExtensionEnableFlowAborted(bool user_initiated) {
   extension_enable_flow_.reset();
 }
+
+bool LauncherControllerHelper::IsValidIDForArcApp(
+    const std::string& app_id) const {
+  const ArcAppListPrefs* arc_prefs = GetArcAppListPrefs();
+  if (arc_prefs && arc_prefs->IsRegistered(app_id)) {
+    return true;
+  }
+
+  if (app_id == arc::kPlayStoreAppId) {
+    if (!arc::IsArcAllowedForProfile(profile()) ||
+        !arc::IsPlayStoreAvailable()) {
+      return false;
+    }
+    const arc::ArcSessionManager* arc_session_manager =
+        arc::ArcSessionManager::Get();
+    DCHECK(arc_session_manager);
+    if (!arc_session_manager->IsAllowed()) {
+      return false;
+    }
+    if (!arc::IsArcPlayStoreEnabledForProfile(profile()) &&
+        arc::IsArcPlayStoreEnabledPreferenceManagedForProfile(profile())) {
+      return false;
+    }
+    return true;
+  }
+
+  return false;
+}
+
+bool LauncherControllerHelper::IsValidIDFromAppService(
+    const std::string& app_id) const {
+  apps::AppServiceProxy* proxy =
+      apps::AppServiceProxyFactory::GetForProfile(profile_);
+  if (!proxy) {
+    return false;
+  }
+  apps::mojom::AppType app_type = proxy->AppRegistryCache().GetAppType(app_id);
+  if (app_type == apps::mojom::AppType::kUnknown ||
+      app_type == apps::mojom::AppType::kArc) {
+    return false;
+  }
+
+  return true;
+}

chrome/browser/ui/ash/launcher/launcher_controller_helper.h

@@ -39,7 +39,7 @@ class LauncherControllerHelper : public ExtensionEnableFlowDelegate {
   // Returns true if |id| is valid for the currently active profile.
   // Used during restore to ignore no longer valid extensions.
   // Note that already running applications are ignored by the restore process.
-  virtual bool IsValidIDForCurrentUser(const std::string& id) const;
+  virtual bool IsValidIDForCurrentUser(const std::string& app_id) const;
 
   void LaunchApp(const ash::ShelfID& id,
                  ash::ShelfLaunchSource source,
@@ -57,6 +57,13 @@ class LauncherControllerHelper : public ExtensionEnableFlowDelegate {
   void ExtensionEnableFlowFinished() override;
   void ExtensionEnableFlowAborted(bool user_initiated) override;
 
+  // Returns true if |id| is a valid ARC app for the currently active profile.
+  bool IsValidIDForArcApp(const std::string& app_id) const;
+
+  // Returns true if |id| is a valid app from AppService. ARC app is not
+  // handled by this method.
+  bool IsValidIDFromAppService(const std::string& app_id) const;
+
   // The currently active profile for the usage of |GetAppID|.
   Profile* profile_;
   std::unique_ptr<ExtensionEnableFlow> extension_enable_flow_;