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 166816 c23d29bb38fd8745427dd786262c02a0e512904b
parent 166815 517ee3c1fba5bca887eeda186fd3f60aafc81903
child 166817 24757ffc7073947d86355da0cd443241e9b7f05f
push id4837
push usermichael.l.comella@gmail.com
push dateTue, 04 Feb 2014 23:10:35 +0000
treeherderfx-team@c23d29bb38fd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcomella
bugs961526
milestone30.0a1
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,