Add Geolocation.PermissionState histogram.

chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java

Index: chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
index ab4f5fae4fcd7b55113ee16a98dd6a737527df97..10d11625bdee957debb0b0eae2cbe7d5de70fc11 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
@@ -5,9 +5,11 @@
 package org.chromium.chrome.browser.omnibox.geo;
 
 import android.Manifest;
+import android.app.Activity;
 import android.content.Context;
 import android.content.pm.PackageManager;
 import android.location.Location;
+import android.location.LocationManager;
 import android.net.Uri;
 import android.os.Build;
 import android.os.Process;
@@ -20,7 +22,9 @@ import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.browser.preferences.website.ContentSetting;
 import org.chromium.chrome.browser.preferences.website.GeolocationInfo;
 import org.chromium.chrome.browser.util.UrlUtilities;
+import org.chromium.ui.base.WindowAndroid;
 
+import java.util.List;
 import java.util.Locale;
 
 /**
@@ -40,6 +44,72 @@ public class GeolocationHeader {
     public static final int UMA_LOCATION_DISABLED_FOR_CHROME_APP = 5;
     public static final int UMA_MAX = 8;
 
+    // Values for the histogram GeolocationHeader.PermissionState.
+    // These are used to back an UMA histogram and so should be treated as append-only.
+    //
+    // These enum values are assigned using the equation:
+    // 36 * !locationAttached + 9 * locationSource + 3 * appPermission + domainPermission
+    // so if new enum values are added, care must be taken to ensure that the equation still holds.
+    // Enum values 38-44 and 47-53 are skipped to comply with the conversion formula.
+    //
+    // In order to keep the names of these constants from being too long, the following were used:
+    // UMA_PERM to indicate UMA location permission related metrics,
+    // APP_YES (instead of APP_GRANTED) to indicate App permission granted,
+    // DOMAIN_YES (instead of DOMAIN_GRANTED) to indicate Domain permission granted.
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_LOCATION = 0;

[dominickn, 2016/11/30 00:08:45]

You might want to use @IntDef here to help with the boilerplate checking of
these enum-like constants. See PermissionDialogController.java for an example.

[pdyson, 2016/11/30 08:00:43]

Done. I've added this here and also to LOCATION_SOURCE_* and PERMISSION_*.

+    public static final int UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_PROMPT_LOCATION = 1;
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_BLOCKED = 2;
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_PROMPT_DOMAIN_YES = 3;
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_PROMPT_DOMAIN_PROMPT = 4;
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_PROMPT_DOMAIN_BLOCKED = 5;
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_BLOCKED_DOMAIN_YES = 6;
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_BLOCKED_DOMAIN_PROMPT = 7;
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_BLOCKED_DOMAIN_BLOCKED = 8;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_YES_DOMAIN_YES_LOCATION = 9;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_YES_DOMAIN_PROMPT_LOCATION = 10;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_YES_DOMAIN_BLOCKED = 11;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_PROMPT_DOMAIN_YES = 12;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_PROMPT_DOMAIN_PROMPT = 13;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_PROMPT_DOMAIN_BLOCKED = 14;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_BLOCKED_DOMAIN_YES = 15;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_BLOCKED_DOMAIN_PROMPT = 16;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_BLOCKED_DOMAIN_BLOCKED = 17;
+    public static final int UMA_PERM_GPS_ONLY_APP_YES_DOMAIN_YES_LOCATION = 18;
+    public static final int UMA_PERM_GPS_ONLY_APP_YES_DOMAIN_PROMPT_LOCATION = 19;
+    public static final int UMA_PERM_GPS_ONLY_APP_YES_DOMAIN_BLOCKED = 20;
+    public static final int UMA_PERM_GPS_ONLY_APP_PROMPT_DOMAIN_YES = 21;
+    public static final int UMA_PERM_GPS_ONLY_APP_PROMPT_DOMAIN_PROMPT = 22;
+    public static final int UMA_PERM_GPS_ONLY_APP_PROMPT_DOMAIN_BLOCKED = 23;
+    public static final int UMA_PERM_GPS_ONLY_APP_BLOCKED_DOMAIN_YES = 24;
+    public static final int UMA_PERM_GPS_ONLY_APP_BLOCKED_DOMAIN_PROMPT = 25;
+    public static final int UMA_PERM_GPS_ONLY_APP_BLOCKED_DOMAIN_BLOCKED = 26;
+    public static final int UMA_PERM_MASTER_OFF_APP_YES_DOMAIN_YES = 27;
+    public static final int UMA_PERM_MASTER_OFF_APP_YES_DOMAIN_PROMPT = 28;
+    public static final int UMA_PERM_MASTER_OFF_APP_YES_DOMAIN_BLOCKED = 29;
+    public static final int UMA_PERM_MASTER_OFF_APP_PROMPT_DOMAIN_YES = 30;
+    public static final int UMA_PERM_MASTER_OFF_APP_PROMPT_DOMAIN_PROMPT = 31;
+    public static final int UMA_PERM_MASTER_OFF_APP_PROMPT_DOMAIN_BLOCKED = 32;
+    public static final int UMA_PERM_MASTER_OFF_APP_BLOCKED_DOMAIN_YES = 33;
+    public static final int UMA_PERM_MASTER_OFF_APP_BLOCKED_DOMAIN_PROMPT = 34;
+    public static final int UMA_PERM_MASTER_OFF_APP_BLOCKED_DOMAIN_BLOCKED = 35;
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_YES_NO_LOCATION = 36;
+    public static final int UMA_PERM_HIGH_ACCURACY_APP_YES_DOMAIN_PROMPT_NO_LOCATION = 37;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_YES_DOMAIN_YES_NO_LOCATION = 45;
+    public static final int UMA_PERM_BATTERY_SAVING_APP_YES_DOMAIN_PROMPT_NO_LOCATION = 46;
+    public static final int UMA_PERM_GPS_ONLY_APP_YES_DOMAIN_YES_NO_LOCATION = 54;
+    public static final int UMA_PERM_GPS_ONLY_APP_YES_DOMAIN_PROMPT_NO_LOCATION = 55;
+    public static final int UMA_PERM_UNKNOWN = 56;
+    public static final int UMA_PERM_MAX = 56;
+
+    public static final int LOCATION_SOURCE_HIGH_ACCURACY = 0;
+    public static final int LOCATION_SOURCE_BATTERY_SAVING = 1;
+    public static final int LOCATION_SOURCE_GPS_ONLY = 2;
+    public static final int LOCATION_SOURCE_MASTER_OFF = 3;
+
+    public static final int PERMISSION_GRANTED = 0;
+    public static final int PERMISSION_PROMPT = 1;
+    public static final int PERMISSION_BLOCKED = 2;
+
     /** The maximum age in milliseconds of a location that we'll send in an X-Geo header. */
     private static final int MAX_LOCATION_AGE = 24 * 60 * 60 * 1000;  // 24 hours
 
