Log metrics for the Location Settings Dialog prompt for geolocation.

chrome/browser/geolocation/geolocation_permission_context_android.cc

Index: chrome/browser/geolocation/geolocation_permission_context_android.cc
diff --git a/chrome/browser/geolocation/geolocation_permission_context_android.cc b/chrome/browser/geolocation/geolocation_permission_context_android.cc
index 57ebc6010d5ae719002b5b9cbfe8f5867ebe9b4b..77e27228297884bed1110f74a0e4a7fb49c4ca02 100644
--- a/chrome/browser/geolocation/geolocation_permission_context_android.cc
+++ b/chrome/browser/geolocation/geolocation_permission_context_android.cc
@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/feature_list.h"
+#include "base/metrics/histogram_macros.h"
 #include "chrome/browser/android/location_settings.h"
 #include "chrome/browser/android/location_settings_impl.h"
 #include "chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.h"
@@ -29,10 +30,20 @@
 #include "content/public/browser/web_contents.h"
 #include "url/gurl.h"
 
+#define LSD_UMA(dse_metric_name, non_dse_metric_name, is_dse, backoff)  \

[Ilya Sherman, 2017/04/04 00:04:40]

LSD?  Sounds psychedelic!  Could you please expand this acronym to something
that's likely to be clearer for readers of the code?

[benwells, 2017/04/04 08:06:38]

Done.

+  if (is_dse) {                                                         \

[Ilya Sherman, 2017/04/04 00:04:40]

nit: What is "dse"?

[benwells, 2017/04/04 08:06:39]

Ah ... Default Search Engine :)

Changed and updated this particular string throughout. There is still DSE is
some existing code and histograms, changing it everywhere would make this a
bigger change and would also make some names really long.

+    UMA_HISTOGRAM_ENUMERATION(dse_metric_name, backoff,                 \
+                              GeolocationPermissionContextAndroid::     \
+                                  LocationSettingsDialogBackOff::kMax); \
+  } else {                                                              \
+    UMA_HISTOGRAM_ENUMERATION(non_dse_metric_name, backoff,             \
+                              GeolocationPermissionContextAndroid::     \
+                                  LocationSettingsDialogBackOff::kMax); \
+  }

[raymes, 2017/04/03 23:30:10]

Could this just be a function? I might be missing some reason why it needs to be
a macro :)

[Ilya Sherman, 2017/04/04 00:04:40]

UMA_HISTOGRAM_ENUMERATION requires runtime-constant names.  However,
UmaHistogramEnumeration() allows runtime-variable names, if you'd prefer to use
a function.

[benwells, 2017/04/04 08:06:39]

UmaHistogramEnumeration taking runtime-variables - how awesome is that!

Done, but I had to fix a tiny thing in UmaHistogramEnumeration to let it be used
with enum classes. Let me know if you want that pulled into a separate CL, or if
I'm just holding it wrong.

