1042943 - Allow users to switch search engine

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[:Margaret Leibovic]

Bug 1022105 was filed about adding a menu and settings, but supporting multiple search engines will also require some plumbing work. Let's do that in here.

I think we should continue to keep the search activity search provider logic separate from Fennec's, but if we continue down that road, we would eventually need to build our own support for OpenSearch plugins.

For this bug, we should just generalize the search suggestion and search result logic so that we can change the URLs they query. This would also help address bug 1039758.

Comment 1

[Eric Edens [:eedens]]

Attachment: bug-1042943-wip.patch

What do you think of this approach?

Adding a new search engine requires implementing the SearchEngine interface. There is a sample implementation for Yahoo.

Looking forward, we want to load search engines from Gecko's XML OpenSearch files. In that case, we wouldn't define concrete classes. Instead, we would have a factory that would return an instance of SearchEngine that was built from the XML file.

Comment 2

[:Margaret Leibovic]

Comment on attachment 8461193 [details] [diff] [review]
bug-1042943-wip.patch

Review of attachment 8461193 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! This is a pretty simple way to solve our immediate issue of supporting a few different search engines. And with this interface in place, we can easily switch to using OpenSearch plugins without needing to change any of our UI code.

Bonus points if you also update SearchFragment to use this new SearchEngine interface for the suggestions URL as well.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +29,2 @@
>      private static final String HIDE_BANNER_SCRIPT = "javascript:(function(){var tag=document.createElement('style');" +
> +            "tag.type='text/css';document.getElementsByTagName('head')[0].appendChild(tag);tag.innerText='%s'})();";

A comment above this would be good explaining the role of the %s.