@@ -112,21 +182,34 @@ public class GeolocationHeader {
      * @return The X-Geo header string or null.
      */
     public static String getGeoHeader(Context context, String url, boolean isIncognito) {
+        boolean locationAttached = true;
         if (!isGeoHeaderEnabledForUrl(context, url, isIncognito, true)) {
-            return null;
+            locationAttached = false;
         }
 
         // Only send X-Geo header if there's a fresh location available.
         Location location = GeolocationTracker.getLastKnownLocation(context);
         if (location == null) {
             recordHistogram(UMA_LOCATION_NOT_AVAILABLE);
-            return null;
-        }
-        if (GeolocationTracker.getLocationAge(location) > MAX_LOCATION_AGE) {
-            recordHistogram(UMA_LOCATION_STALE);
-            return null;
+            locationAttached = false;
+        } else {
+            long locationAge = GeolocationTracker.getLocationAge(location);
+            if (locationAge > MAX_LOCATION_AGE) {
+                recordHistogram(UMA_LOCATION_STALE);
+                locationAttached = false;
+            }
         }
 
+        int locationSource = getLocationSource(context);
+        int appPermission = getGeolocationPermission(context);
+        int domainPermission = getDomainPermission(url, isIncognito);
+
+        // Record the permission state with a histogram.
+        recordPermissionHistogram(
+                locationSource, appPermission, domainPermission, locationAttached);
+
+        if (!locationAttached) return null;
+
         recordHistogram(UMA_HEADER_SENT);
 
         // Timestamp in microseconds since the UNIX epoch.
@@ -155,12 +238,31 @@ public class GeolocationHeader {
     }
 
     static boolean hasGeolocationPermission(Context context) {
+        return getGeolocationPermission(context) == PERMISSION_GRANTED;
+    }
+
+    /**
+     * Returns the geolocation permission.
+     * This permission can be either granted, blocked or prompt.
+     */
+    static int getGeolocationPermission(Context context) {
         int pid = Process.myPid();
         int uid = Process.myUid();
         if (ApiCompatibilityUtils.checkPermission(
                 context, Manifest.permission.ACCESS_COARSE_LOCATION, pid, uid)
                 != PackageManager.PERMISSION_GRANTED) {
-            return false;
+            WindowAndroid window = new WindowAndroid(context);

[dominickn, 2016/11/30 00:08:46]

I'm almost certain that creating a new WindowAndroid here to get the activity is
not the right way to do this.

A better way would be to modify the two callsites of
GeolocationHeader#getGeoHeader to pass a Tab object to this method, and then
call tab.getWindowAndroid().getActivity().get().

[pdyson, 2016/11/30 08:00:43]

I have made this change, but now I need to mock a Tab for testing. I've had a
look around but I haven't found how to do that. Can you point me to an example?

[dominickn, 2016/12/01 00:30:26]

You'll need to change the test case to extend
ChromeActivityTestCaseBase<ChromeActivity>. That's a different subclass of
InstrumentationTestCase that sets up a Chrome activity and tab for you.

Using that, you'll be able to grab the current tab with
getActivity().getActivityTab(). See PermissionTestCaseBase.java for an example -
you'll just need to override startMainActivity() like the PermissionTestCaseBase
does and I *think* that's it.

+            Activity activity = window.activityFromContext(context);
+            if (activity == null || Build.VERSION.SDK_INT < Build.VERSION_CODES.M) {
+                // Can't check for prompt when activity is null, so default to blocked.
+                // No prompt before version M.
+                return PERMISSION_BLOCKED;
+            } else {
+                return activity.shouldShowRequestPermissionRationale(
+                               Manifest.permission.ACCESS_COARSE_LOCATION)
+                        ? PERMISSION_PROMPT
+                        : PERMISSION_BLOCKED;
+            }
         }
 
         // Work around a bug in OnePlus2 devices running Lollipop, where the NETWORK_PROVIDER
@@ -170,10 +272,10 @@ public class GeolocationHeader {
                 && ApiCompatibilityUtils.checkPermission(
                         context, Manifest.permission.ACCESS_FINE_LOCATION, pid, uid)
                         != PackageManager.PERMISSION_GRANTED) {
-            return false;
+            return PERMISSION_BLOCKED;
         }
 
-        return true;
+        return PERMISSION_GRANTED;
     }
 
     /**
@@ -182,6 +284,16 @@ public class GeolocationHeader {
      * scheme, this considers the user's preference for url with the http scheme instead.
      */
     static boolean isLocationDisabledForUrl(Uri uri, boolean isIncognito) {
+        ContentSetting locationPermission = locationPermissionForUrl(uri, isIncognito);
+        return locationPermission == ContentSetting.BLOCK;

[dominickn, 2016/11/30 00:08:45]

Nit: remove the local variable and directly compare the result of the method
call against ContentSetting.BLOCK

[pdyson, 2016/11/30 08:00:43]

Done. Still getting the hang of how much to put on one line in Java.

+    }
+
+    /**
+     * Returns the location permission for sharing their location with url (e.g. via the
+     * geolocation infobar). If the user has not chosen a preference for url and url uses the https
+     * scheme, this returns the user's preference for url with the http scheme instead.
+     */
+    static ContentSetting locationPermissionForUrl(Uri uri, boolean isIncognito) {

[dominickn, 2016/11/30 00:08:45]

Nit: call this locationContentSettingForUrl

[pdyson, 2016/11/30 08:00:42]

Done.

         GeolocationInfo locationSettings = new GeolocationInfo(uri.toString(), null, isIncognito);
         ContentSetting locationPermission = locationSettings.getContentSetting();
 
@@ -197,11 +309,81 @@ public class GeolocationHeader {
             }
         }
 
-        return locationPermission == ContentSetting.BLOCK;
+        return locationPermission;
     }
 
     /** Records a data point for the Geolocation.HeaderSentOrNot histogram. */
     private static void recordHistogram(int result) {
         RecordHistogram.recordEnumeratedHistogram("Geolocation.HeaderSentOrNot", result, UMA_MAX);
     }
+
+    /** Returns the location source. */
+    private static int getLocationSource(Context context) {
+        LocationManager locationManager =
+                (LocationManager) context.getSystemService(Context.LOCATION_SERVICE);
+        List<String> providers = locationManager.getProviders(true);
+        boolean hasNetworkProvider = false;
+        boolean hasGpsProvider = false;
+        for (String provider : providers) {
+            if (!LocationManager.PASSIVE_PROVIDER.equals(provider)) {
+                if (LocationManager.NETWORK_PROVIDER.equals(provider)) {
+                    hasNetworkProvider = true;
+                } else if (LocationManager.GPS_PROVIDER.equals(provider)) {
+                    hasGpsProvider = true;
+                }
+            }
+        }
+        int locationSource;

[dominickn, 2016/11/30 00:08:45]

Nit: this variable is unused.

[pdyson, 2016/11/30 08:00:43]

Done.

+        return hasNetworkProvider
+                ? (hasGpsProvider ? LOCATION_SOURCE_HIGH_ACCURACY : LOCATION_SOURCE_BATTERY_SAVING)
+                : (hasGpsProvider ? LOCATION_SOURCE_GPS_ONLY : LOCATION_SOURCE_MASTER_OFF);
+    }
+
+    /**
+     * Returns the domain permission.
+     * This permission can be either granted, blocked or prompt.
+     */
+    private static int getDomainPermission(String url, boolean isIncognito) {
+        Uri uri = Uri.parse(url);

[dominickn, 2016/11/30 00:08:45]

Nit: remove the local variable and inline Uri.parse(url) into the
locationPermissionForUrl call.

[pdyson, 2016/11/30 08:00:43]

Done.

+        final ContentSetting domainPermission = locationPermissionForUrl(uri, isIncognito);

[dominickn, 2016/11/30 00:08:45]

Nit: I don't think this needs to be final?

[pdyson, 2016/11/30 08:00:42]

Done.

+        switch (domainPermission) {
+            case BLOCK:

[dominickn, 2016/11/30 00:08:45]

I would make BLOCK the default case, in case new ContentSetting types are added
in the future. BLOCK default is safest.

[pdyson, 2016/11/30 08:00:43]

Done.

+                return PERMISSION_BLOCKED;
+            case ASK:
+                return PERMISSION_PROMPT;
+            default:
+                return PERMISSION_GRANTED;
+        }
+    }
+
+    /**
+     * Returns the enum to use in the GeolocationHeader.PermissionState histogram.
+     * Unexpected input values return UMA_PERM_UNKNOWN.
+     * Note that this relies on the enum values being obtainable using a mathematical formula.
+     */
+    private static int getPermissionHistogramEnum(
+            int locationSource, int appPermission, int domainPermission, boolean locationAttached) {
+        // Calculate the ENUM value using an equation.
+        int value = 9 * locationSource + 3 * appPermission + domainPermission;

[dominickn, 2016/11/30 00:08:46]

This calculation is quite magic and makes me very uneasy.

A bigger comment, removing the magic numbers, and even a step by step
explanation of how this is calculated is definitely necessary. Even then, it may
be better just to use ifs to step our way through; that will probably be more
maintainable.

[Ilya Sherman, 2016/11/30 00:33:35]

FWIW, I agree that it would be best to clarify how these enum constant values
are being interpolated.  However, if y'all prefer to use if-stmts or
switch-stmts, I'm okay with that too, even though I think it's actually more
likely to be error-prone due to copy/pasta bugs.

Note that if you do use an equation like this one, you probably don't need the
list of enum constants at the top of the file.

[pdyson, 2016/11/30 08:00:43]

I've put this back to using the switch statements. It's long and ugly, but I'd
rather
be more explicit. Is there a nicer way of doing these switches?

+        if (value < 0 || value > 35) return UMA_PERM_UNKNOWN;
+        // Handle the NO_LOCATION cases.
+        if ((locationSource == LOCATION_SOURCE_HIGH_ACCURACY
+                    || locationSource == LOCATION_SOURCE_BATTERY_SAVING
+                    || locationSource == LOCATION_SOURCE_GPS_ONLY)
+                && appPermission == PERMISSION_GRANTED
+                && (domainPermission == PERMISSION_GRANTED || domainPermission == PERMISSION_PROMPT)
+                && !locationAttached) {
+            value += 36;
+        }
+        return value;
+    }
+
+    /** Records a data point for the GeolocationHeader.PermissionState histogram. */
+    private static void recordPermissionHistogram(
+            int locationSource, int appPermission, int domainPermission, boolean locationAttached) {
+        int result = getPermissionHistogramEnum(
+                locationSource, appPermission, domainPermission, locationAttached);
+        RecordHistogram.recordEnumeratedHistogram(
+                "GeolocationHeader.PermissionState", result, UMA_PERM_MAX);
+    }
 }