Add histograms for location timings for Android omnibox queries.

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 e5cdadee8ec7d86aebbddf5c0aee2122bbd71404..16b8206160db63dc990df884c38dd3f3168e50f9 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
@@ -28,6 +28,7 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.List;
 import java.util.Locale;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Provides methods for building the X-Geo HTTP header, which provides device location to a server
@@ -152,6 +153,14 @@ public class GeolocationHeader {
     @IntDef({PERMISSION_GRANTED, PERMISSION_PROMPT, PERMISSION_BLOCKED})
     private @interface Permission {}
 
+    /** The maximum value for the GeolocationHeader.TimeListening* histograms. */
+    public static final int TIME_LISTENING_HISTOGRAM_MAX_MILLIS = 50 * 60 * 1000; // 50 minutes
+
+    /** The maximum value for the GeolocationHeader.LocationAge* histograms.
+     * Using 24 days as that is the highest number of days that does not overflow int.

[Ilya Sherman, 2016/12/15 01:45:12]

Maybe this histogram doesn't make sense to measure in millis, if you're worried
about int overflow as a constraining factor?

[pdyson, 2016/12/15 05:31:00]

Do you know how to change this? I tried using seconds instead, but
recordCustomTimesHistogram() then just converts the values
into milliseconds and overflows the int.

[Ilya Sherman, 2016/12/15 23:06:19]

You can use recordCustomCountHistogram().  It's pretty much the same as
recordCustomTimesHistogram(), except that it won't try to convert the recorded
values into milliseconds =)

+     */
+    public static final int LOCATION_AGE_HISTOGRAM_MAX_MILLIS = 24 * 24 * 60 * 60 * 1000;
+
     /** 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
 
@@ -160,6 +169,9 @@ public class GeolocationHeader {
 
     private static final String HTTPS_SCHEME = "https";
 
+    /** The time of the first location refresh. Contains Long.MAX_VALUE if not set. */
+    private static long sFirstLocationTime = Long.MAX_VALUE;
+
     /**
      * Requests a location refresh so that a valid location will be available for constructing
      * an X-Geo header in the near future (i.e. within 5 minutes).
@@ -169,6 +181,9 @@ public class GeolocationHeader {
     public static void primeLocationForGeoHeader(Context context) {
         if (!hasGeolocationPermission(context)) return;
 
+        if (sFirstLocationTime == Long.MAX_VALUE) {
+            sFirstLocationTime = System.currentTimeMillis();
+        }
         GeolocationTracker.refreshLastKnownLocation(context, REFRESH_LOCATION_AGE);
     }
 
@@ -227,6 +242,7 @@ public class GeolocationHeader {
         boolean isIncognito = tab.isIncognito();
         boolean locationAttached = true;
         Location location = null;
+        long locationAge = Long.MAX_VALUE;
         if (isGeoHeaderEnabledForUrl(context, url, isIncognito, true)) {
             // Only send X-Geo header if there's a fresh location available.
             location = GeolocationTracker.getLastKnownLocation(context);
@@ -234,7 +250,8 @@ public class GeolocationHeader {
                 recordHistogram(UMA_LOCATION_NOT_AVAILABLE);
                 locationAttached = false;
             } else {
-                if (GeolocationTracker.getLocationAge(location) > MAX_LOCATION_AGE) {
+                locationAge = GeolocationTracker.getLocationAge(location);
+                if (locationAge > MAX_LOCATION_AGE) {
                     recordHistogram(UMA_LOCATION_STALE);
                     locationAttached = false;
                 }
@@ -251,6 +268,17 @@ public class GeolocationHeader {
         recordPermissionHistogram(
                 locationSource, appPermission, domainPermission, locationAttached);
 
+        if (locationSource != LOCATION_SOURCE_MASTER_OFF && appPermission != PERMISSION_BLOCKED
+                && domainPermission != PERMISSION_BLOCKED) {
+            // Record the Location Age with a histogram.
+            recordLocationAgeHistogram(locationSource, locationAge);
+            long duration = sFirstLocationTime == Long.MAX_VALUE
+                    ? Long.MAX_VALUE
+                    : System.currentTimeMillis() - sFirstLocationTime;
+            // Record the Time Listening with a histogram.
+            recordTimeListeningHistogram(locationSource, locationAttached, duration);
+        }
+
         // Note that strictly speaking "location == null" is not needed here as the
         // logic above prevents location being null when locationAttached is true.
         // It is here to prevent problems if the logic above is changed.
@@ -530,4 +558,53 @@ public class GeolocationHeader {
         RecordHistogram.recordEnumeratedHistogram(
                 "Geolocation.Header.PermissionState", result, UMA_PERM_COUNT);
     }
+
+    /** Determines the name for a Time Listening Histogram. Returns empty string if the location
+     * source is MASTER_OFF as we do not record histograms for that case.
+     */
+    private static String getTimeListeningHistogramEnum(
+            int locationSource, boolean locationAttached) {
+        switch (locationSource) {
+            case LOCATION_SOURCE_HIGH_ACCURACY:
+                return locationAttached
+                        ? "Geolocation.Header.TimeListeningHighAccuracyLocationAttached"
+                        : "Geolocation.Header.TimeListeningHighAccuracyLocationNotAttached";
+            case LOCATION_SOURCE_GPS_ONLY:
+                return locationAttached
+                        ? "Geolocation.Header.TimeListeningGpsOnlyLocationAttached"
+                        : "Geolocation.Header.TimeListeningGpsOnlyLocationNotAttached";
+            case LOCATION_SOURCE_BATTERY_SAVING:
+                return locationAttached
+                        ? "Geolocation.Header.TimeListeningBatterySavingLocationAttached"
+                        : "Geolocation.Header.TimeListeningBatterySavingLocationNotAttached";
+            default:

[Ilya Sherman, 2016/12/15 01:45:13]

Should there be something like a NOTREACHED() assertion here?  (That's a C++-ism
-- I'm not familiar enough with Java to know what the appropriate equivalent
is.)

[pdyson, 2016/12/15 05:31:00]

I'm new to Java. Dominick, should I have something like:

throw new IllegalStateException("should not execute");

[dominickn, 2016/12/16 00:06:51]

There's no single pattern. assert false is probably the most common (but
assertions only work in pre L I believe). Exceptions are sometimes called, but
they are very destructive so I disprefer them. The other thing you could do is
Log.e that an error occurred.

+                return "";
+        }
+    }

