1035341 - Search suggestions: change dictionary to OpenSearch

Status: RESOLVED FIXED

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

Description

[Eric Edens [:eedens]]

At the moment, we use a wordlist for doing autocomplete. This needs to be switched to use an OpenSearch provider (at the moment Yahoo).

OpenSearch URLs https://mxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/searchplugins/

Comment 1

[Eric Edens [:eedens]]

Attachment: bug-1035341-wip.patch

For the background thread in AutoCompleteAgentManager -- do we need to manually shut this down? [1]

Lastly, at the time or writing, this patch is ahead of M-C: it is rebased for the pending landing of bug 1033686.


[1] https://github.com/ericedens/FirefoxSearch/commit/0425d2a57009d42348a5cc57d8470eae838b9ca8#diff-a63a66fa32306f5d953476f70a8929fcR68
Github PR: https://github.com/ericedens/FirefoxSearch/pull/20

Comment 2

[Eric Edens [:eedens]]

There's some churn in the patch from removing the wordlist code. The more interesting stuff is in: https://github.com/ericedens/FirefoxSearch/commit/7feb2396b3be08e5e30bbaae06e21590ee7af6b4

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8454616 [details] [diff] [review]
bug-1035341-wip.patch

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

::: mobile/android/search/java/org/mozilla/search/autocomplete/AutoCompleteAgentManager.java
@@ +46,5 @@
> +
> +    /**
> +     * A message type that indicates there is a search query in the obj field.
> +     */
> +    private static final int SEARCH_MESSAGE = 0;

Prefer prefix form:

  MESSAGE_SEARCH
  MESSAGE_QUERY_FINISHED

... or prefer an enum.

