Bug 961526 - More thorough handling of search recording events. r=mcomella
authorRichard Newman <rnewman@mozilla.com>
Mon, 20 Jan 2014 15:36:50 -0800
changeset 166929 c23d29bb38fd8745427dd786262c02a0e512904b
parent 166928 517ee3c1fba5bca887eeda186fd3f60aafc81903
child 166930 24757ffc7073947d86355da0cd443241e9b7f05f
push id26154
push usercbook@mozilla.com
push dateWed, 05 Feb 2014 12:28:27 +0000
treeherdermozilla-central@579cf46bc21e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcomella
bugs961526
milestone30.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 961526 - More thorough handling of search recording events. r=mcomella
mobile/android/base/health/BrowserHealthRecorder.java
--- a/mobile/android/base/health/BrowserHealthRecorder.java
+++ b/mobile/android/base/health/BrowserHealthRecorder.java
@@ -32,19 +32,23 @@ import org.mozilla.gecko.util.ThreadUtil
 
 import org.json.JSONException;
 import org.json.JSONObject;
 
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.OutputStreamWriter;
 import java.nio.charset.Charset;
+import java.util.Arrays;
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Scanner;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * BrowserHealthRecorder is the browser's interface to the Firefox Health
  * Report storage system. It manages environments (a collection of attributes
  * that are tracked longitudinally) on the browser's behalf, exposing a simpler
  * interface for recording changes.
  *
@@ -753,30 +757,30 @@ public class BrowserHealthRecorder imple
 
     /*
      * Searches.
      */
 
     public static final String MEASUREMENT_NAME_SEARCH_COUNTS = "org.mozilla.searches.counts";
     public static final int MEASUREMENT_VERSION_SEARCH_COUNTS = 5;
 
-    public static final String[] SEARCH_LOCATIONS = {
+    public static final Set<String> SEARCH_LOCATIONS = Collections.unmodifiableSet(new HashSet<String>(Arrays.asList(new String[] {
         "barkeyword",
         "barsuggest",
         "bartext",
-    };
+    })));
 
     private void initializeSearchProvider() {
         this.storage.ensureMeasurementInitialized(
             MEASUREMENT_NAME_SEARCH_COUNTS,
             MEASUREMENT_VERSION_SEARCH_COUNTS,
             new MeasurementFields() {
                 @Override
                 public Iterable<FieldSpec> getFields() {
-                    ArrayList<FieldSpec> out = new ArrayList<FieldSpec>(SEARCH_LOCATIONS.length);
+                    ArrayList<FieldSpec> out = new ArrayList<FieldSpec>(SEARCH_LOCATIONS.size());
                     for (String location : SEARCH_LOCATIONS) {
                         // We're not using a counter, because the set of engine
                         // identifiers is potentially unbounded, and thus our
                         // measurement version would have to keep growing as
                         // fields changed. Instead we store discrete values, and
                         // accumulate them into a counting map during processing.
                         out.add(new FieldSpec(location, Field.TYPE_COUNTED_STRING_DISCRETE));
                     }
@@ -798,29 +802,41 @@ public class BrowserHealthRecorder imple
      * @param location one of a fixed set of locations: see {@link #SEARCH_LOCATIONS}.
      */
     public void recordSearch(final String engineID, final String location) {
         if (this.state != State.INITIALIZED) {
             Log.d(LOG_TAG, "Not initialized: not recording search. (" + this.state + ")");
             return;
         }
 
+        final int env = this.env;
+
+        if (env == -1) {
+            Log.d(LOG_TAG, "No environment: not recording search.");
+            return;
+        }
+
         if (location == null) {
             throw new IllegalArgumentException("location must be provided for search.");
         }
 
+        // Ensure that we don't throw when trying to look up the field for an
+        // unknown location. If you add a search location, you must extend the
+        // list of search locations *and update the measurement version*.
+        if (!SEARCH_LOCATIONS.contains(location)) {
+            throw new IllegalArgumentException("Unexpected location: " + location);
+        }
+
         final int day = storage.getDay();
-        final int env = this.env;
         final String key = (engineID == null) ? "other" : engineID;
-        final BrowserHealthRecorder self = this;
 
         ThreadUtils.postToBackgroundThread(new Runnable() {
             @Override
             public void run() {
-                final HealthReportDatabaseStorage storage = self.storage;
+                final HealthReportDatabaseStorage storage = BrowserHealthRecorder.this.storage;
                 if (storage == null) {
                     Log.d(LOG_TAG, "No storage: not recording search. Shutting down?");
                     return;
                 }
 
                 Log.d(LOG_TAG, "Recording search: " + key + ", " + location +
                                " (" + day + ", " + env + ").");
                 final int searchField = storage.getField(MEASUREMENT_NAME_SEARCH_COUNTS,