Collecting statistics on iframe permissions use.

chrome/browser/content_settings/permission_context_uma_util.cc

Index: chrome/browser/content_settings/permission_context_uma_util.cc
diff --git a/chrome/browser/content_settings/permission_context_uma_util.cc b/chrome/browser/content_settings/permission_context_uma_util.cc
index cb2af3914f54879d57b0553bdedfc76ea0cd9d43..a3238e5e1f3daf3c719f9f49c3a0ea70c2e4b9a0 100644
--- a/chrome/browser/content_settings/permission_context_uma_util.cc
+++ b/chrome/browser/content_settings/permission_context_uma_util.cc
@@ -6,6 +6,7 @@
 #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"
@@ -139,8 +140,29 @@ void RecordPermissionAction(ContentSettingsType permission,
           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:
+      NOTREACHED();
+  }
+  return std::string();

[mlamouri (slow - plz ping), 2015/06/30 10:27:20]

nit: could you add a NOTREACHED() here too? Sorry, I should have pointed that
before.

[keenanb, 2015/06/30 21:20:07]

might as well just move NOTREACHED.

+}
+
+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) {
@@ -188,6 +210,27 @@ void RecordPermissionRequest(ContentSettingsType permission,
         static_cast<base::HistogramBase::Sample>(type),
         static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
   }
+
+  // We have dicided not to worry about it,
+  // but statistics will 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.

[mlamouri (slow - plz ping), 2015/06/30 10:27:20]

nit: the wrapping of that comment block seems off. It should wrap at 80
characters.

Also, I think I would leave a comment like this instead:
"""
In order to improve the security models for iframes around permissions,
we need to know the ratio between same origin and cross origin iframes
requesting permissions and the permission state of their embedder when
cross origin.
This ration might be biased if an iframe makes multiple requests that
end up being saved but it was considered as statically insignificant.
"""

[keenanb, 2015/06/30 21:20:08]

Done.

+  if (requesting_origin.GetOrigin() != embedding_origin.GetOrigin()) {
+    ContentSetting embedding_content_setting =
+        host_content_settings_map->GetContentSetting(
+            embedding_origin, embedding_origin, permission, std::string());

[mlamouri (slow - plz ping), 2015/06/30 10:27:20]

Could you get the profile instead of HostContentSettingsMap and call
GetPermissionStatus()? Then, you print the PermissionStatus.

[keenanb, 2015/06/30 21:20:08]

Profile does not expose GetPermissionStatus; it's PermissionContext that does. i
chose not to pass either of those in order to avoid circular dependency.

let me know what you think.

[mlamouri (slow - plz ping), 2015/07/01 11:23:05]

You can get the PermissionManager from a profile
(Profile::GetPermissionManager()).

+    UMA_HISTOGRAM_ENUMERATION("ContentSettings.PermissionRequested.OffOrigin_" +
+                                  PermissionTypeToString(type),
+                              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
@@ -195,8 +238,12 @@ void RecordPermissionRequest(ContentSettingsType permission,
 // 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(