[ImportantSites] Implementing dialog level blacklisting and blacklist expiration

chrome/browser/engagement/important_sites_util.cc

Index: chrome/browser/engagement/important_sites_util.cc
diff --git a/chrome/browser/engagement/important_sites_util.cc b/chrome/browser/engagement/important_sites_util.cc
index cdb5ff7dd5ccc1e2e88d5ec6cac644848c88a0d8..5323aa4cea3dcf0a2e61beead5d65d9f09ce9e21 100644
--- a/chrome/browser/engagement/important_sites_util.cc
+++ b/chrome/browser/engagement/important_sites_util.cc
@@ -14,6 +14,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/banners/app_banner_settings_helper.h"
@@ -22,9 +23,13 @@
 #include "chrome/browser/engagement/site_engagement_score.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_service.h"
+#include "components/prefs/scoped_user_pref_update.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "third_party/WebKit/public/platform/site_engagement.mojom.h"
 #include "url/gurl.h"
@@ -33,6 +38,9 @@ namespace {
 using bookmarks::BookmarkModel;
 using ImportantDomainInfo = ImportantSitesUtil::ImportantDomainInfo;
 
+static const char kTimeLastIgnored[] = "TimeBlacklisted";
+static const int kBlacklistExpirationTimeDays = 30 * 5;
+
 static const char kNumTimesIgnoredName[] = "NumTimesIgnored";
 static const int kTimesIgnoredForBlacklist = 3;
 
@@ -84,6 +92,31 @@ enum CrossedReason {
   CROSSED_REASON_BOUNDARY
 };
 
+void IncrementTimesIgnoredAndSetDate(base::DictionaryValue* dict) {
+  int times_ignored = 0;
+  dict->GetInteger(kNumTimesIgnoredName, &times_ignored);
+  dict->SetInteger(kNumTimesIgnoredName, ++times_ignored);
+  dict->SetString(kTimeLastIgnored,
+                  base::Int64ToString(base::Time::Now().ToInternalValue()));

[dominickn, 2017/02/01 22:22:04]

Instead of a string, can you use a double? It seems much more common to use a
double for internal time values in DictionaryValues than strings, an that will
save the string conversion.

[dmurph, 2017/02/02 23:53:29]

Done.

+}
+
+// Returns if we cleared the blacklist.
+bool ClearBlacklistIfOld(base::DictionaryValue* dict) {
+  std::string date_blacklisted;
+  dict->GetString(kTimeLastIgnored, &date_blacklisted);
+  int64_t time_internal_value;
+  if (!date_blacklisted.empty() &&
+      base::StringToInt64(date_blacklisted, &time_internal_value)) {
+    base::TimeDelta diff =
+        base::Time::Now() - base::Time::FromInternalValue(time_internal_value);
+    if (diff >= base::TimeDelta::FromDays(kBlacklistExpirationTimeDays)) {
+      dict->SetInteger(kNumTimesIgnoredName, 0);
+      return true;
+    }
+  }
+  return false;
+}
+
 CrossedReason GetCrossedReasonFromBitfield(int32_t reason_bitfield) {
   bool durable = (reason_bitfield & (1 << ImportantReason::DURABLE)) != 0;
   bool notifications =
@@ -198,8 +231,11 @@ base::hash_set<std::string> GetBlacklistedImportantDomains(Profile* profile) {
     if (!dict)
       continue;
 
+    bool cleared_blacklist = ClearBlacklistIfOld(dict.get());
+
     int times_ignored = 0;
-    if (!dict->GetInteger(kNumTimesIgnoredName, &times_ignored) ||
+    if (cleared_blacklist ||
+        !dict->GetInteger(kNumTimesIgnoredName, &times_ignored) ||
         times_ignored < kTimesIgnoredForBlacklist) {
       continue;
     }
@@ -320,6 +356,27 @@ void PopulateInfoMapWithHomeScreen(
 
 }  // namespace
 
+bool ImportantSitesUtil::IsDialogDisabled(Profile* profile) {

[dominickn, 2017/02/01 22:22:04]

This method doesn't seem to be called from anywhere?

[dmurph, 2017/02/02 23:53:29]

Changed, we now call it from Java.

+  PrefService* service = profile->GetPrefs();
+  DictionaryPrefUpdate update(service, prefs::kImportantSitesDialogHistory);
+
+  if (ClearBlacklistIfOld(update.Get())) {

[dominickn, 2017/02/01 22:22:04]

Nit: remove {} for consistency.

[dmurph, 2017/02/02 23:53:29]

Done.

+    return false;
+  }
+
+  int times_ignored = 0;
+  update->GetInteger(kNumTimesIgnoredName, &times_ignored);
+  return times_ignored >= kTimesIgnoredForBlacklist;
+}
+
+void ImportantSitesUtil::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  auto dict = base::MakeUnique<base::DictionaryValue>();
+  dict->SetInteger(kNumTimesIgnoredName, 0);
+  registry->RegisterDictionaryPref(prefs::kImportantSitesDialogHistory,
+                                   dict.release());
+}
+
 std::vector<ImportantDomainInfo>
 ImportantSitesUtil::GetImportantRegisterableDomains(Profile* profile,
                                                     size_t max_results) {
@@ -385,29 +442,36 @@ void ImportantSitesUtil::RecordBlacklistedAndIgnoredImportantSites(
         "Storage.ImportantSites.CBDIgnoredReasonCount", reason_bitfield);
   }
 
-  // We use the ignored sites to update our important sites blacklist.
   HostContentSettingsMap* map =
       HostContentSettingsMapFactory::GetForProfile(profile);
-  for (const std::string& ignored_site : ignored_sites) {
-    GURL origin("http://" + ignored_site);
-    std::unique_ptr<base::Value> value = map->GetWebsiteSetting(
-        origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "", nullptr);
-
-    std::unique_ptr<base::DictionaryValue> dict =
-        base::DictionaryValue::From(map->GetWebsiteSetting(
-            origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "",
-            nullptr));
 
-    int times_ignored = 0;
-    if (dict)
-      dict->GetInteger(kNumTimesIgnoredName, &times_ignored);
-    else
-      dict = base::MakeUnique<base::DictionaryValue>();
-    dict->SetInteger(kNumTimesIgnoredName, ++times_ignored);
-
-    map->SetWebsiteSettingDefaultScope(
-        origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "",
-        std::move(dict));
+  // We use the ignored sites to update our important sites blacklist only if
+  // the user chose to blacklist a site.
+  if (ignored_sites.size() != blacklisted_sites.size()) {
+    for (const std::string& ignored_site : ignored_sites) {
+      GURL origin("http://" + ignored_site);
+      std::unique_ptr<base::Value> value = map->GetWebsiteSetting(

[dominickn, 2017/02/01 22:22:04]

What is this line doing? It doesn't appear to be used.

[dmurph, 2017/02/02 23:53:29]

You're right. Removed.

+          origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "",
+          nullptr);
+      std::unique_ptr<base::DictionaryValue> dict =
+          base::DictionaryValue::From(map->GetWebsiteSetting(
+              origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "",
+              nullptr));
+
+      if (!dict)
+        dict = base::MakeUnique<base::DictionaryValue>();
+
+      IncrementTimesIgnoredAndSetDate(dict.get());
+
+      map->SetWebsiteSettingDefaultScope(
+          origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "",
+          std::move(dict));
+    }
+  } else {
+    // Record that the user did not interact with the dialog.
+    PrefService* service = profile->GetPrefs();
+    DictionaryPrefUpdate update(service, prefs::kImportantSitesDialogHistory);
+    IncrementTimesIgnoredAndSetDate(update.Get());
   }
 
   // We clear our blacklist for sites that the user chose.