Bug 925517 - Remove filter on recorded search engine identifiers for Fennec. r=mcomella, a=bajaj
authorRichard Newman <rnewman@mozilla.com>
Fri, 18 Oct 2013 13:13:37 -0700
changeset 166401 be1b5053297025aae1d02f29490d0fbaa16fac40
parent 166400 cbd566346605bd899f4ae675d61219a1fd96d436
child 166402 05e8967817e7d592f640592c741d9110208a747e
push id3066
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 19:58:46 +0000
treeherdermozilla-beta@a31a0dce83aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcomella, bajaj
bugs925517
milestone27.0a2
Bug 925517 - Remove filter on recorded search engine identifiers for Fennec. r=mcomella, a=bajaj
mobile/android/base/BrowserApp.java
mobile/android/base/health/BrowserHealthRecorder.java
mobile/android/base/home/BrowserSearch.java
mobile/android/base/home/SearchEngine.java
mobile/android/base/home/SearchEngineRow.java
--- a/mobile/android/base/BrowserApp.java
+++ b/mobile/android/base/BrowserApp.java
@@ -14,16 +14,17 @@ import org.mozilla.gecko.favicons.LoadFa
 import org.mozilla.gecko.gfx.BitmapUtils;
 import org.mozilla.gecko.gfx.GeckoLayerClient;
 import org.mozilla.gecko.gfx.ImmutableViewportMetrics;
 import org.mozilla.gecko.health.BrowserHealthRecorder;
 import org.mozilla.gecko.health.BrowserHealthReporter;
 import org.mozilla.gecko.home.BrowserSearch;
 import org.mozilla.gecko.home.HomePager;
 import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
+import org.mozilla.gecko.home.SearchEngine;
 import org.mozilla.gecko.menu.GeckoMenu;
 import org.mozilla.gecko.prompts.Prompt;
 import org.mozilla.gecko.util.Clipboard;
 import org.mozilla.gecko.util.GamepadUtils;
 import org.mozilla.gecko.util.HardwareUtils;
 import org.mozilla.gecko.util.StringUtils;
 import org.mozilla.gecko.util.ThreadUtils;
 import org.mozilla.gecko.util.UiAsyncTask;
@@ -1486,25 +1487,28 @@ abstract public class BrowserApp extends
                 Tabs.getInstance().loadUrl(searchUrl, Tabs.LOADURL_USER_ENTERED);
             }
         });
     }
 
     /**
      * Record in Health Report that a search has occurred.
      *
-     * @param identifier
-     *        a search identifier, such as "partnername". Can be null.
+     * @param engine
+     *        a search engine instance. Can be null.
      * @param where
      *        where the search was initialized; one of the values in
      *        {@link BrowserHealthRecorder#SEARCH_LOCATIONS}.
      */
