[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

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!

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

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.

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.

Yep, added to 34.2.

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

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!

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

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.

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

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

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

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.