Update content setting for app banners to store more information.

chrome/browser/android/banners/app_banner_settings_helper.cc

Index: chrome/browser/android/banners/app_banner_settings_helper.cc
diff --git a/chrome/browser/android/banners/app_banner_settings_helper.cc b/chrome/browser/android/banners/app_banner_settings_helper.cc
index ed3298ccbb948de4c283d71f759ae221b8c7b62f..aa418ba193dcba9b5a3cc1f3fbd8afd3d998b279 100644
--- a/chrome/browser/android/banners/app_banner_settings_helper.cc
+++ b/chrome/browser/android/banners/app_banner_settings_helper.cc
@@ -11,35 +11,47 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "content/public/browser/web_contents.h"
+#include "net/base/escape.h"
 #include "url/gurl.h"
 
 namespace {
-std::string SanitizePackageName(std::string package_name) {
-  // DictionaryValue doesn't allow '.' in the keys.  Replace them with ' '
-  // because you can't have a package name with a ' ' in it.
-  std::replace(package_name.begin(), package_name.end(), '.', ' ');
-  return package_name;
+
+std::string SanitizeKeyName(std::string key_name) {
+  // DictionaryValue doesn't allow '.' in the keys. So, first get rid of spaces

[Bernhard Bauer, 2015/01/29 16:49:36]

Yes, it does :) You can use SetWithoutPathExpansion().

[benwells, 2015/01/30 06:27:02]

Done.

+  // by escaping, then replace dots with spaces.
+  key_name = net::EscapeUrlEncodedData(key_name, false);
+  std::replace(key_name.begin(), key_name.end(), '.', ' ');
+  return key_name;
 }
 
-// Max number of apps that a particular site may show a banner for.
+// Max number of apps (including web native apps) that a particular site may

[Bernhard Bauer, 2015/01/29 16:49:35]

What is a web native app? Does that mean Fizz?

[benwells, 2015/01/30 06:27:02]

Yes it means Fizz. I'll explain better.

+// show a banner for.
 const size_t kMaxAppsPerSite = 3;
-}  // namespace
 
