Commit beebbb6d authored by Christopher Lam's avatar Christopher Lam Committed by Commit Bot

Remove BuiltInChromeOsApps::SetHideSettingsAppForTesting().

This CL removes a testing hack which is no longer used.

Bug: None
Change-Id: I68f1d84593ddb6fe45df52500eaeb4fd0767dd56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2400651Reviewed-by: default avatarDominick Ng <dominickn@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805630}
parent 44326d9d
...@@ -78,15 +78,6 @@ BuiltInChromeOsApps::BuiltInChromeOsApps( ...@@ -78,15 +78,6 @@ BuiltInChromeOsApps::BuiltInChromeOsApps(
BuiltInChromeOsApps::~BuiltInChromeOsApps() = default; BuiltInChromeOsApps::~BuiltInChromeOsApps() = default;
bool BuiltInChromeOsApps::hide_settings_app_for_testing_ = false;
// static
bool BuiltInChromeOsApps::SetHideSettingsAppForTesting(bool hide) {
bool old_value = hide_settings_app_for_testing_;
hide_settings_app_for_testing_ = hide;
return old_value;
}
void BuiltInChromeOsApps::Connect( void BuiltInChromeOsApps::Connect(
mojo::PendingRemote<apps::mojom::Subscriber> subscriber_remote, mojo::PendingRemote<apps::mojom::Subscriber> subscriber_remote,
apps::mojom::ConnectOptionsPtr opts) { apps::mojom::ConnectOptionsPtr opts) {
...@@ -97,11 +88,6 @@ void BuiltInChromeOsApps::Connect( ...@@ -97,11 +88,6 @@ void BuiltInChromeOsApps::Connect(
for (const auto& internal_app : app_list::GetInternalAppList(profile_)) { for (const auto& internal_app : app_list::GetInternalAppList(profile_)) {
apps::mojom::AppPtr app = Convert(internal_app); apps::mojom::AppPtr app = Convert(internal_app);
if (!app.is_null()) { if (!app.is_null()) {
if (hide_settings_app_for_testing_ &&
(internal_app.internal_app_name == BuiltInAppName::kSettings)) {
app->show_in_shelf = app->show_in_search =
apps::mojom::OptionalBool::kFalse;
}
apps.push_back(std::move(app)); apps.push_back(std::move(app));
} }
} }
......
...@@ -27,8 +27,6 @@ class BuiltInChromeOsApps : public apps::PublisherBase { ...@@ -27,8 +27,6 @@ class BuiltInChromeOsApps : public apps::PublisherBase {
Profile* profile); Profile* profile);
~BuiltInChromeOsApps() override; ~BuiltInChromeOsApps() override;
static bool SetHideSettingsAppForTesting(bool hide);
private: private:
// apps::mojom::Publisher overrides. // apps::mojom::Publisher overrides.
void Connect(mojo::PendingRemote<apps::mojom::Subscriber> subscriber_remote, void Connect(mojo::PendingRemote<apps::mojom::Subscriber> subscriber_remote,
...@@ -50,67 +48,6 @@ class BuiltInChromeOsApps : public apps::PublisherBase { ...@@ -50,67 +48,6 @@ class BuiltInChromeOsApps : public apps::PublisherBase {
Profile* const profile_; Profile* const profile_;
// Hack to hide the settings app from the app list search box. This is only
// intended to be used in tests.
//
// BuiltInChromeOsApps assumes that the list of internal apps doesn't change
// over the lifetime of the Profile. For example,
// chrome/browser/ui/app_list/internal_app/internal_app_metadata.{cc,h}
// doesn't expose any observer mechanism to be notified if it did change.
//
// Separately, WebAppUiServiceMigrationBrowserTest's
// SettingsSystemWebAppMigration test exercises the case where the
// *implementation* of the built-in Settings app changes, from an older
// technology to being a Web App. Even though there are two implementations,
// any given Profile will typically use only one, depending on whether
// features::kSystemWebApps is enabled, typically via the command line.
// Again, the App Service's BuiltInChromeOsApps handles this, in practice, as
// whether or not it's enabled ought to stay unchanged throughout a session.
//
// Specifically, though, that test (1) starts with features::kSystemWebApps
// disabled (during SetUp) but then (2) enables it (during the test
// function). This isn't in order to explicitly exercise a run-time flag
// change, but it was simply an expedient way to test data migration:
// situation (1) creates some persistent data (in the test's temporary
// storage) in the old implementation's format, and flipping to situation (2)
// checks that the new implementation correctly migrates that data.
//
// An unfortunate side effect of that flip is that the list of internal apps
// changed. The SettingsSystemWebAppMigration test involves an
// AppListModelUpdater, which calls into its AppSearchProvider's and thus the
// App Service (when the App Service is enabled), which then panics because a
// built-in app it previously knew about and exposed via app list search, in
// situation (1), has mysteriously vanished, in situation (2).
//
// A 'proper' fix could be to add an observer mechanism to the list of
// internal apps, and break the previously held simplifying assumption that
// BuiltInChromeOsApps can be relatively stateless as that list does not
// change. There's also some subtlety because App Service methods are
// generally asynchronous (because they're potentially Mojo IPC), so some
// care still needs to be taken where some parts of the system might have
// seen any given app update but others might not yet have.
//
// Doing a 'proper' fix is probably more trouble than it's worth. Given that
// this "change the implementation of the built-in Settings app" will only
// temporarily have two separate implementations, say for a couple of months,
// and a runtime flag flip isn't supported in production, and is only
// exercised in a single test, the simpler (temporary) fix is to opt in to
// hiding the internal Settings app from search.
//
// Note that we only hide it from search. We don't remove the app entirely
// from the list of apps that the BuiltInChromeOsApps publishes, since the
// migration test (and its AppListModelUpdater) still needs to be able to
// refer to the (artificial) situation where both apps co-exist.
//
// TODO(calamity/nigeltao): remove this hack once the System Web App has
// completely replaced the Settings Internal App, hopefully some time in
// 2019Q3.
//
// See also the "web_app::SystemWebAppManager::IsEnabled()" if-check in
// internal_app_metadata.cc. Once that condition is removed, we can probably
// remove this hack too.
static bool hide_settings_app_for_testing_;
DISALLOW_COPY_AND_ASSIGN(BuiltInChromeOsApps); DISALLOW_COPY_AND_ASSIGN(BuiltInChromeOsApps);
}; };
......
...@@ -102,9 +102,6 @@ const std::vector<InternalApp>& GetInternalAppListImpl(bool get_all, ...@@ -102,9 +102,6 @@ const std::vector<InternalApp>& GetInternalAppListImpl(bool get_all,
/*searchable_string_resource_id=*/IDS_INTERNAL_APP_DISCOVER}); /*searchable_string_resource_id=*/IDS_INTERNAL_APP_DISCOVER});
} }
// TODO(calamity/nigeltao): when removing the
// web_app::SystemWebAppManager::IsEnabled condition, we can probably also
// remove the apps::BuiltInChromeOsApps::SetHideSettingsAppForTesting hack.
if (!web_app::SystemWebAppManager::IsEnabled()) { if (!web_app::SystemWebAppManager::IsEnabled()) {
internal_app_list->push_back( internal_app_list->push_back(
{ash::kInternalAppIdSettings, IDS_INTERNAL_APP_SETTINGS, {ash::kInternalAppIdSettings, IDS_INTERNAL_APP_SETTINGS,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment