Collecting statistics on iframe permissions use.

chrome/browser/content_settings/permission_context_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 "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/permission_context_uma_util.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/rappor/rappor_utils.h"
 #include "content/public/browser/permission_type.h"
 #include "content/public/common/origin_util.h"
 #include "url/gurl.h"
 
 // UMA keys need to be statically initialized so plain function would not
 // work. Use a Macro instead.
 #define PERMISSION_ACTION_UMA(secure_origin, permission, permission_secure, \
                               permission_insecure, action)                  \
   UMA_HISTOGRAM_ENUMERATION(permission, action, PERMISSION_ACTION_NUM);     \
@@ skipping 113 matching lines @@
         NOTREACHED() << "PERMISSION " << permission << " not accounted for";
     }
 
     const std::string& rappor_metric = GetRapporMetric(permission, action);
     if (!rappor_metric.empty())
       rappor::SampleDomainAndRegistryFromGURL(
           g_browser_process->rappor_service(), rappor_metric,
           requesting_origin);
 }
 
-void RecordPermissionRequest(ContentSettingsType permission,
-                             const GURL& requesting_origin) {
+std::string PermissionTypeToString(PermissionType permission_type) {
+  switch (permission_type) {
+    case PermissionType::MIDI_SYSEX:
+      return "MidiSysex";
+    case PermissionType::PUSH_MESSAGING:
+      return "PushMessaging";
+    case PermissionType::NOTIFICATIONS:
+      return "Notifications";
+    case PermissionType::GEOLOCATION:
+      return "Geolocation";
+    case PermissionType::PROTECTED_MEDIA_IDENTIFIER:
+      return "ProtectedMediaIdentifier";
+    case PermissionType::NUM:

[jww, 2015/06/29 21:26:59]

nit: Can you make this "default" and then drop the return value at the end?

[keenanb, 2015/06/29 22:43:44]

the way it is now, it will fail fast and loud when a new permission type is
added to PermissionType. using "default" would spoil that.

[jww, 2015/06/29 22:53:23]

Fair.

+      NOTREACHED();
+  }
+  return "";

[jww, 2015/06/29 21:26:59]

If you do leave in this empty string return value, change to return
std::string();

[keenanb, 2015/06/29 22:43:44]

Done.

+}
+
+void RecordPermissionRequest(
+    ContentSettingsType permission,
+    const GURL& requesting_origin,
+    const GURL& embedding_origin,
+    HostContentSettingsMap* host_content_settings_map) {
   bool secure_origin = content::IsOriginSecure(requesting_origin);
   PermissionType type;
   switch (permission) {
     case CONTENT_SETTINGS_TYPE_GEOLOCATION:
       type = PermissionType::GEOLOCATION;
       rappor::SampleDomainAndRegistryFromGURL(
           g_browser_process->rappor_service(),
           "ContentSettings.PermissionRequested.Geolocation.Url",
           requesting_origin);
       break;
@@ skipping 27 matching lines @@
     UMA_HISTOGRAM_ENUMERATION(
         "ContentSettings.PermissionRequested_SecureOrigin",
         static_cast<base::HistogramBase::Sample>(type),
         static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
   } else {
     UMA_HISTOGRAM_ENUMERATION(
         "ContentSettings.PermissionRequested_InsecureOrigin",
         static_cast<base::HistogramBase::Sample>(type),
         static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
   }
+
+  // TODO(keenanb): It may not be worth worrying about,

[jww, 2015/06/29 21:26:59]

Change this to "We have decided not to worry about it for now, but..." (or
something equivalent. The point is, we've considered it so there's no "may" for
now :-)

[keenanb, 2015/06/29 22:43:44]

good point! done.

+  // but statistics would get skewed if, for example,
+  // before the user has made a permission decision,
+  // getCurrentPosition gets called ten times each second
+  // and generates ten permission requests each second.
+  // That may be a job for RAPPOR.
+

[jww, 2015/06/29 21:26:59]

nit: extraneous newline.

[keenanb, 2015/06/29 22:43:44]

Done.

+  DCHECK(requesting_origin.GetOrigin() == requesting_origin);
+  DCHECK(embedding_origin.GetOrigin() == embedding_origin);

[jww, 2015/06/29 21:26:59]

Why are these DCHECKs necessary? In particular, nothing in the comments to this
function indicate that requesting_origin or embedding_origin *must* be origin
only, so a future caller might use this function and be very surprised by this
behavior.

Thus, you need to either:
(a) Document this behavior in the header file comments for this function.
(b) Remove this DCHECK and (as mentioned below), call GetOrigin() when the
origins are being used.

Unless there's a very good reason not to, I strongly prefer (b) as (a) is not a
common paradigm in Chrome.

[keenanb, 2015/06/29 22:43:45]

(b) it is.

+  bool cross_origin_iframe = (requesting_origin != embedding_origin);

[jww, 2015/06/29 21:26:59]

If you're able to get rid of the above DCHECKs, change this comparison to
(request_origin.GetOrigin() != embedding_origin.GetOrigin()).

[keenanb, 2015/06/29 22:43:44]

Done.

+
+  if (cross_origin_iframe) {
+    ContentSetting embedding_content_setting =
+        host_content_settings_map->GetContentSetting(
+            embedding_origin, embedding_origin, permission, std::string());
+
+    UMA_HISTOGRAM_ENUMERATION("ContentSettings.PermissionRequested.OffOrigin_" +
+                                  PermissionTypeToString(type),

[jww, 2015/06/29 21:26:59]

Formatting looks a little funky to me here (but I could be mistaken); make sure
to run git cl format.

[keenanb, 2015/06/29 22:43:44]

i believe i did. i'll make sure to again.

+                              embedding_content_setting,
+                              CONTENT_SETTING_NUM_SETTINGS);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION(
+        "ContentSettings.PermissionRequested.SameOrigin",
+        static_cast<base::HistogramBase::Sample>(type),
+        static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
+  }
 }
 
 } // namespace
 
 // Make sure you update histograms.xml permission histogram_suffix if you
 // add new permission
 void PermissionContextUmaUtil::PermissionRequested(
-    ContentSettingsType permission, const GURL& requesting_origin) {
-  RecordPermissionRequest(permission, requesting_origin);
+    ContentSettingsType permission,
+    const GURL& requesting_origin,
+    const GURL& embedding_origin,
+    HostContentSettingsMap* host_content_settings_map) {
+  RecordPermissionRequest(permission, requesting_origin, embedding_origin,
+                          host_content_settings_map);
 }
 
 void PermissionContextUmaUtil::PermissionGranted(
     ContentSettingsType permission, const GURL& requesting_origin) {
   RecordPermissionAction(permission, GRANTED, requesting_origin);
 }
 
 void PermissionContextUmaUtil::PermissionDenied(
     ContentSettingsType permission, const GURL& requesting_origin) {
   RecordPermissionAction(permission, DENIED, requesting_origin);
 }
 
 void PermissionContextUmaUtil::PermissionDismissed(
     ContentSettingsType permission, const GURL& requesting_origin) {
   RecordPermissionAction(permission, DISMISSED, requesting_origin);
 }
 
 void PermissionContextUmaUtil::PermissionIgnored(
     ContentSettingsType permission, const GURL& requesting_origin) {
   RecordPermissionAction(permission, IGNORED, requesting_origin);
 }