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

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Petruta Horea [:phorea]]

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

Comment 1

[Petruta Horea [:phorea]]

Reproduced with Aurora 42.0a2 with browser.urlbar.unifiedcomplete set to true.

Comment 2

[Drew Willcoxon :adw]

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.

Comment 3

[Drew Willcoxon :adw]

Attachment: patch

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.

Comment 4

[Drew Willcoxon :adw]

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 5

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e62766258856

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8650810 [details] [diff] [review]
patch

Clearing review, try saw the same failures that I thought I had fixed locally.

Comment 7

[Drew Willcoxon :adw]

Attachment: patch v2

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.

Comment 8

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52dea5aa21db

Comment 9

[Drew Willcoxon :adw]

Most recent try looks good.

Comment 10

[Drew Willcoxon :adw]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce62942bcbc2

Comment 12

[Drew Willcoxon :adw]

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

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8651284 [details] [diff] [review]
patch v2

OK, let's take it!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd7b9630b3d8

Comment 15

[Petruta Horea [:phorea]]

Verified as fixed using Aurora 42.0a2 and Nightly 43.0a1 2015-09-01.