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..daf4375897af893b78ee1b21ac278d1837597759 100644
--- a/chrome/browser/content_settings/permission_context_uma_util.cc
+++ b/chrome/browser/content_settings/permission_context_uma_util.cc
@@ -139,8 +139,52 @@ void RecordPermissionAction(ContentSettingsType permission,
           requesting_origin);
 }
 
+std::string PermissionTypeToString(PermissionType permission_type) {
+  switch (permission_type) {
+    case PermissionType::MIDI_SYSEX:
+      return "MidiSysex";
+    case PermissionType::PUSH_MESSAGING:
+      return "PushMessagin";
+    case PermissionType::NOTIFICATIONS:
+      return "Notifications";
+    case PermissionType::GEOLOCATION:
+      return "Geolocation";
+    case PermissionType::PROTECTED_MEDIA_IDENTIFIER:
+      return "ProtectedMediaIdentifier";
+    default:
+      // TODO(keenanb)

[jww, 2015/06/23 05:32:58]

I don't really think this is necessary at all, so per felt's comment, I would
just get rid of the TODO. Given that this will just be used for UMA collection,
it's not really a big deal if it reaches an unknown content setting or
permission type (thus no need to assert unreached).

[mlamouri (slow - plz ping), 2015/06/23 10:02:32]

You might actually want to have no default case so the compilation will break if
a new PermissionType is added.

[keenanb, 2015/06/24 22:26:11]

Done.

[keenanb, 2015/06/24 22:26:11]

Done.

+      // NOTREACHED() << "PERMISSION TYPE " <<
+      //    permission_type << " not accounted for";
+      return "UnknownPermissionType";
+  }
+}
+
+std::string ContentSettingToString(ContentSetting content_setting) {
+  switch (content_setting) {
+    case CONTENT_SETTING_DEFAULT:
+      return "Default";
+    case CONTENT_SETTING_ALLOW:
+      return "Allow";
+    case CONTENT_SETTING_BLOCK:
+      return "Block";
+    case CONTENT_SETTING_ASK:
+      return "Ask";
+    case CONTENT_SETTING_SESSION_ONLY:
+      return "SessionOnly";
+    case CONTENT_SETTING_DETECT_IMPORTANT_CONTENT:
+      return "DetectImportant";
+    default:
+      // TODO(keenanb)
+      // NOTREACHED() << "CONTENT SETTING "

[jww, 2015/06/23 05:32:58]

See above comment.

[mlamouri (slow - plz ping), 2015/06/23 10:02:32]

ditto. Also, you probably don't need SessionOnly, Default and DetectImportant.
They are not used for permissions AFAICT.

[keenanb, 2015/06/24 22:26:11]

Done.

[keenanb, 2015/06/24 22:26:11]

Done.

+      //    << content_setting << " not accounted for";
+      return "UnknownContentSetting";
+  }
+}
+
 void RecordPermissionRequest(ContentSettingsType permission,
-                             const GURL& requesting_origin) {
+                             HostContentSettingsMap* hostContentSettingsMap,
+                             const GURL& requesting_origin,
+                             const GURL& embedding_origin) {
   bool secure_origin = content::IsOriginSecure(requesting_origin);
   PermissionType type;
   switch (permission) {
@@ -188,6 +232,46 @@ void RecordPermissionRequest(ContentSettingsType permission,
         static_cast<base::HistogramBase::Sample>(type),
         static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
   }
+
+  // TODO(keenanb): skip if another permission request is already pending.
+  // otherwise statistics get skewed when, for example,
+  // getCurrentPosition gets called ten times each second.
+  // (it could even be best to check for pending requests
+  // in permission_context_uma_util which calls this function.)
+  // just make sure to invoke the callback.
+
+  std::string permission_type_string = PermissionTypeToString(type);
+
+  // FIXME(keenanb): find out if using the same value
+  // for both primary and secondary GURLs could be bad or subpar.
+  ContentSetting embedding_content_setting =
+      hostContentSettingsMap->GetContentSettingAndMaybeUpdateLastUsage(

[mlamouri (slow - plz ping), 2015/06/23 10:02:32]

I'm not sure you want to call the "MaybeUpdateLastUsage" variant of
GetContentSetting(). Recording a permission shouldn't mark the permission as
being used.

[keenanb, 2015/06/24 22:26:11]

Done.

+          embedding_origin, embedding_origin, permission, std::string());
+  std::string embedding_content_setting_string =
+      ContentSettingToString(embedding_content_setting);
+
+  // TODO(keenanb): is calling GetOrigin redundant?

[felt, 2015/06/22 22:56:01]

Test out the behavior, do you need to call GetOrigin?

[keenanb, 2015/06/24 22:26:11]

i had tested it. it looks to be always a pure origin. but "looks to be" isn't
good enough. there are many different call sites for the enclosing function.
we'll call GetOrigin to be safe.

+  // check calling code arguments.
+  bool requester_is_off_origin_iframe =
+      requesting_origin.GetOrigin() != embedding_origin.GetOrigin();
+  if (requester_is_off_origin_iframe) {

[mlamouri (slow - plz ping), 2015/06/23 10:02:32]

I guess you can get ride of the bool and directly do the check here. Also, you
could do a direct string comparaison. These two strings should be origins. I
think it would be fair to make this an explicit contract by adding DCHECK() at
the beginning of the method (ie. DCHECK(requesting_orinig.GetOring() ==
requesting_origin) and same for embedding_origin)

[keenanb, 2015/06/24 22:26:11]

thanks for the tip. partly done. i'd rather keep the bool because it makes the
code easy to read and extend. feel free to comment on this issue again on the
next iteration though.

+    // Summing over all top frame content setting values
+    // would give the total of all iframe requests.
+    UMA_HISTOGRAM_ENUMERATION(
+        base::StringPrintf(

[jww, 2015/06/23 05:32:58]

Use base::SStringPrintf instead to avoid the call to c_str() (which is often not
a good thing to do, since that assumes ASCII):
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

[keenanb, 2015/06/24 22:26:11]

Acknowledged.

+            "ContentSettings.PermissionRequested_IFrame.%s.TopFrame%s",
+            permission_type_string.c_str(),
+            embedding_content_setting_string.c_str()),
+        static_cast<base::HistogramBase::Sample>(type),
+        static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
+  } else {
+    // Record when a top frame requests a permission for comparison.
+    UMA_HISTOGRAM_ENUMERATION(

[jww, 2015/06/23 05:32:58]

See above re: SStringPrintf.

[keenanb, 2015/06/24 22:26:11]

Acknowledged.

+        base::StringPrintf("ContentSettings.PermissionRequested_TopFrame.%s",

[jww, 2015/06/23 05:32:58]

Personally, I think this would be a little easier to follow if there was only
one UMA_HISTOGRAM_UNUMERATION() call, and the "IFrame"/"TopFrame" was another
%s.

[keenanb, 2015/06/24 22:26:11]

good point. but that would only almost work. the extra "TopFrame%s" in the first
case would require and extra and nested printf to make it work that way. i'm
going to try to improve it in other ways.

+                           permission_type_string.c_str()),
+        static_cast<base::HistogramBase::Sample>(type),
+        static_cast<base::HistogramBase::Sample>(PermissionType::NUM));
+  }
 }
 
 } // namespace
@@ -195,8 +279,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,
+    HostContentSettingsMap* hostContentSettingsMap,
+    const GURL& requesting_origin,
+    const GURL& embedding_origin) {
+  RecordPermissionRequest(permission, hostContentSettingsMap, requesting_origin,
+                          embedding_origin);
 }
 
 void PermissionContextUmaUtil::PermissionGranted(