Commit 2b2eac93 authored by tby's avatar tby Committed by Chromium LUCI CQ

[Settings search] Fix search results not appearing for some users

We currently wait for an "I'm ready" signal from the Settings app before
we serve settings search results. Unfortunately this signal is flaky,
and leads to some users never seeing settings results. This CL removes
the check altogether as a temporary measure, while we investigate a
root-cause fix.

UX impact: there's a very short window of time after logging in where
settings results won't do anything, because the Settings app isn't ready
yet. This is short enough that we don't believe it's an issue.

Bug: 1167575
Change-Id: If63770453febbf48d2fe721706daee9ccedcd608
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2631822
Commit-Queue: Tony Yeoman <tby@chromium.org>
Reviewed-by: default avatarRachel Wong <wrong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845842}
parent 741f45ce
......@@ -186,6 +186,8 @@ OsSettingsProvider::OsSettingsProvider(Profile* profile)
search_results_observer_receiver_.BindNewPipeAndPassRemote());
app_service_proxy_ = apps::AppServiceProxyFactory::GetForProfile(profile_);
DCHECK(app_service_proxy_);
Observe(&app_service_proxy_->AppRegistryCache());
auto icon_type =
(base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon))
......@@ -230,9 +232,6 @@ void OsSettingsProvider::Start(const base::string16& query) {
// - we don't have an icon to display with results.
if (!search_handler_) {
return;
} else if (!settings_app_ready_) {
LogError(Error::kSettingsAppNotReady);
return;
} else if (icon_.isNull()) {
LogError(Error::kNoSettingsIcon);
return;
......@@ -287,18 +286,14 @@ void OsSettingsProvider::OnAppUpdate(const apps::AppUpdate& update) {
if (update.AppId() != web_app::kOsSettingsAppId)
return;
// Request the Settings app icon when either the readiness is changed to
// kReady, or the icon has been updated, signalled by IconKeyChanged.
bool update_icon = false;
if (update.ReadinessChanged()) {
settings_app_ready_ = update.Readiness() == apps::mojom::Readiness::kReady;
if (settings_app_ready_)
update_icon = true;
} else if (update.IconKeyChanged()) {
update_icon = true;
}
// TODO(crbug.com/1068851): We previously disabled this search provider until
// the app service signalled that the settings app is ready. But this signal
// is flaky, so sometimes search provider was permanently disabled. Once the
// signal is reliable, we should re-add the check.
if (update_icon) {
// Request the Settings app icon when either the readiness or the icon has
// changed.
if (update.ReadinessChanged() || update.IconKeyChanged()) {
auto icon_type =
(base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon))
? apps::mojom::IconType::kStandard
......@@ -387,6 +382,9 @@ OsSettingsProvider::FilterResults(
}
void OsSettingsProvider::OnLoadIcon(apps::mojom::IconValuePtr icon_value) {
if (icon_value.is_null())
return;
auto icon_type =
(base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon))
? apps::mojom::IconType::kStandard
......
......@@ -129,9 +129,6 @@ class OsSettingsProvider
apps::AppServiceProxy* app_service_proxy_;
gfx::ImageSkia icon_;
// Whether the app service has signalled the settings app as ready.
bool settings_app_ready_ = false;
// Last query. It is reset when view is closed.
base::string16 last_query_;
mojo::Receiver<chromeos::settings::mojom::SearchResultsObserver>
......
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