1233602 - Search history items don't appear if search engine suggestions are disabled

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andrzej Hunt :ahunt]

1. Check that Search History is enabled, Search suggestions are disabled
(Settings->Search->"Show search suggestions"=false / "Show search history"=true)
2. Kill and restart firefox
3. Make a search, e.g. "cat" (tap urlbar, type cat, select the first search item)
4. Select urlbar, type "ca"

Expected: search row shows "ca", "@cat" (replace @ with clock icon)
Actual: search row shows "ca" only

Comment 1

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1233602 - Don't depend on search suggestions to show search history r=liuche

Review commit: https://reviewboard.mozilla.org/r/28561/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28561/

Comment 2

[Chenxia Liu [:liuche]]

Comment on attachment 8700062 [details]
MozReview Request: Bug 1233602 - Don't depend on search suggestions to show search history r=liuche

https://reviewboard.mozilla.org/r/28561/#review25619

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:662
(Diff revision 1)
> +            final SharedPreferences prefs = GeckoSharedPrefs.forApp(getContext());

Great, this is exactly what we need to check. Don't put it in setSearchEngines though, because search history should have nothing to do with search engines.

It looks like updateSearchEngines (which calls setSearchEngines) is called in onResume, so we should do this there, because we'll want to check every time we open up search in case that pref has been changed.

Comment 3

[Andrzej Hunt :ahunt]

Comment on attachment 8700062 [details]
MozReview Request: Bug 1233602 - Don't depend on search suggestions to show search history r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28561/diff/1-2/

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 8700062 [details]
MozReview Request: Bug 1233602 - Don't depend on search suggestions to show search history r=liuche

https://reviewboard.mozilla.org/r/28561/#review25667

::: mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java:719
(Diff revision 2)
> +        final SharedPreferences prefs = GeckoSharedPrefs.forApp(getContext());

Sorry, I wasn't clear enough - search history should be disjoint from search engines so we should put this in onResume (not in updateSearchEngineBar, or any of the search engine methods).

Comment 5

[Andrzej Hunt :ahunt]

Comment on attachment 8700062 [details]
MozReview Request: Bug 1233602 - Don't depend on search suggestions to show search history r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28561/diff/2-3/

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8700062 [details]
MozReview Request: Bug 1233602 - Don't depend on search suggestions to show search history r=liuche

https://reviewboard.mozilla.org/r/28561/#review25693

Comment 7

[Pulsebot]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6ce05f1d409

Comment 9

[Teodora Vermesan (:TeoVermesan)]

Search for "cat" and after that for "ca", "cat" is displayed as a search history near "ca" in the search row, so:
Verified as fixed using:
Device: Nexus 7 (Android 5.1.1)
Build: Firefox for Android 46.0a1 (2015-01-06)

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8700062 [details]
MozReview Request: Bug 1233602 - Don't depend on search suggestions to show search history r=liuche

Approval Request Comment
[Feature/regressing bug #]: Bug 1200367 - add a separate search history pref
[User impact if declined]: History suggestions aren't shown if search history is enabled but search engine aren't enabled.
[Describe test coverage new/current, TreeHerder]: Manual testing, verified by QA in trunk.
[Risks and why]: Medium risk - a new variable added to track the history preference, which is used when determining whether to process suggestions.
[String/UUID change made/needed]: none

Note: a separate patch is needed for beta, due to path changes, and will be added in the next comment.

Comment 11

[Andrzej Hunt :ahunt]

Attachment: history_without_suggestions_beta.patch

Here is a separate backport for beta - identical to aurora/trunk except for patch changes.

Approval Request Comment
[Feature/regressing bug #]: Bug 1200367 - add a separate search history pref
[User impact if declined]: History suggestions aren't shown if search history is enabled but search engine aren't enabled.
[Describe test coverage new/current, TreeHerder]: Manual testing, verified by QA in trunk.
[Risks and why]: Medium risk - a new variable added to track the history preference, which is used when determining whether to process suggestions.
[String/UUID change made/needed]: none

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8705249 [details] [diff] [review]
history_without_suggestions_beta.patch

This is another key scenario, taking it to Beta44.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700062 [details]
MozReview Request: Bug 1233602 - Don't depend on search suggestions to show search history r=liuche

Aurora45+

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1cb670465ef

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2285043ff5c6

Comment 16

[Carsten Book [:Tomcat]]

beta uplift failed:

applying history_without_suggestions_beta.patch
patching file mobile/android/base/home/BrowserSearch.java
Hunk #1 FAILED at 14
Hunk #2 FAILED at 22
Hunk #3 FAILED at 35
Hunk #4 succeeded at 158 with fuzz 2 (offset -1 lines).
Hunk #5 succeeded at 272 with fuzz 2 (offset 2 lines).
Hunk #6 FAILED at 593
4 out of 6 hunks FAILED -- saving rejects to file mobile/android/base/home/BrowserSearch.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh history_without_suggestions_beta.patch

Comment 17

[Andrzej Hunt :ahunt]

(In reply to Carsten Book [:Tomcat] from comment #16)
> beta uplift failed:
> 
> applying history_without_suggestions_beta.patch
> patching file mobile/android/base/home/BrowserSearch.java
> Hunk #1 FAILED at 14
> Hunk #2 FAILED at 22
> Hunk #3 FAILED at 35
> Hunk #4 succeeded at 158 with fuzz 2 (offset -1 lines).
> Hunk #5 succeeded at 272 with fuzz 2 (offset 2 lines).
> Hunk #6 FAILED at 593
> 4 out of 6 hunks FAILED -- saving rejects to file
> mobile/android/base/home/BrowserSearch.java.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh
> history_without_suggestions_beta.patch

It looks like the patch is already in beta (and trying to apply it again gives me the same output as you got):

http://hg.mozilla.org/releases/mozilla-beta/rev/2285043ff5c6

Comment 18

[Flaviu Cos]

Verified as fixed in builds:
- Firefox 44 Beta 8;
- 45.0a2 2016-01-12;
Device: Nexus 5 (Android 6.0.1);