Implement permission embargo suppression metrics.

chrome/browser/permissions/permission_context_base.cc

Index: chrome/browser/permissions/permission_context_base.cc
diff --git a/chrome/browser/permissions/permission_context_base.cc b/chrome/browser/permissions/permission_context_base.cc
index c8b35964949f5f7c3045d5089a37b1488d340d92..011576f20bba26de1a4f9d96af83a98e55f4d1cf 100644
--- a/chrome/browser/permissions/permission_context_base.cc
+++ b/chrome/browser/permissions/permission_context_base.cc
@@ -23,7 +23,6 @@
 #include "chrome/browser/permissions/permission_request_impl.h"
 #include "chrome/browser/permissions/permission_request_manager.h"
 #include "chrome/browser/permissions/permission_uma_util.h"
-#include "chrome/browser/permissions/permission_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/common/chrome_features.h"
@@ -49,12 +48,6 @@ const char PermissionContextBase::kPermissionsKillSwitchFieldStudy[] =
 const char PermissionContextBase::kPermissionsKillSwitchBlockedValue[] =
     "blocked";
 
-PermissionResult::PermissionResult(ContentSetting cs,
-                                   PermissionStatusSource pss)
-    : content_setting(cs), source(pss) {}
-
-PermissionResult::~PermissionResult() {}
-
 PermissionContextBase::PermissionContextBase(
     Profile* profile,
     const ContentSettingsType content_settings_type)
