Record metric of geo permission before and after showing the disclosure.

chrome/browser/android/search_geolocation_disclosure_tab_helper.cc

 // Copyright 2016 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 "chrome/browser/android/search_geolocation_disclosure_tab_helper.h"
 
 #include "base/feature_list.h"
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h"
 #include "chrome/browser/permissions/permission_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
@@ skipping 54 matching lines @@
 
 // static
 void SearchGeolocationDisclosureTabHelper::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterBooleanPref(prefs::kSearchGeolocationDisclosureDismissed,
                                 false);
   registry->RegisterIntegerPref(prefs::kSearchGeolocationDisclosureShownCount,
                                 0);
   registry->RegisterInt64Pref(prefs::kSearchGeolocationDisclosureLastShowDate,
                               0);
+  registry->RegisterBooleanPref(
+      prefs::kSearchGeolocationPreDisclosureMetricsRecorded, false);
+  registry->RegisterBooleanPref(
+      prefs::kSearchGeolocationPostDisclosureMetricsRecorded, false);
 }
 
 void SearchGeolocationDisclosureTabHelper::
-    MaybeShowDefaultSearchGeolocationDisclosure(GURL gurl) {
-  // Don't show the infobar if the user has dismissed it.
-  PrefService* prefs = GetProfile()->GetPrefs();
-  bool dismissed_already =
-      prefs->GetBoolean(prefs::kSearchGeolocationDisclosureDismissed);
-  if (dismissed_already)
-    return;
-
-  // Or if they've seen it enough times already.
-  int shown_count =
-      prefs->GetInteger(prefs::kSearchGeolocationDisclosureShownCount);
-  if (shown_count >= GetMaxShowCount())
-    return;
-
-  // Or if it has been shown too recently.
-  base::Time last_shown = base::Time::FromInternalValue(
-      prefs->GetInt64(prefs::kSearchGeolocationDisclosureLastShowDate));
-  if (base::Time::Now() - last_shown <
-      base::TimeDelta::FromDays(GetDaysPerShow())) {
-    return;
-  }
-
+    MaybeShowDefaultSearchGeolocationDisclosure(const GURL& gurl) {
   // Only show the disclosure for default search navigations from the omnibox.
   TemplateURLService* template_url_service =
       TemplateURLServiceFactory::GetForProfile(GetProfile());
   bool is_search_url =
       template_url_service->IsSearchResultsPageFromDefaultSearchProvider(
           gurl);
   if (!is_search_url)
     return;
 
   // Only show the disclosure if Google is the default search engine.
   TemplateURL* default_search =
       template_url_service->GetDefaultSearchProvider();
   if (!default_search ||
       !default_search->url_ref().HasGoogleBaseURLs(
           template_url_service->search_terms_data())) {
     return;
   }
 
+  // Record metrics for the state of permissions before the disclosure has been
+  // shown.
+  RecordPreDisclosureMetrics(gurl);
+
+  // Don't show the infobar if the user has dismissed it, or they've seen it
+  // enough times already.
+  PrefService* prefs = GetProfile()->GetPrefs();
+  bool dismissed_already =
+      prefs->GetBoolean(prefs::kSearchGeolocationDisclosureDismissed);
+  int shown_count =
+      prefs->GetInteger(prefs::kSearchGeolocationDisclosureShownCount);
+  if (dismissed_already || shown_count >= GetMaxShowCount()) {
+    // Record metrics for the state of permissions after the disclosure has been
+    // shown.
+    RecordPostDisclosureMetrics(gurl);

[gone, 2016/11/22 21:36:59]

Why does this happen up here instead of at the end of the function, where the
shown count goes up?

[benwells, 2016/11/22 21:42:34]

I will update the comment to the below. Is it clear?

Record metrics for the state of permissions after the disclosure has been shown.
This is not done immediately after showing the last disclosure (i.e. at the end
of this function), but on the next omnibox search, to allow the metric to
capture changes to settings done by the user as a result of clicking on the
Settings link in the disclosure. 

[gone, 2016/11/22 21:48:22]

Ah yeah, that'd be good. Thanks!

[benwells, 2016/11/23 02:31:22]

Done.

+    return;
+  }
+
+  // Or if it has been shown too recently.
+  base::Time last_shown = base::Time::FromInternalValue(
+      prefs->GetInt64(prefs::kSearchGeolocationDisclosureLastShowDate));
+  if (base::Time::Now() - last_shown <
+      base::TimeDelta::FromDays(GetDaysPerShow())) {
+    return;
+  }
+
   // Check that the Chrome app has geolocation permission.
   JNIEnv* env = base::android::AttachCurrentThread();
   if (!Java_GeolocationHeader_hasGeolocationPermission(env))
     return;
 
   // Only show the disclosure if the geolocation permission is set to ASK
   // (i.e. has not been explicitly set or revoked).
   blink::mojom::PermissionStatus status =
       PermissionManager::Get(GetProfile())
           ->GetPermissionStatus(content::PermissionType::GEOLOCATION, gurl,
                                 gurl);
   if (status != blink::mojom::PermissionStatus::ASK)
     return;
 
   // All good, let's show the disclosure and increment the shown count.
   SearchGeolocationDisclosureInfoBarDelegate::Create(web_contents(), gurl);
   shown_count++;
   prefs->SetInteger(prefs::kSearchGeolocationDisclosureShownCount, shown_count);
   prefs->SetInt64(prefs::kSearchGeolocationDisclosureLastShowDate,
                   base::Time::Now().ToInternalValue());
 }
 
+void SearchGeolocationDisclosureTabHelper::RecordPreDisclosureMetrics(
+    const GURL& gurl) {
+  PrefService* prefs = GetProfile()->GetPrefs();
+  if (!prefs->GetBoolean(
+          prefs::kSearchGeolocationPreDisclosureMetricsRecorded)) {
+    blink::mojom::PermissionStatus status =
+        PermissionManager::Get(GetProfile())
+            ->GetPermissionStatus(content::PermissionType::GEOLOCATION, gurl,
+                                  gurl);
+    UMA_HISTOGRAM_ENUMERATION("GeolocationDisclosure.PreDisclosurePermission",
+                              static_cast<base::HistogramBase::Sample>(status),
+                              static_cast<base::HistogramBase::Sample>(
+                                  blink::mojom::PermissionStatus::LAST) +

[Ilya Sherman, 2016/11/22 22:36:16]

Near its definition, please document that this enum is used to back an UMA
histogram, and should therefore be treated as append-only (if it's not
documented this way already).

[benwells, 2016/11/23 02:31:22]

Done.

+                                  1);
+    prefs->SetBoolean(prefs::kSearchGeolocationPreDisclosureMetricsRecorded,
+                      true);
+  }
+}
+
+void SearchGeolocationDisclosureTabHelper::RecordPostDisclosureMetrics(
+    const GURL& gurl) {
+  PrefService* prefs = GetProfile()->GetPrefs();
+  if (!prefs->GetBoolean(
+          prefs::kSearchGeolocationPostDisclosureMetricsRecorded)) {
+    blink::mojom::PermissionStatus status =
+        PermissionManager::Get(GetProfile())
+            ->GetPermissionStatus(content::PermissionType::GEOLOCATION, gurl,
+                                  gurl);
+    UMA_HISTOGRAM_ENUMERATION("GeolocationDisclosure.PostDisclosurePermission",
+                              static_cast<base::HistogramBase::Sample>(status),
+                              static_cast<base::HistogramBase::Sample>(
+                                  blink::mojom::PermissionStatus::LAST) +
+                                  1);
+    prefs->SetBoolean(prefs::kSearchGeolocationPostDisclosureMetricsRecorded,
+                      true);
+  }
+}
+
 Profile* SearchGeolocationDisclosureTabHelper::GetProfile() {
   return Profile::FromBrowserContext(web_contents()->GetBrowserContext());
 }