@@ +57,5 @@
> +    public AutoCompleteAgentManager(Context context, ArrayAdapter<AutoCompleteModel> adapter) {
> +        resultsAdapter = adapter;
> +
> +        // Pre-load the shared buffer with empty row models.
> +        synchronized (resultBuffer) {

This is inside the constructor, so apart from some thorny Java edge cases you don't need to worry about synchronization at this point. You can rely on code in the constructor having run prior to the reference being used by other code.

If you really want, you can actually move this code to a static initializer block below resultBuffer, which simplifies your constructor.

@@ +65,5 @@
> +        }
> +
> +        uiHandler = new UiHandler(context.getMainLooper());
> +
> +        HandlerThread backgroundThread = new HandlerThread(getClass().toString());

You probably want getClass().getSimpleName().

@@ +66,5 @@
> +
> +        uiHandler = new UiHandler(context.getMainLooper());
> +
> +        HandlerThread backgroundThread = new HandlerThread(getClass().toString());
> +        backgroundThread.start();

Have I ranted yet about how Android ignored Java's excellent Executors framework? *sigh*

@@ +80,5 @@
>       */
> +    public void getSuggestions(String queryString) {
> +        // Remove any pending searches from the queue.
> +        if (backgroundHandler.hasMessages(SEARCH_MESSAGE)) {
> +            backgroundHandler.removeMessages(SEARCH_MESSAGE);

Just call `removeMessages` directly. The check is redundant.

@@ +95,1 @@
>              super(looper);

Lights are gonna find me...

@@ +105,5 @@
> +                        // of the array.
> +                        if (rowModel.isEmpty()) {
> +                            break;
> +                        } else {
> +                            resultsAdapter.add(rowModel);

Did you know about resultsAdapter.addAll?

This might suggest a different approach to you, which I'll find you on IRC to discuss.

::: mobile/android/search/java/org/mozilla/search/autocomplete/AutoCompleteModel.java
@@ +15,5 @@
>  
>      // The text that should immediately jump out to the user;
>      // for example, the name of a restaurant or the title
>      // of a website.
> +    private String mainText;

I would strongly prefer for this to *not* be mutable. If you make this class a 'record' -- that is, all of its fields are final -- things get a lot simpler. A small amount of garbage creation is vastly preferable to complicated concurrency management.

::: mobile/android/search/java/org/mozilla/search/autocomplete/AutoCompleteYahooAgent.java
@@ +31,5 @@
> +    public AutoCompleteYahooAgent() throws AutoCompleteYahooAgentException {
> +        xmlParser = new YahooXmlParser();
> +    }
> +
> +    public String[] getSuggestions(String query, int numSuggestions) {

List<String>.

Comment 4

[Eric Edens [:eedens]]

Attachment: p.1.removedict.patch

Remove the wordlist that is being replaced with Yahoo search suggestions.

Comment 5

[Eric Edens [:eedens]]

Attachment: p.2.renames.patch

Rename AutoComplete* to AC*

Comment 6

[Eric Edens [:eedens]]

Attachment: p.3.addyahoo.patch

Add search suggestions from Yahoo, including an abstract ACBaseAgent that manages work on a background thread. Unlike an AsyncTask, it doesn't get destroyed on every use. Unlike a Service, it doesn't outlive the UI.

Comment 7

[Eric Edens [:eedens]]

(In reply to Richard Newman [:rnewman] from comment #3)

Thanks Richard --

The smaller, concrete items I updated  (renaming static fields and using getClass().getSimpleName()).

For the larger items:

1) ACModel is no longer mutable
2) The background thread and foreground thread have a shared List<ACModel> instead of a shared array.
3) The threads use atomic counters for invalidation of stale results 
4) The foreground thread sends a *copy* of the shared List<ACModel> back to the consumer
5) Coordination of all of this happens in the abstract class ACBaseAgent

The patches are aligned with this PR: https://github.com/ericedens/FirefoxSearch/pull/20

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8455076 [details] [diff] [review]
p.2.renames.patch

Do we really want this rename?

Comment 9

[Eric Edens [:eedens]]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 8455076 [details] [diff] [review]
> p.2.renames.patch
> 
> Do we really want this rename?

Since `AutoComplete` is so long, it was making it hard to quickly differentiate between class names [1]. 

In general, though, the packaging and naming could definitely benefit from feedback and review. We could drop p.2.renames.patch, and then address the naming and packaging in a separate bug. 


[1] https://lxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/autocomplete/

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Eric Edens [:eedens] from comment #9)
> (In reply to Mark Finkle (:mfinkle) from comment #8)
> > Comment on attachment 8455076 [details] [diff] [review]
> > p.2.renames.patch
> > 
> > Do we really want this rename?
> 
> Since `AutoComplete` is so long, it was making it hard to quickly
> differentiate between class names [1]. 

I can appreciate the concern over long names, even in Java :)

> 
> In general, though, the packaging and naming could definitely benefit from
> feedback and review. We could drop p.2.renames.patch, and then address the
> naming and packaging in a separate bug. 

That sounds better to me. the "AC" prefix is a bit harsh. If we want to shorten the name, maybe we drop any prefix since the code is in "search/autocomplete/" anyway.

> [1]
> https://lxr.mozilla.org/mozilla-central/source/mobile/android/search/java/
> org/mozilla/search/autocomplete/

As an old timer at Mozilla, I applaud you use of LXR. Also, "mobile/android/search/java/org/mozilla/search/autocomplete/" might be a problem we should look at sometime later too. Let me guess - it's good for IDEs?

Comment 11

[Eric Edens [:eedens]]

Attachment: p.2.addyahoo.v2.patch

Backed-out the AutoComplete renaming.

Comment 12

[Eric Edens [:eedens]]

Comment on attachment 8456239 [details] [diff] [review]
p.2.addyahoo.v2.patch

Talked with margaret, bnicholson, and mfinkle offline -- Fennec already has a great pattern for making these queries using AsyncTaskQueries, so I'm going to back up and follow that approach.

https://lxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#749

Comment 13

[:Margaret Leibovic]

Zoink!

Comment 14

[:Margaret Leibovic]

Attachment: Use OpenSearch for autocomplete suggestions

I think this is probably good enough to land for right now, but I want to make sure I'm not making any obvious mistakes with the suggestion loader.

Lucas, I'd mainly like your feedback on the changes in SearchFragment.java. To fill you in here, we decided to model the search suggestion autocomplete in the search activity off of what we already do in BrowserSearch. I just copied a version of SuggestClient.java into this part of the tree for now, but the longer term plan would be to use the same version as Fennec, once we require building this search activity as part of Fennec.

For now, I decided to just hard-code in the Yahoo! suggestion template, but in the future we will need to make that more robust to support multiple search engines (we already need to do some refactoring in other places to support that, anyway).

Comment 15

[:Margaret Leibovic]

Also, PR is here: https://github.com/ericedens/FirefoxSearch/pull/23

Comment 16

[Eric Edens [:eedens]]

Comment on attachment 8456540 [details] [diff] [review]
Use OpenSearch for autocomplete suggestions

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

::: mobile/android/search/java/org/mozilla/search/autocomplete/AutoCompleteAdapter.java
@@ +33,5 @@
>              view = (AutoCompleteRowView) convertView;
>          }
>  
>          view.setOnJumpListener(acceptsJumpTaps);
> +        view.setMainText(getItem(position));

