Implement permissions embargo for prompts which are repeatedly ignored.

chrome/browser/permissions/permission_decision_auto_blocker.cc

Index: chrome/browser/permissions/permission_decision_auto_blocker.cc
diff --git a/chrome/browser/permissions/permission_decision_auto_blocker.cc b/chrome/browser/permissions/permission_decision_auto_blocker.cc
index 4dcc5fa94e37834bcfe3a68b5c09ad93c9e9c56f..5490ddfda197491ac4a527e835a523916871a2c7 100644
--- a/chrome/browser/permissions/permission_decision_auto_blocker.cc
+++ b/chrome/browser/permissions/permission_decision_auto_blocker.cc
@@ -31,14 +31,22 @@ namespace {
 // from an origin before it is automatically blocked.
 int g_prompt_dismissals_before_block = 3;
 
-// The number of days that an origin will stay under embargo for a requested
-// permission due to blacklisting.
-int g_blacklist_embargo_days = 7;
+// The number of times that users may ignore a permission prompt from an origin
+// before it is automatically blocked.
+int g_prompt_ignores_before_block = 4;
 
 // The number of days that an origin will stay under embargo for a requested
 // permission due to repeated dismissals.
 int g_dismissal_embargo_days = 7;
 
+// The number of days that an origin will stay under embargo for a requested
+// permission due to repeated ignores.
+int g_ignore_embargo_days = 7;
+
+// The number of days that an origin will stay under embargo for a requested
+// permission due to blacklisting.
+int g_blacklist_embargo_days = 7;
+
 // Maximum time in milliseconds to wait for safe browsing service to check a
 // url for blacklisting. After this amount of time, the check will be aborted
 // and the url will be treated as not safe.
@@ -109,6 +117,22 @@ int GetActionCount(const GURL& url,
   return current_count;
 }
 
+bool IsUnderEmbargo(base::DictionaryValue* permission_dict,
+                    const base::Feature& feature,
+                    const char* key,
+                    base::Time current_time,
+                    base::TimeDelta offset) {
+  double embargo_date = -1;
+
+  if (base::FeatureList::IsEnabled(feature) &&
+      permission_dict->GetDouble(key, &embargo_date)) {
+    if (current_time < base::Time::FromInternalValue(embargo_date) + offset)
+      return true;
+  }
+
+  return false;
+}
+
 }  // namespace
 
 // PermissionDecisionAutoBlocker::Factory --------------------------------------
@@ -156,14 +180,18 @@ const char PermissionDecisionAutoBlocker::kPromptIgnoreCountKey[] =
     "ignore_count";
 
 // static
-const char PermissionDecisionAutoBlocker::kPermissionBlacklistEmbargoKey[] =
-    "blacklisting_embargo_days";
-
-// static
 const char PermissionDecisionAutoBlocker::kPermissionDismissalEmbargoKey[] =
     "dismissal_embargo_days";
 
 // static
