Local NTP: expire blocked promo IDs after 28 days

This is an outcome of a Privacy Working Group review.

Bug: 1003508
Change-Id: Idfba7d00a965bbe89d2213281633554dc90c65ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1854799Reviewed-by: Kyle Milka <kmilka@chromium.org>
Commit-Queue: Dan Beam <dbeam@chromium.org>
Auto-Submit: Dan Beam <dbeam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705324}

chrome/browser/search/promos/promo_service.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/feature_list.h"
+#include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/search/ntp_features.h"
 #include "chrome/common/pref_names.h"
@@ -26,6 +27,9 @@
 
 namespace {
 
+// The number of days until a blocklist entry expires.
+const int kDaysThatBlocklistExpiresIn = 28;
+
 const char kNewTabPromosApiPath[] = "/async/newtab_promos";
 
 const char kXSSIResponsePreamble[] = ")]}'";
@@ -187,7 +191,7 @@ void PromoService::OnJsonParsed(base::Value value) {
   PromoService::Status status;
 
   if (JsonToPromoData(value, &result)) {
-    bool is_blocked = IsBlocked(result->promo_id);
+    bool is_blocked = IsBlockedAfterClearingExpired(result->promo_id);
     if (is_blocked)
       result = PromoData();
     status = is_blocked ? Status::OK_BUT_BLOCKED : Status::OK_WITH_PROMO;
@@ -213,7 +217,7 @@ void PromoService::Shutdown() {
 
 // static
 void PromoService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
-  registry->RegisterListPref(prefs::kNtpPromoBlocklist);
+  registry->RegisterDictionaryPref(prefs::kNtpPromoBlocklist);
 }
 
 void PromoService::AddObserver(PromoServiceObserver* observer) {
@@ -225,11 +229,14 @@ void PromoService::RemoveObserver(PromoServiceObserver* observer) {
 }
 
 void PromoService::BlocklistPromo(const std::string& promo_id) {
-  if (!CanBlockPromos() || promo_id.empty() || IsBlocked(promo_id))
+  if (!CanBlockPromos() || promo_id.empty() ||
+      IsBlockedAfterClearingExpired(promo_id)) {
     return;
+  }
 
-  ListPrefUpdate update(pref_service_, prefs::kNtpPromoBlocklist);
-  update->Append(promo_id);
+  DictionaryPrefUpdate update(pref_service_, prefs::kNtpPromoBlocklist);
+  double now = base::Time::Now().ToDeltaSinceWindowsEpoch().InSecondsF();
+  update->SetDoubleKey(promo_id, now);
 
   if (promo_data_ && promo_data_->promo_id == promo_id) {
     promo_data_ = PromoData();
@@ -256,16 +263,34 @@ void PromoService::NotifyObservers() {
   }
 }
 
-bool PromoService::IsBlocked(const std::string& promo_id) const {
+bool PromoService::IsBlockedAfterClearingExpired(
+    const std::string& promo_id) const {
   if (promo_id.empty() || !CanBlockPromos())
     return false;
 
-  const auto* blocklist = pref_service_->GetList(prefs::kNtpPromoBlocklist);
-  for (const auto& blocked : blocklist->GetList()) {
-    if (blocked.GetString() == promo_id)
-      return true;
+  auto expired_delta = base::TimeDelta::FromDays(kDaysThatBlocklistExpiresIn);
+  auto expired_time = base::Time::Now() - expired_delta;
+  double expired = expired_time.ToDeltaSinceWindowsEpoch().InSecondsF();
+
+  bool found = false;
+
+  std::vector<std::string> expired_ids;
+
+  for (const auto& blocked :
+       pref_service_->GetDictionary(prefs::kNtpPromoBlocklist)->DictItems()) {
+    if (!blocked.second.is_double() || blocked.second.GetDouble() < expired)
+      expired_ids.emplace_back(blocked.first);
+    else if (!found && blocked.first == promo_id)
+      found = true;  // Don't break; keep clearing expired prefs.
+  }
+
+  if (!expired_ids.empty()) {
+    DictionaryPrefUpdate update(pref_service_, prefs::kNtpPromoBlocklist);
+    for (const std::string& key : expired_ids)
+      update->RemoveKey(key);
   }
-  return false;
+
+  return found;
 }
 
 GURL PromoService::GetLoadURLForTesting() const {

chrome/browser/search/promos/promo_service.h

@@ -85,8 +85,9 @@ class PromoService : public KeyedService {
 
   void NotifyObservers();
 
-  // Whether or not |promo_id| has been blocked by the user.
-  bool IsBlocked(const std::string& promo_id) const;
+  // Clears any expired blocklist entries and determines whether |promo_id| has
+  // been blocked by the user.
+  bool IsBlockedAfterClearingExpired(const std::string& promo_id) const;
 
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
   std::unique_ptr<network::SimpleURLLoader> simple_loader_;

chrome/browser/search/promos/promo_service_unittest.cc

@@ -61,7 +61,7 @@ class PromoServiceTest : public testing::Test {
   }
 
   PromoService* service() { return service_.get(); }
-  PrefService* pref_service() { return &pref_service_; }
+  PrefService* prefs() { return &pref_service_; }
 
  private:
   // Required to run tests from UI and threads.
@@ -208,8 +208,9 @@ TEST_F(PromoServiceTest, GoodPromoWithBlockedID) {
   feature_list.InitAndEnableFeature(features::kDismissNtpPromos);
 
   {
-    ListPrefUpdate update(pref_service(), prefs::kNtpPromoBlocklist);
-    update->Append("42");
+    DictionaryPrefUpdate update(prefs(), prefs::kNtpPromoBlocklist);
+    base::Time recent = base::Time::Now() - base::TimeDelta::FromHours(2);
+    update->SetDoubleKey("42", recent.ToDeltaSinceWindowsEpoch().InSecondsF());
   }
 
   std::string response_string =
@@ -248,14 +249,73 @@ TEST_F(PromoServiceTest, BlocklistPromo) {
   EXPECT_EQ(service()->promo_data(), promo);
   EXPECT_EQ(service()->promo_status(), PromoService::Status::OK_WITH_PROMO);
 
-  ASSERT_EQ(0u, pref_service()->GetList(prefs::kNtpPromoBlocklist)->GetSize());
+  ASSERT_EQ(0u, prefs()->GetDictionary(prefs::kNtpPromoBlocklist)->size());
 
   service()->BlocklistPromo("42");
 
   EXPECT_EQ(service()->promo_data(), PromoData());
   EXPECT_EQ(service()->promo_status(), PromoService::Status::OK_BUT_BLOCKED);
 
-  const auto* blocklist = pref_service()->GetList(prefs::kNtpPromoBlocklist);
-  ASSERT_EQ(1u, blocklist->GetSize());
-  EXPECT_EQ("42", blocklist->GetList()[0].GetString());
+  const auto* blocklist = prefs()->GetDictionary(prefs::kNtpPromoBlocklist);
+  ASSERT_EQ(1u, blocklist->size());
+  ASSERT_TRUE(blocklist->HasKey("42"));
+}
+
+TEST_F(PromoServiceTest, BlocklistExpiration) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kDismissNtpPromos);
+
+  {
+    DictionaryPrefUpdate update(prefs(), prefs::kNtpPromoBlocklist);
+    ASSERT_EQ(0u, update->size());
+    base::Time past = base::Time::Now() - base::TimeDelta::FromDays(365);
+    update->SetDoubleKey("42", past.ToDeltaSinceWindowsEpoch().InSecondsF());
+  }
+
+  ASSERT_EQ(1u, prefs()->GetDictionary(prefs::kNtpPromoBlocklist)->size());
+
+  std::string response_string =
+      "{\"update\":{\"promos\":{\"middle\":\"<style></style><div><script></"
+      "script></div>\", \"log_url\":\"/log_url?id=42\", \"id\": \"42\"}}}";
+  SetUpResponseWithData(service()->GetLoadURLForTesting(), response_string);
+
+  service()->Refresh();
+  base::RunLoop().RunUntilIdle();
+
+  // The year-old entry of {promo_id: "42", time: <1y ago>} should be gone.
+  ASSERT_EQ(0u, prefs()->GetDictionary(prefs::kNtpPromoBlocklist)->size());
+
+  // The promo should've still been shown, as expiration should take precedence.
+  PromoData promo;
+  promo.promo_html = "<style></style><div><script></script></div>";
+  promo.promo_log_url = GURL("https://www.google.com/log_url?id=42");
+  promo.promo_id = "42";
+
+  EXPECT_EQ(service()->promo_data(), promo);
+  EXPECT_EQ(service()->promo_status(), PromoService::Status::OK_WITH_PROMO);
+}
+
+TEST_F(PromoServiceTest, BlocklistWrongExpiryType) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kDismissNtpPromos);
+
+  {
+    DictionaryPrefUpdate update(prefs(), prefs::kNtpPromoBlocklist);
+    ASSERT_EQ(0u, update->size());
+    update->SetDoubleKey("42", 5);
+    update->SetStringKey("84", "wrong type");
+  }
+
+  ASSERT_GT(prefs()->GetDictionary(prefs::kNtpPromoBlocklist)->size(), 0u);
+
+  std::string response_string =
+      "{\"update\":{\"promos\":{\"middle\":\"<style></style><div><script></"
+      "script></div>\", \"log_url\":\"/log_url?id=42\", \"id\": \"42\"}}}";
+  SetUpResponseWithData(service()->GetLoadURLForTesting(), response_string);
+
+  service()->Refresh();
+  base::RunLoop().RunUntilIdle();
+
+  // All the invalid formats should've been removed from the pref.
+  ASSERT_EQ(0u, prefs()->GetDictionary(prefs::kNtpPromoBlocklist)->size());
 }