It's a good simplification to remove this ATM; if we need different autocomplete rows in the future (eg: URL with site name), then this abstraction would become useful again.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchFragment.java
@@ +144,5 @@
>              }
>          });
>  
> +        // TODO: Don't hard-code this template string
> +        final String template = "https://search.yahoo.com/sugg/ff?output=fxjson&appid=ffm&command=__searchTerms__&nresults=4";

Is the nresults param supposed to match SUGGESTION_MAX from above?

@@ +171,5 @@
>      /**
>       * Handler for clicks of individual items.
>       */
>      @Override
>      public void onItemClick(AdapterView<?> parent, View view, int position, long id) {

We can remove this (or pull the anon listener from above down here)

Comment 17

[:Margaret Leibovic]

Attachment: (v2) Use OpenSearch for autocomplete suggestions

Rebased and addressed comments. I think that this is good enough to land, since it's definitely the direction we want to be going, and we can file follow-ups for any issues that arise.

I will file a new bug for fixing the hard-coded template URL to do something smarter.

Comment 18

[Eric Edens [:eedens]]

Comment on attachment 8457469 [details] [diff] [review]
(v2) Use OpenSearch for autocomplete suggestions

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

Looks good :)

::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchFragment.java
@@ +48,5 @@
> +    // Timeout for the suggestion client to respond
> +    private static final int SUGGESTION_TIMEOUT = 3000;
> +
> +    // Maximum number of results returned by the suggestion client
> +    private static final int SUGGESTION_MAX = 3;

Let's set this to 5

@@ +140,5 @@
>              }
>          });
>  
> +        // TODO: Don't hard-code this template string
> +        final String template = "https://search.yahoo.com/sugg/ff?output=fxjson&appid=ffm&command=__searchTerms__&nresults=4";

Concat with SUGGESTION_MAX instead of `4`

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 8457469 [details] [diff] [review]
(v2) Use OpenSearch for autocomplete suggestions

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