@@ -80,20 +73,6 @@ void PermissionContextBase::RequestPermission(
     const BrowserPermissionCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // First check if this permission has been disabled.
-  if (IsPermissionKillSwitchOn()) {
-    // Log to the developer console.
-    web_contents->GetMainFrame()->AddMessageToConsole(
-        content::CONSOLE_MESSAGE_LEVEL_INFO,
-        base::StringPrintf(
-            "%s permission has been blocked.",
-            PermissionUtil::GetPermissionString(content_settings_type_)
-                .c_str()));
-    // The kill switch is enabled for this permission; Block all requests.
-    callback.Run(CONTENT_SETTING_BLOCK);
-    return;
-  }
-
   GURL requesting_origin = requesting_frame.GetOrigin();
   GURL embedding_origin = web_contents->GetLastCommittedURL().GetOrigin();
 
@@ -118,15 +97,42 @@ void PermissionContextBase::RequestPermission(
 
   if (result.content_setting == CONTENT_SETTING_ALLOW ||
       result.content_setting == CONTENT_SETTING_BLOCK) {
+    if (result.source == PermissionStatusSource::KILL_SWITCH) {
+      // Block the request and log to the developer console.
+      web_contents->GetMainFrame()->AddMessageToConsole(
+          content::CONSOLE_MESSAGE_LEVEL_INFO,
+          base::StringPrintf(
+              "%s permission has been blocked.",
+              PermissionUtil::GetPermissionString(content_settings_type_)
+                  .c_str()));
+      callback.Run(CONTENT_SETTING_BLOCK);
+      return;
+    }
+
+    // If we are under embargo, record the embargo reason for which we have
+    // suppressed the prompt. Explicitly switch to ensure that any new
+    // PermissionStatusSource values are dealt with appropriately.
+    switch (result.source) {
+      case PermissionStatusSource::MULTIPLE_DISMISSALS:
+        PermissionUmaUtil::RecordEmbargoPromptSuppression(
+            PermissionEmbargoStatus::REPEATED_DISMISSALS);
+        break;
+      case PermissionStatusSource::SAFE_BROWSING_BLACKLIST:
+        PermissionUmaUtil::RecordEmbargoPromptSuppression(
+            PermissionEmbargoStatus::PERMISSIONS_BLACKLISTING);
+        break;
+      case PermissionStatusSource::UNSPECIFIED:
+      case PermissionStatusSource::KILL_SWITCH:
+      case PermissionStatusSource::NUM:
+        break;
+    }

[raymes, 2017/02/22 23:13:50]

nit: Could we make the conversion a helper function in permission uma util?

[dominickn, 2017/02/22 23:38:54]

This can only be done by proxying the call to RecordEmbargoPromptSuppression in
the helper method entirely since we only record something for 2 cases. I don't
know if this is actually clearer or not.

[raymes, 2017/02/23 01:28:58]

I agree - it's not ideal. We could alternatively have a function which does the
conversion and returns a bool if it should be recorded, e.g.

bool PermissionStatusSourceToPermissionEmbargoStatus(PermissionStatusSource
source, PermissionEmbargoStatus* result);

I don't feel too strongly though.

     NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
                         false /* persist */, result.content_setting);
     return;
   }
 
   // Asynchronously check whether the origin should be blocked from making this
-  // permission request. It may be on the Safe Browsing API blacklist, or it may
-  // have been dismissed too many times in a row. If the origin is allowed to
-  // request, that request will be made to ContinueRequestPermission().
+  // permission request, e.g. it may be on the Safe Browsing API blacklist.
   PermissionDecisionAutoBlocker::GetForProfile(profile_)->UpdateEmbargoedStatus(
       content_settings_type_, requesting_origin, web_contents,
       base::Bind(&PermissionContextBase::ContinueRequestPermission,
@@ -150,15 +156,21 @@ void PermissionContextBase::ContinueRequestPermission(
             "%s permission has been auto-blocked.",
             PermissionUtil::GetPermissionString(content_settings_type_)
                 .c_str()));
-    // Permission has been automatically blocked.
-    PermissionUmaUtil::RecordPermissionEmbargoStatus(
+    // Permission has been automatically blocked. Record that the prompt was
+    // suppressed and that we hit the blacklist.
+    PermissionUmaUtil::RecordEmbargoPromptSuppression(
+        PermissionEmbargoStatus::PERMISSIONS_BLACKLISTING);
+    PermissionUmaUtil::RecordEmbargoStatus(
         PermissionEmbargoStatus::PERMISSIONS_BLACKLISTING);
     callback.Run(CONTENT_SETTING_BLOCK);
     return;
   }
 
+  // We are going to show a prompt now.
   PermissionUmaUtil::PermissionRequested(
       content_settings_type_, requesting_origin, embedding_origin, profile_);
+  PermissionUmaUtil::RecordEmbargoPromptSuppression(
+      PermissionEmbargoStatus::NOT_EMBARGOED);
 
   DecidePermission(web_contents, id, requesting_origin, embedding_origin,
                    user_gesture, callback);
@@ -167,13 +179,10 @@ void PermissionContextBase::ContinueRequestPermission(
 PermissionResult PermissionContextBase::GetPermissionStatus(
     const GURL& requesting_origin,
     const GURL& embedding_origin) const {
-  // TODO(raymes): Ensure we return appropriate decision reasons in the
-  // PermissionResult. We should add these as each is needed.
-
   // If the permission has been disabled through Finch, block all requests.
   if (IsPermissionKillSwitchOn()) {
     return PermissionResult(CONTENT_SETTING_BLOCK,
-                            PermissionStatusSource::UNSPECIFIED);
+                            PermissionStatusSource::KILL_SWITCH);
   }
 
   if (IsRestrictedToSecureOrigins() &&
@@ -184,19 +193,16 @@ PermissionResult PermissionContextBase::GetPermissionStatus(
 
   ContentSetting content_setting =
       GetPermissionStatusInternal(requesting_origin, embedding_origin);
-  if (content_setting == CONTENT_SETTING_ASK &&
-      PermissionDecisionAutoBlocker::GetForProfile(profile_)->IsUnderEmbargo(
-          content_settings_type_, requesting_origin)) {
-    return PermissionResult(CONTENT_SETTING_BLOCK,
-                            PermissionStatusSource::UNSPECIFIED);
+  if (content_setting == CONTENT_SETTING_ASK) {
+    return PermissionDecisionAutoBlocker::GetForProfile(profile_)
+        ->GetEmbargoResult(content_settings_type_, requesting_origin);

[raymes, 2017/02/22 23:13:50]

Could we DCHECK that the content setting of the return value here is ASK or
BLOCK? I think this code is making an assumption that it will never be ALLOW.

[dominickn, 2017/02/22 23:38:54]

Done.

   }
 
   return PermissionResult(content_setting, PermissionStatusSource::UNSPECIFIED);
 }
 
-void PermissionContextBase::ResetPermission(
-    const GURL& requesting_origin,
-    const GURL& embedding_origin) {
+void PermissionContextBase::ResetPermission(const GURL& requesting_origin,
+                                            const GURL& embedding_origin) {
   HostContentSettingsMapFactory::GetForProfile(profile_)
       ->SetContentSettingDefaultScope(requesting_origin, embedding_origin,
                                       content_settings_storage_type(),
@@ -324,7 +330,7 @@ void PermissionContextBase::PermissionDecided(
         embargo_status = PermissionEmbargoStatus::REPEATED_DISMISSALS;
       }
     }
-    PermissionUmaUtil::RecordPermissionEmbargoStatus(embargo_status);
+    PermissionUmaUtil::RecordEmbargoStatus(embargo_status);
   }
 
   NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,