-bool AppBannerSettingsHelper::IsAllowed(content::WebContents* web_contents,
-                                        const GURL& origin_url,
-                                        const std::string& package_name) {
-  std::string sanitized_package_name = SanitizePackageName(package_name);
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  if (profile->IsOffTheRecord() || web_contents->GetURL() != origin_url ||
-      sanitized_package_name.empty()) {
-    return false;
-  }
+// Oldest could show banner event we care about, in days.
+const unsigned int kOldestCouldShowBannerEventInDays = 14;
 
-  // Check if this combination has been previously disabled.
-  HostContentSettingsMap* settings = profile->GetHostContentSettingsMap();
+// Dictionary key to use for the 'could show banner' events.
+const char kCouldShowBannerEventsKey[] = "couldShowBannerEvents";
+
+// 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);

[Bernhard Bauer, 2015/01/29 16:49:36]

If we are using local time here, there could of course be weird edge cases with
time zone changes, but I guess we don't care about that, as the 14 days are more
of a heuristic anyway?

[benwells, 2015/01/30 06:27:02]

Yep that's right, its a heuristic. I think the same edge cases exist with UTC
time and someone changing their clock.

I think local time works better for the heuristics, which are about having the
banner appear if a site is visited on different days. This will feel more
predictable if the days are local days.

+  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) {
   if (!settings)
-    return false;
+    return scoped_ptr<base::DictionaryValue>();
+
   scoped_ptr<base::Value> value =
       settings->GetWebsiteSetting(origin_url,
                                   origin_url,
@@ -47,66 +59,155 @@ bool AppBannerSettingsHelper::IsAllowed(content::WebContents* web_contents,
                                   std::string(),
                                   NULL);
   if (!value.get()) {
-    // We've never blocked a banner on this site.
-    return true;
-  } else if (value->IsType(base::Value::TYPE_DICTIONARY)) {
-    // We expect to get a Dictionary back, where the keys are the package names.
-    base::DictionaryValue* banner_dict =
-        static_cast<base::DictionaryValue*>(value.get());
-    bool is_allowed = false;
-    if (banner_dict->GetBoolean(sanitized_package_name, &is_allowed)) {
-      return is_allowed;
-    } else {
-      return banner_dict->size() < ::kMaxAppsPerSite;
-    }
-  } else {
-    // Somehow the value we got back is not a dictionary (e.g. the settings were
-    // corrupted by malware).  Try to clear it out.
-    ContentSettingsPattern pattern(ContentSettingsPattern::FromURL(origin_url));
-    if (pattern.IsValid()) {
-      settings->SetWebsiteSetting(pattern,
-                                  ContentSettingsPattern::Wildcard(),
-                                  CONTENT_SETTINGS_TYPE_APP_BANNER,
-                                  std::string(),
-                                  NULL);
-      return true;
+    return scoped_ptr<base::DictionaryValue>();
+  }
+
+  if (!value->IsType(base::Value::TYPE_DICTIONARY)) {
+    return scoped_ptr<base::DictionaryValue>();
+  }
+
+  return scoped_ptr<base::DictionaryValue>(

[Bernhard Bauer, 2015/01/29 16:49:35]

You can use make_scoped_ptr() here, which is a bit less to type.

[benwells, 2015/01/30 06:27:02]

nice, thanks. I think I've used it everywhere I can.

+      static_cast<base::DictionaryValue*>(value.release()));
+}
+
+base::DictionaryValue* GetAppDict(base::DictionaryValue* origin_dict,
+                                  const std::string& key_name) {
+  base::DictionaryValue* app_dict = nullptr;
+  if (!origin_dict->GetDictionary(key_name, &app_dict)) {
+    // Don't allow more than kMaxAppsPerSite dictionaries.
+    if (origin_dict->size() < kMaxAppsPerSite) {
+      app_dict = new base::DictionaryValue();
+      origin_dict->Set(key_name, scoped_ptr<base::DictionaryValue>(app_dict));

[Bernhard Bauer, 2015/01/29 16:49:36]

Same here, just use make_scoped_ptr.

[benwells, 2015/01/30 06:27:02]

Done.

     }
   }
 
-  return false;
+  return app_dict;
 }
 
-void AppBannerSettingsHelper::Block(content::WebContents* web_contents,
-                                    const GURL& origin_url,
-                                    const std::string& package_name) {
-  std::string sanitized_package_name = SanitizePackageName(package_name);
-  DCHECK(!sanitized_package_name.empty());
+}  // namespace
 
+void RecordCouldShowBannerEvent(content::WebContents* web_contents,
+                                const GURL& origin_url,
+                                const std::string& package_name_or_start_url,
+                                base::Time time) {
+  std::string app_key = SanitizeKeyName(package_name_or_start_url);
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  HostContentSettingsMap* settings = profile->GetHostContentSettingsMap();
+  if (profile->IsOffTheRecord() || web_contents->GetURL() != origin_url ||
+      app_key.empty()) {
+    return;
+  }
+
   ContentSettingsPattern pattern(ContentSettingsPattern::FromURL(origin_url));
-  if (!settings || !pattern.IsValid())
+  if (!pattern.IsValid())
     return;
 
-  scoped_ptr<base::Value> value =
-      settings->GetWebsiteSetting(origin_url,
-                                  origin_url,
-                                  CONTENT_SETTINGS_TYPE_APP_BANNER,
-                                  std::string(),
-                                  NULL);
-  base::DictionaryValue* banner_dict = NULL;
-  if (value.get() && value->IsType(base::Value::TYPE_DICTIONARY)) {
-    banner_dict = static_cast<base::DictionaryValue*>(value.release());
-  } else {
-    banner_dict = new base::DictionaryValue();
+  HostContentSettingsMap* settings = profile->GetHostContentSettingsMap();
+  scoped_ptr<base::DictionaryValue> origin_dict = GetOriginDict(
+      settings, origin_url);
+  if (!origin_dict)
+    origin_dict.reset(new base::DictionaryValue());
+
+  base::DictionaryValue* app_dict = GetAppDict(origin_dict.get(), app_key);
+  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,
+                  scoped_ptr<base::ListValue>(could_show_list));
   }
 
+  // Trim any items that are older than we should care about. If the list
+  // already contains the date of interest, just exit.
+  base::Time date = DateFromTime(time);
+  for (base::ValueVector::iterator it = could_show_list->begin();
+       it != could_show_list->end(); ++it) {
+    if ((*it)->IsType(base::Value::TYPE_DOUBLE)) {
+      double internal_date;
+      DCHECK((*it)->GetAsDouble(&internal_date));

[Bernhard Bauer, 2015/01/29 16:49:35]

This will be compiled out in a Release build! The correct way to do this is to
set a success boolean value, then DCHECK that.

[benwells, 2015/01/30 06:27:02]

Ah, of course! Thanks for finding that. I don't think the DCHECK is really
necessary anyway, I just just being paranoid.

+      base::Time other_date = base::Time::FromInternalValue(internal_date);
+      if (other_date == date)
+        return;
+
+      base::TimeDelta delta = date - other_date;
+      if (delta <= base::TimeDelta::FromDays(kOldestCouldShowBannerEventInDays))
+        continue;
+    }
+    could_show_list->Erase(it, nullptr);
+  }
+
+  could_show_list->AppendDouble(date.ToInternalValue());
+  settings->SetWebsiteSetting(pattern,
+                              ContentSettingsPattern::Wildcard(),
+                              CONTENT_SETTINGS_TYPE_APP_BANNER,
+                              std::string(),
+                              origin_dict.get());
+}
+
+bool AppBannerSettingsHelper::IsAllowed(
+    content::WebContents* web_contents,
+    const GURL& origin_url,
+    const std::string& package_name_or_start_url) {
+  std::string app_key = SanitizeKeyName(package_name_or_start_url);
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  if (profile->IsOffTheRecord() || web_contents->GetURL() != origin_url ||
+      app_key.empty()) {
+    return false;
+  }
+
+  HostContentSettingsMap* settings = profile->GetHostContentSettingsMap();
+  scoped_ptr<base::DictionaryValue> origin_dict = GetOriginDict(
+      settings, origin_url);
+
+  if (!origin_dict)
+    return true;
+
+  base::DictionaryValue* app_dict = GetAppDict(origin_dict.get(), app_key);
+  if (!app_dict)
+    return true;
+
+  bool has_blocked;
+  if (!app_dict->GetBoolean(kHasBlockedKey, &has_blocked))
+    return true;
+
+  return !has_blocked;
+}
+
+void AppBannerSettingsHelper::Block(
+    content::WebContents* web_contents,
+    const GURL& origin_url,
+    const std::string& package_name_or_start_url) {
+  std::string app_key = SanitizeKeyName(package_name_or_start_url);
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  if (profile->IsOffTheRecord() || web_contents->GetURL() != origin_url ||
+      app_key.empty()) {
+    return;
+  }
+
+  ContentSettingsPattern pattern(ContentSettingsPattern::FromURL(origin_url));
+  if (!pattern.IsValid())
+    return;
+
+  HostContentSettingsMap* settings = profile->GetHostContentSettingsMap();
+  scoped_ptr<base::DictionaryValue> origin_dict = GetOriginDict(
+      settings, origin_url);
+
+  if (!origin_dict)
+    return;
+
+  base::DictionaryValue* app_dict = GetAppDict(origin_dict.get(), app_key);
+  if (!app_dict)
+    return;
+
   // Update the setting and save it back.
-  banner_dict->SetBoolean(sanitized_package_name, false);
+  app_dict->SetBoolean(kHasBlockedKey, false);
   settings->SetWebsiteSetting(pattern,
                               ContentSettingsPattern::Wildcard(),
                               CONTENT_SETTINGS_TYPE_APP_BANNER,
                               std::string(),
-                              banner_dict);
+                              origin_dict.get());
 }