Remove deprecated and out of date permissions metrics.

chrome/browser/permissions/permission_uma_util.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/permissions/permission_uma_util.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 73 matching lines @@
       action_str = "Revoked";
       break;
     default:
       NOTREACHED();
       break;
   }
 
   std::string permission_str = PermissionUtil::GetPermissionString(permission);
   if (permission_str.empty())
     return "";
-  return base::StringPrintf("ContentSettings.PermissionActions_%s.%s.Url",
+  return base::StringPrintf("ContentSettings.PermissionActions_%s.%s.Url2",
                             permission_str.c_str(), action_str.c_str());
 }
 
 void RecordPermissionRequest(PermissionType permission,
                              const GURL& requesting_origin,
                              const GURL& embedding_origin,
                              Profile* profile) {
   rappor::RapporServiceImpl* rappor_service =
       g_browser_process->rappor_service();
   if (rappor_service) {
     if (permission == PermissionType::GEOLOCATION) {
-      // TODO(dominickn): remove this deprecated metric - crbug.com/605836.
-      rappor::SampleDomainAndRegistryFromGURL(
-          rappor_service, "ContentSettings.PermissionRequested.Geolocation.Url",
-          requesting_origin);
       rappor_service->RecordSampleString(
           "ContentSettings.PermissionRequested.Geolocation.Url2",
           rappor::LOW_FREQUENCY_ETLD_PLUS_ONE_RAPPOR_TYPE,
           rappor::GetDomainAndRegistrySampleFromGURL(requesting_origin));
     } else if (permission == PermissionType::NOTIFICATIONS) {
-      // TODO(dominickn): remove this deprecated metric - crbug.com/605836.
-      rappor::SampleDomainAndRegistryFromGURL(
-          rappor_service,
-          "ContentSettings.PermissionRequested.Notifications.Url",
-          requesting_origin);
       rappor_service->RecordSampleString(
           "ContentSettings.PermissionRequested.Notifications.Url2",
           rappor::LOW_FREQUENCY_ETLD_PLUS_ONE_RAPPOR_TYPE,
           rappor::GetDomainAndRegistrySampleFromGURL(requesting_origin));
     } else if (permission == PermissionType::MIDI ||
                permission == PermissionType::MIDI_SYSEX) {
-      // TODO(dominickn): remove this deprecated metric - crbug.com/605836.
-      rappor::SampleDomainAndRegistryFromGURL(
-          rappor_service, "ContentSettings.PermissionRequested.Midi.Url",
-          requesting_origin);
       rappor_service->RecordSampleString(
           "ContentSettings.PermissionRequested.Midi.Url2",
           rappor::LOW_FREQUENCY_ETLD_PLUS_ONE_RAPPOR_TYPE,
           rappor::GetDomainAndRegistrySampleFromGURL(requesting_origin));
     } else if (permission == PermissionType::PROTECTED_MEDIA_IDENTIFIER) {
       rappor_service->RecordSampleString(
           "ContentSettings.PermissionRequested.ProtectedMedia.Url2",
           rappor::LOW_FREQUENCY_ETLD_PLUS_ONE_RAPPOR_TYPE,
           rappor::GetDomainAndRegistrySampleFromGURL(requesting_origin));
     }
   }
 
   bool secure_origin = content::IsOriginSecure(requesting_origin);
   UMA_HISTOGRAM_ENUMERATION(
       "ContentSettings.PermissionRequested",
       static_cast<base::HistogramBase::Sample>(permission),
       static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
   if (secure_origin) {
     UMA_HISTOGRAM_ENUMERATION(
         "ContentSettings.PermissionRequested_SecureOrigin",
         static_cast<base::HistogramBase::Sample>(permission),
         static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
   } else {
     UMA_HISTOGRAM_ENUMERATION(
         "ContentSettings.PermissionRequested_InsecureOrigin",
         static_cast<base::HistogramBase::Sample>(permission),
         static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
   }

[raymes, 2017/02/23 05:54:57]

Do you still use this one? The comment in histograms.xml is:

Note this is probably not the metric you want - it does not correspond to
    the total number of times websites request a permission. Also, because
    specific permissions have code that can automatically block or grant
    permissions based on things like incognito, installed extensions etc., this
    does also not correspond to the number of times users are prompted to allow
    permissions.

    For a better metric to track how often users are prompted, use
    ContentSettings.PermissionsActions*.

I'm ok to keep it if you still use it though?

[dominickn, 2017/02/23 06:11:43]

I don't use it. Happy to remove. But I'm not sure if anyone else uses it, maybe
we should send a PSA out?

[raymes, 2017/02/23 06:30:55]

Sending out a PSA sounds good. We don't have to block this CL on it.

 
   // In order to gauge the compatibility risk of implementing an improved
   // iframe permissions security model, we would like to know the ratio of
   // same-origin to cross-origin permission requests. Our estimate of this
   // ratio could be somewhat biased by repeated requests coming from a
   // single frame, but we expect this to be insignificant.
   if (requesting_origin.GetOrigin() != embedding_origin.GetOrigin()) {
     content::PermissionManager* manager = profile->GetPermissionManager();
     if (!manager)
       return;
     blink::mojom::PermissionStatus embedding_permission_status =
         manager->GetPermissionStatus(permission, embedding_origin,
                                      embedding_origin);
 
     base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
         "Permissions.Requested.CrossOrigin_" +
             PermissionUtil::GetPermissionString(permission),
         1, static_cast<int>(blink::mojom::PermissionStatus::LAST),
         static_cast<int>(blink::mojom::PermissionStatus::LAST) + 1,
         base::HistogramBase::kUmaTargetedHistogramFlag);
     histogram->Add(static_cast<int>(embedding_permission_status));
   } else {
     UMA_HISTOGRAM_ENUMERATION(
         "Permissions.Requested.SameOrigin",
         static_cast<base::HistogramBase::Sample>(permission),
         static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
   }

[raymes, 2017/02/23 05:54:57]

Lines from here up to 146 can be removed.

[dominickn, 2017/02/23 06:11:43]

Done.

 }
 
 }  // anonymous namespace
 
 // PermissionReportInfo -------------------------------------------------------
 PermissionReportInfo::PermissionReportInfo(
     const GURL& origin,
     ContentSettingsType permission,
     PermissionAction action,
     PermissionSourceUI source_ui,
@@ skipping 498 matching lines @@
         autoblocker->GetDismissCount(requesting_origin, permission),
         autoblocker->GetIgnoreCount(requesting_origin, permission));
     g_browser_process->safe_browsing_service()
         ->ui_manager()->ReportPermissionAction(report_info);
   }
 
   bool secure_origin = content::IsOriginSecure(requesting_origin);
 
   switch (permission) {
     case CONTENT_SETTINGS_TYPE_GEOLOCATION:
-      PERMISSION_ACTION_UMA(secure_origin, "Permissions.Action.Geolocation",
-                            "Permissions.Action.SecureOrigin.Geolocation",
-                            "Permissions.Action.InsecureOrigin.Geolocation",
-                            action);
+      // Geolocation is disabled on insecure origins, so there's no need to
+      // record metrics for secure/insecue.
+      UMA_HISTOGRAM_ENUMERATION("Permissions.Action.Geolocation",
+                                static_cast<int>(action),
+                                static_cast<int>(PermissionAction::NUM));
       break;
     case CONTENT_SETTINGS_TYPE_NOTIFICATIONS:
       PERMISSION_ACTION_UMA(secure_origin, "Permissions.Action.Notifications",
                             "Permissions.Action.SecureOrigin.Notifications",
                             "Permissions.Action.InsecureOrigin.Notifications",
                             action);
       break;
     case CONTENT_SETTINGS_TYPE_MIDI_SYSEX:
       PERMISSION_ACTION_UMA(secure_origin, "Permissions.Action.MidiSysEx",
                             "Permissions.Action.SecureOrigin.MidiSysEx",
                             "Permissions.Action.InsecureOrigin.MidiSysEx",
                             action);
       break;
     case CONTENT_SETTINGS_TYPE_PUSH_MESSAGING:
-      PERMISSION_ACTION_UMA(secure_origin, "Permissions.Action.PushMessaging",
-                            "Permissions.Action.SecureOrigin.PushMessaging",
-                            "Permissions.Action.InsecureOrigin.PushMessaging",
-                            action);
+      // Push messaging is disabled on insecure origins, so there's no need to
+      // record metrics for secure/insecue.
+      UMA_HISTOGRAM_ENUMERATION("Permissions.Action.PushMessaging",
+                                static_cast<int>(action),
+                                static_cast<int>(PermissionAction::NUM));

[raymes, 2017/02/23 05:54:57]

Is there actually a benefit to changing these ones, even though we shouldn't be
getting insecure reports? 

[dominickn, 2017/02/23 06:11:42]

It's mainly following the lead of the mediastream permissions, which do this as
well (see below). Since we'll never unrestrict these permissions from insecure
origins, I don't see why we would care about the split. :)

[raymes, 2017/02/23 06:30:55]

Ok, sounds good to me

       break;
     case CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER:
       PERMISSION_ACTION_UMA(secure_origin, "Permissions.Action.ProtectedMedia",
                             "Permissions.Action.SecureOrigin.ProtectedMedia",
                             "Permissions.Action.InsecureOrigin.ProtectedMedia",
                             action);
       break;
     case CONTENT_SETTINGS_TYPE_DURABLE_STORAGE:
       PERMISSION_ACTION_UMA(secure_origin, "Permissions.Action.DurableStorage",
                             "Permissions.Action.SecureOrigin.DurableStorage",
@@ skipping 18 matching lines @@
                             "Permissions.Action.InsecureOrigin.Flash", action);
       break;
     // The user is not prompted for these permissions, thus there is no
     // permission action recorded for them.
     default:
       NOTREACHED() << "PERMISSION "
                    << PermissionUtil::GetPermissionString(permission)
                    << " not accounted for";
   }
 
-  // Retrieve the name of the RAPPOR metric. Currently, the new metric name is
-  // the deprecated name with "2" on the end, e.g.
-  // ContentSettings.PermissionActions_Geolocation.Granted.Url2. For simplicity,
-  // we retrieve the deprecated name and append the "2" for the new name.
-  // TODO(dominickn): remove the deprecated metric and replace it solely with
-  // the new one in GetRapporMetric - crbug.com/605836.
-  const std::string deprecated_metric = GetRapporMetric(permission, action);
+  const std::string rappor_metric = GetRapporMetric(permission, action);
   rappor::RapporServiceImpl* rappor_service =
       g_browser_process->rappor_service();
-  if (!deprecated_metric.empty() && rappor_service) {
-    rappor::SampleDomainAndRegistryFromGURL(rappor_service, deprecated_metric,
-                                            requesting_origin);
-
-    std::string rappor_metric = deprecated_metric + "2";
+  if (!rappor_metric.empty() && rappor_service) {
     rappor_service->RecordSampleString(
         rappor_metric, rappor::LOW_FREQUENCY_ETLD_PLUS_ONE_RAPPOR_TYPE,
         rappor::GetDomainAndRegistrySampleFromGURL(requesting_origin));
   }
 }