[ImportantSites] Implementing dialog level blacklisting and blacklist expiration

chrome/browser/engagement/important_sites_util.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/engagement/important_sites_util.h"
 
 #include <algorithm>
 #include <map>
 #include <memory>
 #include <set>
 #include <utility>
 
 #include "base/containers/hash_tables.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/banners/app_banner_settings_helper.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #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"
 
 namespace {
 using bookmarks::BookmarkModel;
 using ImportantDomainInfo = ImportantSitesUtil::ImportantDomainInfo;
 
+// Note: These values are stored on both the per-site content settings
+// dictionary and the dialog preference dictionary.
+
+static const char kTimeLastIgnored[] = "TimeLastIgnored";
+static const int kBlacklistExpirationTimeDays = 30 * 5;
+
 static const char kNumTimesIgnoredName[] = "NumTimesIgnored";
 static const int kTimesIgnoredForBlacklist = 3;
 
 // These are the maximum # of bookmarks we can use as signals. If the user has
 // <= kMaxBookmarks, then we just use those bookmarks. Otherwise we filter all
 // bookmarks on site engagement > 0, sort, and trim to kMaxBookmarks.
 static const int kMaxBookmarks = 5;
 
 // Do not change the values here, as they are used for UMA histograms.
 enum ImportantReason {
@@ skipping 31 matching lines @@
   CROSSED_NOTIFICATIONS = 1,
   CROSSED_ENGAGEMENT = 2,
   CROSSED_NOTIFICATIONS_AND_ENGAGEMENT = 3,
   CROSSED_DURABLE_AND_ENGAGEMENT = 4,
   CROSSED_NOTIFICATIONS_AND_DURABLE = 5,
   CROSSED_NOTIFICATIONS_AND_DURABLE_AND_ENGAGEMENT = 6,
   CROSSED_REASON_UNKNOWN = 7,
   CROSSED_REASON_BOUNDARY
 };
 
+void RecordIgnore(base::DictionaryValue* dict) {
+  int times_ignored = 0;
+  dict->GetInteger(kNumTimesIgnoredName, &times_ignored);
+  dict->SetInteger(kNumTimesIgnoredName, ++times_ignored);
+  dict->SetDouble(kTimeLastIgnored, base::Time::Now().ToDoubleT());
+}
+
+// If we should blacklist the item with the given dictionary ignored record.
+bool ShouldSuppressItem(base::DictionaryValue* dict) {
+  double last_ignored_time = 0;
+  int times_ignored = 0;

[dominickn, 2017/02/03 21:14:51]

Nit: move times_ignored down to line 117 (right before it's used). If we early
exit from this method times_ignored is never touched.

[dmurph, 2017/02/04 00:52:25]

Done.

+  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);

[dominickn, 2017/02/03 21:14:51]

Perhaps dict->Remove(kTimeLastIgnored) as well?

[dmurph, 2017/02/04 00:52:25]

Done.

+      return false;
+    }
+  }
+
+  if (dict->GetInteger(kNumTimesIgnoredName, &times_ignored) &&

[dominickn, 2017/02/03 21:14:51]

Nit: just do

return (dict->GetInteger(kNumTimesIgnoredName, &times_ignored) && times_ignored
>= kTimesIgnoredForBlacklist);

And remove the last two returns here.

[dmurph, 2017/02/04 00:52:25]

Nice catch, thanks.

+      times_ignored >= kTimesIgnoredForBlacklist) {
+    return true;
+  }
+
+  return false;
+}
+
 CrossedReason GetCrossedReasonFromBitfield(int32_t reason_bitfield) {
   bool durable = (reason_bitfield & (1 << ImportantReason::DURABLE)) != 0;
   bool notifications =
       (reason_bitfield & (1 << ImportantReason::NOTIFICATIONS)) != 0;
   bool engagement = (reason_bitfield & (1 << ImportantReason::ENGAGEMENT)) != 0;
   if (durable && notifications && engagement)
     return CROSSED_NOTIFICATIONS_AND_DURABLE_AND_ENGAGEMENT;
   else if (notifications && durable)
     return CROSSED_NOTIFICATIONS_AND_DURABLE;
   else if (notifications && engagement)
@@ skipping 94 matching lines @@
     }
 
     std::unique_ptr<base::DictionaryValue> dict =
         base::DictionaryValue::From(map->GetWebsiteSetting(
             origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "",
             nullptr));
 
     if (!dict)
       continue;
 
