871650 - Restore search suggestions support in editing mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

This has regressed with the changes in bug 869494.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: (1/4) Add missing default constructors to AnimatedHeightLayout

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: (2/4) Factor out method to show soft input in BrowserToolbar

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: (3/4) Factor out cursor loader callbacks into separate class

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: (4/4) Restore search suggestions support

Comment 5

[Lucas Rocha (:lucasr)]

FYI: the opt-in UI will come in a separate patch.

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 760377 [details] [diff] [review]
(4/4) Restore search suggestions support

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

Nice, I like how these loaders are static nested classes that contain local references to the parameters they act on. It reduces the potential for complex threading issues.

::: mobile/android/base/BrowserApp.java
@@ +1818,5 @@
>  
>      @Override
>      public void onUrlOpen(String url) {
>          openUrl(url);
>      }

Maybe add a comment above this one too describing the interface it's implementing?

::: mobile/android/base/BrowserSearch.java
@@ +130,4 @@
>  
>          mUrlOpenListener = null;
> +        mSearchListener = null;
> +        mEditSuggestionListener = null;

What's the point of having all of these null initializations in the constructor?

@@ +165,5 @@
> +        // The search engines list is reused beyond the life-cycle of
> +        // this fragment.
> +        if (mSearchEngines == null) {
> +            mSearchEngines = new ArrayList<SearchEngine>();
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:Get", null));

Since the search engines are held between the fragment being attached and reattached, it might make more sense to put this chunk and the registerEventListener in onCreate(), and unregister in onDestroy(). Those methods are tied to the fragment's existence and not its attachment state, which is the same for mSearchEngines.

@@ +215,5 @@
>      }
>  
> +    @Override
> +    public void handleMessage(String event, final JSONObject message) {
> +        if (event.equals("SearchEngines:Data")) {

You could unregister the listener early here since this is a one-time event.

@@ +238,5 @@
> +        getLoaderManager().restartLoader(SUGGESTION_LOADER_ID, null, mSuggestionLoaderCallbacks);
> +    }
> +
> +    private void setSuggestions(ArrayList<String> suggestions) {
> +        if (mSuggestClient != null) {

Any reason for this null check? mSuggestClient isn't used in the following lines, and I believe mSuggestClient is guaranteed to be non-null anyway since this method is only called in the SuggestionAsyncLoader callbacks.

@@ +342,5 @@
>          if (TextUtils.equals(mSearchTerm, searchTerm)) {
>              return;
>          }
>  
>          mSearchTerm = searchTerm;

mSearchTerm is used in the CursorAdapter, so I think we need a notifyDataSetChanged() here.

It might also be nice if the SearchAdapter had its own local references with corresponding setters, each of which could call notifyDataSetChanged(). That would help reduce the fragility of changing these variables that are scattered all over the place -- up to you though.

@@ +419,5 @@
>      }
>  
> +    private static class SuggestionAsyncLoader extends AsyncTaskLoader<ArrayList<String>> {
> +        private SuggestClient mSuggestClient;
> +        private String mSearchTerm;

Nit: Make both of these final

@@ +505,5 @@
> +        @Override
> +        public int getViewTypeCount() {
> +            // view can be either a standard awesomebar row, a search engine
> +            // row, or a suggestion row
> +            return 3;

I'd factor this out as a constant so it's easy to notice next to the the ROW_* constants.

::: mobile/android/base/SearchEngineRow.java
@@ +61,5 @@
> +        super(context, attrs, defStyle);
> +
> +        mUrlOpenListener = null;
> +        mSearchListener = null;
> +        mEditSuggestionListener = null;

These are also redundant. I guess I can tolerate these null initializations if you really like them, but maybe it'd be better to put them at the variable declarations?

@@ +106,5 @@
> +        mUserEnteredView.setOnClickListener(mClickListener);
> +
> +        mUserEnteredTextView = (TextView) findViewById(R.id.suggestion_text);
> +
> +        mSearchEngine = null;

Same here about the null initialization...unnecessarily takes up space IMO.

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Comment on attachment 760377 [details] [diff] [review]
> (4/4) Restore search suggestions support
> 
> Review of attachment 760377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, I like how these loaders are static nested classes that contain local
> references to the parameters they act on. It reduces the potential for
> complex threading issues.

This is actually a requirement for Loaders for exactly this reason. They cannot be non-static inner classes. The framework throws an exception if you try to do it differently.

> ::: mobile/android/base/BrowserApp.java
> @@ +1818,5 @@
> >  
> >      @Override
> >      public void onUrlOpen(String url) {
> >          openUrl(url);
> >      }
> 
> Maybe add a comment above this one too describing the interface it's
> implementing?

Yep, done.  

> ::: mobile/android/base/BrowserSearch.java
> @@ +130,4 @@
> >  
> >          mUrlOpenListener = null;
> > +        mSearchListener = null;
> > +        mEditSuggestionListener = null;
> 
> What's the point of having all of these null initializations in the
> constructor?

This is just a C-ism of mine (you have to explicitly initialize variables). Generally speaking though, I prefer to be explicit about state initialization instead of scattering it over different parts of the code.

> @@ +165,5 @@
> > +        // The search engines list is reused beyond the life-cycle of
> > +        // this fragment.
> > +        if (mSearchEngines == null) {
> > +            mSearchEngines = new ArrayList<SearchEngine>();
> > +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:Get", null));
> 
> Since the search engines are held between the fragment being attached and
> reattached, it might make more sense to put this chunk and the
> registerEventListener in onCreate(), and unregister in onDestroy(). Those
> methods are tied to the fragment's existence and not its attachment state,
> which is the same for mSearchEngines.

Good point. Had to add a null-check on the adapter in setSearchEngines() to account for the case where gecko returns before the fragment is attached to the activity.

> @@ +215,5 @@
> >      }
> >  
> > +    @Override
> > +    public void handleMessage(String event, final JSONObject message) {
> > +        if (event.equals("SearchEngines:Data")) {
> 
> You could unregister the listener early here since this is a one-time event.

This will make the register/unregister calls potentially asymmetric which might cause exceptions. I would have to check if gecko has already returned the search engines during fragment destruction which would just add unnecessary complexity. There's no harm in leaving the listener registered until the fragment is destroyed. So, let's keep things simple. 

> @@ +238,5 @@
> > +        getLoaderManager().restartLoader(SUGGESTION_LOADER_ID, null, mSuggestionLoaderCallbacks);
> > +    }
> > +
> > +    private void setSuggestions(ArrayList<String> suggestions) {
> > +        if (mSuggestClient != null) {
> 
> Any reason for this null check? mSuggestClient isn't used in the following
> lines, and I believe mSuggestClient is guaranteed to be non-null anyway
> since this method is only called in the SuggestionAsyncLoader callbacks.

Indeed. Removed.

> @@ +342,5 @@
> >          if (TextUtils.equals(mSearchTerm, searchTerm)) {
> >              return;
> >          }
> >  
> >          mSearchTerm = searchTerm;
> 
> mSearchTerm is used in the CursorAdapter, so I think we need a
> notifyDataSetChanged() here.

Nice catch, fixed.

> It might also be nice if the SearchAdapter had its own local references with
> corresponding setters, each of which could call notifyDataSetChanged(). That
> would help reduce the fragility of changing these variables that are
> scattered all over the place -- up to you though.

Good point. Although replicating the state in the adapter might also lead to confusion. There's only one place where search term is set. Let's keep things simple for now. 

> @@ +419,5 @@
> >      }
> >  
> > +    private static class SuggestionAsyncLoader extends AsyncTaskLoader<ArrayList<String>> {
> > +        private SuggestClient mSuggestClient;
> > +        private String mSearchTerm;
> 
> Nit: Make both of these final

Done.

> @@ +505,5 @@
> > +        @Override
> > +        public int getViewTypeCount() {
> > +            // view can be either a standard awesomebar row, a search engine
> > +            // row, or a suggestion row
> > +            return 3;
> 
> I'd factor this out as a constant so it's easy to notice next to the the
> ROW_* constants.

Done.

> ::: mobile/android/base/SearchEngineRow.java
> @@ +61,5 @@
> > +        super(context, attrs, defStyle);
> > +
> > +        mUrlOpenListener = null;
> > +        mSearchListener = null;
> > +        mEditSuggestionListener = null;
> 
> These are also redundant. I guess I can tolerate these null initializations
> if you really like them, but maybe it'd be better to put them at the
> variable declarations?

My approach to initialization is that it should happen all in place (usually the constructor) for clarity. This is why I try to avoid initializing stuff on declaration.

> @@ +106,5 @@
> > +        mUserEnteredView.setOnClickListener(mClickListener);
> > +
> > +        mUserEnteredTextView = (TextView) findViewById(R.id.suggestion_text);
> > +
> > +        mSearchEngine = null;
> 
> Same here about the null initialization...unnecessarily takes up space IMO.

I don't feel strongly about it. So, removed.

Comment 8

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/projects/fig/rev/d179425dcd43
http://hg.mozilla.org/projects/fig/rev/5d3cb1e097d4
http://hg.mozilla.org/projects/fig/rev/6509a6fd9bd5
http://hg.mozilla.org/projects/fig/rev/ea79867c3ffd

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d179425dcd43
https://hg.mozilla.org/mozilla-central/rev/5d3cb1e097d4
https://hg.mozilla.org/mozilla-central/rev/6509a6fd9bd5
https://hg.mozilla.org/mozilla-central/rev/ea79867c3ffd