+
 namespace {
 
 int g_day_offset_for_testing = 0;
-
 const char* g_dse_origin_for_testing = nullptr;
 
 base::Time GetTimeNow() {
@@ -40,8 +51,69 @@ base::Time GetTimeNow() {
          base::TimeDelta::FromDays(g_day_offset_for_testing);
 }
 
+void LogLSDShowEvent(
+    bool is_dse,
+    GeolocationPermissionContextAndroid::LocationSettingsDialogBackOff
+        backoff_level) {
+  LSD_UMA(GeolocationPermissionContextAndroid::kGeolocationLSDShowDSEMetric,
+          GeolocationPermissionContextAndroid::kGeolocationLSDShowNonDSEMetric,
+          is_dse, backoff_level);
+}
+
+void LogLSDSuppressEvent(
+    bool is_dse,
+    GeolocationPermissionContextAndroid::LocationSettingsDialogBackOff
+        backoff_level) {
+  LSD_UMA(
+      GeolocationPermissionContextAndroid::kGeolocationLSDSuppressDSEMetric,
+      GeolocationPermissionContextAndroid::kGeolocationLSDSuppressNonDSEMetric,
+      is_dse, backoff_level);
+}
+
+void LogLSDAcceptEvent(
+    bool is_dse,
+    GeolocationPermissionContextAndroid::LocationSettingsDialogBackOff
+        backoff_level) {
+  LSD_UMA(
+      GeolocationPermissionContextAndroid::kGeolocationLSDAcceptDSEMetric,
+      GeolocationPermissionContextAndroid::kGeolocationLSDAcceptNonDSEMetric,
+      is_dse, backoff_level);
+}
+
+void LogLSDDenyEvent(
+    bool is_dse,
+    GeolocationPermissionContextAndroid::LocationSettingsDialogBackOff
+        backoff_level) {
+  LSD_UMA(GeolocationPermissionContextAndroid::kGeolocationLSDDenyDSEMetric,
+          GeolocationPermissionContextAndroid::kGeolocationLSDDenyNonDSEMetric,
+          is_dse, backoff_level);
+}
+
 }  // namespace
 
+const char GeolocationPermissionContextAndroid::kGeolocationLSDShowDSEMetric[] =
+    "GeolocationSettingsDialog.ShowEvent.DSE";

[raymes, 2017/04/03 23:30:10]

nit: should this just be called LocationSettingsDialog for consistency with
other places?

[benwells, 2017/04/04 01:46:23]

I deliberately made it start with Geo to be near the other strings
alphabetically. I think it makes editing histograms, and finding things on the
uma site easier.

+const char
+    GeolocationPermissionContextAndroid::kGeolocationLSDShowNonDSEMetric[] =
+        "GeolocationSettingsDialog.ShowEvent.NonDSE";

[Ilya Sherman, 2017/04/04 00:04:40]

It looks like all of these names have a "DSE" or "NonDSE" suffix.  Could that
just be added programmatically, rather than doubling the number of string
constants?

[benwells, 2017/04/04 01:46:23]

Sure, together with using runtime variables in UmaHistogramEnumeration this will
make the code a lot nicer.

Do you mind if I hard code the metric strings in the corresponding unit test,
instead of having functions or something to get the name with the suffix?

[Ilya Sherman, 2017/04/04 05:08:04]

I don't mind -- I actually prefer simpler, more hardcoded tests.  They're
generally easier to read and understand.

[benwells, 2017/04/04 08:06:38]

Done.

+const char
+    GeolocationPermissionContextAndroid::kGeolocationLSDSuppressDSEMetric[] =
+        "GeolocationSettingsDialog.SuppressEvent.DSE";
+const char
+    GeolocationPermissionContextAndroid::kGeolocationLSDSuppressNonDSEMetric[] =
+        "GeolocationSettingsDialog.SuppressEvent.NonDSE";
+const char
+    GeolocationPermissionContextAndroid::kGeolocationLSDAcceptDSEMetric[] =
+        "GeolocationSettingsDialog.AcceptEvent.DSE";
+const char
+    GeolocationPermissionContextAndroid::kGeolocationLSDAcceptNonDSEMetric[] =
+        "GeolocationSettingsDialog.AcceptEvent.NonDSE";
+const char GeolocationPermissionContextAndroid::kGeolocationLSDDenyDSEMetric[] =
+    "GeolocationSettingsDialog.DenyEvent.DSE";
+const char
+    GeolocationPermissionContextAndroid::kGeolocationLSDDenyNonDSEMetric[] =
+        "GeolocationSettingsDialog.DenyEvent.NonDSE";
+
 // static
 void GeolocationPermissionContextAndroid::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {
@@ -173,7 +245,7 @@ void GeolocationPermissionContextAndroid::UserMadePermissionDecision(
   // If the user has accepted geolocation, reset the location settings dialog
   // backoff.
   if (content_setting == CONTENT_SETTING_ALLOW)
-    ResetLocationSettingsBackOff(requesting_origin);
+    ResetLocationSettingsBackOff(IsRequestingOriginDSE(requesting_origin));
 }
 
 void GeolocationPermissionContextAndroid::NotifyPermissionSet(
@@ -183,6 +255,7 @@ void GeolocationPermissionContextAndroid::NotifyPermissionSet(
     const BrowserPermissionCallback& callback,
     bool persist,
     ContentSetting content_setting) {
+  bool is_dse = IsRequestingOriginDSE(requesting_origin);
   if (content_setting == CONTENT_SETTING_ALLOW &&
       !location_settings_->IsSystemLocationSettingEnabled()) {
     // There is no need to check CanShowLocationSettingsDialog here again, as it
@@ -191,20 +264,21 @@ void GeolocationPermissionContextAndroid::NotifyPermissionSet(
     // content setting is ASK the backoff should be ignored. However if we get
     // here and the content setting was ASK, the user must have accepted which
     // would reset the backoff.
-    if (IsInLocationSettingsBackOff(requesting_origin)) {
+    if (IsInLocationSettingsBackOff(is_dse)) {
       FinishNotifyPermissionSet(id, requesting_origin, embedding_origin,
                                 callback, false /* persist */,
                                 CONTENT_SETTING_BLOCK);
+      LogLSDSuppressEvent(is_dse, LocationSettingsBackOffLevel(is_dse));
       return;
     }
 
+    LogLSDShowEvent(is_dse, LocationSettingsBackOffLevel(is_dse));
     content::WebContents* web_contents =
         content::WebContents::FromRenderFrameHost(
             content::RenderFrameHost::FromID(id.render_process_id(),
                                              id.render_frame_id()));
     location_settings_->PromptToEnableSystemLocationSetting(
-        IsRequestingOriginDSE(requesting_origin) ? SEARCH : DEFAULT,
-        web_contents,
+        is_dse ? SEARCH : DEFAULT, web_contents,
         base::BindOnce(
             &GeolocationPermissionContextAndroid::OnLocationSettingsDialogShown,
             weak_factory_.GetWeakPtr(), id, requesting_origin, embedding_origin,
@@ -259,69 +333,71 @@ GeolocationPermissionContextAndroid::UpdatePermissionStatusWithDeviceStatus(
 
 std::string
 GeolocationPermissionContextAndroid::GetLocationSettingsBackOffLevelPref(
-    const GURL& requesting_origin) const {
-  if (IsRequestingOriginDSE(requesting_origin))
-    return prefs::kLocationSettingsBackoffLevelDSE;
-
-  return prefs::kLocationSettingsBackoffLevelDefault;
+    bool is_dse) const {
+  return is_dse ? prefs::kLocationSettingsBackoffLevelDSE
+                : prefs::kLocationSettingsBackoffLevelDefault;
 }
 
 std::string
 GeolocationPermissionContextAndroid::GetLocationSettingsNextShowPref(
-    const GURL& requesting_origin) const {
-  if (IsRequestingOriginDSE(requesting_origin))
-    return prefs::kLocationSettingsNextShowDSE;
-
-  return prefs::kLocationSettingsNextShowDefault;
+    bool is_dse) const {
+  return is_dse ? prefs::kLocationSettingsNextShowDSE
+                : prefs::kLocationSettingsNextShowDefault;
 }
 
 bool GeolocationPermissionContextAndroid::IsInLocationSettingsBackOff(
-    const GURL& requesting_origin) const {
-  base::Time next_show =
-      base::Time::FromInternalValue(profile()->GetPrefs()->GetInt64(
-          GetLocationSettingsNextShowPref(requesting_origin)));
+    bool is_dse) const {
+  base::Time next_show = base::Time::FromInternalValue(
+      profile()->GetPrefs()->GetInt64(GetLocationSettingsNextShowPref(is_dse)));
 
   return GetTimeNow() < next_show;
 }
 
 void GeolocationPermissionContextAndroid::ResetLocationSettingsBackOff(
-    const GURL& requesting_origin) {
+    bool is_dse) {
   PrefService* prefs = profile()->GetPrefs();
-  prefs->ClearPref(GetLocationSettingsNextShowPref(requesting_origin));
-  prefs->ClearPref(GetLocationSettingsBackOffLevelPref(requesting_origin));
+  prefs->ClearPref(GetLocationSettingsNextShowPref(is_dse));
+  prefs->ClearPref(GetLocationSettingsBackOffLevelPref(is_dse));
 }
 
 void GeolocationPermissionContextAndroid::UpdateLocationSettingsBackOff(
-    const GURL& requesting_origin) {
-  // Backoff levels:
-  // 0: 1 week
-  // 1: 1 month
-  // 2: 3 months
-  PrefService* prefs = profile()->GetPrefs();
-  int backoff_level =
-      prefs->GetInteger(GetLocationSettingsBackOffLevelPref(requesting_origin));
+    bool is_dse) {
+  LocationSettingsDialogBackOff backoff_level =
+      LocationSettingsBackOffLevel(is_dse);
 
   base::Time next_show = GetTimeNow();
   switch (backoff_level) {
-    case 0:
+    case LocationSettingsDialogBackOff::kOneWeek:

[raymes, 2017/04/03 23:30:10]

I wonder if 0 should represent "kNoBackOff". In other words instead of
representing what the backoff will be changed to, it instead represents what the
backoff was last applied. It might make the metrics a bit easier to read. For
example it would mean that you would read something like
Shown
10, no backoff
1, 1 week backoff,
1, 1 month backoff
2, 3 month backoff

For Suppress, "no backoff" would never appear since it should be impossible to
get into that state.

It's not a major thing, but just a thought :)

[benwells, 2017/04/04 08:06:39]

Done.

       next_show += base::TimeDelta::FromDays(7);
+      backoff_level = LocationSettingsDialogBackOff::kOneMonth;
       break;
-    case 1:
+    case LocationSettingsDialogBackOff::kOneMonth:
       next_show += base::TimeDelta::FromDays(30);
+      backoff_level = LocationSettingsDialogBackOff::kThreeMonths;
       break;
-    default:
+    case LocationSettingsDialogBackOff::kThreeMonths:
       next_show += base::TimeDelta::FromDays(90);
+      break;
+    default:
+      NOTREACHED();
   }
 
-  if (backoff_level < 2)
-    backoff_level++;
-
-  prefs->SetInteger(GetLocationSettingsBackOffLevelPref(requesting_origin),
-                    backoff_level);
-  prefs->SetInt64(GetLocationSettingsNextShowPref(requesting_origin),
+  PrefService* prefs = profile()->GetPrefs();
+  prefs->SetInteger(GetLocationSettingsBackOffLevelPref(is_dse),
+                    static_cast<int>(backoff_level));
+  prefs->SetInt64(GetLocationSettingsNextShowPref(is_dse),
                   next_show.ToInternalValue());
 }
 
+GeolocationPermissionContextAndroid::LocationSettingsDialogBackOff
+GeolocationPermissionContextAndroid::LocationSettingsBackOffLevel(
+    bool is_dse) const {
+  PrefService* prefs = profile()->GetPrefs();
+  int int_backoff =
+      prefs->GetInteger(GetLocationSettingsBackOffLevelPref(is_dse));
+  return static_cast<LocationSettingsDialogBackOff>(int_backoff);
+}
+
 bool GeolocationPermissionContextAndroid::IsLocationAccessPossible(
     content::WebContents* web_contents,
     const GURL& requesting_origin,
@@ -379,12 +455,13 @@ bool GeolocationPermissionContextAndroid::CanShowLocationSettingsDialog(
   if (!base::FeatureList::IsEnabled(features::kLsdPermissionPrompt))
     return false;
 
+  bool is_dse = IsRequestingOriginDSE(requesting_origin);
   // If this isn't the default search engine, a gesture is needed.
-  if (!IsRequestingOriginDSE(requesting_origin) && !user_gesture) {
+  if (!is_dse && !user_gesture) {
     return false;
   }
 
-  if (!ignore_backoff && IsInLocationSettingsBackOff(requesting_origin))
+  if (!ignore_backoff && IsInLocationSettingsBackOff(is_dse))
     return false;
 
   return location_settings_->CanPromptToEnableSystemLocationSetting();
@@ -398,10 +475,13 @@ void GeolocationPermissionContextAndroid::OnLocationSettingsDialogShown(
     bool persist,
     ContentSetting content_setting,
     LocationSettingsDialogOutcome prompt_outcome) {
+  bool is_dse = IsRequestingOriginDSE(requesting_origin);
   if (prompt_outcome == GRANTED) {
-    ResetLocationSettingsBackOff(requesting_origin);
+    LogLSDAcceptEvent(is_dse, LocationSettingsBackOffLevel(is_dse));
+    ResetLocationSettingsBackOff(is_dse);
   } else {
-    UpdateLocationSettingsBackOff(requesting_origin);
+    LogLSDDenyEvent(is_dse, LocationSettingsBackOffLevel(is_dse));
+    UpdateLocationSettingsBackOff(is_dse);
     content_setting = CONTENT_SETTING_BLOCK;
     persist = false;
   }