SearchBar Search Suggestion is no longer able to disable

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: alice0775, Assigned: jaws)

Tracking

({regression})

34 Branch
Firefox 34
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox33 verified, firefox34+ verified)

Details

Attachments

(2 attachments)

[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!
Blocks: 1007979
Keywords: regression
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.
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
OS: Windows 7 → All
Hardware: x86_64 → All
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.
Flags: needinfo?(mmucci) → needinfo?(gavin.sharp)
Yep, added to 34.2.
Flags: needinfo?(gavin.sharp)
QA Whiteboard: [qa+]
We may want to move that pref checking into the JSM so it applies to all consumers of the module.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 34.2
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!
QA Contact: camelia.badau
Posted patch PatchSplinter Review
Confirmed that the tests fail when the tests are run without the other changes and pass with the changes.
Attachment #8468664 - Flags: review?(MattN+bmo)
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.
Attachment #8468664 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/integration/fx-team/rev/8cb9028743ac
Flags: in-testsuite+
Whiteboard: [fixed in fx-team]
https://hg.mozilla.org/mozilla-central/rev/8cb9028743ac
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team]
Target Milestone: --- → Firefox 34
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 34.0a1 (buildID: 20140817030204).
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
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
Attachment #8478732 - Flags: approval-mozilla-aurora?
Attachment #8478732 - Attachment is patch: true
Attachment #8478732 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.