Add UMA for autoblocking and embargoing.

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 97834772a53a020cf6ae1aa3ad7c76d6e52b7bb1..db566a204832f2cf2a3c090b53978fdd6583eb1e 100644
--- a/chrome/browser/permissions/permission_decision_auto_blocker.cc
+++ b/chrome/browser/permissions/permission_decision_auto_blocker.cc
@@ -113,6 +113,18 @@ int GetActionCount(const GURL& url,
   return current_count;
 }
 
+bool PreviouslyEmbargoed(Profile* profile,

[dominickn, 2017/01/27 06:05:47]

Call this WasPreviouslyEmbargoed. Add a comment saying,

"Returns true if |url| has been embargoed from requesting |permission| for the
reason corresponding to |key|."

[meredithl, 2017/01/29 23:48:29]

Done.

+                         const GURL& url,
+                         content::PermissionType permission,
+                         const char* key) {
+  HostContentSettingsMap* map =
+      HostContentSettingsMapFactory::GetForProfile(profile);
+  std::unique_ptr<base::DictionaryValue> dict = GetOriginDict(map, url);
+  base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
+      dict.get(), PermissionUtil::GetPermissionString(permission));
+  return permission_dict->HasKey(key);
+}
+
 }  // namespace
 
 // PermissionDecisionAutoBlocker::Factory --------------------------------------
@@ -227,9 +239,18 @@ bool PermissionDecisionAutoBlocker::RecordDismissAndEmbargo(
 
   if (base::FeatureList::IsEnabled(features::kBlockPromptsIfDismissedOften) &&
       current_dismissal_count >= g_prompt_dismissals_before_block) {
+    if (PreviouslyEmbargoed(profile_, url, permission,
+                            kPermissionDismissalEmbargoKey)) {
+      PermissionUmaUtil::RecordRepeatedEmbargo(
+          PermissionEmbargoReason::REPEATED_DISMISSALS);
+    }
+
     PlaceUnderEmbargo(permission, url, kPermissionDismissalEmbargoKey);
+    PermissionUmaUtil::RecordPermissionEmbargoReason(
+        PermissionEmbargoReason::REPEATED_DISMISSALS);
     return true;
   }
+

[dominickn, 2017/01/27 06:05:47]

Nit: probably don't need this extra newline

[meredithl, 2017/01/29 23:48:29]

Done.

   return false;
 }
 
@@ -341,12 +362,22 @@ void PermissionDecisionAutoBlocker::CheckSafeBrowsingResult(
     base::Callback<void(bool)> callback,
     bool should_be_embargoed) {
   if (should_be_embargoed) {
-    // Requesting site is blacklisted for this permission, update the content
-    // setting to place it under embargo.
+    if (PreviouslyEmbargoed(profile_, request_origin, permission,
+                            kPermissionBlacklistEmbargoKey)) {
+      PermissionUmaUtil::RecordRepeatedEmbargo(
+          PermissionEmbargoReason::PERMISSIONS_BLACKLISTING);
+    }
+
     PlaceUnderEmbargo(permission, request_origin,
                       kPermissionBlacklistEmbargoKey);
+    PermissionUmaUtil::RecordPermissionEmbargoReason(
+        PermissionEmbargoReason::PERMISSIONS_BLACKLISTING);
+  } else {
+    PermissionUmaUtil::RecordPermissionEmbargoReason(

[dominickn, 2017/01/27 06:05:47]

Will this double count since you're calling RecordPermissionEmbargoReason in
PermissionDecided()?

[meredithl, 2017/01/29 23:48:29]

Hmm, I believe so. Imo that should have been picked up in the tests, is there a
way I can be sure that this doesn't happen? It appears to behave as expected.

[dominickn, 2017/01/30 00:06:20]

You don't actually have any test which checks the NOT_EMBARGOED enum.

You need a test where you check the histograms for a permission that's not
blacklisted, and verify that only one NOT_EMBARGOED is recorded.

[meredithl, 2017/01/30 00:34:33]

I do a check of the histograms in Permission Context Base unit tests for the
NOT_EMBARGOED enum. I can't test it in the autoblocker, all I can do is make
sure that it hasn't recorded PERMISSIONS_BLACKLISTING or REPEATED_DISMISSAL.

[dominickn, 2017/01/30 00:42:36]

Right, but those tests don't have a safe browsing database manager and the
blacklisting feature enabled. This codepath doesn't get exercised unless those
two are present - you've only added the embargo histogram testing to the
non-blacklisting tests. :)

[meredithl, 2017/01/30 02:04:47]

Okay, I've added an extra test in PDAB to check the SafeBrowsingResponse enum,
and one in PermissionContextBase to check the EmbargoReason enum. I used
ExpectUniqueSample, not sure if that's best.

+        PermissionEmbargoReason::NOT_EMBARGOED);
   }
-  callback.Run(should_be_embargoed /* permission blocked */);
+
+  callback.Run(should_be_embargoed);
 }
 
 // static