From 4f2bc284703ec5c2cfaf532d2f2369fa4aebd497 Mon Sep 17 00:00:00 2001 From: Georg Neis Date: Sun, 28 May 2023 02:50:46 +0000 Subject: [PATCH] ShowFeedbackPage: Wait for SWA registration Otherwise the SWA launch might fail. This is probably rare in the wild but easily happens in tests. Also document the timing issue in system_web_app_ui_utils.h. Bug: chromium:1446083 Change-Id: I5cf8944c6368f886652e0bbb63ba2e3c7d8e38cc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4560814 Reviewed-by: Jenny Zhang Commit-Queue: Georg Neis Reviewed-by: Xiangdong Kong Reviewed-by: Jiewei Qian Cr-Commit-Position: refs/heads/main@{#1150104} --- chrome/browser/feedback/show_feedback_page.cc | 32 ++++++++++++------- .../system_web_apps/system_web_app_ui_utils.h | 6 +++- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/chrome/browser/feedback/show_feedback_page.cc b/chrome/browser/feedback/show_feedback_page.cc index 51982968346e3e..5b8a83eb54b60e 100644 --- a/chrome/browser/feedback/show_feedback_page.cc +++ b/chrome/browser/feedback/show_feedback_page.cc @@ -30,6 +30,7 @@ #include "base/functional/bind.h" #include "base/strings/strcat.h" #include "chrome/browser/ash/crosapi/browser_manager.h" +#include "chrome/browser/ash/system_web_apps/system_web_app_manager.h" #include "chrome/browser/signin/identity_manager_factory.h" #include "chrome/browser/ui/ash/system_web_apps/system_web_app_ui_utils.h" #include "chromeos/components/kiosk/kiosk_utils.h" @@ -172,7 +173,7 @@ bool IsFromUserInteraction(FeedbackSource source) { } } -void OnLacrosActiveTabUrlFeteched( +void OnLacrosActiveTabUrlFetched( Profile* profile, chrome::FeedbackSource source, const std::string& description_template, @@ -188,6 +189,13 @@ void OnLacrosActiveTabUrlFeteched( description_placeholder_text, category_tag, extra_diagnostics, std::move(autofill_metadata)); } + +void LaunchFeedbackSWA(Profile* profile, const GURL& url) { + ash::SystemAppLaunchParams params; + params.url = url; + ash::LaunchSystemWebAppAsync(profile, ash::SystemWebAppType::OS_FEEDBACK, + std::move(params)); +} #endif // BUILDFLAG(IS_CHROMEOS_ASH) #if !BUILDFLAG(IS_CHROMEOS_LACROS) @@ -222,18 +230,18 @@ void RequestFeedbackFlow(const GURL& page_url, include_bluetooth_logs = IsFromUserInteraction(source); show_questionnaire = IsFromUserInteraction(source); } - // Disable a new feedback tool for kiosk, because SWAs are disabled there. + // Disable the new feedback tool for kiosk, because SWAs are disabled there. if (!chromeos::IsKioskSession() && base::FeatureList::IsEnabled(ash::features::kOsFeedback)) { - // TODO(crbug.com/1407646): Include autofill metadata into CrOS new - // feedback tool. - ash::SystemAppLaunchParams params{}; - params.url = BuildFeedbackUrl( - extra_diagnostics, description_template, description_placeholder_text, - category_tag, page_url, source, std::move(autofill_metadata)); - - ash::LaunchSystemWebAppAsync(profile, ash::SystemWebAppType::OS_FEEDBACK, - std::move(params)); + // TODO(crbug.com/1407646): Include autofill metadata into CrOS new feedback + // tool. + GURL url = BuildFeedbackUrl(extra_diagnostics, description_template, + description_placeholder_text, category_tag, + page_url, source, std::move(autofill_metadata)); + + // Wait for all SWAs to be registered before continuing. + ash::SystemWebAppManager::Get(profile)->on_apps_synchronized().Post( + FROM_HERE, base::BindOnce(&LaunchFeedbackSWA, profile, url)); return; } #endif // BUILDFLAG(IS_CHROMEOS_ASH) @@ -289,7 +297,7 @@ void ShowFeedbackPage(const Browser* browser, if (!browser && crosapi::BrowserManager::Get()->IsRunning() && crosapi::BrowserManager::Get()->GetActiveTabUrlSupported()) { crosapi::BrowserManager::Get()->GetActiveTabUrl(base::BindOnce( - &OnLacrosActiveTabUrlFeteched, profile, source, description_template, + &OnLacrosActiveTabUrlFetched, profile, source, description_template, description_placeholder_text, category_tag, extra_diagnostics, std::move(autofill_metadata))); } else { diff --git a/chrome/browser/ui/ash/system_web_apps/system_web_app_ui_utils.h b/chrome/browser/ui/ash/system_web_apps/system_web_app_ui_utils.h index d34fb8e53eb162..60c5a233d4a028 100644 --- a/chrome/browser/ui/ash/system_web_apps/system_web_app_ui_utils.h +++ b/chrome/browser/ui/ash/system_web_apps/system_web_app_ui_utils.h @@ -70,8 +70,12 @@ struct SystemAppLaunchParams { // - Other unsuitable profiles (e.g. Sign-in profile): Don't launch, and send // a crash report // -// In tests, remember to use content::TestNavigationObserver to wait the +// In tests, remember to use content::TestNavigationObserver to wait for the // navigation. +// +// NOTE: LaunchSystemWebAppAsync may have no effect if called before the initial +// registration of system web apps has completed. To avoid this, first await the +// ash::SystemWebAppManager::on_apps_synchronized event. void LaunchSystemWebAppAsync( Profile* profile, SystemWebAppType type,