Restore search suggestions support in editing mode

RESOLVED FIXED in Firefox 26

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 26
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-fig)

Attachments

(4 attachments)

(Assignee)

Description

6 years ago
This has regressed with the changes in bug 869494.
(Assignee)

Updated

6 years ago
Depends on: 877870
(Assignee)

Updated

6 years ago
Assignee: nobody → lucasr.at.mozilla
(Assignee)

Comment 1

6 years ago
Created attachment 760374 [details] [diff] [review]
(1/4) Add missing default constructors to AnimatedHeightLayout
Attachment #760374 - Flags: review?(bnicholson)
(Assignee)

Comment 2

6 years ago
Created attachment 760375 [details] [diff] [review]
(2/4) Factor out method to show soft input in BrowserToolbar
Attachment #760375 - Flags: review?(bnicholson)
(Assignee)

Comment 3

6 years ago
Created attachment 760376 [details] [diff] [review]
(3/4) Factor out cursor loader callbacks into separate class
Attachment #760376 - Flags: review?(bnicholson)
(Assignee)

Comment 4

6 years ago
Created attachment 760377 [details] [diff] [review]
(4/4) Restore search suggestions support
Attachment #760377 - Flags: review?(bnicholson)
(Assignee)

Comment 5

6 years ago
FYI: the opt-in UI will come in a separate patch.
Attachment #760374 - Flags: review?(bnicholson) → review+
Attachment #760375 - Flags: review?(bnicholson) → review+
Attachment #760376 - Flags: review?(bnicholson) → review+
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.
Attachment #760377 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 882185
You need to log in before you can comment on or make changes to this bug.