-    private static void recordSearch(String identifier, String where) {
-        Log.i(LOGTAG, "Recording search: " + identifier + ", " + where);
+    private static void recordSearch(SearchEngine engine, String where) {
+        Log.i(LOGTAG, "Recording search: " +
+                      ((engine == null) ? "null" : engine.name) +
+                      ", " + where);
         try {
+            String identifier = (engine == null) ? "other" : engine.getEngineIdentifier();
             JSONObject message = new JSONObject();
             message.put("type", BrowserHealthRecorder.EVENT_SEARCH);
             message.put("location", where);
             message.put("identifier", identifier);
             GeckoAppShell.getEventDispatcher().dispatchEvent(message);
         } catch (Exception e) {
             Log.w(LOGTAG, "Error recording search.", e);
         }
@@ -2346,19 +2350,19 @@ abstract public class BrowserApp extends
     public void onUrlOpen(String url, EnumSet<OnUrlOpenListener.Flags> flags) {
         if (!maybeSwitchToTab(url, flags)) {
             openUrl(url);
         }
     }
 
     // BrowserSearch.OnSearchListener
     @Override
-    public void onSearch(String engineId, String text) {
-        recordSearch(engineId, "barsuggest");
-        openUrl(text, engineId);
+    public void onSearch(SearchEngine engine, String text) {
+        recordSearch(engine, "barsuggest");
+        openUrl(text, engine.name);
     }
 
     // BrowserSearch.OnEditSuggestionListener
     @Override
     public void onEditSuggestion(String suggestion) {
         mBrowserToolbar.onEditSuggestion(suggestion);
     }
 
--- a/mobile/android/base/health/BrowserHealthRecorder.java
+++ b/mobile/android/base/health/BrowserHealthRecorder.java
@@ -736,102 +736,24 @@ 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 = 4;
+    public static final int MEASUREMENT_VERSION_SEARCH_COUNTS = 5;
 
     public static final String[] SEARCH_LOCATIONS = {
         "barkeyword",
         "barsuggest",
         "bartext",
     };
 
-    // See services/healthreport/providers.jsm. Sorry for the duplication.
-    // THIS LIST MUST BE SORTED per java.lang.Comparable<String>.
-    private static final String[] SEARCH_PROVIDERS = {
-        "amazon-co-uk",
-        "amazon-de",
-        "amazon-en-GB",
-        "amazon-france",
-        "amazon-it",
-        "amazon-jp",
-        "amazondotcn",
-        "amazondotcom",
-        "amazondotcom-de",
-
-        "aol-en-GB",
-        "aol-web-search",
-
-        "bing",
-
-        "eBay",
-        "eBay-de",
-        "eBay-en-GB",
-        "eBay-es",
-        "eBay-fi",
-        "eBay-france",
-        "eBay-hu",
-        "eBay-in",
-        "eBay-it",
-
-        "google",
-        "google-jp",
-        "google-ku",
-        "google-maps-zh-TW",
-
-        "mailru",
-
-        "mercadolibre-ar",
-        "mercadolibre-cl",
-        "mercadolibre-mx",
-
-        "seznam-cz",
-
-        "twitter",
-        "twitter-de",
-        "twitter-ja",
-
-        "wikipedia",            // Manually added.
-
-        "yahoo",
-        "yahoo-NO",
-        "yahoo-answer-zh-TW",
-        "yahoo-ar",
-        "yahoo-bid-zh-TW",
-        "yahoo-br",
-        "yahoo-ch",
-        "yahoo-cl",
-        "yahoo-de",
-        "yahoo-en-GB",
-        "yahoo-es",
-        "yahoo-fi",
-        "yahoo-france",
-        "yahoo-fy-NL",
-        "yahoo-id",
-        "yahoo-in",
-        "yahoo-it",
-        "yahoo-jp",
-        "yahoo-jp-auctions",
-        "yahoo-mx",
-        "yahoo-sv-SE",
-        "yahoo-zh-TW",
-
-        "yandex",
-        "yandex-ru",
-        "yandex-slovari",
-        "yandex-tr",
-        "yandex.by",
-        "yandex.ru-be",
-    };
-
     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);
@@ -850,51 +772,34 @@ public class BrowserHealthRecorder imple
         // Do this here, rather than in a centralized registration spot, in
         // case the above throws and we wind up handling events that we can't
         // store.
         this.dispatcher.registerEventListener(EVENT_KEYWORD_SEARCH, this);
         this.dispatcher.registerEventListener(EVENT_SEARCH, this);
     }
 
     /**
-     * Return the field key for the search provider. This turns null and
-     * non-partner providers into "other".
-     *
-     * @param engine an engine identifier, such as "yandex"
-     * @return the key to use, such as "other" or "yandex".
-     */
-    protected String getEngineKey(final String engine) {
-        if (engine == null) {
-            return "other";
-        }
-
-        // This is inefficient. Optimize if necessary.
-        boolean found = (0 <= java.util.Arrays.binarySearch(SEARCH_PROVIDERS, engine));
-        return found ? engine : "other";
-    }
-
-    /**
      * Record a search.
      *
-     * @param engine the string identifier for the engine, or null if it's not a partner.
+     * @param engineID the string identifier for the engine. Can be <code>null</code>.
      * @param location one of a fixed set of locations: see {@link #SEARCH_LOCATIONS}.
      */
-    public void recordSearch(final String engine, final String location) {
+    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;
         }
 
         if (location == null) {
             throw new IllegalArgumentException("location must be provided for search.");
         }
 
         final int day = storage.getDay();
         final int env = this.env;
-        final String key = getEngineKey(engine);
+        final String key = (engineID == null) ? "other" : engineID;
         final BrowserHealthRecorder self = this;
 
         ThreadUtils.postToBackgroundThread(new Runnable() {
             @Override
             public void run() {
                 final HealthReportDatabaseStorage storage = self.storage;
                 if (storage == null) {
                     Log.d(LOG_TAG, "No storage: not recording search. Shutting down?");
--- a/mobile/android/base/home/BrowserSearch.java
+++ b/mobile/android/base/home/BrowserSearch.java
@@ -8,18 +8,18 @@ package org.mozilla.gecko.home;
 import org.mozilla.gecko.AutocompleteHandler;
 import org.mozilla.gecko.GeckoAppShell;
 import org.mozilla.gecko.GeckoEvent;
 import org.mozilla.gecko.PrefsHelper;
 import org.mozilla.gecko.R;
 import org.mozilla.gecko.Tab;
 import org.mozilla.gecko.Tabs;
 import org.mozilla.gecko.db.BrowserDB.URLColumns;
-import org.mozilla.gecko.gfx.BitmapUtils;
 import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
+import org.mozilla.gecko.home.SearchEngine;
 import org.mozilla.gecko.home.SearchLoader.SearchCursorLoader;
 import org.mozilla.gecko.util.GeckoEventListener;
 import org.mozilla.gecko.util.StringUtils;
 import org.mozilla.gecko.util.ThreadUtils;
 
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
@@ -127,17 +127,17 @@ public class BrowserSearch extends HomeF
 
     // Whether the suggestions will fade in when shown
     private boolean mAnimateSuggestions;
 
     // Opt-in prompt view for search suggestions
     private View mSuggestionsOptInPrompt;
 
     public interface OnSearchListener {
-        public void onSearch(String engineId, String text);
+        public void onSearch(SearchEngine engine, String text);
     }
 
     public interface OnEditSuggestionListener {
         public void onEditSuggestion(String suggestion);
     }
 
     public static BrowserSearch newInstance() {
         BrowserSearch browserSearch = new BrowserSearch();
@@ -388,17 +388,17 @@ public class BrowserSearch extends HomeF
         if (mSuggestionLoaderCallbacks == null) {
             mSuggestionLoaderCallbacks = new SuggestionLoaderCallbacks();
         }
 
         getLoaderManager().restartLoader(LOADER_ID_SUGGESTION, null, mSuggestionLoaderCallbacks);
     }
 
     private void setSuggestions(ArrayList<String> suggestions) {
-        mSearchEngines.get(0).suggestions = suggestions;
+        mSearchEngines.get(0).setSuggestions(suggestions);
         mAdapter.notifyDataSetChanged();
     }
 
     private void setSearchEngines(JSONObject data) {
         // This method is called via a Runnable posted from the Gecko thread, so
         // it's possible the fragment and/or its view has been destroyed by the
         // time we get here. If so, just abort.
         if (mView == null) {
@@ -412,41 +412,38 @@ public class BrowserSearch extends HomeF
             final boolean suggestionsPrompted = suggest.getBoolean("prompted");
             final JSONArray engines = data.getJSONArray("searchEngines");
 
             mSuggestionsEnabled = suggest.getBoolean("enabled");
 
             ArrayList<SearchEngine> searchEngines = new ArrayList<SearchEngine>();
             for (int i = 0; i < engines.length(); i++) {
                 final JSONObject engineJSON = engines.getJSONObject(i);
-                final String name = engineJSON.getString("name");
-                final String identifier = engineJSON.getString("identifier");
-                final String iconURI = engineJSON.getString("iconURI");
-                final Bitmap icon = BitmapUtils.getBitmapFromDataURI(iconURI);
+                final SearchEngine engine = new SearchEngine(engineJSON);
 
-                if (name.equals(suggestEngine) && suggestTemplate != null) {
-                    // Suggest engine should be at the front of the list
-                    searchEngines.add(0, new SearchEngine(name, identifier, icon));
+                if (engine.name.equals(suggestEngine) && suggestTemplate != null) {
+                    // Suggest engine should be at the front of the list.
+                    searchEngines.add(0, engine);
 
                     // The only time Tabs.getInstance().getSelectedTab() should
                     // be null is when we're restoring after a crash. We should
                     // never restore private tabs when that happens, so it
                     // should be safe to assume that null means non-private.
                     Tab tab = Tabs.getInstance().getSelectedTab();
                     final boolean isPrivate = (tab != null && tab.isPrivate());
 
                     // Only create a new instance of SuggestClient if it hasn't been
                     // set yet. e.g. Robocop tests might set it directly before search
                     // engines are loaded.
                     if (mSuggestClient == null && !isPrivate) {
                         mSuggestClient = new SuggestClient(getActivity(), suggestTemplate,
                                 SUGGESTION_TIMEOUT, SUGGESTION_MAX);
                     }
                 } else {
-                    searchEngines.add(new SearchEngine(name, identifier, icon));
+                    searchEngines.add(engine);
                 }
             }
 
             mSearchEngines = searchEngines;
 
             if (mAdapter != null) {
                 mAdapter.notifyDataSetChanged();
             }
@@ -708,17 +705,17 @@ public class BrowserSearch extends HomeF
             }
 
             // If the suggestion row only contains one item (the user-entered
             // query), allow the entire row to be clickable; clicking the row
             // has the same effect as clicking the single suggestion. If the
             // row contains multiple items, clicking the row will do nothing.
             final int index = getEngineIndex(position);
             if (index != -1) {
-                return mSearchEngines.get(index).suggestions.isEmpty();
+                return !mSearchEngines.get(index).hasSuggestions();
             }
 
             return true;
         }
 
         // Add the search engines to the number of reported results.
         @Override
         public int getCount() {
@@ -739,17 +736,17 @@ public class BrowserSearch extends HomeF
             if (type == ROW_SEARCH || type == ROW_SUGGEST) {
                 final SearchEngineRow row = (SearchEngineRow) view;
                 row.setOnUrlOpenListener(mUrlOpenListener);
                 row.setOnSearchListener(mSearchListener);
                 row.setOnEditSuggestionListener(mEditSuggestionListener);
                 row.setSearchTerm(mSearchTerm);
 
                 final SearchEngine engine = mSearchEngines.get(getEngineIndex(position));
-                final boolean animate = (mAnimateSuggestions && engine.suggestions.size() > 0);
+                final boolean animate = (mAnimateSuggestions && engine.hasSuggestions());
                 row.updateFromSearchEngine(engine, animate);
                 if (animate) {
                     // Only animate suggestions the first time they are shown
                     mAnimateSuggestions = false;
                 }
             } else {
                 // Account for the search engines
                 position -= getSuggestEngineCount();
--- a/mobile/android/base/home/SearchEngine.java
+++ b/mobile/android/base/home/SearchEngine.java
@@ -1,29 +1,93 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.home;
 
+import org.mozilla.gecko.gfx.BitmapUtils;
+
+import org.json.JSONException;
+import org.json.JSONObject;
+
 import android.graphics.Bitmap;
+import android.util.Log;
 
 import java.util.ArrayList;
+import java.util.List;
 
-class SearchEngine {
-    public String name;
-    public String identifier;
-    public Bitmap icon;
-    public ArrayList<String> suggestions;
+public class SearchEngine {
+    public static final String LOG_TAG = "GeckoSearchEngine";
+
+    public final String name;             // Never null.
+    public final String identifier;       // Can be null.
+
+    private final Bitmap icon;
+    private volatile List<String> suggestions = new ArrayList<String>();   // Never null.
+
+    public SearchEngine(JSONObject engineJSON) throws JSONException {
+        if (engineJSON == null) {
+            throw new IllegalArgumentException("Can't instantiate SearchEngine from null JSON.");
+        }
 
-    public SearchEngine(String name, String identifier) {
-        this(name, identifier, null);
+        this.name = getString(engineJSON, "name");
+        if (this.name == null) {
+            throw new IllegalArgumentException("Cannot have an unnamed search engine.");
+        }
+
+        this.identifier = getString(engineJSON, "identifier");
+
+        final String iconURI = getString(engineJSON, "iconURI");
+        if (iconURI == null) {
+            Log.w(LOG_TAG, "iconURI is null for search engine " + this.name);
+            this.icon = null;
+            return;
+        }
+        this.icon = BitmapUtils.getBitmapFromDataURI(iconURI);
     }
 
-    public SearchEngine(String name, String identifier, Bitmap icon) {
-        this.name = name;
-        this.identifier = identifier;
-        this.icon = icon;
-        this.suggestions = new ArrayList<String>();
+    private static String getString(JSONObject data, String key) throws JSONException {
+        if (data.isNull(key)) {
+            return null;
+        }
+        return data.getString(key);
+    }
+
+    /**
+     * @return a non-null string suitable for use by FHR.
+     */
+    public String getEngineIdentifier() {
+        if (this.identifier != null) {
+            return this.identifier;
+        }
+        if (this.name != null) {
+            return "other-" + this.name;
+        }
+        return "other";
+    }
+
+    public boolean hasSuggestions() {
+        return !this.suggestions.isEmpty();
+    }
+
+    public int getSuggestionsCount() {
+        return this.suggestions.size();
+    }
+
+    public Iterable<String> getSuggestions() {
+        return this.suggestions;
+    }
+
+    public void setSuggestions(List<String> suggestions) {
+        if (suggestions == null) {
+            this.suggestions = new ArrayList<String>();
+            return;
+        }
+        this.suggestions = suggestions;
+    }
+
+    public Bitmap getIcon() {
+        return this.icon;
     }
 }
 
--- a/mobile/android/base/home/SearchEngineRow.java
+++ b/mobile/android/base/home/SearchEngineRow.java
@@ -77,17 +77,17 @@ class SearchEngineRow extends AnimatedHe
                 // If we're not clicking the user-entered view (the first suggestion item)
                 // and the search matches a URL pattern, go to that URL. Otherwise, do a
                 // search for the term.
                 if (v != mUserEnteredView && !StringUtils.isSearchQuery(suggestion, false)) {
                     if (mUrlOpenListener != null) {
                         mUrlOpenListener.onUrlOpen(suggestion, EnumSet.noneOf(OnUrlOpenListener.Flags.class));
                     }
                 } else if (mSearchListener != null) {
-                    mSearchListener.onSearch(mSearchEngine.name, suggestion);
+                    mSearchListener.onSearch(mSearchEngine, suggestion);
                 }
             }
         };
 
         mLongClickListener = new OnLongClickListener() {
             @Override
             public boolean onLongClick(View v) {
                 if (mEditSuggestionListener != null) {
@@ -130,17 +130,17 @@ class SearchEngineRow extends AnimatedHe
     }
 
     /**
      * Perform a search for the user-entered term.
      */
     public void performUserEnteredSearch() {
         String searchTerm = getSuggestionTextFromView(mUserEnteredView);
         if (mSearchListener != null) {
-            mSearchListener.onSearch(mSearchEngine.name, searchTerm);
+            mSearchListener.onSearch(mSearchEngine, searchTerm);
         }
     }
 
     public void setSearchTerm(String searchTerm) {
         mUserEnteredTextView.setText(searchTerm);
 
         // mSearchEngine is not set in the first call to this method; the content description
         // is instead initially set in updateFromSearchEngine.
@@ -157,66 +157,67 @@ class SearchEngineRow extends AnimatedHe
         mSearchListener = listener;
     }
 
     public void setOnEditSuggestionListener(OnEditSuggestionListener listener) {
         mEditSuggestionListener = listener;
     }
 
     public void updateFromSearchEngine(SearchEngine searchEngine, boolean animate) {
-        // Update search engine reference
+        // Update search engine reference.
         mSearchEngine = searchEngine;
 
-        // Set the search engine icon (e.g., Google) for the row
-        mIconView.updateAndScaleImage(mSearchEngine.icon, mSearchEngine.name);
+        // Set the search engine icon (e.g., Google) for the row.
+        mIconView.updateAndScaleImage(mSearchEngine.getIcon(), mSearchEngine.getEngineIdentifier());
 
-        // Set the initial content description
+        // Set the initial content description.
         setDescriptionOnSuggestion(mUserEnteredTextView, mUserEnteredTextView.getText().toString());
 
-        // Add additional suggestions given by this engine
+        // Add additional suggestions given by this engine.
         final int recycledSuggestionCount = mSuggestionView.getChildCount();
-        final int suggestionCount = mSearchEngine.suggestions.size();
 
-        for (int i = 0; i < suggestionCount; i++) {
+        int suggestionCounter = 0;
+        for (String suggestion : mSearchEngine.getSuggestions()) {
             final View suggestionItem;
 
-            // Reuse suggestion views from recycled view, if possible
-            if (i + 1 < recycledSuggestionCount) {
-                suggestionItem = mSuggestionView.getChildAt(i + 1);
+            // Reuse suggestion views from recycled view, if possible.
+            if (suggestionCounter + 1 < recycledSuggestionCount) {
+                suggestionItem = mSuggestionView.getChildAt(suggestionCounter + 1);
                 suggestionItem.setVisibility(View.VISIBLE);
             } else {
                 suggestionItem = mInflater.inflate(R.layout.suggestion_item, null);
 
                 suggestionItem.setOnClickListener(mClickListener);
                 suggestionItem.setOnLongClickListener(mLongClickListener);
 
                 final ImageView magnifier =
                         (ImageView) suggestionItem.findViewById(R.id.suggestion_magnifier);
                 magnifier.setVisibility(View.GONE);
 
                 mSuggestionView.addView(suggestionItem);
             }
 
-            final String suggestion = mSearchEngine.suggestions.get(i);
             setSuggestionOnView(suggestionItem, suggestion);
 
             if (animate) {
                 AlphaAnimation anim = new AlphaAnimation(0, 1);
                 anim.setDuration(ANIMATION_DURATION);
-                anim.setStartOffset(i * ANIMATION_DURATION);
+                anim.setStartOffset(suggestionCounter * ANIMATION_DURATION);
                 suggestionItem.startAnimation(anim);
             }
+
+            ++suggestionCounter;
         }
 
-        // Hide extra suggestions that have been recycled
-        for (int i = suggestionCount + 1; i < recycledSuggestionCount; i++) {
+        // Hide extra suggestions that have been recycled.
+        for (int i = suggestionCounter + 1; i < recycledSuggestionCount; ++i) {
             mSuggestionView.getChildAt(i).setVisibility(View.GONE);
         }
 
-        // Make sure mSelectedView is still valid
+        // Make sure mSelectedView is still valid.
         if (mSelectedView >= mSuggestionView.getChildCount()) {
             mSelectedView = mSuggestionView.getChildCount() - 1;
         }
     }
 
     @Override
     public boolean onKeyDown(int keyCode, android.view.KeyEvent event) {
         final View suggestion = mSuggestionView.getChildAt(mSelectedView);