+const char PermissionDecisionAutoBlocker::kPermissionIgnoreEmbargoKey[] =
+    "ignore_embargo_days";
+
+// static
+const char PermissionDecisionAutoBlocker::kPermissionBlacklistEmbargoKey[] =
+    "blacklisting_embargo_days";
+
+// static
 PermissionDecisionAutoBlocker* PermissionDecisionAutoBlocker::GetForProfile(
     Profile* profile) {
   return PermissionDecisionAutoBlocker::Factory::GetForProfile(profile);
@@ -172,30 +200,46 @@ PermissionDecisionAutoBlocker* PermissionDecisionAutoBlocker::GetForProfile(
 // static
 void PermissionDecisionAutoBlocker::UpdateFromVariations() {
   int prompt_dismissals = -1;
-  int blacklist_embargo_days = -1;
+  int prompt_ignores = -1;
   int dismissal_embargo_days = -1;
+  int ignore_embargo_days = -1;
+  int blacklist_embargo_days = -1;
+
   std::string dismissals_value = variations::GetVariationParamValueByFeature(
       features::kBlockPromptsIfDismissedOften, kPromptDismissCountKey);
-  std::string blacklist_embargo_value =
-      variations::GetVariationParamValueByFeature(
-          features::kPermissionsBlacklist, kPermissionBlacklistEmbargoKey);
+  std::string ignores_value = variations::GetVariationParamValueByFeature(
+      features::kBlockPromptsIfIgnoredOften, kPromptIgnoreCountKey);
   std::string dismissal_embargo_value =
       variations::GetVariationParamValueByFeature(
           features::kBlockPromptsIfDismissedOften,
           kPermissionDismissalEmbargoKey);
+  std::string ignore_embargo_value =
+      variations::GetVariationParamValueByFeature(
+          features::kBlockPromptsIfIgnoredOften, kPermissionIgnoreEmbargoKey);
+  std::string blacklist_embargo_value =
+      variations::GetVariationParamValueByFeature(
+          features::kPermissionsBlacklist, kPermissionBlacklistEmbargoKey);
+
   // If converting the value fails, stick with the current value.
   if (base::StringToInt(dismissals_value, &prompt_dismissals) &&
       prompt_dismissals > 0) {
     g_prompt_dismissals_before_block = prompt_dismissals;
   }
-  if (base::StringToInt(blacklist_embargo_value, &blacklist_embargo_days) &&
-      blacklist_embargo_days > 0) {
-    g_blacklist_embargo_days = blacklist_embargo_days;
+  if (base::StringToInt(ignores_value, &prompt_ignores) && prompt_ignores > 0) {
+    g_prompt_ignores_before_block = prompt_ignores;
   }
   if (base::StringToInt(dismissal_embargo_value, &dismissal_embargo_days) &&
       dismissal_embargo_days > 0) {
     g_dismissal_embargo_days = dismissal_embargo_days;
   }
+  if (base::StringToInt(ignore_embargo_value, &ignore_embargo_days) &&

[benwells, 2017/03/31 04:47:14]

These variables are all confusingly named. For g_foo, you should use local foo
and foo_val or foo_str or foo_value.

So, g_ignore_embargo_days, ignore_embargo_days, and ignore_embargo_days_value.

You can probs remove the prompt_ from go_prompt_ignores_before_block as well.

[dominickn, 2017/04/02 23:45:57]

Done.

+      ignore_embargo_days > 0) {
+    g_ignore_embargo_days = ignore_embargo_days;
+  }
+  if (base::StringToInt(blacklist_embargo_value, &blacklist_embargo_days) &&
+      blacklist_embargo_days > 0) {
+    g_blacklist_embargo_days = blacklist_embargo_days;
+  }
 }
 
 void PermissionDecisionAutoBlocker::CheckSafeBrowsingBlacklist(
@@ -232,29 +276,27 @@ PermissionResult PermissionDecisionAutoBlocker::GetEmbargoResult(
       GetOriginDict(map, request_origin);
   base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
       dict.get(), PermissionUtil::GetPermissionString(permission));
-  double embargo_date = -1;
 
   base::Time current_time = clock_->Now();
-  if (base::FeatureList::IsEnabled(features::kPermissionsBlacklist) &&
-      permission_dict->GetDouble(kPermissionBlacklistEmbargoKey,
-                                 &embargo_date)) {
-    if (current_time <
-        base::Time::FromInternalValue(embargo_date) +
-            base::TimeDelta::FromDays(g_blacklist_embargo_days)) {
-      return PermissionResult(CONTENT_SETTING_BLOCK,
-                              PermissionStatusSource::SAFE_BROWSING_BLACKLIST);
-    }
+  if (IsUnderEmbargo(permission_dict, features::kPermissionsBlacklist,

[benwells, 2017/03/31 04:47:14]

Nice factoring :)

[dominickn, 2017/04/02 23:45:57]

:)

+                     kPermissionBlacklistEmbargoKey, current_time,
+                     base::TimeDelta::FromDays(g_blacklist_embargo_days))) {
+    return PermissionResult(CONTENT_SETTING_BLOCK,
+                            PermissionStatusSource::SAFE_BROWSING_BLACKLIST);
   }
 
-  if (base::FeatureList::IsEnabled(features::kBlockPromptsIfDismissedOften) &&
-      permission_dict->GetDouble(kPermissionDismissalEmbargoKey,
-                                 &embargo_date)) {
-    if (current_time <
-        base::Time::FromInternalValue(embargo_date) +
-            base::TimeDelta::FromDays(g_dismissal_embargo_days)) {
-      return PermissionResult(CONTENT_SETTING_BLOCK,
-                              PermissionStatusSource::MULTIPLE_DISMISSALS);
-    }
+  if (IsUnderEmbargo(permission_dict, features::kBlockPromptsIfDismissedOften,
+                     kPermissionDismissalEmbargoKey, current_time,
+                     base::TimeDelta::FromDays(g_dismissal_embargo_days))) {
+    return PermissionResult(CONTENT_SETTING_BLOCK,
+                            PermissionStatusSource::MULTIPLE_DISMISSALS);
+  }
+
+  if (IsUnderEmbargo(permission_dict, features::kBlockPromptsIfIgnoredOften,
+                     kPermissionIgnoreEmbargoKey, current_time,
+                     base::TimeDelta::FromDays(g_ignore_embargo_days))) {
+    return PermissionResult(CONTENT_SETTING_BLOCK,
+                            PermissionStatusSource::MULTIPLE_IGNORES);
   }
 
   return PermissionResult(CONTENT_SETTING_ASK,
@@ -287,11 +329,18 @@ bool PermissionDecisionAutoBlocker::RecordDismissAndEmbargo(
   return false;
 }
 
-int PermissionDecisionAutoBlocker::RecordIgnore(
+bool PermissionDecisionAutoBlocker::RecordIgnoreAndEmbargo(
     const GURL& url,
     ContentSettingsType permission) {
-  return RecordActionInWebsiteSettings(url, permission, kPromptIgnoreCountKey,
-                                       profile_);
+  int current_ignore_count = RecordActionInWebsiteSettings(
+      url, permission, kPromptIgnoreCountKey, profile_);
+
+  if (base::FeatureList::IsEnabled(features::kBlockPromptsIfIgnoredOften) &&
+      current_ignore_count >= g_prompt_ignores_before_block) {
+    PlaceUnderEmbargo(url, permission, kPermissionIgnoreEmbargoKey);
+    return true;
+  }
+  return false;
 }
 
 void PermissionDecisionAutoBlocker::RemoveCountsByUrl(