Closed Bug 1194669 Opened 4 years ago Closed 4 years ago

Prompt to opt-in/out search suggestions still shown after answered in another window

Categories

(Firefox :: Address Bar, defect, P1)

43 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox41 --- unaffected
firefox42 --- verified
firefox43 --- verified

People

(Reporter: petruta.rasa, Assigned: adw)

References

Details

(Whiteboard: [suggestions][fxsearch])

Attachments

(1 file, 1 obsolete file)

Reproduced with Nightly 43.0a1 20150813030208 under all platforms.

Steps to reproduce:
1. Open several browser windows
2. Start typing in the urlbar - notice the prompt to opt-in/out search suggestions
3. Select Yes OR NO
4. Select another window and type in the urlbar

AR:
The prompt is displayed again.

ER:
The prompt shouldn't be displayed again if the user already made his choice

Note: Couldn't check on Dev Edition 42.0a2 (2015-08-13) where setting the prefs browser.urlbar.userMadeSearchSuggestionsChoice;true and browser.urlbar.suggest.searches;true doesn't display the prompt
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [suggestions][fxsearch]
Reproduced with Aurora 42.0a2 with browser.urlbar.unifiedcomplete set to true.
Assignee: nobody → adw
Blocks: 959567
Status: NEW → ASSIGNED
No longer depends on: 958204, 959567
Rank: 7
OK, this is only a problem for the windows where you've typed in the urlbar before making a choice.  If you just open a bunch of windows, go to one, start typing in the urlbar, and then make a choice, then the other windows correctly do not show the notification when you type in them.
Attached patch patch (obsolete) — Splinter Review
Instead of hiding the notification directly when Yes or No is clicked, have the pref observer hide it.  Then the urlbar where Yes or No is clicked only sets the pref, and the pref observers of all the urlbars hide their notifications.
Attachment #8650810 - Flags: review?(mak77)
Comment on attachment 8650810 [details] [diff] [review]
patch

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

The new `yield transitionPromise`s in the test were necessary to prevent failures where, despite also yielding on the searchPromise, the test continued before the search suggestions actually appeared.  Not sure why.
Comment on attachment 8650810 [details] [diff] [review]
patch

Clearing review, try saw the same failures that I thought I had fixed locally.
Attachment #8650810 - Flags: review?(mak77)
Attached patch patch v2Splinter Review
There were a couple of problems:

(1) The new multipleWindows test should wait for the notification transition to finish, not for the search to complete, before checking whether the notification is visible.

(2) For some reason the condition that promiseSearchComplete waits for seems to be wrong in this case:

      return win.gURLBar.controller.searchStatus >=
        Ci.nsIAutoCompleteController.STATUS_COMPLETE_NO_MATCH;

So instead of waiting for that condition, wait for suggestions to actually appear in the results.  And the suggestions are actually appearing when the test fails, it's just that they're appearing after promiseSearchComplete's condition happens.  It's not a meaningful failure.

(3) This might not actually be a problem, but I made the test wait for a pref observer to fire every time it sets the userMadeSearchSuggestionsChoice pref, since that's what causes the notification to be hidden.  I made this change before the previous two fixes, and I'm not sure it's necessary anymore, but it can't hurt.
Attachment #8650810 - Attachment is obsolete: true
Attachment #8651284 - Flags: review?(mak77)
Most recent try looks good.
Attachment #8651284 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/ce62942bcbc2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment on attachment 8651284 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]:
Search suggestions opt-in notification in the urlbar, bug 959567

[User impact if declined]:
If you have multiple windows open and dismiss the notification in one window, it will still appear in the other windows.

[Describe test coverage new/current, TreeHerder]:
New automated test, manual testing

[Risks and why]:
Low risk: well tested, still plenty of time to bake on Aurora before Beta merge, and this patch only affects the notification (but the patch touches urlbar code to do that)

[String/UUID change made/needed]:
None
Attachment #8651284 - Flags: approval-mozilla-aurora?
Comment on attachment 8651284 [details] [diff] [review]
patch v2

OK, let's take it!
Attachment #8651284 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed using Aurora 42.0a2 and Nightly 43.0a1 2015-09-01.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.