[System Web Apps] Fix settings window behavior in kiosk mode

After crrev.com/c/2395298, the settings window opening would cause cause
kiosk to crash. Previuosly, one of the lines was always returning false
when checking SystemWebAppManager::IsEnabled, thus never exectuted.
After the change, it was only executed in kiosk mode, which caused crashes.

Bug: 1157279,1159274
Change-Id: I1456230547bb7d6e05a1c1515c3980000d88fa73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2593381Reviewed-by: Jenny Zhang <jennyz@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Anatoliy Potapchuk <apotapchuk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842001}

chrome/browser/chromeos/login/app_mode/kiosk_browsertest.cc

@@ -83,6 +83,7 @@
 #include "chrome/browser/ui/webui/chromeos/login/kiosk_enable_screen_handler.h"
 #include "chrome/browser/ui/webui/chromeos/login/user_creation_screen_handler.h"
 #include "chrome/browser/ui/webui/chromeos/login/welcome_screen_handler.h"
+#include "chrome/browser/ui/webui/settings/chromeos/constants/routes.mojom-forward.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
@@ -1347,6 +1348,23 @@ IN_PROC_BROWSER_TEST_F(KioskDeviceOwnedTest, GetVolumeList) {
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
+IN_PROC_BROWSER_TEST_F(KioskDeviceOwnedTest, OpenA11ySettings) {
+  StartAppLaunchFromLoginScreen(
+      NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE);
+  WaitForAppLaunchWithOptions(true /* check_launch_data */,
+                              false /* terminate_app */,
+                              true /* keep_app_open */);
+
+  auto* settings_manager = chrome::SettingsWindowManager::GetInstance();
+  Profile* profile = ProfileManager::GetPrimaryUserProfile();
+
+  settings_manager->ShowOSSettings(
+      profile, chromeos::settings::mojom::kManageAccessibilitySubpagePath);
+
+  Browser* settings_browser = settings_manager->FindBrowserForProfile(profile);
+  ASSERT_TRUE(settings_browser);
+}
+
 IN_PROC_BROWSER_TEST_F(KioskDeviceOwnedTest, SettingsWindow) {
   StartAppLaunchFromLoginScreen(
       NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE);

chrome/browser/chromeos/login/app_mode/web_kiosk_browsertest.cc

@@ -18,10 +18,13 @@
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
 #include "chrome/browser/chromeos/ownership/fake_owner_settings_service.h"
 #include "chrome/browser/chromeos/policy/device_local_account.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client_test_helper.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/settings_window_manager_chromeos.h"
 #include "chrome/browser/ui/webui/chromeos/login/error_screen_handler.h"
+#include "chrome/browser/ui/webui/settings/chromeos/constants/routes.mojom-forward.h"
 #include "chrome/browser/web_applications/components/web_application_info.h"
 #include "components/account_id/account_id.h"
 #include "content/public/test/browser_task_environment.h"
@@ -279,4 +282,20 @@ IN_PROC_BROWSER_TEST_F(WebKioskTest, KeyboardConfigPolicy) {
   ExpectKeyboardConfig();
 }
 
+IN_PROC_BROWSER_TEST_F(WebKioskTest, OpenA11ySettings) {
+  SetOnline(true);
+  PrepareAppLaunch();
+  LaunchApp();
+  KioskSessionInitializedWaiter().Wait();
+
+  auto* settings_manager = chrome::SettingsWindowManager::GetInstance();
+  Profile* profile = ProfileManager::GetPrimaryUserProfile();
+
+  settings_manager->ShowOSSettings(
+      profile, chromeos::settings::mojom::kManageAccessibilitySubpagePath);
+
+  Browser* settings_browser = settings_manager->FindBrowserForProfile(profile);
+  ASSERT_TRUE(settings_browser);
+}
+
 }  // namespace chromeos

chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc

@@ -22,6 +22,8 @@
 #include "chrome/browser/ui/app_list/page_break_constants.h"
 #include "chrome/browser/ui/app_list/test/fake_app_list_model_updater.h"
 #include "chrome/browser/ui/settings_window_manager_chromeos.h"
+#include "chrome/browser/web_applications/components/web_app_id_constants.h"
+#include "chrome/browser/web_applications/test/web_app_install_test_utils.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ -38,6 +40,8 @@ using testing::ElementsAre;
 
 namespace {
 
+const char kOsSettingsUrl[] = "chrome://os-settings/";
+
 scoped_refptr<extensions::Extension> MakeApp(
     const std::string& name,
     const std::string& id,
@@ -466,10 +470,14 @@ class AppListInternalAppSyncableServiceTest
   AppListInternalAppSyncableServiceTest() {
     chrome::SettingsWindowManager::ForceDeprecatedSettingsWindowForTesting();
   }
-  ~AppListInternalAppSyncableServiceTest() override = default;
 
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
+  void SetUp() override {
+    AppListSyncableServiceTest::SetUp();
+    web_app::InstallDummyWebApp(testing_profile(), kOsSettingsUrl,
+                                GURL(kOsSettingsUrl));
+  }
+
+  ~AppListInternalAppSyncableServiceTest() override = default;
 };
 
 TEST_F(AppListInternalAppSyncableServiceTest, ExistingDefaultPageBreak) {
@@ -515,7 +523,7 @@ TEST_F(AppListInternalAppSyncableServiceTest, DefaultPageBreakFirstTimeUser) {
 
   // Since internal apps are added by default, we'll use the settings apps to
   // test the ordering.
-  auto* settings_app_sync_item = GetSyncItem(ash::kInternalAppIdSettings);
+  auto* settings_app_sync_item = GetSyncItem(web_app::kOsSettingsAppId);
   auto* hosted_app_sync_item = GetSyncItem(kHostedAppId);
   ASSERT_TRUE(settings_app_sync_item);
   ASSERT_TRUE(hosted_app_sync_item);
@@ -1084,9 +1092,8 @@ TEST_F(AppListSyncableServiceTest, PageBreakWithOverflowItem) {
   auto ordered_items = GetIdsOfSortedItemsFromModelUpdater();
   EXPECT_THAT(
       ordered_items,
-      ElementsAre(kItemIdA1, kItemIdA2, kPageBreakItemId1,
-                  kItemIdB1, kItemIdB2, kItemIdB3, kPageBreakItemId2,
-                  kItemIdC1, kPageBreakItemId3));
+      ElementsAre(kItemIdA1, kItemIdA2, kPageBreakItemId1, kItemIdB1, kItemIdB2,
+                  kItemIdB3, kPageBreakItemId2, kItemIdC1, kPageBreakItemId3));
 
   // On device 1, move A1 from page 1 to page 2 and insert it between B1 and B2.
   // Device 2 should get the following 3 sync changes from device 1:
@@ -1141,9 +1148,9 @@ TEST_F(AppListSyncableServiceTest, PageBreakWithOverflowItem) {
   // B3 C1 [pagebreak 3]
   auto ordered_items_after_sync = GetIdsOfSortedItemsFromModelUpdater();
   EXPECT_THAT(ordered_items_after_sync,
-              ElementsAre(kItemIdA2, kPageBreakItemId1,
-                          kItemIdB1, kItemIdA1, kItemIdB2, kNewPageBreakItemId,
-                          kItemIdB3, kItemIdC1, kPageBreakItemId3));
+              ElementsAre(kItemIdA2, kPageBreakItemId1, kItemIdB1, kItemIdA1,
+                          kItemIdB2, kNewPageBreakItemId, kItemIdB3, kItemIdC1,
+                          kPageBreakItemId3));
 }
 
 TEST_F(AppListSyncableServiceTest, FirstAvailablePosition) {

chrome/browser/ui/app_list/internal_app/internal_app_metadata.cc

@@ -69,24 +69,7 @@ const std::vector<InternalApp>& GetInternalAppListImpl(bool get_all,
             /*searchable=*/false,
             /*show_in_launcher=*/false, apps::BuiltInAppName::kContinueReading,
             /*searchable_string_resource_id=*/0}});
-
-  static base::NoDestructor<std::vector<InternalApp>> internal_app_list;
-  internal_app_list->clear();
-  internal_app_list->insert(internal_app_list->begin(),
-                            internal_app_list_static->begin(),
-                            internal_app_list_static->end());
-
-  if (chrome::SettingsWindowManager::UseDeprecatedSettingsWindow(profile)) {
-    internal_app_list->push_back(
-        {ash::kInternalAppIdSettings, IDS_INTERNAL_APP_SETTINGS,
-         IDR_SETTINGS_LOGO_192,
-         /*recommendable=*/true,
-         /*searchable=*/true,
-         /*show_in_launcher=*/true, apps::BuiltInAppName::kSettings,
-         /*searchable_string_resource_id=*/0});
-  }
-
-  return *internal_app_list;
+  return *internal_app_list_static;
 }
 
 }  // namespace

chrome/browser/ui/settings_window_manager_browsertest_chromeos.cc

@@ -187,17 +187,3 @@ IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, OpenSettings) {
   chrome::ShowSettingsSubPage(browser(), chrome::kAutofillSubPage);
   EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
 }
-
-IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, KioskMode) {
-  base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kForceAppMode);
-
-  // Open settings window.
-  settings_manager_->ShowOSSettings(browser()->profile());
-  Browser* settings_browser =
-      settings_manager_->FindBrowserForProfile(browser()->profile());
-  ASSERT_TRUE(settings_browser);
-
-  // In kiosk mode, browser should be created, but it should not be a system web
-  // app.
-  EXPECT_EQ(settings_browser->app_controller(), nullptr);
-}