Closed Bug 1206628 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox43 --- unaffected
firefox44 --- verified
fennec 44+ ---

People

(Reporter: TeoVermesan, Assigned: psd)

References

Details

Attachments

(1 file, 4 obsolete files)

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
tracking-fennec: --- → ?
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).
Depends on: 1186683
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.
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 43+
Assignee: michael.l.comella → prabhjyotsingh95
Attached patch 1206628.patch (obsolete) — Splinter Review
This fixes bug 1206639 as well! :)
Attachment #8668000 - Flags: review?(michael.l.comella)
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?
Attached patch 1206628.patch (obsolete) — Splinter Review
@Margaret, Thanks! updated :)
Attachment #8668000 - Attachment is obsolete: true
Attachment #8668000 - Flags: review?(michael.l.comella)
Attachment #8668301 - Flags: review?(michael.l.comella)
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.
Attachment #8668301 - Flags: review?(michael.l.comella) → feedback+
Attached patch 1206628-2.patch (obsolete) — Splinter Review
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
Attachment #8668301 - Attachment is obsolete: true
Attachment #8672309 - Flags: review?(michael.l.comella)
Blocks: 1207340
Duplicate of this bug: 1206639
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.
Attachment #8672309 - Flags: review?(michael.l.comella) → feedback+
Attached patch 1206628-3.patch (obsolete) — Splinter Review
Attachment #8672309 - Attachment is obsolete: true
Attachment #8673794 - Flags: review?(michael.l.comella)
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 < ...`
Attachment #8673794 - Flags: review?(michael.l.comella) → feedback+
Attached patch 1206628-4.patchSplinter Review
Attachment #8673794 - Attachment is obsolete: true
Attachment #8675225 - Flags: review?(michael.l.comella)
Comment on attachment 8675225 [details] [diff] [review]
1206628-4.patch

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

Nice! Thanks Prabhjyot!
Attachment #8675225 - Flags: review?(michael.l.comella) → review+
https://hg.mozilla.org/mozilla-central/rev/8ea4740134e1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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)
You need to log in before you can comment on or make changes to this bug.