1696187 - In certain regions, Amazon search shortcut from remote settings gets pinned when it wasn't without remote settings

Status: RESOLVED FIXED

(Firefox :: Top Sites, defect, P1)

Description

[Adrian Florinescu [:aflorinescu]]

follow-up from bug 1691478

[Environment:]

Windows 10, Mac 10.14.6, Ubuntu 20.04

[Steps]:

1. Start an en-ca localized older version (87.0a1) with an user.js that has
   user_pref("browser.search.region", "CA");
   user_pref("browser.topsites.useRemoteSetting", false);
2. Note down the topsites: the order would be amazon.ca is on pos 5 unpinned.
3. Close Fx down, edit the said user.js and change user_pref("browser.topsites.useRemoteSetting", true);
4. Download latest beta - 87.b05
5. Start beta 87.b05 with the above profile. (migration scenario)

[Actual Result:]

amazon.ca is pinned and on position 1

[Expected Result:]

amazon.ca is pinned and on position 5

[Note:]

Unpinning in the above actual result will send the amazon.ca to position 5 (order 50).

Comment 1

[Adrian Florinescu [:aflorinescu]]

Generally speaking this extends to all scenarios in which there is a search shortcut in legacy(not pinned) and it will get migrated to Remote Settings.
For an existing profile, before the migration, there would be a given position for the search shortcut, for example, I'd have like the first row with frecency items, then the defaults, out of which there would be an unpinned search shortcut.
After migrating to a RS version, the search shortcut would get pinned in the first available position which might give the perception that we want to impose a certain search shortcut in the topsites poleposition.

Comment 2

[Adrian Florinescu [:aflorinescu]]

(In reply to Adrian Florinescu [:aflorinescu] from comment #1)

Generally speaking this extends to all scenarios in which there is a search shortcut in legacy(not pinned) and it will get migrated to Remote Settings.

The list of legacy regions that have no pinned amazon search-shortcuts by default is as follows:

1. all regions excluding ["DE", "FR", "GB", "IT", "JP", "US"]
2. all regions that are specified in the list https://searchfox.org/mozilla-central/source/browser/components/newtab/lib/DefaultSites.jsm#19 and are not in 1.
3. all regions that have had specific user action -> removing pin for search-shortcut when the search-shortcut was pinned

Comment 3

[Dão Gottwald [:dao]]

Given the potential large number of affected regions, I'm going to restore pre-remote-settings behavior here.

Comment 4

[Dão Gottwald [:dao]]

Attachment: Bug 1696187 - Enable URL top site -> search shortcut conversion for remote settings. r=mikedeboer

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68e43222106e
Enable URL top site -> search shortcut conversion for remote settings. r=mikedeboer

Comment 6

[Dão Gottwald [:dao]]

Julien, apart from the above one-line patch, the other half of the fix here are updates to the top-sites remote setting data. Those are already done and reviewed, but we'd need them to be in the remote settings dump that will ship with 87. Is that viable?

Comment 7

[Julien Cristau [:jcristau]]

When do you expect to have updated data in remote settings? We'll have another update tomorrow, which might be the last for 87.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Julien Cristau [:jcristau] from comment #7)

When do you expect to have updated data in remote settings? We'll have another update tomorrow, which might be the last for 87.

The data is already updated!

Comment 9

[Julien Cristau [:jcristau]]

Alright, then I expect the new dump will be in 87.0b9.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 9208030 [details]
Bug 1696187 - Enable URL top site -> search shortcut conversion for remote settings. r=mikedeboer

Beta/Release Uplift Approval Request

• User impact if declined: We'd pin the amazon search shortcut when we didn't do before in certain regions such as CA
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Adrian is already testing this.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This one-liner enables behavior that we've already shipped without remote settings.
• String changes made/needed:

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/68e43222106e

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9208030 [details]
Bug 1696187 - Enable URL top site -> search shortcut conversion for remote settings. r=mikedeboer

approved for 87.0b9

Comment 13

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/25294364d094

Comment 14

[Adrian Florinescu [:aflorinescu]]

Given the tests done on the beta 87.b9 try build this fix looks fine with a wide regression area. A configuration issue surfaced - bug 1697763 -, but this is Remote Settings config area and does not affect the code.
I will redo the verification with a smaller regression area once the uplift hits b9 and b9 shall be available.

Comment 15

[Julien Cristau [:jcristau]]

RS dump update:
https://hg.mozilla.org/releases/mozilla-beta/rev/92f89dc01fdcb006948329d5f508dc99a1651098

Comment 16

[Adrian Florinescu [:aflorinescu]]

Reverified the fix on Windows 10 & Mac 10.14.6 with 87.b9 focusing both main pinned regions and regions out of the legacy default pinned logic (for amazon etld).

To sumarize, the issue before the fix was that for all regions containing amazon etld, we would've pinned amazon when migrating (Firefox xx to Firefox 87), regardless if the amazon was pinned or not in legacy. With the fix, now migrating from legacy to RS, we will persist the pinned topsites, regardless if they are user action pins or default pins.