[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;
 
+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;
 
 // 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 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 =
       (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;
 
+    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;
     }
 
     ignoring_domains.insert(origin.host());
   }
   return ignoring_domains;
 }
 
 void PopulateInfoMapWithSiteEngagement(
@@ skipping 100 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);
+
+  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) {
   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 (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.
   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));
@@ skipping 13 matching lines @@
                                                          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));
 }