-    int times_ignored = 0;
-    if (!dict->GetInteger(kNumTimesIgnoredName, &times_ignored) ||
-        times_ignored < kTimesIgnoredForBlacklist) {
-      continue;
-    }
-
-    ignoring_domains.insert(origin.host());
+    if (ShouldSuppressItem(dict.get()))
+      ignoring_domains.insert(origin.host());
   }
   return ignoring_domains;
 }
 
 void PopulateInfoMapWithSiteEngagement(
     Profile* profile,
     blink::mojom::EngagementLevel minimum_engagement,
     std::map<GURL, double>* engagement_map,
     base::hash_map<std::string, ImportantDomainInfo>* output) {
   SiteEngagementService* service = SiteEngagementService::Get(profile);
@@ skipping 95 matching lines @@
     GURL origin(site.primary_pattern.ToString());
     if (!AppBannerSettingsHelper::WasLaunchedRecently(profile, origin, now))
       continue;
     MaybePopulateImportantInfoForReason(origin, &content_origins,
                                         ImportantReason::HOME_SCREEN, output);
   }
 }
 
 }  // namespace
 
+bool ImportantSitesUtil::IsDialogDisabled(Profile* profile) {
+  PrefService* service = profile->GetPrefs();
+  DictionaryPrefUpdate update(service, prefs::kImportantSitesDialogHistory);
+
+  return ShouldSuppressItem(update.Get());
+}
+
+void ImportantSitesUtil::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterDictionaryPref(prefs::kImportantSitesDialogHistory);
+}
+
 std::vector<ImportantDomainInfo>
 ImportantSitesUtil::GetImportantRegisterableDomains(Profile* profile,
                                                     size_t max_results) {
   base::hash_map<std::string, ImportantDomainInfo> important_info;
   std::map<GURL, double> engagement_map;
 
   PopulateInfoMapWithSiteEngagement(
       profile, blink::mojom::EngagementLevel::MEDIUM, &engagement_map,
       &important_info);
 
@@ skipping 45 matching lines @@
     RECORD_UMA_FOR_IMPORTANT_REASON(
         "Storage.ImportantSites.CBDChosenReason",
         "Storage.ImportantSites.CBDChosenReasonCount", reason_bitfield);
   }
   for (int32_t reason_bitfield : ignored_sites_reason_bitfield) {
     RECORD_UMA_FOR_IMPORTANT_REASON(
         "Storage.ImportantSites.CBDIgnoredReason",
         "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 (!blacklisted_sites.empty()) {
+    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>();
 
+      RecordIgnore(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);
+    RecordIgnore(update.Get());
+  }
+
+  // We clear our blacklist for sites that the user chose.
+  for (const std::string& blacklisted_site : blacklisted_sites) {
+    GURL origin("http://" + blacklisted_site);
+    std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
+    dict->SetInteger(kNumTimesIgnoredName, 0);
+    dict->SetDouble(kTimeLastIgnored, base::Time::Now().ToDoubleT());

[dominickn, 2017/02/03 21:14:51]

Nit: since you're clearing the kNumTimesIgnored value, should you just leave the
TimeLastIgnored as unset?

[dmurph, 2017/02/04 00:52:25]

Ah true. Done.

     map->SetWebsiteSettingDefaultScope(
         origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "",
         std::move(dict));
-  }
-
-  // We clear our blacklist for sites that the user chose.
-  for (const std::string& ignored_site : blacklisted_sites) {
-    GURL origin("http://" + ignored_site);
-    std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
-    dict->SetInteger(kNumTimesIgnoredName, 0);
-    map->SetWebsiteSettingDefaultScope(
-        origin, origin, CONTENT_SETTINGS_TYPE_IMPORTANT_SITE_INFO, "",
-        std::move(dict));
   }
 
   // Finally, record our old crossed-stats.
   // Note: we don't plan on adding new metrics here, this is just for the finch
   // experiment to give us initial data on what signals actually mattered.
   for (int32_t reason_bitfield : blacklisted_sites_reason_bitfield) {
     UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason",
                               GetCrossedReasonFromBitfield(reason_bitfield),
                               CROSSED_REASON_BOUNDARY);
   }
 }
 
 void ImportantSitesUtil::MarkOriginAsImportantForTesting(Profile* profile,
                                                          const GURL& origin) {
   SiteEngagementScore::SetParamValuesForTesting();
   // First get data from site engagement.
   SiteEngagementService* site_engagement_service =
       SiteEngagementService::Get(profile);
   site_engagement_service->ResetScoreForURL(
       origin, SiteEngagementScore::GetMediumEngagementBoundary());
   DCHECK(site_engagement_service->IsEngagementAtLeast(
       origin, blink::mojom::EngagementLevel::MEDIUM));
 }