[Ilya Sherman, 2016/12/15 01:45:13]

nit: Please leave a blank line after this one.

[pdyson, 2016/12/15 05:31:00]

Done.

+    /** Records a data point for one of the GeolocationHeader.TimeListening* histograms. */
+    private static void recordTimeListeningHistogram(
+            int locationSource, boolean locationAttached, long duration) {
+        String name = getTimeListeningHistogramEnum(locationSource, locationAttached);
+        if (name.equals("")) return;
+        duration = duration >= 0 ? duration : Long.MAX_VALUE;

[Ilya Sherman, 2016/12/15 01:45:12]

Why might the duration be less than 0?

[pdyson, 2016/12/15 05:31:00]

I've removed this test. It should not be less than zero.

This was useful when I was developing this code as it caught the case where I
did arithmetic
involving Long.MAX_VALUE, but I now explicitly test for that above, so I can
remove this
here.

+        RecordHistogram.recordCustomTimesHistogram(
+                name, duration, 1, TIME_LISTENING_HISTOGRAM_MAX_MILLIS, TimeUnit.MILLISECONDS, 50);
+    }
+
+    /** Records a data point for one of the GeolocationHeader.LocationAge* histograms. */
+    private static void recordLocationAgeHistogram(int locationSource, long duration) {
+        String name = "";
+        if (locationSource == LOCATION_SOURCE_HIGH_ACCURACY) {
+            name = "Geolocation.Header.LocationAgeHighAccuracy";
+        } else if (locationSource == LOCATION_SOURCE_GPS_ONLY) {
+            name = "Geolocation.Header.LocationAgeGpsOnly";
+        } else if (locationSource == LOCATION_SOURCE_BATTERY_SAVING) {
+            name = "Geolocation.Header.LocationAgeBatterySaving";
+        } else {
+            return;

[Ilya Sherman, 2016/12/15 01:45:13]

Ditto

+        }
+        duration = duration >= 0 ? duration : Long.MAX_VALUE;

[Ilya Sherman, 2016/12/15 01:45:13]

Ditto

[pdyson, 2016/12/15 05:31:00]

I'm removed this line. The method GeolocationTracker.getLocationAge(), which
provides the location age, already has this test.

+        RecordHistogram.recordCustomTimesHistogram(
+                name, duration, 1, LOCATION_AGE_HISTOGRAM_MAX_MILLIS, TimeUnit.MILLISECONDS, 50);
+    }
 }