Use heuristic to work out when to prompt for app install banners.

The heuristic ensures that the user is not prompted too much, and is only
prompted after showing sustained engagement with a site.

BUG=452825

Review URL: https://codereview.chromium.org/886643003

Cr-Commit-Position: refs/heads/master@{#315055}

chrome/browser/android/banners/app_banner_manager.cc

@@ -69,7 +69,9 @@ 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 {
@@ -77,9 +79,10 @@ void AppBannerManager::Block() const {
     return;
 
   if (!web_app_data_.IsEmpty()) {
-    AppBannerSettingsHelper::Block(web_contents(),
-                                   web_contents()->GetURL(),
-                                   web_app_data_.start_url.spec());
+    AppBannerSettingsHelper::RecordBannerEvent(
+        web_contents(), web_contents()->GetURL(),
+        web_app_data_.start_url.spec(),
+        AppBannerSettingsHelper::APP_BANNER_EVENT_DID_BLOCK, base::Time::Now());
   }
 }
 
@@ -92,6 +95,12 @@ bool AppBannerManager::OnButtonClicked() const {
     return true;
 
   if (!web_app_data_.IsEmpty()) {
+    AppBannerSettingsHelper::RecordBannerEvent(
+        web_contents(), web_contents()->GetURL(),
+        web_app_data_.start_url.spec(),
+        AppBannerSettingsHelper::APP_BANNER_EVENT_DID_ADD_TO_HOMESCREEN,
+        base::Time::Now());
+
     InstallManifestApp(web_app_data_, *app_icon_.get());
     return true;
   }
@@ -183,10 +192,35 @@ void AppBannerManager::OnDidCheckHasServiceWorker(bool has_service_worker) {
     if (icon_url.is_empty())
       return;
 
+    RecordCouldShowBanner(web_app_data_.start_url.spec());
+    if (!CheckIfShouldShow(web_app_data_.start_url.spec()))
+      return;
+
     FetchIcon(icon_url);
   }
 }
 
+void AppBannerManager::RecordCouldShowBanner(
+    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());
+}
+
+bool AppBannerManager::CheckIfShouldShow(
+    const std::string& package_or_start_url) {
+  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());
+  return true;
+}
+
 bool AppBannerManager::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(AppBannerManager, message)
