To properly enable search suggestions respecting user choice we need to store the user-made choice, along with the fact the user made a choice. This will need to be uplifted to Beta 54 and ESR 52.

Ritu, what's the deadline for a mostly-safe patch (we basically need to mirror a preference value to another pref)

Regardless of what we will end up doing, this shouldn't hurt anyone.

Comment on attachment 8866702 [details] Bug 1364002 - Store the user-made search suggestion choice along with userMadeSearchSuggestionChoice. https://reviewboard.mozilla.org/r/138320/#review141568 Doesn't the constructor run for each window? Ideally we would run that check once per Firefox session, so if it can be in nsBrowserGlue.js that would be ideal. Not super-critical though, as I expect your patch in bug 1344924 to remove this code in 55.

(In reply to Panos Astithas [:past] (please needinfo?) from comment #4) > Doesn't the constructor run for each window? Ideally we would run that check > once per Firefox session, so if it can be in nsBrowserGlue.js that would be > ideal. well yes, I was trying to keep the code together, the only cost we pay is a single call to this._prefs.prefHasUserValue("searchSuggestionsChoice") after the pref is set. Bug 1344924 won't completely remove this code, it will still need to check that searchSuggestionsChoice == suggest.searches or change the user value of suggest.searches. Potentially we could do that in a UI migration, if we don't care about downgrade/upgrade paths. Indeed if the user would downgrade from 55 to 54 with the user pref set to false, then on upgrade we'd enable suggestions :/ If we care about the downgrade path, then we should either do that directly in nsBrowserGlue::_finalUIStartup (not great imo, there's too much unrelated stuff there) or keep doing it in the constructor and accept to pay a trivial cost of fetching a couple prefs on window open (not that common). Do you prefer me to move this code to _finalUIStartup?

It seems cleaner to me to do it in _migrateUI (or _finalUIStartup, but _migrateUI is all about similar pref migrations). I agree that the cost is not great, but people are right now trying to squeeze every last ounce of performance out of browser startup and window open, and it would go against their work to take the easy path here. Maybe 55 is too soon, but shouldn't we stop checking searchSuggestionsChoice in 56 or 57? I don't think we need to support downgrades across 2 or 3 releases. In that case maybe taking the runtime hit in the constructor would be OK.

yes, we can remove it in 56 and ignore edge-cases. As I said in comment 5, migrateUI is not the right place, it would break on downgrades (we don't downgrade the ui version).

OK, then both options are fine to me. Downgrades are likely to cause much worse problems than inadvertently enabling search suggestions.

I hurried too much, what can't be in migrateUI is the change needed for Bug 1344924. The change here must run just once so it sounds ok! Btw, working on a new version of the patch.

if not that we just landed 2 new ui migrations in 55 :(

Panos, do you wanna have a second look since the patch changed?

Looks good to me. I would add a comment that we will be removing this code soon and add a reference to the removal bug.

(In reply to Marco Bonardo [::mak] from comment #1) > Ritu, what's the deadline for a mostly-safe patch (we basically need to > mirror a preference value to another pref) Hi Marco, I think it depends on what kind of a change you are making. Also, is this planned for 55 or 54? Gerry, FYI

(In reply to Ritu Kothari (:ritu) from comment #14) > (In reply to Marco Bonardo [::mak] from comment #1) > > Ritu, what's the deadline for a mostly-safe patch (we basically need to > > mirror a preference value to another pref) > > Hi Marco, I think it depends on what kind of a change you are making. Also, > is this planned for 55 or 54? Gerry, FYI both 55 and 54.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/5498721c72fa Store the user-made search suggestion choice along with userMadeSearchSuggestionChoice. r=past

Comment on attachment 8866702 [details] Bug 1364002 - Store the user-made search suggestion choice along with userMadeSearchSuggestionChoice. Approval Request Comment [Feature/Bug causing the regression]: Search suggestions in the location bar [User impact if declined]: We won't be able to respect user choice regarding search suggestions in the planned 55 opt-out feature [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: We are just mirroring a pref [String changes made/needed]: none

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected and any side effect on a latest Nightly build? Thanks!

Simona, please verify this on latest Nightly.

I want to clarify what the patch does, since that may help QA: 1. if the user had previously (previous build) answered to the urlbar opt-in notification, we will store a browser.urlbar.searchSuggestionChoice pref with the same value as browser.urlbar.suggest.searches 2. if browser.urlbar.suggest.searches changes, we mirror the new value in browser.urlbar.searchSuggestionChoice

Verified as fixed by updating an older version of Nightly 55 (Build ID: 20170510030301) to the latest Nightly 55 (Build ID: 20170517030204) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12. During verification I covered the following scenarios: 1. Ignored the URL bar opt-in notification and updated: - the preference browser.urlbar.searchSuggestionsChoice is not displayed in about:config - the URL bar opt-in notification is displayed when typing in the URL bar asking if I want to enable the search suggestions. 2. Selected Yes in the URL bar opt-in notification and updated Nightly: - the preference browser.urlbar.searchSuggestionsChoice is displayed in about:config and it is set to true (the same value that the preference browser.urlbar.suggest.searches have) - search suggestions are enabled 3. Selected No in the URL bar opt-in notification and updated Nightly: - the preference browser.urlbar.searchSuggestionsChoice is displayed in about:config and it is set to false(the same value that the preference browser.urlbar.suggest.searches have) - search suggestions are disabled 4. Selected Yes in the URL bar opt-in notification, but in about:preferences unchecked "Show search suggestions in location bar results" and updated - the preference browser.urlbar.searchSuggestionsChoice is displayed in about:config and it is set to false(the same value that the preference browser.urlbar.suggest.searches have) - search suggestions are disabled 5. Selected No in the URL bar opt-in notification, but in about:preferences unchecked "Show search suggestions in location bar results" and updated - the preference browser.urlbar.searchSuggestionsChoice is displayed in about:config and it is set to true (the same value that the preference browser.urlbar.suggest.searches have) - search suggestions are enabled

Comment on attachment 8866702 [details] Bug 1364002 - Store the user-made search suggestion choice along with userMadeSearchSuggestionChoice. Polish a UI behavior regarding search suggestions and was verified. Beta54+. Should be in 54 beta 9.

needs a rebase for beta grafting 417435:5498721c72fa "Bug 1364002 - Store the user-made search suggestion choice along with userMadeSearchSuggestionChoice. r=past" merging browser/base/content/urlbarBindings.xml merging browser/components/nsBrowserGlue.js warning: conflicts while merging browser/components/nsBrowserGlue.js! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')

should be a trivial rebase, beta has just less UI migrations in browserGlue. I'll directly take care of that landing.

Doesn't apply to esr either.

doesn't the beta patch apply?

The Beta patch (comment 27) applies cleanly to ESR52 for me.

OK, thanks & sorry!

For ESR approval, please also see bug 1368477, it handles an edge case this left out, it's a oneliner that would be great to land on top of this patch.

note, if it's simpler for you I can merge the 2 patches here.

Comment on attachment 8866702 [details] Bug 1364002 - Store the user-made search suggestion choice along with userMadeSearchSuggestionChoice. This is a core scenario, fix has stabilized on Beta54 for ~3 weeks, ESR52+

I can confirm this issue is fixed, I verified using Fx 54.0 (build ID: 20170605204906) on Windows 10 x64, Mac OS X 10.12.4 and Ubuntu 14.04 LTS x64. Cheers!

I also verified using Fx 52.2.0ESR.