[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..96195f9230939c3c741be3d75599282ab5dca61e 100644
--- a/chrome/browser/engagement/important_sites_util.cc
+++ b/chrome/browser/engagement/important_sites_util.cc
@@ -22,9 +22,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 +37,9 @@ namespace {
 using bookmarks::BookmarkModel;
 using ImportantDomainInfo = ImportantSitesUtil::ImportantDomainInfo;
 
+static const char kTimeLastIgnored[] = "TimeBlacklisted";

[dominickn, 2017/02/03 19:30:54]

I'll mention that it took me the longest time to realise that the ignored /
blacklist stuff was generic across *both* sites and the dialog itself.

Perhaps make this constant "TimeLastIgnored" to match the constant name?

[dmurph, 2017/02/03 20:03:29]

Done, and added a comment note.

+static const int kBlacklistExpirationTimeDays = 30 * 5;
+
 static const char kNumTimesIgnoredName[] = "NumTimesIgnored";
 static const int kTimesIgnoredForBlacklist = 3;
 
@@ -84,6 +91,27 @@ enum CrossedReason {
   CROSSED_REASON_BOUNDARY
 };
 
+void IncrementTimesIgnoredAndSetDate(base::DictionaryValue* dict) {

[dominickn, 2017/02/03 19:30:54]

Nit: perhaps just RecordIgnore()?

[dmurph, 2017/02/03 20:03:29]

Done.

+  int times_ignored = 0;
+  dict->GetInteger(kNumTimesIgnoredName, &times_ignored);
+  dict->SetInteger(kNumTimesIgnoredName, ++times_ignored);
+  dict->SetDouble(kTimeLastIgnored, base::Time::Now().ToDoubleT());
+}
+
+// Returns if we cleared the blacklist.
+bool ClearBlacklistIfOld(base::DictionaryValue* dict) {

[dominickn, 2017/02/03 19:30:54]

I'm not sure why the times_ignored < kTimesIgnoredForBlacklist logic isn't also
in here. It makes sense to me to have this method:

1. was the last ignored time too long ago? reset, don't suppress
2. otherwise, check the total number of ignores, and suppress as necessary

As a name, I think "ShouldSuppress()" reads more cleanly (especially when this
is inlined in an if conditional), which involves flipping some of the logic in
here. WDYT?

[dmurph, 2017/02/03 20:03:29]

Yeah, that sounds much better. Thanks!

+  double last_ignored_time = 0;
+  if (dict->GetDouble(kTimeLastIgnored, &last_ignored_time)) {
+    base::TimeDelta diff =
+        base::Time::Now() - base::Time::FromDoubleT(last_ignored_time);
+    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 +226,11 @@ base::hash_set<std::string> GetBlacklistedImportantDomains(Profile* profile) {
     if (!dict)
       continue;
 
+    bool cleared_blacklist = ClearBlacklistIfOld(dict.get());

[dominickn, 2017/02/03 19:30:54]

Inline this in the if statement? cleared_blacklist is a lot less descriptive
than if (ShouldSuppress(dict.get()))

[dmurph, 2017/02/03 20:03:29]

Done.

+
     int times_ignored = 0;
-    if (!dict->GetInteger(kNumTimesIgnoredName, &times_ignored) ||
+    if (cleared_blacklist ||

[dominickn, 2017/02/03 19:30:54]

Much nicer would be:

if (ShouldSuppress(dict.get()))
  continue

With the ignore logic folded into ShouldSuppress()

[dmurph, 2017/02/03 20:03:29]

Done.

+        !dict->GetInteger(kNumTimesIgnoredName, &times_ignored) ||
         times_ignored < kTimesIgnoredForBlacklist) {
       continue;
     }
@@ -320,6 +351,26 @@ void PopulateInfoMapWithHomeScreen(
 
 }  // namespace
 
+bool ImportantSitesUtil::IsDialogDisabled(Profile* profile) {
+  PrefService* service = profile->GetPrefs();
+  DictionaryPrefUpdate update(service, prefs::kImportantSitesDialogHistory);
+
+  if (ClearBlacklistIfOld(update.Get()))

[dominickn, 2017/02/03 19:30:54]

Just having a return ShouldSuppress(update.Get()); would be much nicer here.

[dmurph, 2017/02/03 20:03:29]

Done.

+    return false;
+
+  int times_ignored = 0;
+  update->GetInteger(kNumTimesIgnoredName, &times_ignored);
+  return times_ignored >= kTimesIgnoredForBlacklist;
+}
+
+void ImportantSitesUtil::RegisterProfilePrefs(

[dominickn, 2017/02/03 19:30:54]

Is it necessary to register the pref with a default value like this?
dict->GetInteger should leave the out variable untouched if it fails so you
should just be able to get away without registering anything.

[dmurph, 2017/02/03 20:03:29]

I need to register the pref at least. Yeah, I can leave the default value out.

+    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 +436,33 @@ 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));
+  // 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::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);
+      if (!dict)
+        dict = base::MakeUnique<base::DictionaryValue>();
 
-    map->SetWebsiteSettingDefaultScope(
-        origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "",
-        std::move(dict));
+      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.