Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(264)

Unified Diff: chrome/browser/banners/app_banner_manager.cc

Issue 2620613006: Check if a PWA is installed before checking the service worker and icon. (Closed)
Patch Set: Add missing enum value Created 3 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « chrome/browser/banners/app_banner_manager.h ('k') | chrome/browser/banners/app_banner_settings_helper.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/banners/app_banner_manager.cc
diff --git a/chrome/browser/banners/app_banner_manager.cc b/chrome/browser/banners/app_banner_manager.cc
index 55e03e54125f80d4a2d922e12309374743c0ca21..908b9320c3e3e46788d85a6d25fce1cf1f01bd7f 100644
--- a/chrome/browser/banners/app_banner_manager.cc
+++ b/chrome/browser/banners/app_banner_manager.cc
@@ -240,14 +240,7 @@ void AppBannerManager::OnDidGetManifest(const InstallableData& data) {
}
void AppBannerManager::PerformInstallableCheck() {
- if (IsWebAppInstalled(web_contents()->GetBrowserContext(),
- manifest_.start_url, manifest_url_) &&
- !IsDebugMode()) {
- ReportStatus(web_contents(), ALREADY_INSTALLED);
- Stop();
- }
-
- if (!is_active_)
+ if (!CheckIfShouldShowBanner())
return;
// Fetch and verify the other required information.
@@ -338,13 +331,6 @@ void AppBannerManager::Stop() {
void AppBannerManager::SendBannerPromptRequest() {
RecordCouldShowBanner();
- // Given all of the other checks that have been made, the only possible reason
- // for stopping now is that the app has been added to the homescreen.
- if (!IsDebugMode() && !CheckIfShouldShowBanner()) {
- Stop();
- return;
- }
-
TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_CREATED);
event_request_id_ = ++gCurrentRequestID;
@@ -450,37 +436,43 @@ void AppBannerManager::RecordCouldShowBanner() {
}
bool AppBannerManager::CheckIfShouldShowBanner() {
- content::WebContents* contents = web_contents();
- DCHECK(contents);
+ if (IsDebugMode())
+ return true;
+ // Check whether we are permitted to show the banner. If we have already
+ // added this site to homescreen, or if the banner has been shown too
+ // recently, prevent the banner from being shown.
+ content::WebContents* contents = web_contents();
InstallableStatusCode code = AppBannerSettingsHelper::ShouldShowBanner(
contents, validated_url_, GetAppIdentifier(), GetCurrentTime());
- if (code == NO_ERROR_DETECTED)
- return true;
- switch (code) {
- case ALREADY_INSTALLED:
- banners::TrackDisplayEvent(banners::DISPLAY_EVENT_INSTALLED_PREVIOUSLY);
- break;
- case PREVIOUSLY_BLOCKED:
- banners::TrackDisplayEvent(banners::DISPLAY_EVENT_BLOCKED_PREVIOUSLY);
- break;
- case PREVIOUSLY_IGNORED:
- banners::TrackDisplayEvent(banners::DISPLAY_EVENT_IGNORED_PREVIOUSLY);
- break;
- case INSUFFICIENT_ENGAGEMENT:
- banners::TrackDisplayEvent(banners::DISPLAY_EVENT_NOT_VISITED_ENOUGH);
- break;
- default:
- break;
+ if (code == NO_ERROR_DETECTED &&
+ IsWebAppInstalled(contents->GetBrowserContext(), manifest_.start_url,
+ manifest_url_)) {
+ code = ALREADY_INSTALLED;
}
- // If we are in debug mode, AppBannerSettingsHelper::ShouldShowBanner must
- // return NO_ERROR_DETECTED (bypass flag is set) or we must not have entered
- // this method.
- DCHECK(!IsDebugMode());
- ReportStatus(web_contents(), code);
- return false;
+ if (code != NO_ERROR_DETECTED) {
+ switch (code) {
+ case ALREADY_INSTALLED:
+ banners::TrackDisplayEvent(banners::DISPLAY_EVENT_INSTALLED_PREVIOUSLY);
+ break;
+ case PREVIOUSLY_BLOCKED:
+ banners::TrackDisplayEvent(banners::DISPLAY_EVENT_BLOCKED_PREVIOUSLY);
+ break;
+ case PREVIOUSLY_IGNORED:
+ banners::TrackDisplayEvent(banners::DISPLAY_EVENT_IGNORED_PREVIOUSLY);
+ break;
+ case PACKAGE_NAME_OR_START_URL_EMPTY:
+ break;
+ default:
+ NOTREACHED();
+ }
+ ReportStatus(contents, code);
+ Stop();
+ return false;
+ }
+ return true;
}
void AppBannerManager::OnBannerPromptReply(
« no previous file with comments | « chrome/browser/banners/app_banner_manager.h ('k') | chrome/browser/banners/app_banner_settings_helper.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698