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..1c813addf92cbabcf7ed59f18832a6587daf8136 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:

[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) {
@@ -188,6 +210,33 @@ void RecordPermissionRequest(ContentSettingsType permission,
         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
@@ -195,8 +244,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(