Only looked at the SearchFragment code. It would be nice if you encapsulated the Loader logic behind a separate API so that you can add tests for it. See, for example, how SearchLoader.java does it in Fennec.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchFragment.java
@@ +228,5 @@
> +        public Loader<ArrayList<String>> onCreateLoader(int id, Bundle args) {
> +            // suggestClient is set to null in onDestroyView(), so using it
> +            // safely here relies on the fact that onCreateLoader() is called
> +            // synchronously in restartLoader().
> +            return new SuggestionAsyncLoader(getActivity(), suggestClient, searchBar.getText().toString());

The input text should be passed via the Bundle arguments otherwise you might get all sorts of races between the time the text changed and loader actually being created.

@@ +237,5 @@
> +            autoCompleteAdapter.clear();
> +            for (String s : suggestions) {
> +                autoCompleteAdapter.add(s);
> +            }
> +            autoCompleteAdapter.notifyDataSetChanged();

Encapsulate all this code into a single update() method in AutoCompleteAdapter instead of relying on the caller to do the right thing e.g. an update() call should always trigger notifyDataSetChanged().

@@ +244,5 @@
> +        @Override
> +        public void onLoaderReset(Loader<ArrayList<String>> loader) {
> +            if (autoCompleteAdapter != null) {
> +                autoCompleteAdapter.clear();
> +                autoCompleteAdapter.notifyDataSetChanged();

Same here. A clear() call should always trigger notifyDataSetChanged(). Move it behind the clear() call.

Comment 20

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #19)
> Comment on attachment 8457469 [details] [diff] [review]
> (v2) Use OpenSearch for autocomplete suggestions
> 
> Review of attachment 8457469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Only looked at the SearchFragment code. It would be nice if you encapsulated
> the Loader logic behind a separate API so that you can add tests for it.
> See, for example, how SearchLoader.java does it in Fennec.

Interesting. So if I were to do that, would I just turn SuggestionAsyncLoader into a more encapsulated API that also takes care of doing the LoaderManager calls? I took most of this logic straight from what we do in BrowserSearch, and in there the suggestion loader logic isn't encapsulated in the same way the cursor loader logic is. Maybe I will investigate doing this in a follow-up, since I want to land this basic functionality for us to start using.

> :::
> mobile/android/search/java/org/mozilla/search/autocomplete/SearchFragment.
> java
> @@ +228,5 @@
> > +        public Loader<ArrayList<String>> onCreateLoader(int id, Bundle args) {
> > +            // suggestClient is set to null in onDestroyView(), so using it
> > +            // safely here relies on the fact that onCreateLoader() is called
> > +            // synchronously in restartLoader().
> > +            return new SuggestionAsyncLoader(getActivity(), suggestClient, searchBar.getText().toString());
> 
> The input text should be passed via the Bundle arguments otherwise you might
> get all sorts of races between the time the text changed and loader actually
> being created.

Okay, I'll fix this.

> @@ +237,5 @@
> > +            autoCompleteAdapter.clear();
> > +            for (String s : suggestions) {
> > +                autoCompleteAdapter.add(s);
> > +            }
> > +            autoCompleteAdapter.notifyDataSetChanged();
> 
> Encapsulate all this code into a single update() method in
> AutoCompleteAdapter instead of relying on the caller to do the right thing
> e.g. an update() call should always trigger notifyDataSetChanged().
> 
> @@ +244,5 @@
> > +        @Override
> > +        public void onLoaderReset(Loader<ArrayList<String>> loader) {
> > +            if (autoCompleteAdapter != null) {
> > +                autoCompleteAdapter.clear();
> > +                autoCompleteAdapter.notifyDataSetChanged();
> 
> Same here. A clear() call should always trigger notifyDataSetChanged(). Move
> it behind the clear() call.

Good call on both these comments, I moved more of the logic into AutoCompleteAdapter.

Comment 21

[:Margaret Leibovic]

Attachment: (v3) Use OpenSearch for autocomplete suggestions

Updated to address comments.

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8458117 [details] [diff] [review]
(v3) Use OpenSearch for autocomplete suggestions

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

rs.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SuggestClient.java
@@ +84,5 @@
> +                json = convertStreamToString(in);
> +            } finally {
> +                if (urlConnection != null)
> +                    urlConnection.disconnect();
> +                if (in != null) {

Blergh, you know how I hate this kind of thing :D

More precise try blocks, please!

@@ +93,5 @@
> +                    }
> +                }
> +            }
> +
> +            if (json != null) {

Early return.

@@ +131,5 @@
> +        ConnectivityManager connectivity = (ConnectivityManager) mContext
> +                .getSystemService(Context.CONNECTIVITY_SERVICE);
> +        if (connectivity == null)
> +            return null;
> +        return connectivity.getActiveNetworkInfo();

IIRC all of this is platform version dependent. Comment accordingly?

Comment 23

[:Margaret Leibovic]

fx-team has been closed all day, but we merged this into FirefoxSearch in the mean time:
https://github.com/ericedens/FirefoxSearch/commit/b1f3ff7fd034cb3e623db8896badc11019d11b4b

Comment 24

[:Margaret Leibovic]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fe3df7a66b3