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

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

Issue 886643003: Use heuristic to work out when to prompt for app install banners. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Remove other patch Created 5 years, 10 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
Index: chrome/browser/android/banners/app_banner_manager.cc
diff --git a/chrome/browser/android/banners/app_banner_manager.cc b/chrome/browser/android/banners/app_banner_manager.cc
index c08e3e963cc37ed6a4df2d70422b24b3b71ec00a..799271a73089f305a5a1be122a16183c5c906111 100644
--- a/chrome/browser/android/banners/app_banner_manager.cc
+++ b/chrome/browser/android/banners/app_banner_manager.cc
@@ -68,16 +68,18 @@ void AppBannerManager::BlockBanner(JNIEnv* env,
GURL url(ConvertJavaStringToUTF8(env, jurl));
std::string package_name = ConvertJavaStringToUTF8(env, jpackage);
- AppBannerSettingsHelper::Block(web_contents(), url, package_name);
+ AppBannerSettingsHelper::RecordBannerEvent(
+ web_contents(), url, package_name,
+ AppBannerSettingsHelper::APP_BANNER_EVENT_DID_BLOCK, base::Time::Now());
}
void AppBannerManager::Block() const {
if (!web_contents() || manifest_.IsEmpty())
return;
- AppBannerSettingsHelper::Block(web_contents(),
- web_contents()->GetURL(),
- manifest_.start_url.spec());
+ AppBannerSettingsHelper::RecordBannerEvent(
+ web_contents(), web_contents()->GetURL(), manifest_.start_url.spec(),
+ AppBannerSettingsHelper::APP_BANNER_EVENT_DID_BLOCK, base::Time::Now());
}
void AppBannerManager::Install() const {
@@ -85,6 +87,11 @@ void AppBannerManager::Install() const {
return;
if (!manifest_.IsEmpty()) {
+ AppBannerSettingsHelper::RecordBannerEvent(
+ web_contents(), web_contents()->GetURL(), manifest_.start_url.spec(),
+ AppBannerSettingsHelper::APP_BANNER_EVENT_DID_ADD_TO_HOMESCREEN,
+ base::Time::Now());
+
InstallManifestApp(manifest_, *app_icon_.get());
}
}
@@ -171,10 +178,30 @@ void AppBannerManager::OnDidCheckHasServiceWorker(bool has_same) {
if (icon_url.is_empty())
return;
+ if (!CheckIfShouldShow(manifest_.start_url.spec()))
+ return;
+
FetchIcon(icon_url);
}
}
+bool AppBannerManager::CheckIfShouldShow(
+ const std::string& package_or_start_url) {
+ AppBannerSettingsHelper::RecordBannerEvent(
+ web_contents(), validated_url_, package_or_start_url,
+ AppBannerSettingsHelper::APP_BANNER_EVENT_COULD_SHOW, base::Time::Now());
+ if (!AppBannerSettingsHelper::ShouldShowBanner(web_contents(), validated_url_,
+ package_or_start_url,
+ base::Time::Now())) {
+ return false;
+ }
+
+ AppBannerSettingsHelper::RecordBannerEvent(
+ web_contents(), validated_url_, package_or_start_url,
+ AppBannerSettingsHelper::APP_BANNER_EVENT_DID_SHOW, base::Time::Now());
gone 2015/02/05 22:21:10 Seems strange to record that we did show it in a f
benwells 2015/02/06 16:36:28 Good point. I've split this into two functions: Re
+ return true;
+}
+
bool AppBannerManager::OnMessageReceived(const IPC::Message& message) {
bool handled = true;
IPC_BEGIN_MESSAGE_MAP(AppBannerManager, message)
@@ -235,11 +262,8 @@ void AppBannerManager::OnDidRetrieveMetaTagContent(
banners::TrackDisplayEvent(DISPLAY_BANNER_REQUESTED);
- if (!AppBannerSettingsHelper::IsAllowed(web_contents(),
- expected_url,
- tag_content)) {
+ if (!CheckIfShouldShow(tag_content))
return;
- }
// Send the info to the Java side to get info about the app.
JNIEnv* env = base::android::AttachCurrentThread();

Powered by Google App Engine
This is Rietveld 408576698