1064880 - Keep track of the current search engine in MainActivity

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

I want to do some refactoring to keep track of the current search engine in MainActivity, since this will help us get the engine branding for bug 1049600.

Comment 1

[:Margaret Leibovic]

Attachment: Keep track of the current search engine in MainActivity

This patch moves the SearchEngineManager logic out of SuggestionsFragment and PostSearchFragment, centralizing it in MainActivity. This will give us more flexibility as more things (like the branding) depend on knowing about the current search engine.

Comment 2

[:Margaret Leibovic]

Attachment: Keep track of the current search engine in MainActivity

Un-bitrotted. Also flagging nalexander in case he has time to get to this first.

I'll want this for my patch for bug 1049600.

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8487825 [details] [diff] [review]
Keep track of the current search engine in MainActivity

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

This looks okay to me, but:

1) Please be explicit about the engine concurrency regime.

2) I think you should strongly consider re-creating all your fragments when the engine changes.  Your MainActivity (or something else) "owns" the current engine; everything else maintains a final reference to the current engine.  No concurrency, no updating, no thinking.  No threaded update callbacks.  The Activity has an "engine Loader" and it rolls out changes as the Loader updates its state.  Food for thought.

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +56,5 @@
>      private SearchState searchState = SearchState.PRESEARCH;
>      private EditState editState = EditState.WAITING;
>  
> +    private SearchEngineManager searchEngineManager;
> +    private SearchEngine engine;

Add comments explaining what the concurrency regime around engine is.  What would it take to make searchEngineManager final?

@@ +252,5 @@
>          }
>      }
>  
> +    private void startSearch(final String query) {
> +        if (this.engine != null) {

Why |this.engine|?

@@ +257,5 @@
> +            postSearchFragment.startSearch(engine, query);
> +            return;
> +        }
> +
> +        // This will only happen if startSearch is called before the getEngine

What is "This" here?  You mean that we'll only see |this.engine == null| in this case?

@@ +261,5 @@
> +        // This will only happen if startSearch is called before the getEngine
> +        // call in onCreate is completed.
> +        searchEngineManager.getEngine(new SearchEngineCallback() {
> +            @Override
> +            public void execute(SearchEngine engine) {

I'd prefer not to shadow, here, since you're explicitly handling an unusual case.  How about |foundEngine|?

@@ +267,5 @@
> +            }
> +        });
> +    }
> +
> +    // SearchEngineCallback implementation

This comment isn't useful; our tools will tell us what you're implementing.  What would be useful is when this is called, and what you're doing here.  Will this ever be called more than once (for initialization)?  Ah, I see the answer is yes; you'll see this whenever the search engine is changed.  OK.

@@ +269,5 @@
> +    }
> +
> +    // SearchEngineCallback implementation
> +    @Override
> +    public void execute(SearchEngine engine) {

Be explicit about what thread this is invoked on.  I'm not convinced your use of |this.engine| is concurrent-safe.

Comment 4

[:Margaret Leibovic]

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8487825 [details] [diff] [review]
> Keep track of the current search engine in MainActivity
> 
> Review of attachment 8487825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks okay to me, but:
> 
> 1) Please be explicit about the engine concurrency regime.
> 
> 2) I think you should strongly consider re-creating all your fragments when
> the engine changes.  Your MainActivity (or something else) "owns" the
> current engine; everything else maintains a final reference to the current
> engine.  No concurrency, no updating, no thinking.  No threaded update
> callbacks.  The Activity has an "engine Loader" and it rolls out changes as
> the Loader updates its state.  Food for thought.

Yeah, our current use of fragments doesn't really take advantage of fragments (it treats them more like custom views). This does seem like a good opprotunity to 

> ::: mobile/android/search/java/org/mozilla/search/MainActivity.java
> @@ +56,5 @@
> >      private SearchState searchState = SearchState.PRESEARCH;
> >      private EditState editState = EditState.WAITING;
> >  
> > +    private SearchEngineManager searchEngineManager;
> > +    private SearchEngine engine;
> 
> Add comments explaining what the concurrency regime around engine is.

I'll add a comment explaining that engine should only be used on the main thread. The search activity is very UI-centric, so in general everything runs on the main thread, except for where we explicitly use async tasks.

> What would it take to make searchEngineManager final?

We would have to refactor SearchEngineManager to handle dynamically register/unregister its SharedPreferencesListener, since we'd still want to unregister the listener in onDestroy().

> @@ +252,5 @@
> >          }
> >      }
> >  
> > +    private void startSearch(final String query) {
> > +        if (this.engine != null) {
> 
> Why |this.engine|?

I think in one version of this patch I had this method take `engine` as a parameter and set the member variable... or something like that... in any case, I'll remove the `this` :)

> @@ +257,5 @@
> > +            postSearchFragment.startSearch(engine, query);
> > +            return;
> > +        }
> > +
> > +        // This will only happen if startSearch is called before the getEngine
> 
> What is "This" here?  You mean that we'll only see |this.engine == null| in
> this case?

Correct, I'll clarify that comment.

> @@ +261,5 @@
> > +        // This will only happen if startSearch is called before the getEngine
> > +        // call in onCreate is completed.
> > +        searchEngineManager.getEngine(new SearchEngineCallback() {
> > +            @Override
> > +            public void execute(SearchEngine engine) {
> 
> I'd prefer not to shadow, here, since you're explicitly handling an unusual
> case.  How about |foundEngine|?

Not sure I follow. You mean for the parameter name?

> @@ +267,5 @@
> > +            }
> > +        });
> > +    }
> > +
> > +    // SearchEngineCallback implementation
> 
> This comment isn't useful; our tools will tell us what you're implementing. 
> What would be useful is when this is called, and what you're doing here. 
> Will this ever be called more than once (for initialization)?  Ah, I see the
> answer is yes; you'll see this whenever the search engine is changed.  OK.

Sorry, I can make that comment better. I found this comment useful myself because `execute` is a pretty vague method name, although I suppose the SearchEngine parameter makes it clear that this has something to do with the search engine.

> @@ +269,5 @@
> > +    }
> > +
> > +    // SearchEngineCallback implementation
> > +    @Override
> > +    public void execute(SearchEngine engine) {
> 
> Be explicit about what thread this is invoked on.  I'm not convinced your
> use of |this.engine| is concurrent-safe.

We designed SearchEngineManager to always run `execute` on the main thread:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java#53
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java#84

But I can add a comment here as well to make that clear without needing to look at the SearchEngineManager implementation.

Comment 5

[:Margaret Leibovic]

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

Oops, didn't finish this thought.

> > 2) I think you should strongly consider re-creating all your fragments when
> > the engine changes.  Your MainActivity (or something else) "owns" the
> > current engine; everything else maintains a final reference to the current
> > engine.  No concurrency, no updating, no thinking.  No threaded update
> > callbacks.  The Activity has an "engine Loader" and it rolls out changes as
> > the Loader updates its state.  Food for thought.
> 
> Yeah, our current use of fragments doesn't really take advantage of
> fragments (it treats them more like custom views). This does seem like a
> good opprotunity to 

... think about this, but it feels out of the scope of this particular bug, since the main goal here is to do enough refactoring to let me implement our branding designs.

Comment 6

[:Margaret Leibovic]

Landed with comments addressed:
https://hg.mozilla.org/integration/fx-team/rev/6c5e6061b704
https://github.com/mozilla/fennec-search/commit/ddcf5c6610e0210d57016e8603a3b3b8376e7acf

We can look into resetting the fragments in a separate bug.

Comment 7

[Carsten Book [:Tomcat]]

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