@@ -237,11 +271,9 @@ void AppBannerManager::OnDidRetrieveMetaTagContent(
 
   banners::TrackDisplayEvent(DISPLAY_BANNER_REQUESTED);
 
-  if (!AppBannerSettingsHelper::IsAllowed(web_contents(),
-                                          expected_url,
-                                          tag_content)) {
+  RecordCouldShowBanner(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();

chrome/browser/android/banners/app_banner_manager.h

@@ -138,6 +138,13 @@ class AppBannerManager : public chrome::BitmapFetcherDelegate,
   // Called when the result of the CheckHasServiceWorker query has completed.
   void OnDidCheckHasServiceWorker(bool has_service_worker);
 
+  // Record that the banner could be shown at this point, if the triggering
+  // heuristic allowed.
+  void RecordCouldShowBanner(const std::string& package_or_start_url);
+
+  // Check if the banner should be shown.
+  bool CheckIfShouldShow(const std::string& package_or_start_url);
+
   // Fetches the icon for an app.
   scoped_ptr<chrome::BitmapFetcher> fetcher_;
   GURL validated_url_;

chrome/browser/banners/app_banner_settings_helper.cc

@@ -23,21 +23,28 @@ const size_t kMaxAppsPerSite = 3;
 // Oldest could show banner event we care about, in days.
 const unsigned int kOldestCouldShowBannerEventInDays = 14;
 
-// Dictionary key to use for the 'could show banner' events.
-const char kCouldShowBannerEventsKey[] = "couldShowBannerEvents";
+// Number of times that the banner could have been shown before the banner will
+// actually be triggered.
+const unsigned int kCouldShowEventsToTrigger = 2;
+
+// Number of days that showing the banner will prevent it being seen again for.
+const unsigned int kMinimumDaysBetweenBannerShows = 60;
+
+// Number of days that the banner being blocked will prevent it being seen again
+// for.
+const unsigned int kMinimumBannerBlockedToBannerShown = 90;
+
+// Dictionary keys to use for the events.
+const char* kBannerEventKeys[] = {
+    "couldShowBannerEvents",
+    "didShowBannerEvent",
+    "didBlockBannerEvent",
+    "didAddToHomescreenEvent",
+};
 
 // Dictionary key to use whether the banner has been blocked.
 const char kHasBlockedKey[] = "hasBlocked";
 
-base::Time DateFromTime(base::Time time) {
-  base::Time::Exploded exploded;
-  time.LocalExplode(&exploded);
-  exploded.hour = 0;
-  exploded.minute = 0;
-  exploded.second = 0;
-  return base::Time::FromLocalExploded(exploded);
-}
-
 scoped_ptr<base::DictionaryValue> GetOriginDict(
     HostContentSettingsMap* settings,
     const GURL& origin_url) {
@@ -72,10 +79,11 @@ base::DictionaryValue* GetAppDict(base::DictionaryValue* origin_dict,
 
 }  // namespace
 
-void AppBannerSettingsHelper::RecordCouldShowBannerEvent(
+void AppBannerSettingsHelper::RecordBannerEvent(
     content::WebContents* web_contents,
     const GURL& origin_url,
     const std::string& package_name_or_start_url,
+    AppBannerEvent event,
     base::Time time) {
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
@@ -99,50 +107,92 @@ void AppBannerSettingsHelper::RecordCouldShowBannerEvent(
   if (!app_dict)
     return;
 
-  base::ListValue* could_show_list = nullptr;
-  if (!app_dict->GetList(kCouldShowBannerEventsKey, &could_show_list)) {
-    could_show_list = new base::ListValue();
-    app_dict->Set(kCouldShowBannerEventsKey, make_scoped_ptr(could_show_list));
-  }
+  std::string event_key(kBannerEventKeys[event]);
 
-  // Trim any items that are older than we should care about. For comparisons
-  // the times are converted to local dates.
-  base::Time date = DateFromTime(time);
-  base::ValueVector::iterator it = could_show_list->begin();
-  while (it != could_show_list->end()) {
-    if ((*it)->IsType(base::Value::TYPE_DOUBLE)) {
-      double internal_date;
-      (*it)->GetAsDouble(&internal_date);
-      base::Time other_date =
-          DateFromTime(base::Time::FromInternalValue(internal_date));
-      // This date has already been added. Don't add the date again, and don't
-      // bother trimming values as it will have been done the first time the
-      // date was added (unless the local date has changed, which we can live
-      // with).
-      if (other_date == date)
-        return;
-
-      base::TimeDelta delta = date - other_date;
-      if (delta <
-          base::TimeDelta::FromDays(kOldestCouldShowBannerEventInDays)) {
-        ++it;
-        continue;
+  if (event == APP_BANNER_EVENT_COULD_SHOW) {
+    base::ListValue* could_show_list = nullptr;
+    if (!app_dict->GetList(event_key, &could_show_list)) {
+      could_show_list = new base::ListValue();
+      app_dict->Set(event_key, make_scoped_ptr(could_show_list));
+    }
+
+    // Trim any items that are older than we should care about. For comparisons
+    // the times are converted to local dates.
+    base::Time date = time.LocalMidnight();
+    base::ValueVector::iterator it = could_show_list->begin();
+    while (it != could_show_list->end()) {
+      if ((*it)->IsType(base::Value::TYPE_DOUBLE)) {
+        double internal_date;
+        (*it)->GetAsDouble(&internal_date);
+        base::Time other_date =
+            base::Time::FromInternalValue(internal_date).LocalMidnight();
+        // This date has already been added. Don't add the date again, and don't
+        // bother trimming values as it will have been done the first time the
+        // date was added (unless the local date has changed, which we can live
+        // with).
+        if (other_date == date)
+          return;
+
+        base::TimeDelta delta = date - other_date;
+        if (delta <
+            base::TimeDelta::FromDays(kOldestCouldShowBannerEventInDays)) {
+          ++it;
+          continue;
+        }
       }
+
+      // Either this date is older than we care about, or it isn't a date, so
+      // remove it;
+      it = could_show_list->Erase(it, nullptr);
     }
 
-    // Either this date is older than we care about, or it isn't a date, so
-    // remove it;
-    it = could_show_list->Erase(it, nullptr);
+    // Dates are stored in their raw form (i.e. not local dates) to be resilient
+    // to time zone changes.
+    could_show_list->AppendDouble(time.ToInternalValue());
+  } else {
+    app_dict->SetDouble(event_key, time.ToInternalValue());
   }
-
-  // Dates are stored in their raw form (i.e. not local dates) to be resilient
-  // to time zone changes.
-  could_show_list->AppendDouble(time.ToInternalValue());
   settings->SetWebsiteSetting(pattern, ContentSettingsPattern::Wildcard(),
                               CONTENT_SETTINGS_TYPE_APP_BANNER, std::string(),
                               origin_dict.release());
 }
 
+bool AppBannerSettingsHelper::ShouldShowBanner(
+    content::WebContents* web_contents,
+    const GURL& origin_url,
+    const std::string& package_name_or_start_url,
+    base::Time time) {
+  // Don't show if it has been added to the homescreen.
+  base::Time added_time =
+      GetSingleBannerEvent(web_contents, origin_url, package_name_or_start_url,
+                           APP_BANNER_EVENT_DID_ADD_TO_HOMESCREEN);
+  if (!added_time.is_null())
+    return false;
+
+  base::Time blocked_time =
+      GetSingleBannerEvent(web_contents, origin_url, package_name_or_start_url,
+                           APP_BANNER_EVENT_DID_BLOCK);
+
+  // Null times are in the distant past, so the delta between real times and
+  // null events will always be greater than the limits.
+  if (time - blocked_time <
+      base::TimeDelta::FromDays(kMinimumBannerBlockedToBannerShown)) {
+    return false;
+  }
+
+  base::Time shown_time =
+      GetSingleBannerEvent(web_contents, origin_url, package_name_or_start_url,
+                           APP_BANNER_EVENT_DID_SHOW);
+  if (time - shown_time <
+      base::TimeDelta::FromDays(kMinimumDaysBetweenBannerShows)) {
+    return false;
+  }
+
+  std::vector<base::Time> could_show_events = GetCouldShowBannerEvents(
+      web_contents, origin_url, package_name_or_start_url);
+  return could_show_events.size() >= kCouldShowEventsToTrigger;
+}
+
 std::vector<base::Time> AppBannerSettingsHelper::GetCouldShowBannerEvents(
     content::WebContents* web_contents,
     const GURL& origin_url,
@@ -165,8 +215,9 @@ std::vector<base::Time> AppBannerSettingsHelper::GetCouldShowBannerEvents(
   if (!app_dict)
     return result;
 
+  std::string event_key(kBannerEventKeys[APP_BANNER_EVENT_COULD_SHOW]);
   base::ListValue* could_show_list = nullptr;
-  if (!app_dict->GetList(kCouldShowBannerEventsKey, &could_show_list))
+  if (!app_dict->GetList(event_key, &could_show_list))
     return result;
 
   for (auto value : *could_show_list) {
@@ -181,6 +232,39 @@ std::vector<base::Time> AppBannerSettingsHelper::GetCouldShowBannerEvents(
   return result;
 }
 
+base::Time AppBannerSettingsHelper::GetSingleBannerEvent(
+    content::WebContents* web_contents,
+    const GURL& origin_url,
+    const std::string& package_name_or_start_url,
+    AppBannerEvent event) {
+  DCHECK(event != APP_BANNER_EVENT_COULD_SHOW);
+  DCHECK(event < APP_BANNER_EVENT_NUM_EVENTS);
+
+  if (package_name_or_start_url.empty())
+    return base::Time();
+
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  HostContentSettingsMap* settings = profile->GetHostContentSettingsMap();
+  scoped_ptr<base::DictionaryValue> origin_dict =
+      GetOriginDict(settings, origin_url);
+
+  if (!origin_dict)
+    return base::Time();
+
+  base::DictionaryValue* app_dict =
+      GetAppDict(origin_dict.get(), package_name_or_start_url);
+  if (!app_dict)
+    return base::Time();
+
+  std::string event_key(kBannerEventKeys[event]);
+  double internal_time;
+  if (!app_dict->GetDouble(event_key, &internal_time))
+    return base::Time();
+
+  return base::Time::FromInternalValue(internal_time);
+}
+
 bool AppBannerSettingsHelper::IsAllowed(
     content::WebContents* web_contents,
     const GURL& origin_url,

chrome/browser/banners/app_banner_settings_helper.h

@@ -17,37 +17,62 @@ class WebContents;
 
 class GURL;
 
-// Utility class for reading and updating ContentSettings for app banners.
+// Utility class to record banner events for the given package or start url.
+//
+// These events are used to decide when banners should be shown, using a
+// heuristic based on how many different days in a recent period of time (for
+// example the past two weeks) the banner could have been shown, when it was
+// last shown, when it was last blocked, and when it was last installed (for
+// ServiceWorker style apps - native apps can query whether the app was
+// installed directly).
+//
+// The desired effect is to have banners appear once a user has demonstrated
+// an ongoing relationship with the app, and not to pester the user too much.
+//
+// For most events only the last event is recorded. The exception are the
+// could show events. For these a list of the events is maintained. At most
+// one event is stored per day, and events outside the window the heuristic
+// uses are discarded. Local times are used to enforce these rules, to ensure
+// what we count as a day matches what the user perceives to be days.
 class AppBannerSettingsHelper {
  public:
-  // TODO(benwells): Use this method to implement smarter triggering logic.
-  // See http://crbug.com/452825.
-  // Records that a banner could have been shown for the given package or start
-  // url.
-  //
-  // These events are used to decide when banners should be shown, using a
-  // heuristic based on how many different days in a recent period of time (for
-  // example the past two weeks) the banner could have been shown. The desired
-  // effect is to have banners appear once a user has demonstrated an ongoing
-  // relationship with the app.
-  //
-  // At most one event is stored per day, and events outside the window the
-  // heuristic uses are discarded. Local times are used to enforce these rules,
-  // to ensure what we count as a day matches what the user perceives to be
-  // days.
-  static void RecordCouldShowBannerEvent(
-      content::WebContents* web_contents,
-      const GURL& origin_url,
-      const std::string& package_name_or_start_url,
-      base::Time time);
+  enum AppBannerEvent {
+    APP_BANNER_EVENT_COULD_SHOW,
+    APP_BANNER_EVENT_DID_SHOW,
+    APP_BANNER_EVENT_DID_BLOCK,
+    APP_BANNER_EVENT_DID_ADD_TO_HOMESCREEN,
+    APP_BANNER_EVENT_NUM_EVENTS,
+  };
+
+  static void RecordBannerEvent(content::WebContents* web_contents,
+                                const GURL& origin_url,
+                                const std::string& package_name_or_start_url,
+                                AppBannerEvent event,
+                                base::Time time);
+
+  // Determine if the banner should be shown, given the recorded events for the
+  // supplied app.
+  static bool ShouldShowBanner(content::WebContents* web_contents,
+                               const GURL& origin_url,
+                               const std::string& package_name_or_start_url,
+                               base::Time time);
 
   // Gets the could have been shown events that are stored for the given package
-  // or start url. This is only used for testing.
+  // or start url. This is only exposed for testing.
   static std::vector<base::Time> GetCouldShowBannerEvents(
       content::WebContents* web_contents,
       const GURL& origin_url,
       const std::string& package_name_or_start_url);
 
+  // Get the recorded event for an event type that only records the last event.
+  // Should not be used with APP_BANNER_EVENT_COULD_SHOW. This is only exposed
+  // for testing.
+  static base::Time GetSingleBannerEvent(
+      content::WebContents* web_contents,
+      const GURL& origin_url,
+      const std::string& package_name_or_start_url,
+      AppBannerEvent event);
+
   // Checks if a URL is allowed to show a banner for the given package or start
   // url.
   static bool IsAllowed(content::WebContents* web_contents,