@@ +87,5 @@
>  
>          @Override
>          public void onReceivedTitle(WebView view, String title) {
>              super.onReceivedTitle(view, title);
> +            view.loadUrl(String.format(HIDE_BANNER_SCRIPT, searchEngine.getInjectableCss()));

Maybe we should move the HIDE_BANNER_SCRIPT declaration to inside this class, since it's very specific to injecting styles.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
@@ +33,5 @@
> +    /**
> +     * Transform a search query to a URL that can be loaded
> +     * in the webview.
> +     */
> +    public String urlForQuery(String query, Facet facet);

I don't think we should worry about facets in this bug, although it is cool to see how we can expand this interface to support them.

::: mobile/android/search/java/org/mozilla/search/providers/Yahoo.java
@@ +7,5 @@
> +import android.net.Uri;
> +
> +import org.mozilla.search.Constants;
> +
> +public class Yahoo implements SearchEngine {

I would maybe call this YahooSearchEngine, just to make it clearer what this is.

@@ +20,5 @@
> +    }
> +
> +    @Override
> +    public boolean isExitUrl(String url) {
> +        return !url.contains(Constants.YAHOO_WEB_SEARCH_RESULTS_FILTER);

You should move the Yahoo-specific constants out of Constants.java and into this class instead.

Comment 3

[Eric Edens [:eedens]]

Attachment: bug-1042943-wip.v2.patch

Comment 4

[:Margaret Leibovic]

Comment on attachment 8468016 [details] [diff] [review]
bug-1042943-wip.v2.patch

Review of attachment 8468016 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! I'm still a bit concerned about the racy initialization logic, but overall I like where this is heading.

I want lucasr to take a look at the SharedPreferences logic, since he worked on GeckoSharedPrefs. He may have some good advice for best practices.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +66,5 @@
> +        if (searchEngine != null) {
> +            setUrl(searchEngine.resultsUriForQuery(query));
> +        } else {
> +            Log.e(LOG_TAG, "Unable to start search. Default search engine not initialized yet.");
> +        }

It feels a bit fragile that we're waiting on some external call to set the search engine, and that we'll just drop any searches that are made before that call happens. It's one thing to drop search suggestions, but if this fails the app will seem really broken.

As I mentioned before, I think we could get around this by using some sort of callback interface for getting the search engine.

So, if we had a reference to a SearchEngineManager instance in here, we could do something like:

manager.getSearchEngine(new SearchEngineCallback() {
  @Override
  public void onEngineFound(SearchEngine engine) {
    setUrl(searchEngine.resultsUriForQuery(query);
  }
});

Then, in most cases, this would be a fast synchronous call, since the manager would already have a cached value for the current engine, but the first time this is called we would get the value from SharedPreferences.

The downside of this approach is that as a consumer you don't really know when onEngineFound will be called, so even if it is really fast most of the time, there is some uncertainty.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchFragment.java
@@ +39,5 @@
>   * world should be very very limited.
>   * <p/>
>   * TODO: Add more search providers (other than the dictionary)
>   */
>  public class SearchFragment extends Fragment implements AcceptsJumpTaps {

This will need some rebasing on top of my patch for bug 1046485. Sorry!

@@ +231,5 @@
>          editText.setActive(true);
>          suggestionDropdown.setVisibility(View.VISIBLE);
>      }
>  
> +    public void setSearchEngine(SearchEngine defaultEngine) {

Nit: I would just call this `searchEngine` or `engine`.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
@@ +31,5 @@
> +        return String.format(HIDE_BANNER_SCRIPT, getInjectableCss());
> +    }
> +
> +    /**
> +     * Return whether this URL should be opened in an

Nit: Use the javadoc-style @return notation.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +26,5 @@
> +        YAHOO
> +    }
> +
> +    public SearchEngineManager(Context appContext, SearchEngineChangeListener changeListener) {
> +        this.appContext = appContext;

I like that you're making it clear what context is expected here.

@@ +42,5 @@
> +    private void fetchDefaultSearchEngine() {
> +        final AsyncTask<Void, Void, String> task = new AsyncTask<Void, Void, String>() {
> +            @Override
> +            protected String doInBackground(Void... params) {
> +                final SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(appContext);

I don't think we want to be using getDefaultSharedPreferences. Did you look into GeckoSharedPrefs? We introduced that to better scope our preferences.

@@ +48,5 @@
> +            }
> +
> +            @Override
> +            protected void onPostExecute(String engineName) {
> +                if (engineName != null) {

If this is null, shouldn't we still set the current engine to some default?

@@ +56,5 @@
> +        };
> +        task.execute();
> +    }
> +
> +    private void setSearchEngine(String engineName) {

Nit: I would call this `setCurrentEngine`.

@@ +66,5 @@
> +                currentEngine = new Google();
> +                break;
> +            case YAHOO:
> +                currentEngine = new Yahoo();
> +                break;

We should add a default case here that throws an exception, since that would mean that we got some unexpected engine name.

::: mobile/android/search/res/xml/search_preferences.xml
@@ +13,5 @@
> +        android:dialogTitle="@string/pref_searchProvider_title"
> +        android:summary="%s"
> +        android:entries="@array/pref_searchProvider_entries"
> +        android:entryValues="@array/pref_searchProvider_values"
> +        android:defaultValue="@string/pref_searchProvider_default" />

I don't think we should make the default value a localized string, since I imagine this will correspond to some hard-coded default value in SearchEngineManager.

I think we should file a follow-up bug that's specifically about letting different locales specify different search engines, and that would also include letting them choose different defaults.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8468016 [details] [diff] [review]
bug-1042943-wip.v2.patch

Review of attachment 8468016 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +63,5 @@
>      }
>  
> +    public void startSearch(String query) {
> +        if (searchEngine != null) {
> +            setUrl(searchEngine.resultsUriForQuery(query));

IRC discussion: rather than having a searchEngine reference here, ask SearchEngineManager to do the work.

This will probably require you to define a delegate interface for SEM, so it can tell the UI when there's an icon, a name, whatever, and do that each time it changes.

Comment 7

[Eric Edens [:eedens]]

Attachment: bug-1042943-wip.v3.patch

Incorporated richard's feedback -- the SearchEngineMediator is responsible for delegating the actual requests to the proper search engine. 

Additionally, there is SearchEngineChangeObserver which listens for changes to the underlying shared prefs, and then broadcasts the change event to a listener. This is used by SuggestionsFragment and SearchEngineMediator.

Comment 8

[Eric Edens [:eedens]]

Attachment: bug-1042943-wip.v4.patch

- Gracefully handle when shared prefs doesn't have a search engine (use default value)

Comment 9

[:Margaret Leibovic]

Comment on attachment 8471095 [details] [diff] [review]
bug-1042943-wip.v4.patch

Review of attachment 8471095 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is too complicated. I'm worried that we may have lost sight of the goal here - we just want to be able to change the search engine.

I would prefer to go back to having a single SearchEngineManager class that has listens for changes to preferences and has APIs for consumers to get the things they need to know about the default search engine, as well as listen for changes to the default search engine. Your last patch was pretty close to what we wanted - we just needed a way to deal with the asynchronous initialization.

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +37,5 @@
>  
>      private static final String KEY_SEARCH_STATE = "search_state";
>      private static final String KEY_EDIT_STATE = "edit_state";
>      private static final String KEY_QUERY = "query";
> +    private static final String LOG_TAG = "MainActivity";

This is unused.

@@ +154,5 @@
>      @Override
>      protected void onDestroy() {
>          super.onDestroy();
> +
> +        searchEngineMediator = null;

Where is this initialized?

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +55,3 @@
>          progressBar = null;
> +
> +        if (webview != null) {

When would webview be null?

::: mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java
@@ +52,5 @@
>  
>      @Override
> +    protected void onResume() {
> +        super.onResume();
> +        PreferenceManager.getDefaultSharedPreferences(this).registerOnSharedPreferenceChangeListener(this);

You still haven't addressed my feedback to use GeckoSharedPrefs.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SuggestClient.java
@@ +26,5 @@
>  public class SuggestClient {
>      private static final String LOGTAG = "GeckoSuggestClient";
>      private static final String USER_AGENT = "";
>  
> +    public static final String GECKO_SEARCH_TERMS_URL_PARAM = "__searchTerms__";

Let's not change SuggestClient here. Let's fix bug 1048525 first, then make changes in the main SuggestClient.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java
@@ +189,5 @@
> +            if (suggestClient != null) {
> +                return new SuggestionAsyncLoader(getActivity(), suggestClient, args.getString(KEY_SEARCH_TERM));
> +            } else {
> +                Log.e(LOG_TAG, "Autocomplete setup failed; suggestClient not ready yet.");
> +                return null;

This still has the same racy initialization problem. I still think we need some sort of asynchronous "get me the default search engine"/"get me whatever I need to know about the search engine" API.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineChangeObserver.java
@@ +26,5 @@
> +        PreferenceManager.getDefaultSharedPreferences(appContext)
> +                .registerOnSharedPreferenceChangeListener(this);
> +    }
> +
> +    public interface SearchEngineChangeListener {

It is confusing that there is both a SearchEngineChangeListener and a SearchEngineChangeObserver interface.

Instead of making this SearchEngineChangeObserver class, I think you should just make this all part of SearchEngineMediator.

@@ +43,5 @@
> +     *
> +     * @param onSuccess a Runnable to be called after successfully looking up the search engine
> +     *                  and notifying the listener. This will run on the UI thread.
> +     */
> +    public void refresh(final Runnable onSuccess) {

I think a better API would be to create a callback interface that has a method that uses the engine name as a parameter.

@@ +55,5 @@
> +            @Override
> +            protected void onPostExecute(String engineName) {
> +                if (searchEngineChanged(engineName) && onSuccess != null) {
> +                    onSuccess.run();
> +                }

If you're not going to always call onSuccess, you should also have an onError callback. This current implementation doesn't give consumers the opportunity to do anything if this fails.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineFactory.java
@@ +5,5 @@
> +package org.mozilla.search.providers;
> +
> +import org.mozilla.search.Constants;
> +
> +public class SearchEngineFactory {

Let's add some class comments explaining what all these new things are for.

@@ +13,5 @@
> +        GOOGLE,
> +        YAHOO
> +    }
> +
> +    public static SearchEngine getInstance(String engineName) throws ClassNotFoundException {

The `getInstance` method name is usually reserved for getting an instance of the class you're calling the method on. I think something like `createSearchEngine` would be a better name for this.

@@ +20,5 @@
> +        }
> +        return getInstance(Engine.valueOf(engineName));
> +    }
> +
> +    public static SearchEngine getInstance(Engine engine) throws ClassNotFoundException {

I don't see this method used anywhere. I think we could get rid of this second signature and just put this method body inside the first `getInstance` declaration.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineMediator.java
@@ +38,5 @@
> +        }
> +        browserDriver.loadInternalUrl(currentEngine.resultsUriForQuery(query));
> +    }
> +
> +    public void htmlTitleReady() {

Why does something that manages search engines need to know about html titles? It feels like something is going wrong with the abstraction layers here.

Comment 10

[Eric Edens [:eedens]]

Thanks for the feedback!

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 8471095 [details] [diff] [review]
> bug-1042943-wip.v4.patch
> 
> Review of attachment 8471095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is too complicated. I'm worried that we may have lost sight of
> the goal here - we just want to be able to change the search engine.
> 
> I would prefer to go back to having a single SearchEngineManager class that
> has listens for changes to preferences and has APIs for consumers to get the
> things they need to know about the default search engine, as well as listen
> for changes to the default search engine. Your last patch was pretty close
> to what we wanted - we just needed a way to deal with the asynchronous
> initialization.

I'll rollback to the previous patch.

> ::: mobile/android/search/java/org/mozilla/search/MainActivity.java
> @@ +37,5 @@
> >  
> >      private static final String KEY_SEARCH_STATE = "search_state";
> >      private static final String KEY_EDIT_STATE = "edit_state";
> >      private static final String KEY_QUERY = "query";
> > +    private static final String LOG_TAG = "MainActivity";
> 
> This is unused.

Good catch!

> 
> @@ +154,5 @@
> >      @Override
> >      protected void onDestroy() {
> >          super.onDestroy();
> > +
> > +        searchEngineMediator = null;
> 
> Where is this initialized?

Good catch -- dead code from a previous patch.

> ::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
> @@ +55,3 @@
> >          progressBar = null;
> > +
> > +        if (webview != null) {
> 
> When would webview be null?

Good catch.

> :::
> mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java
> @@ +52,5 @@
> >  
> >      @Override
> > +    protected void onResume() {
> > +        super.onResume();
> > +        PreferenceManager.getDefaultSharedPreferences(this).registerOnSharedPreferenceChangeListener(this);
> 
> You still haven't addressed my feedback to use GeckoSharedPrefs.

My mistake -- I thought it was "nice to do" and should be done sometime in the future. But looking through the comments, I can't find where I got that impression. I'll make sure to have it in the next patch.

> :::
> mobile/android/search/java/org/mozilla/search/autocomplete/SuggestClient.java
> @@ +26,5 @@
> >  public class SuggestClient {
> >      private static final String LOGTAG = "GeckoSuggestClient";
> >      private static final String USER_AGENT = "";
> >  
> > +    public static final String GECKO_SEARCH_TERMS_URL_PARAM = "__searchTerms__";
> 
> Let's not change SuggestClient here. Let's fix bug 1048525 first, then make
> changes in the main SuggestClient.

Fixed bug 1048525

> :::
> mobile/android/search/java/org/mozilla/search/autocomplete/
> SuggestionsFragment.java
> @@ +189,5 @@
> > +            if (suggestClient != null) {
> > +                return new SuggestionAsyncLoader(getActivity(), suggestClient, args.getString(KEY_SEARCH_TERM));
> > +            } else {
> > +                Log.e(LOG_TAG, "Autocomplete setup failed; suggestClient not ready yet.");
> > +                return null;
> 
> This still has the same racy initialization problem. I still think we need
> some sort of asynchronous "get me the default search engine"/"get me
> whatever I need to know about the search engine" API.

In general, the async API that you've described sounds fine.

In this particular case, async won't work, since `onCreateLoader` is expecting an immediate return value. Here are the options that we talked about offline, and I thought we agreed on doing #2. No?

1. Make a blocking call to init suggestClient
2. Drop the autocomplete search (this is being done above)
3. Fallback onto the default search engine.

> :::
> mobile/android/search/java/org/mozilla/search/providers/
> SearchEngineChangeObserver.java
> @@ +26,5 @@
> > +        PreferenceManager.getDefaultSharedPreferences(appContext)
> > +                .registerOnSharedPreferenceChangeListener(this);
> > +    }
> > +
> > +    public interface SearchEngineChangeListener {
> 
> It is confusing that there is both a SearchEngineChangeListener and a
> SearchEngineChangeObserver interface.
> 
> Instead of making this SearchEngineChangeObserver class, I think you should
> just make this all part of SearchEngineMediator.

I'll make this simpler / clearer.
 
> @@ +43,5 @@
> > +     *
> > +     * @param onSuccess a Runnable to be called after successfully looking up the search engine
> > +     *                  and notifying the listener. This will run on the UI thread.
> > +     */
> > +    public void refresh(final Runnable onSuccess) {
> 
> I think a better API would be to create a callback interface that has a
> method that uses the engine name as a parameter.

Sounds good.

> @@ +55,5 @@
> > +            @Override
> > +            protected void onPostExecute(String engineName) {
> > +                if (searchEngineChanged(engineName) && onSuccess != null) {
> > +                    onSuccess.run();
> > +                }
> 
> If you're not going to always call onSuccess, you should also have an
> onError callback. This current implementation doesn't give consumers the
> opportunity to do anything if this fails.

Sounds good.

> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineFactory.
> java
> @@ +5,5 @@
> > +package org.mozilla.search.providers;
> > +
> > +import org.mozilla.search.Constants;
> > +
> > +public class SearchEngineFactory {
> 
> Let's add some class comments explaining what all these new things are for.

Sounds good.

> @@ +13,5 @@
> > +        GOOGLE,
> > +        YAHOO
> > +    }
> > +
> > +    public static SearchEngine getInstance(String engineName) throws ClassNotFoundException {
> 
> The `getInstance` method name is usually reserved for getting an instance of
> the class you're calling the method on. I think something like
> `createSearchEngine` would be a better name for this.

Changed to `createInstance`

> @@ +20,5 @@
> > +        }
> > +        return getInstance(Engine.valueOf(engineName));
> > +    }
> > +
> > +    public static SearchEngine getInstance(Engine engine) throws ClassNotFoundException {
> 
> I don't see this method used anywhere. I think we could get rid of this
> second signature and just put this method body inside the first
> `getInstance` declaration.

Sounds good.

> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineMediator.
> java
> @@ +38,5 @@
> > +        }
> > +        browserDriver.loadInternalUrl(currentEngine.resultsUriForQuery(query));
> > +    }
> > +
> > +    public void htmlTitleReady() {
> 
> Why does something that manages search engines need to know about html
> titles? It feels like something is going wrong with the abstraction layers
> here.

This'll go away with the callback pattern you've described.

Comment 11

[Eric Edens [:eedens]]

Attachment: bug-1042943-wip.v5.patch

Major changes:
  - Uses GeckoSharedPrefs
  - SearchEngineManager has a single callback interface `withDefaultSearchEngine(SearchEngineCallback callback)`

Comment 12

[:Margaret Leibovic]

Comment on attachment 8471903 [details] [diff] [review]
bug-1042943-wip.v5.patch

Review of attachment 8471903 [details] [diff] [review]:
-----------------------------------------------------------------

Much better! This is definitely going in the right direction.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +76,5 @@
>  
> +    public void startSearch(final String query) {
> +        if (searchEngineManager == null || webview == null) {
> +            Log.e(LOG_TAG, "Search started before PostSearchFragment finished initializing.");
> +            return;

Does this actually happen in practice? Right now the fragments are all attached right when MainActivity is created, so I don't think this is possible.

@@ +81,5 @@
>          }
> +        searchEngineManager.withDefaultSearchEngine(new SearchEngineManager.SearchEngineCallback() {
> +            @Override
> +            public void run(AbstractSearchEngine engine) {
> +                String url = engine.resultsUriForQuery(query);

Nit: final.

@@ +108,5 @@
>  
> +                    // We keep URLs in the webview that are either about:blank or a search engine result page.
> +                    // Keeping the URL in the webview is a noop. For all other URLs, we hault the
> +                    // webview and send the URL to fennec.
> +                    if (!(TextUtils.equals(url, Constants.ABOUT_BLANK) || engine.isSearchResultsPage(url))) {

Nit: To save on indentation, you could reverse this condition and just return early if it's not met.

@@ -121,5 @@
>      private class ChromeClient extends WebChromeClient {
>  
>          @Override
> -        public void onReceivedTitle(WebView view, String title) {
> -            super.onReceivedTitle(view, title);

Don't we still need this super call?

::: mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java
@@ +39,3 @@
>  
> +    public static final String PREF_CLEAR_HISTORY_KEY = "SearchPreferenceActivity.PREF_CLEAR_HISTORY_KEY";
> +    public static final String PREF_SEARCH_ENGINE_KEY = "SearchPreferenceActivity.PREF_SEARCH_ENGINE_KEY";

How did you come up with these key names? Is this a pattern we follow in the rest of our codebase?

::: mobile/android/search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java
@@ +133,5 @@
>          }
>      }
>  
> +    @Override
> +    public void onStart() {

Why are these things happening in onStart/onStop? Why not just do this in onAttach/onDetach?

@@ +138,5 @@
> +        super.onStart();
> +        searchEngineManager = new SearchEngineManager(getActivity().getApplicationContext(), this);
> +
> +        // Manually fetch the current search engine.
> +        searchEngineManager.refresh();

I don't like this pattern of making a refresh call, so that onSearchEngineChanged will be called, since that's not intuitive if you're not familiar with the implementation of SearchEngineManager.

I think a more intuitive way of doing this would be to create a new SearchEngineManager instance, then make a call to get the search engine, and use that to initialize suggestClient.

We should still keep a listener to listen for changes, and make sure to update suggestClient if the engine has changed, but that shouldn't be part of the initialization logic.

@@ +160,5 @@
>      public void loadSuggestions(String query) {
>          final Bundle args = new Bundle();
>          args.putString(KEY_SEARCH_TERM, query);
> +        final LoaderManager loaderManager = getLoaderManager();
> +        if (loaderManager != null) {

When will loaderManager be null?

@@ +192,5 @@
>          public Loader<List<Suggestion>> onCreateLoader(int id, Bundle args) {
> +            // Suggest client will be null in two cases. First, if the user is really fast
> +            // and starts typing before the search engine manager is able to read shared prefs.
> +            // Second, it could be null if there is an underlying error that doesn't allow us
> +            // to read the default engine.

I'm okay with this compromise for the first case. For the second case, we should make sure we throw somewhere earlier if there's an unexpected error. So, I would expect we only get null here if we just haven't initialized suggestClient yet.

@@ +197,5 @@
> +            //
> +            // Regardless, if suggest client isn't available, then we drop the user's current search.
> +            if (suggestClient != null) {
> +                return new SuggestionAsyncLoader(getActivity(), suggestClient, args.getString(KEY_SEARCH_TERM));
> +            } else {

Nit: You don't need an else statement if the if statement returns.

::: mobile/android/search/java/org/mozilla/search/providers/AbstractSearchEngine.java
@@ +9,5 @@
> +/**
> + * Extend this class to add a new search engine to
> + * the search activity.
> + */
> +public abstract class AbstractSearchEngine {

I think just calling this SearchEngine would be fine. It would make things a bit briefer when using this class name in other places.

@@ +13,5 @@
> +public abstract class AbstractSearchEngine {
> +
> +    // Boilerplate bookmarklet-style JS for injecting CSS into the
> +    // head of a web page. The actual CSS is inserted at `%s`.
> +    private static final String HIDE_BANNER_SCRIPT =

This JS doesn't actually have anything to do with hiding the banner. Since it just injects CSS, maybe a better name would be something like STYLE_INJECTION_SCRIPT.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +16,5 @@
> +    private final Context context;
> +    private DefaultSearchEngineChangeListener defaultSearchEngineChangeListener;
> +    private AbstractSearchEngine engine;
> +
> +    public static interface DefaultSearchEngineChangeListener {

I think we could just call this EngineChangeListener. The word "default" in here is a bit confusing, because I would think of "default" as the thing that is built into the app, not what the user selected.

@@ +21,5 @@
> +        public void onSearchEngineChanged(AbstractSearchEngine engine);
> +    }
> +
> +    public static interface SearchEngineCallback {
> +        void run(AbstractSearchEngine engine);

I would rename this method to something like `onEngineFound` (basically something more specific to search engines than `run`).

@@ +28,5 @@
> +    public SearchEngineManager(Context context) {
> +        this.context = context;
> +    }
> +
> +    public SearchEngineManager(Context context, DefaultSearchEngineChangeListener defaultSearchEngineChangeListener) {

I think I would prefer to just have a setEngineChangeListener method to set this, instead of making it part of the constructor.

@@ +56,5 @@
> +     *                   of SearchEngineFactory.Engine. If null, then it will use the
> +     *                   default search engine.
> +     */
> +    private void searchEngineChanged(String engineName) {
> +        engine = SearchEngineFactory.createInstance(engineName);

Instead of making a separate SearchEngineFactory class, I think you could just have a private method in this class to help you convert a String to the correct SearchEngine class.

@@ +72,5 @@
> +     *
> +     * @param callback a SearchEngineCallback to be called after successfully looking up the search engine
> +     *                 and notifying the searchEngineChangeListener. This will run on the UI thread.
> +     */
> +    public void refresh(final SearchEngineCallback callback) {

Instead of calling this `refresh`, I would call it `getEngineFromPrefs`. I would also rename `withDefaultSearchEngine` to `getEngine`. The `getEngine` method would check to see if there's a cached engine, then call the callback with that engine (like it currently does), but then if there's no cached engine it calls `getEngineFromPrefs` to asynchronously call the callback.

@@ +81,5 @@
> +            }
> +
> +            @Override
> +            protected void onPostExecute(String engineName) {
> +                searchEngineChanged(engineName);

We should only call this if the engine actually changed.

Comment 13

[Eric Edens [:eedens]]

Attachment: bug-1042943-wip.v6.patch

(In reply to :Margaret Leibovic from comment #12)
> ::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
> @@ +76,5 @@
> >  
> > +    public void startSearch(final String query) {
> > +        if (searchEngineManager == null || webview == null) {
> > +            Log.e(LOG_TAG, "Search started before PostSearchFragment finished initializing.");
> > +            return;
> 
> Does this actually happen in practice? Right now the fragments are all
> attached right when MainActivity is created, so I don't think this is
> possible.

My concern is that startSearch is a public method, so it can be called outside of the fragment lifecycle; eg:

  PostSearchFragment fragment = new PostSearchFragment();
  fragment.startSearch("cats");  /// Booom!

But you are correct -- in our current usage, the checks shouldn't be required. I'd prefer to keep things defensive, but if you feel strongly about removing the null checks, we can.


> @@ +81,5 @@
> >          }
> > +        searchEngineManager.withDefaultSearchEngine(new SearchEngineManager.SearchEngineCallback() {
> > +            @Override
> > +            public void run(AbstractSearchEngine engine) {
> > +                String url = engine.resultsUriForQuery(query);
> 
> Nit: final.

SGTM


> @@ +108,5 @@
> >  
> > +                    // We keep URLs in the webview that are either about:blank or a search engine result page.
> > +                    // Keeping the URL in the webview is a noop. For all other URLs, we hault the
> > +                    // webview and send the URL to fennec.
> > +                    if (!(TextUtils.equals(url, Constants.ABOUT_BLANK) || engine.isSearchResultsPage(url))) {
> 
> Nit: To save on indentation, you could reverse this condition and just
> return early if it's not met.

Good call -- this makes the code a lot clearer.

> @@ -121,5 @@
> >      private class ChromeClient extends WebChromeClient {
> >  
> >          @Override
> > -        public void onReceivedTitle(WebView view, String title) {
> > -            super.onReceivedTitle(view, title);
> 
> Don't we still need this super call?

I don't think so, since onReceivedTitle is a stub? https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/core/java/android/webkit/WebChromeClient.java#34

> :::
> mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java
> @@ +39,3 @@
> >  
> > +    public static final String PREF_CLEAR_HISTORY_KEY = "SearchPreferenceActivity.PREF_CLEAR_HISTORY_KEY";
> > +    public static final String PREF_SEARCH_ENGINE_KEY = "SearchPreferenceActivity.PREF_SEARCH_ENGINE_KEY";
> 
> How did you come up with these key names? Is this a pattern we follow in the
> rest of our codebase?

The key names were a best effort :)  But this was helpful: https://lxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#545

Here's a stab: let me know what you think.
  "search.not_a_preference.clear_history"
  "search.engines.default"

> :::
> mobile/android/search/java/org/mozilla/search/autocomplete/
> SuggestionsFragment.java
> @@ +133,5 @@
> >          }
> >      }
> >  
> > +    @Override
> > +    public void onStart() {
> 
> Why are these things happening in onStart/onStop? Why not just do this in
> onAttach/onDetach?

Just overly-aggressive resource cleanup. Your proposal is a lot cleaner, though. 

> @@ +138,5 @@
> > +        super.onStart();
> > +        searchEngineManager = new SearchEngineManager(getActivity().getApplicationContext(), this);
> > +
> > +        // Manually fetch the current search engine.
> > +        searchEngineManager.refresh();
> 
> I don't like this pattern of making a refresh call, so that
> onSearchEngineChanged will be called, since that's not intuitive if you're
> not familiar with the implementation of SearchEngineManager.
> 
> I think a more intuitive way of doing this would be to create a new
> SearchEngineManager instance, then make a call to get the search engine, and
> use that to initialize suggestClient.
> 
> We should still keep a listener to listen for changes, and make sure to
> update suggestClient if the engine has changed, but that shouldn't be part
> of the initialization logic.

Sounds good!

> @@ +160,5 @@
> >      public void loadSuggestions(String query) {
> >          final Bundle args = new Bundle();
> >          args.putString(KEY_SEARCH_TERM, query);
> > +        final LoaderManager loaderManager = getLoaderManager();
> > +        if (loaderManager != null) {
> 
> When will loaderManager be null?

It's a race for suggestClient; see onCreateLoader.

> @@ +192,5 @@
> >          public Loader<List<Suggestion>> onCreateLoader(int id, Bundle args) {
> > +            // Suggest client will be null in two cases. First, if the user is really fast
> > +            // and starts typing before the search engine manager is able to read shared prefs.
> > +            // Second, it could be null if there is an underlying error that doesn't allow us
> > +            // to read the default engine.
> 
> I'm okay with this compromise for the first case. For the second case, we
> should make sure we throw somewhere earlier if there's an unexpected error.
> So, I would expect we only get null here if we just haven't initialized
> suggestClient yet.

Sounds good 

> @@ +197,5 @@
> > +            //
> > +            // Regardless, if suggest client isn't available, then we drop the user's current search.
> > +            if (suggestClient != null) {
> > +                return new SuggestionAsyncLoader(getActivity(), suggestClient, args.getString(KEY_SEARCH_TERM));
> > +            } else {
> 
> Nit: You don't need an else statement if the if statement returns.

Sounds good

> :::
> mobile/android/search/java/org/mozilla/search/providers/AbstractSearchEngine.
> java
> @@ +9,5 @@
> > +/**
> > + * Extend this class to add a new search engine to
> > + * the search activity.
> > + */
> > +public abstract class AbstractSearchEngine {
> 
> I think just calling this SearchEngine would be fine. It would make things a
> bit briefer when using this class name in other places.

Sounds good

> @@ +13,5 @@
> > +public abstract class AbstractSearchEngine {
> > +
> > +    // Boilerplate bookmarklet-style JS for injecting CSS into the
> > +    // head of a web page. The actual CSS is inserted at `%s`.
> > +    private static final String HIDE_BANNER_SCRIPT =
> 
> This JS doesn't actually have anything to do with hiding the banner. Since
> it just injects CSS, maybe a better name would be something like
> STYLE_INJECTION_SCRIPT.

Sounds good.

> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> @@ +16,5 @@
> > +    private final Context context;
> > +    private DefaultSearchEngineChangeListener defaultSearchEngineChangeListener;
> > +    private AbstractSearchEngine engine;
> > +
> > +    public static interface DefaultSearchEngineChangeListener {
> 
> I think we could just call this EngineChangeListener. The word "default" in
> here is a bit confusing, because I would think of "default" as the thing
> that is built into the app, not what the user selected.

Sounds good

> @@ +21,5 @@
> > +        public void onSearchEngineChanged(AbstractSearchEngine engine);
> > +    }
> > +
> > +    public static interface SearchEngineCallback {
> > +        void run(AbstractSearchEngine engine);
> 
> I would rename this method to something like `onEngineFound` (basically
> something more specific to search engines than `run`).

Sounds good.

> @@ +28,5 @@
> > +    public SearchEngineManager(Context context) {
> > +        this.context = context;
> > +    }
> > +
> > +    public SearchEngineManager(Context context, DefaultSearchEngineChangeListener defaultSearchEngineChangeListener) {
> 
> I think I would prefer to just have a setEngineChangeListener method to set
> this, instead of making it part of the constructor.

Sounds good.

> @@ +56,5 @@
> > +     *                   of SearchEngineFactory.Engine. If null, then it will use the
> > +     *                   default search engine.
> > +     */
> > +    private void searchEngineChanged(String engineName) {
> > +        engine = SearchEngineFactory.createInstance(engineName);
> 
> Instead of making a separate SearchEngineFactory class, I think you could
> just have a private method in this class to help you convert a String to the
> correct SearchEngine class.

My preference is to keep this in SearchEngineFactory so that it's very clear where to register new engines. If you feel strongly, though, we can inline.

> @@ +72,5 @@
> > +     *
> > +     * @param callback a SearchEngineCallback to be called after successfully looking up the search engine
> > +     *                 and notifying the searchEngineChangeListener. This will run on the UI thread.
> > +     */
> > +    public void refresh(final SearchEngineCallback callback) {
> 
> Instead of calling this `refresh`, I would call it `getEngineFromPrefs`. I
> would also rename `withDefaultSearchEngine` to `getEngine`. The `getEngine`
> method would check to see if there's a cached engine, then call the callback
> with that engine (like it currently does), but then if there's no cached
> engine it calls `getEngineFromPrefs` to asynchronously call the callback.

Sounds good.

> @@ +81,5 @@
> > +            }
> > +
> > +            @Override
> > +            protected void onPostExecute(String engineName) {
> > +                searchEngineChanged(engineName);
> 
> We should only call this if the engine actually changed.

I like this idea -- I'll make this as a pre-condition check in searchEngineChanged.

Comment 14

[:Margaret Leibovic]

(In reply to Eric Edens [:eedens] from comment #13)
> Created attachment 8472587 [details] [diff] [review]
> bug-1042943-wip.v6.patch
> 
> (In reply to :Margaret Leibovic from comment #12)
> > ::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
> > @@ +76,5 @@
> > >  
> > > +    public void startSearch(final String query) {
> > > +        if (searchEngineManager == null || webview == null) {
> > > +            Log.e(LOG_TAG, "Search started before PostSearchFragment finished initializing.");
> > > +            return;
> > 
> > Does this actually happen in practice? Right now the fragments are all
> > attached right when MainActivity is created, so I don't think this is
> > possible.
> 
> My concern is that startSearch is a public method, so it can be called
> outside of the fragment lifecycle; eg:
> 
>   PostSearchFragment fragment = new PostSearchFragment();
>   fragment.startSearch("cats");  /// Booom!
> 
> But you are correct -- in our current usage, the checks shouldn't be
> required. I'd prefer to keep things defensive, but if you feel strongly
> about removing the null checks, we can.

Sure, we can keep them. This is just a new change, since before we weren't doing any null checks before using the webview.


> > @@ -121,5 @@
> > >      private class ChromeClient extends WebChromeClient {
> > >  
> > >          @Override
> > > -        public void onReceivedTitle(WebView view, String title) {
> > > -            super.onReceivedTitle(view, title);
> > 
> > Don't we still need this super call?
> 
> I don't think so, since onReceivedTitle is a stub?
> https://android.googlesource.com/platform/frameworks/base/+/refs/heads/
> master/core/java/android/webkit/WebChromeClient.java#34

Sounds good. It just wasn't clear to me why it was being removed.

> > :::
> > mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java
> > @@ +39,3 @@
> > >  
> > > +    public static final String PREF_CLEAR_HISTORY_KEY = "SearchPreferenceActivity.PREF_CLEAR_HISTORY_KEY";
> > > +    public static final String PREF_SEARCH_ENGINE_KEY = "SearchPreferenceActivity.PREF_SEARCH_ENGINE_KEY";
> > 
> > How did you come up with these key names? Is this a pattern we follow in the
> > rest of our codebase?
> 
> The key names were a best effort :)  But this was helpful:
> https://lxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.
> js#545
> 
> Here's a stab: let me know what you think.
>   "search.not_a_preference.clear_history"
>   "search.engines.default"

The prefs in mobile.js are all gecko prefs, so those are stored as part of the gecko profile, instead of shared preferences, but looking at some of the shared preferences in the tree, these seem like fine choices.

> > @@ +160,5 @@
> > >      public void loadSuggestions(String query) {
> > >          final Bundle args = new Bundle();
> > >          args.putString(KEY_SEARCH_TERM, query);
> > > +        final LoaderManager loaderManager = getLoaderManager();
> > > +        if (loaderManager != null) {
> > 
> > When will loaderManager be null?
> 
> It's a race for suggestClient; see onCreateLoader.

Looking at that logic, I see that it is possible to now return null in onCreateLoader, but I don't think getLoaderManager() will ever return null:
http://developer.android.com/reference/android/app/Fragment.html#getLoaderManager%28%29

I think what you actually want to do is add a null check in onLoadFinished.

> > @@ +56,5 @@
> > > +     *                   of SearchEngineFactory.Engine. If null, then it will use the
> > > +     *                   default search engine.
> > > +     */
> > > +    private void searchEngineChanged(String engineName) {
> > > +        engine = SearchEngineFactory.createInstance(engineName);
> > 
> > Instead of making a separate SearchEngineFactory class, I think you could
> > just have a private method in this class to help you convert a String to the
> > correct SearchEngine class.
> 
> My preference is to keep this in SearchEngineFactory so that it's very clear
> where to register new engines. If you feel strongly, though, we can inline.

I think we should just put it in SearchEngineManager. If we just have one class that manages search engines (SearchEngineManager), it should be pretty clear where to add new engines.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8472587 [details] [diff] [review]
bug-1042943-wip.v6.patch

Review of attachment 8472587 [details] [diff] [review]:
-----------------------------------------------------------------

So close! Let's just do one more pass and then land this!

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +70,5 @@
> +    @Override
> +    public void onDetach() {
> +        super.onDetach();
> +
> +        if (searchEngineManager != null) {

onDetach will never be called before onAttach, so I don't think you need this check.

@@ +79,5 @@
>  
> +    public void startSearch(final String query) {
> +        if (searchEngineManager == null || webview == null) {
> +            Log.e(LOG_TAG, "Search started before PostSearchFragment finished initializing.");
> +            return;

In my last comment I said I was okay with these checks, but now that I think about it, I think I'd prefer we just crash if we get into an unexpected state. Null checks seem innocent, but they can easily cover up bugs.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java
@@ +90,5 @@
> +            public void onEngineFound(SearchEngine engine) {
> +                suggestClient = new SuggestClient(getActivity(), engine.getSuggestionTemplate(GECKO_SEARCH_TERMS_URL_PARAM),
> +                        SUGGESTION_TIMEOUT, Constants.SUGGESTION_MAX);
> +            }
> +        });

Nice, I think this is really straightforward :)

@@ +102,3 @@
>          suggestionLoaderCallbacks = null;
>          autoCompleteAdapter = null;
> +        if (searchEngineManager != null) {

I don't think searchEngineManger will ever be null here.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineFactory.java
@@ +35,5 @@
> +        // engine that isn't defined in the above enum. Right now, we
> +        // return the default engine. To make this robust, we could
> +        // fix the underlying issue in the shared prefs.
> +        Log.e(LOG_TAG, "Couldn't find search engine for name " + engineName);
> +        return createInstance(Constants.DEFAULT_SEARCH_ENGINE);

I'm not sure how I feel about falling back to a default here if there isn't a valid engine name. I feel like this should never be the case, so we should just throw if we end up in this state.

Maybe we should consider splitting up the "get me a search engine from this name string" and the "get me the default search engine" functionality. So, if we have a string that we got from prefs, we should fetch the right search engine. Otherwise, we should use a default string and build an engine from that. But we should do that default logic where we're reading the pref.

Basically, I think that whatever is in charge of cranking out SearchEngine objects shouldn't be concerned with preferences and defaults.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +22,5 @@
> +    }
> +
> +    public static interface SearchEngineCallback {
> +        void onEngineFound(SearchEngine engine);
> +    }

I'd like to see if we can combine these two interfaces, since they're essentially doing the same thing. Perhaps we can change setEngineChangeListener to take a SearchEngineCallback? I'll leave it up to you to figure out a good generic name for this thing :)

@@ +49,5 @@
> +        }
> +    }
> +
> +    public void destroy() {
> +        GeckoSharedPrefs.forApp(context).unregisterOnSharedPreferenceChangeListener(this);

Good call making sure to unregister this listener.

@@ +70,5 @@
> +            }
> +
> +            @Override
> +            protected void onPostExecute(String engineName) {
> +                searchEngineChanged(engineName);

I don't think we actually need this here, right? If the search engine changes, we'll hear about it in the shared prefs change listener.

I think what we actually want here is to cache the engine if this is the first time we're reading this pref. So I think we need to split up the searchEngineChanged method. Instead of calling the engineChangeListener there, you can just call that in onSharedPreferenceChanged. And then you can just add calls in here and in onSharedPreferenceChanged to update the cached value directly.

@@ +73,5 @@
> +            protected void onPostExecute(String engineName) {
> +                searchEngineChanged(engineName);
> +                if (callback != null) {
> +                    callback.onEngineFound(engine);
> +                }

If there's no callback, I think we can just bail at the top of getEngineFromPrefs, instead of reading the pref then doing nothing. (I know that we are caching the engine, but if there's no consumer using it right now, we can wait to fetch it and cache it when someone actually needs to use it.)

Comment 16

[Eric Edens [:eedens]]

Attachment: bug-1042943-fix.v1.patch

(In reply to :Margaret Leibovic from comment #14)

> > > @@ +160,5 @@
> > > >      public void loadSuggestions(String query) {
> > > >          final Bundle args = new Bundle();
> > > >          args.putString(KEY_SEARCH_TERM, query);
> > > > +        final LoaderManager loaderManager = getLoaderManager();
> > > > +        if (loaderManager != null) {
> > > 
> > > When will loaderManager be null?
> > 
> > It's a race for suggestClient; see onCreateLoader.
> 
> Looking at that logic, I see that it is possible to now return null in
> onCreateLoader, but I don't think getLoaderManager() will ever return null:
> http://developer.android.com/reference/android/app/Fragment.
> html#getLoaderManager%28%29
> 
> I think what you actually want to do is add a null check in onLoadFinished.

You're right -- doing a null check on getLoaderManager is worthless. We need to do a null check on the loader *itself*, and then optionally init or restart the loader.

Without this check, there will be a NPE thrown in restartLoader: 

https://android.googlesource.com/platform/frameworks/support/+/refs/heads/master/v4/java/android/support/v4/app/LoaderManager.java#641

> > > @@ +56,5 @@
> > > > +     *                   of SearchEngineFactory.Engine. If null, then it will use the
> > > > +     *                   default search engine.
> > > > +     */
> > > > +    private void searchEngineChanged(String engineName) {
> > > > +        engine = SearchEngineFactory.createInstance(engineName);
> > > 
> > > Instead of making a separate SearchEngineFactory class, I think you could
> > > just have a private method in this class to help you convert a String to the
> > > correct SearchEngine class.
> > 
> > My preference is to keep this in SearchEngineFactory so that it's very clear
> > where to register new engines. If you feel strongly, though, we can inline.
> 
> I think we should just put it in SearchEngineManager. If we just have one
> class that manages search engines (SearchEngineManager), it should be pretty
> clear where to add new engines.

You got it :)


(In reply to :Margaret Leibovic from comment #15)
> Comment on attachment 8472587 [details] [diff] [review]
> bug-1042943-wip.v6.patch
> 
> Review of attachment 8472587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So close! Let's just do one more pass and then land this!
> 
> ::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
> @@ +70,5 @@
> > +    @Override
> > +    public void onDetach() {
> > +        super.onDetach();
> > +
> > +        if (searchEngineManager != null) {
> 
> onDetach will never be called before onAttach, so I don't think you need
> this check.

Sounds good

> @@ +79,5 @@
> >  
> > +    public void startSearch(final String query) {
> > +        if (searchEngineManager == null || webview == null) {
> > +            Log.e(LOG_TAG, "Search started before PostSearchFragment finished initializing.");
> > +            return;
> 
> In my last comment I said I was okay with these checks, but now that I think
> about it, I think I'd prefer we just crash if we get into an unexpected
> state. Null checks seem innocent, but they can easily cover up bugs.

Sounds good -- I'll remove this null check. And perhaps be more conscientious about using them in the future as well :)


> @@ +102,3 @@
> >          suggestionLoaderCallbacks = null;
> >          autoCompleteAdapter = null;
> > +        if (searchEngineManager != null) {
> 
> I don't think searchEngineManger will ever be null here.

Removed the null check.

> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineFactory.
> java
> @@ +35,5 @@
> > +        // engine that isn't defined in the above enum. Right now, we
> > +        // return the default engine. To make this robust, we could
> > +        // fix the underlying issue in the shared prefs.
> > +        Log.e(LOG_TAG, "Couldn't find search engine for name " + engineName);
> > +        return createInstance(Constants.DEFAULT_SEARCH_ENGINE);
> 
> I'm not sure how I feel about falling back to a default here if there isn't
> a valid engine name. I feel like this should never be the case, so we should
> just throw if we end up in this state.
> 
> Maybe we should consider splitting up the "get me a search engine from this
> name string" and the "get me the default search engine" functionality. So,
> if we have a string that we got from prefs, we should fetch the right search
> engine. Otherwise, we should use a default string and build an engine from
> that. But we should do that default logic where we're reading the pref.
> 
> Basically, I think that whatever is in charge of cranking out SearchEngine
> objects shouldn't be concerned with preferences and defaults.

Sounds good.

> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> @@ +22,5 @@
> > +    }
> > +
> > +    public static interface SearchEngineCallback {
> > +        void onEngineFound(SearchEngine engine);
> > +    }
> 
> I'd like to see if we can combine these two interfaces, since they're
> essentially doing the same thing. Perhaps we can change
> setEngineChangeListener to take a SearchEngineCallback? I'll leave it up to
> you to figure out a good generic name for this thing :)

I think this will do:

    public static interface SearchEngineCallback {
        public void execute(SearchEngine engine);
    }

> @@ +70,5 @@
> > +            }
> > +
> > +            @Override
> > +            protected void onPostExecute(String engineName) {
> > +                searchEngineChanged(engineName);
> 
> I don't think we actually need this here, right? If the search engine
> changes, we'll hear about it in the shared prefs change listener.
> 
> I think what we actually want here is to cache the engine if this is the
> first time we're reading this pref. So I think we need to split up the
> searchEngineChanged method. Instead of calling the engineChangeListener
> there, you can just call that in onSharedPreferenceChanged. And then you can
> just add calls in here and in onSharedPreferenceChanged to update the cached
> value directly.
> 
> @@ +73,5 @@
> > +            protected void onPostExecute(String engineName) {
> > +                searchEngineChanged(engineName);
> > +                if (callback != null) {
> > +                    callback.onEngineFound(engine);
> > +                }
> 
> If there's no callback, I think we can just bail at the top of
> getEngineFromPrefs, instead of reading the pref then doing nothing. (I know
> that we are caching the engine, but if there's no consumer using it right
> now, we can wait to fetch it and cache it when someone actually needs to use
> it.)

Sounds good

Comment 17

[:Margaret Leibovic]

Comment on attachment 8473176 [details] [diff] [review]
bug-1042943-fix.v1.patch

Review of attachment 8473176 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! Just a few more comments.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +29,5 @@
> +    }
> +
> +    public static interface SearchEngineCallback {
> +        public void execute(SearchEngine engine);
> +    }

Nice, this works well.

@@ +37,5 @@
> +        GeckoSharedPrefs.forApp(context).registerOnSharedPreferenceChangeListener(this);
> +    }
> +
> +    public void setChangeListener(SearchEngineCallback changeListener) {
> +        this.changeListener = changeListener;

Nit: I think we should call these things changeCallback, since they're now implementing the SearchEngineCallback interface.

@@ +87,5 @@
> +
> +    @Override
> +    public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key) {
> +        if (TextUtils.equals(SearchPreferenceActivity.PREF_SEARCH_ENGINE_KEY, key)) {
> +            if (updateEngine(sharedPreferences.getString(key, null)) && changeListener != null) {

If the value of the PREF_SEARCH_ENGINE_KEY pref is changing, updateEngine should always return true here.

So I think this method body should just be:

if (TextUtils.equals(SearchPreferenceActivity.PREF_SEARCH_ENGINE_KEY, key)) {
  updateEngine(engineName);

  if (changeCallback != null) {
    changeCallback.execute(engine);
  }
}

@@ +106,5 @@
> +
> +        // Bail if the engine isn't going to change.
> +        if (engine != null && TextUtils.equals(engine.getName(), engineName)) {
> +            return false;
> +        }

Given the fact that this method is only called when the search engine has indeed changed, I don't think we need this check, and I don't think this method needs to have a return value.

@@ +117,5 @@
> +            engine = createEngine(engineName);
> +            return true;
> +        } catch (ClassNotFoundException e) {
> +            e.printStackTrace();
> +        }

This is essentially no different than logging the error in createEngine. When I suggested throwing an exception, I wasn't expecting that we would just be catching and logging it.

@@ +121,5 @@
> +        }
> +
> +        // Try again using default engine.
> +        Log.e(LOG_TAG, "Search engine not found for " + engineName + " reverting to default engine.");
> +        return updateEngine(Constants.DEFAULT_SEARCH_ENGINE);

You could just call createEngine directly here, since we know what the result of all the other checks would be.

If you follow my comment down below, this method could be simplified to:

private void updateEngine(String engineName) {
  try {
    engine = createEngine(engineName);
  } catch (IllegalArgumentException e) {
    engine = createEngine(Constants.DEFAULT_SEARCH_ENGINE);
  }
}

@@ +136,5 @@
> +        }
> +
> +        // If we reach this point, then shared prefs contains a search
> +        // engine that isn't defined in the above enum.
> +        throw new ClassNotFoundException("Could not find search engine named " + engineName);

This should really be an IllegalArgumentExcpetion.

::: mobile/android/search/strings/search_strings.dtd
@@ +19,5 @@
> +<!ENTITY pref_searchProvider_title 'Search engine'>
> +
> +<!ENTITY pref_searchProvider_bing 'Bing'>
> +<!ENTITY pref_searchProvider_google 'Google'>
> +<!ENTITY pref_searchProvider_yahoo 'Yahoo'>

Thinking about this more, these probably shouldn't be localized strings, but rather they should be pulled from the search engines themselves. But we can address this in a follow-up, since there's more work to do to make these search engines localizeable (different locales should be able to ship different search engines). This would probably fall into supporting open search plugins.

Comment 18

[Eric Edens [:eedens]]

Attachment: bug-1042943-fix.v2.patch

Thanks for the feedback! It's a lot cleaner now. :)

When this is ready to land, it'll need to be coordinated with bug 1048525, since both touch SuggestFragment.

PR: https://github.com/mozilla/fennec-search/pull/49/files

> @@ +37,5 @@
> > +        GeckoSharedPrefs.forApp(context).registerOnSharedPreferenceChangeListener(this);
> > +    }
> > +
> > +    public void setChangeListener(SearchEngineCallback changeListener) {
> > +        this.changeListener = changeListener;
> 
> Nit: I think we should call these things changeCallback, since they're now
> implementing the SearchEngineCallback interface.

SGTM

> @@ +87,5 @@
> > +
> > +    @Override
> > +    public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key) {
> > +        if (TextUtils.equals(SearchPreferenceActivity.PREF_SEARCH_ENGINE_KEY, key)) {
> > +            if (updateEngine(sharedPreferences.getString(key, null)) && changeListener != null) {
> 
> If the value of the PREF_SEARCH_ENGINE_KEY pref is changing, updateEngine
> should always return true here.
> 
> So I think this method body should just be:
> 
> if (TextUtils.equals(SearchPreferenceActivity.PREF_SEARCH_ENGINE_KEY, key)) {
>   updateEngine(engineName);
> 
>   if (changeCallback != null) {
>     changeCallback.execute(engine);
>   }
> }

SGTM


> @@ +106,5 @@
> > +
> > +        // Bail if the engine isn't going to change.
> > +        if (engine != null && TextUtils.equals(engine.getName(), engineName)) {
> > +            return false;
> > +        }
> 
> Given the fact that this method is only called when the search engine has
> indeed changed, I don't think we need this check, and I don't think this
> method needs to have a return value.

SGTM

> @@ +117,5 @@
> > +            engine = createEngine(engineName);
> > +            return true;
> > +        } catch (ClassNotFoundException e) {
> > +            e.printStackTrace();
> > +        }
> 
> This is essentially no different than logging the error in createEngine.
> When I suggested throwing an exception, I wasn't expecting that we would
> just be catching and logging it.
> @@ +121,5 @@
> > +        }
> > +
> > +        // Try again using default engine.
> > +        Log.e(LOG_TAG, "Search engine not found for " + engineName + " reverting to default engine.");
> > +        return updateEngine(Constants.DEFAULT_SEARCH_ENGINE);
> 
> You could just call createEngine directly here, since we know what the
> result of all the other checks would be.
> 
> If you follow my comment down below, this method could be simplified to:
> 
> private void updateEngine(String engineName) {
>   try {
>     engine = createEngine(engineName);
>   } catch (IllegalArgumentException e) {
>     engine = createEngine(Constants.DEFAULT_SEARCH_ENGINE);
>   }
> }
> 

I updated the error logic based on our IRC conversation: If there's an error instantiating engineName, it will try instantiating the default engine. If that fails, and IllegalArgumentException is thrown to the app.

Comment 19

[:Margaret Leibovic]

Comment on attachment 8473333 [details] [diff] [review]
bug-1042943-fix.v2.patch

Review of attachment 8473333 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +111,5 @@
> +        }
> +
> +        try {
> +            engine = createEngine(engineName);
> +            return;

Nit: You can get rid of this return and just put the fallback engine assignment inside the catch statement.

Comment 20

[Eric Edens [:eedens]]

Attachment: bug-1042943-fix.v3.patch

For landing, this patch can land first, followed by bug 1048525


(In reply to :Margaret Leibovic from comment #19)
> Comment on attachment 8473333 [details] [diff] [review]
> bug-1042943-fix.v2.patch
> 
> Review of attachment 8473333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> @@ +111,5 @@
> > +        }
> > +
> > +        try {
> > +            engine = createEngine(engineName);
> > +            return;
> 
> Nit: You can get rid of this return and just put the fallback engine
> assignment inside the catch statement.

Good call :)

Comment 21

[Eric Edens [:eedens]]

https://github.com/mozilla/fennec-search/commit/74ef2b2c221edfaca1721da778ef5e504fdfb26b

Comment 22

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/b4c5016145e2

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4c5016145e2

Comment 24

[Aaron Train [:aaronmt]]

I believe Bing as per our searchplugin has special params for tablet specific search iirc, was that added in these patches?

Comment 25

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #24)
> I believe Bing as per our searchplugin has special params for tablet
> specific search iirc, was that added in these patches?

No, it wasn't. I'm going to file a follow-up bug about using the open search plugins we ship with Fennec to get all the right search engine details.