Skip to content

Commit

Permalink
ShowFeedbackPage: Wait for SWA registration
Browse files Browse the repository at this point in the history
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 <jennyz@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Xiangdong Kong <xiangdongkong@google.com>
Reviewed-by: Jiewei Qian <qjw@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150104}
  • Loading branch information
GeorgNeis authored and Chromium LUCI CQ committed May 28, 2023
1 parent c0495a0 commit 4f2bc28
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 13 deletions.
32 changes: 20 additions & 12 deletions chrome/browser/feedback/show_feedback_page.cc
Expand Up @@ -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"
Expand Down Expand Up @@ -172,7 +173,7 @@ bool IsFromUserInteraction(FeedbackSource source) {
}
}

void OnLacrosActiveTabUrlFeteched(
void OnLacrosActiveTabUrlFetched(
Profile* profile,
chrome::FeedbackSource source,
const std::string& description_template,
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down
Expand Up @@ -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,
Expand Down

0 comments on commit 4f2bc28

Please sign in to comment.