1206628 - Newly added search engine is not displayed in the quick search bar only after going to the search submenu from Settings

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Moto X (Android 4.4)
Build: Firefox for Android 43.0a1 (2015-09-20)

Steps to reproduce:
1. Visit imdb.com
2. Tap in the search field to bring up the action bar. 
3. Choose "Add Search Engine"
4. Open a new tab and tap "test" in URL Bar
5. Scroll the quick search bar to choose "imdb" search engine

Expected results:
- "imdb" search engine is present in the quick search bar


Actual results:
- "imdb" is not present in the quick search bar
- after going to Settings -> Customize -> Search and then go back three times to be at about:home, tap something in the URL Bar and scroll the quick search bar, imdb is displayed

Note:
-good build: 17-09
-bad build: 18-09
pushlog:http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7d613b3bcfe1e865378bfac37de64560d1234ec&tochange=11dc79e232110ba6de5179e46dfbda77b52a88c3

Comment 1

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

This is a pre-existing bug uncovered by bug 1186683, which no longer creates BrowserSearch (and thus requests the search engines) each time it is shown.

When the search engines get updated, we should make sure that we 1) send out an updated list of search engines to holders and 2) update this list in BrowserSearch. Alternatively, we can retrieve a list of search engines each time BrowserSearch is shown and only update the list when there is a change (which will cause a visible refresh to the user).

Comment 2

[Prabhjyot Sodhi [:psd]]

A simple solution could be to remove the BrowserSearch fragment each time the search engines are updated. And given that users won't modify that list very often, this solution should suffice.

Or we could what Michael suggested. I'll go through the code.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

We backed out bug 1186683 from 43.

Comment 4

[Prabhjyot Sodhi [:psd]]

Attachment: 1206628.patch

This fixes bug 1206639 as well! :)

Comment 5

[:Margaret Leibovic]

Comment on attachment 8668000 [details] [diff] [review]
1206628.patch

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

::: mobile/android/base/home/BrowserSearch.java
@@ +230,5 @@
>      }
>  
>      @Override
> +    public void onHiddenChanged(boolean hidden) {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:GetVisible", null));

Shouldn't we only do this when the activity becomes visible, not when it's both hidden and made visible?

Comment 6

[Prabhjyot Sodhi [:psd]]

Attachment: 1206628.patch

@Margaret, Thanks! updated :)

Comment 7

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8668301 [details] [diff] [review]
1206628.patch

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

This appears to fix the issue but I think it's unfortunate that we:
  1) need to poll for new search engines (rather than being notified when the list gets updated)
  2) don't short-circuit the search engine update if the search engines have not changed, causing a re-layout

I don't think there's a good way to fix #1 – I searched the code for "SearchEngines:" and couldn't find any relevant mesages when the search engines are updated and adding one is likely a nightmare.

However, I think we can prevent #2 and since layout is expensive, I think we should. We can do this by comparing their names and maybe if the search suggestions are enabled.

Comment 8

[Prabhjyot Sodhi [:psd]]

Attachment: 1206628-2.patch

Added a new function to check if there is a difference in the current mSearchEngines and the received searchEngines, and re-layout'ing if there is

Comment 10

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8672309 [details] [diff] [review]
1206628-2.patch

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

Looking pretty good, just a few nits.

I'll be sure to be faster on the turn around review. :)

::: mobile/android/base/home/BrowserSearch.java
@@ +567,5 @@
>          mSearchHistorySuggestions = savedSuggestions;
>          mAdapter.notifyDataSetChanged();
>      }
>  
> +    private boolean checkForUpdatesInList(ArrayList<SearchEngine> searchEngines) {

nit: -> shouldCheckForUpdatesInSearchEngines

"should" is kind of a convention that makes `if (shouldCheck...)` more readable and the function obviously of return type boolean (e.g. `checkForUpdatesInList` sounds like it has side effects).

Also, I don't know what list is

@@ +575,5 @@
> +
> +        Iterator newEngines = searchEngines.iterator();
> +        Iterator oldEngines = mSearchEngines.iterator();
> +
> +        while (oldEngines.hasNext()) {

This happens often enough that, to avoid the memory churn of allocating Iterators, I'd say we should use standard for loops since we have ArrayLists (if you have questions about this, let me know).

However, for future reference, the outer loop would have been a good place for for-each notation:

for (SearchEngine engine : oldEngines) ...

@@ +642,1 @@
>              mSearchEngines = Collections.unmodifiableList(searchEngines);

We shouldn't update these variables, nor updateSearchEngineBar if the search engines are not updated.

We should still show the suggestions opt in, however.

Comment 11

[Prabhjyot Sodhi [:psd]]

Attachment: 1206628-3.patch

Comment 12

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8673794 [details] [diff] [review]
1206628-3.patch

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

Pretty close – just a few more nits because I was unclear last time!

::: mobile/android/base/home/BrowserSearch.java
@@ +567,5 @@
>          mSearchHistorySuggestions = savedSuggestions;
>          mAdapter.notifyDataSetChanged();
>      }
>  
> +    private boolean shouldCheckForUpdatesInSearchEngines(ArrayList<SearchEngine> searchEngines) {

nit: -> shouldUpdateSearchEngines

Sorry, I screwed up there. fwiw, if you think something I say is wrong, you are welcome to question me! :)

@@ +574,5 @@
> +        }
> +
> +        int engineNum = 0;
> +
> +        for (SearchEngine engine : mSearchEngines) {

Sorry, I was unclear. The for-each loop allocates an Iterator under the hood – I just wanted to let you know about the notation in case you weren't aware.

Can you make this a traditional for loop? `for (int i = 0; i < ...`

Comment 13

[Prabhjyot Sodhi [:psd]]

Attachment: 1206628-4.patch

Comment 14

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8675225 [details] [diff] [review]
1206628-4.patch

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

Nice! Thanks Prabhjyot!

Comment 15

[Prabhjyot Sodhi [:psd]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=940234d6d914

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ea4740134e1

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Newly added search engine is displayed in the quick search bar.
Also after changing the default search engine, its favicon is displayed in BrowserSearch screen.
Verified as fixed using:
Device: Samsung S5 (Android 4.4.4)
Build: Firefox for Android 44.0a1 (2015-10-23)