Log site engagement scores for permission actions

chrome/browser/permissions/permission_uma_util.cc

Index: chrome/browser/permissions/permission_uma_util.cc
diff --git a/chrome/browser/permissions/permission_uma_util.cc b/chrome/browser/permissions/permission_uma_util.cc
index 45305b15c1a75ddd61e955a735d4e95ff0861ab9..5744f47eb2561f57fccd405b8d406f298d51b372 100644
--- a/chrome/browser/permissions/permission_uma_util.cc
+++ b/chrome/browser/permissions/permission_uma_util.cc
@@ -11,6 +11,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/browser/permissions/permission_decision_auto_blocker.h"
 #include "chrome/browser/permissions/permission_request.h"
 #include "chrome/browser/permissions/permission_util.h"
@@ -25,6 +26,7 @@
 #include "components/rappor/public/rappor_utils.h"
 #include "components/rappor/rappor_service_impl.h"
 #include "content/public/browser/permission_type.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/common/origin_util.h"
 #include "url/gurl.h"
 
@@ -62,6 +64,68 @@ namespace {
 
 static bool gIsFakeOfficialBuildForTest = false;
 
+std::string GetPermissionRequestString(PermissionRequestType type) {
+  switch (type) {
+    case PermissionRequestType::MULTIPLE:
+      return "AudioAndVideoCapture";
+    case PermissionRequestType::QUOTA:
+      return "Quota";
+    case PermissionRequestType::DOWNLOAD:
+      return "MultipleDownload";
+    case PermissionRequestType::REGISTER_PROTOCOL_HANDLER:
+      return "RegisterProtocolHandler";
+    case PermissionRequestType::PERMISSION_GEOLOCATION:
+      return "Geolocation";
+    case PermissionRequestType::PERMISSION_MIDI_SYSEX:
+      return "MidiSysEx";
+    case PermissionRequestType::PERMISSION_NOTIFICATIONS:
+      return "Notifications";
+    case PermissionRequestType::PERMISSION_PROTECTED_MEDIA_IDENTIFIER:
+      return "ProtectedMediaIdentifier";
+    case PermissionRequestType::PERMISSION_PUSH_MESSAGING:
+      return "PushMessaging";
+    case PermissionRequestType::PERMISSION_FLASH:
+      return "Flash";
+    case PermissionRequestType::PERMISSION_MEDIASTREAM_MIC:
+      return "AudioCapture";
+    case PermissionRequestType::PERMISSION_MEDIASTREAM_CAMERA:
+      return "VideoCapture";
+    default:
+      NOTREACHED();
+      return "";
+  }
+}
+
+void RecordEngagementMetric(const std::vector<PermissionRequest*>& requests,
+                            const content::WebContents* web_contents,
+                            const std::string& action) {
+  PermissionRequestType type = requests[0]->GetPermissionRequestType();
+  if (requests.size() > 1)
+    type = PermissionRequestType::MULTIPLE;
+
+  // This is only hit if kUsePermissionManagerForMediaRequests is off, since it
+  // is now on by default we'll just silenty drop this.
+  if (type == PermissionRequestType::MEDIA_STREAM)
+    return;
+
+  DCHECK(action == "Accepted" || action == "Denied" || action == "Dismissed" ||
+         action == "Ignored");

[raymes, 2017/07/06 05:03:15]

Are we interested in recording the total requests? Or should we just compute
this from the sum of all these?

[Timothy Loh, 2017/07/19 06:03:43]

I don't think we need to record the total requests.

+  std::string name = "Permissions.Engagement." + action + '.' +
+                     GetPermissionRequestString(type);

[raymes, 2017/07/06 05:03:15]

Hmm, I think what we're really interested in is what the engagement is for each
permission type? I wonder if we should just use the content settings type here?
That would also save needing to have the function to convert the request type to
a string. It would also make it easier to interpret the meaning of multiple
prompts since they would be broken down by permission.

[Timothy Loh, 2017/07/19 06:03:43]

At least as far as we only group mic+camera right now, it seems potentially more
useful to have this separate? All the other Permissions.Prompt UMA logs based on
PermissionRequestType too, they just don't have separate histograms for separate
request types.

[raymes, 2017/07/20 03:37:13]

Ok - if you think so I'm ok. It just seemed simpler to use the
ContentSettingsType here to me since we already measure a lot of stuff based on
that. 

I'm still not fully convinced that recording multiple is more useful than
individual permissions though, it seems potentially a bit harder to answer the
question "what is the engagement for requests for microphone"? 

+
+  SiteEngagementService* site_engagement_service = SiteEngagementService::Get(
+      Profile::FromBrowserContext(web_contents->GetBrowserContext()));
+  double engagement_score =
+      site_engagement_service->GetScore(requests[0]->GetOrigin());
+
+  // This is manually expanded from the UMA_HISTOGRAM_PERCENTAGE macro as the
+  // macro does not work with a dynamically calculated histogram name.
+

[raymes, 2017/07/06 05:03:15]

nit: no newline needed

[Timothy Loh, 2017/07/19 06:03:43]

Done.

+  base::LinearHistogram::FactoryGet(
+      name, 1, 101, 102, base::HistogramBase::kUmaTargetedHistogramFlag)
+      ->Add(engagement_score);
+}
+
 const std::string GetRapporMetric(ContentSettingsType permission,
                                   PermissionAction action) {
   std::string action_str;
@@ -390,13 +454,29 @@ void PermissionUmaUtil::PermissionPromptShown(
 }
 
 void PermissionUmaUtil::PermissionPromptAccepted(
-    const std::vector<PermissionRequest*>& requests) {
+    const std::vector<PermissionRequest*>& requests,
+    const content::WebContents* web_contents) {
   RecordPromptDecided(requests, /*accepted=*/true);
+  RecordEngagementMetric(requests, web_contents, "Accepted");
 }
 
 void PermissionUmaUtil::PermissionPromptDenied(
-    const std::vector<PermissionRequest*>& requests) {
+    const std::vector<PermissionRequest*>& requests,
+    const content::WebContents* web_contents) {
   RecordPromptDecided(requests, /*accepted=*/false);
+  RecordEngagementMetric(requests, web_contents, "Denied");
+}
+
+void PermissionUmaUtil::PermissionPromptDismissed(
+    const std::vector<PermissionRequest*>& requests,
+    const content::WebContents* web_contents) {
+  RecordEngagementMetric(requests, web_contents, "Dismissed");
+}
+
+void PermissionUmaUtil::PermissionPromptIgnored(
+    const std::vector<PermissionRequest*>& requests,
+    const content::WebContents* web_contents) {
+  RecordEngagementMetric(requests, web_contents, "Ignored");
 }
 
 void PermissionUmaUtil::RecordPermissionPromptShown(