1048198 - SearchBar Search Suggestion is no longer able to disable

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Regressed since Nightly34.0a1

Steps To Reproduce:

1. Click SearchBar icon
2. Select "Manage Search Engines…" to open Search engine manager dialog
3. Turn off check box "Show search suggestions" And Click OK
4. Type something(e.g. mozilla) into SearchBar

Actual Results:
Drop down list include suggestions

Expected Results:
Drop down list should not include suggestions

Comment 1

[Alice0775 White]

Regression window(fx)
Good:
https://hg.mozilla.org/integration/fx-team/rev/94e41f59be31
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140729110404
Bad:
https://hg.mozilla.org/integration/fx-team/rev/54d57bd38f51
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140729113008
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=94e41f59be31&tochange=54d57bd38f51

Regressed by:
54d57bd38f51	Matthew Noorenberghe — Bug 1007979 - Refactor nsSearchSuggestions into a reusable JSM. r=adw Original JSM by mconnor

Please provide automate test too!

Comment 2

[Petruta Horea [:phorea], Desktop Test Engineering]

Reproducible also after right clicking in the Search Bar and deselecting "Show Suggestions".

Comment 3

[Matthew N. [:MattN]]

The code to check _suggestEnabled was accidentally removed.

Marco, this is a regression I caused in bug 1007979 so this should probably be added to the list of bugs for the next iteration.

Comment 4

[Marco Mucci [:MarcoM]]

Forwarding to Gavin to consider including in the Iteration 34.2 Backlog.

(In reply to Matthew N. [:MattN] from comment #3)
> The code to check _suggestEnabled was accidentally removed.
> 
> Marco, this is a regression I caused in bug 1007979 so this should probably
> be added to the list of bugs for the next iteration.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Yep, added to 34.2.

Comment 6

[Matthew N. [:MattN]]

We may want to move that pref checking into the JSM so it applies to all consumers of the module.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Petruta, I assigned Camelia as QA contact since she's the feature owner for preferences, but if you want to take this over, I think you could go ahead since you've already worked on this!

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Confirmed that the tests fail when the tests are run without the other changes and pass with the changes.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8468664 [details] [diff] [review]
Patch

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

Nice tests checking the pref observer!

::: toolkit/components/search/SearchSuggestionController.jsm
@@ +22,5 @@
> +/**
> + * Search suggestions will be shown if gSuggestionsEnabled is true. Global because
> + * only one pref observer is needed for all instances.
> + */
> +let gSuggestionsEnabled = Services.prefs.getBoolPref(BROWSER_SUGGEST_PREF);

Would you mind renaming this to gRemoteEnabled or gRemoteSuggestionsEnabled because I often forget whether it means all suggestions or only remote.

@@ +24,5 @@
> + * only one pref observer is needed for all instances.
> + */
> +let gSuggestionsEnabled = Services.prefs.getBoolPref(BROWSER_SUGGEST_PREF);
> +Services.prefs.addObserver(BROWSER_SUGGEST_PREF, function(aSubject, aTopic, aData) {
> +  gSuggestionsEnabled = Services.prefs.getBoolPref(BROWSER_SUGGEST_PREF);

nsSearchSuggestions.js observed XPCOM shutdown so should this too? I wonder if that was to avoid getting detected as a leak? Might be worth checking out if there is useful history there and/or checking for leaks detected in a debug build.

::: toolkit/components/search/nsSearchSuggestions.js
@@ +5,2 @@
>  const XPCOM_SHUTDOWN_TOPIC              = "xpcom-shutdown";
>  const NS_PREFBRANCH_PREFCHANGE_TOPIC_ID = "nsPref:changed";

These two can go too AFAICT.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Try push to check for leaks, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b7d77d864fb4

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 34.0a1 (buildID: 20140817030204).

Comment 14

[Drew Willcoxon :adw]

Attachment: Aurora/33 patch

This is the patch that actually landed, and it applies cleanly to Aurora/33.

Bug 1054516 is the uplift bug for this feature.

mozilla-aurora try push with this and patches for other bugs mentioned in bug 1054516: https://tbpl.mozilla.org/?tree=Try&rev=fba51f37ff40

Approval Request Comment
[Feature/regressing bug #]:
this bug was caused by bug 1007979, which is needed to uplift search suggestions in about:home/newtab (bug 612453, bug 1028985)

[User impact if declined]:
no search suggestions in about:home/newtab

[Describe test coverage new/current, TBPL]:
currently covered by automated tests on 34; this patch adds tests to 33; see also try link above

[Risks and why]:
this patch alone has low risk; we want about:home/newtab search suggestions on 33 as stated in bug 1054516

[String/UUID change made/needed]:
none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/02feec56dfae

Comment 16

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified that the suggestions are no longer shown if disabled.
Used latest Aurora 33.0a2 20140828004002 under Win 7 64-bit, Ubuntu 13.04 and Mac OSX 10.8.5.