1482398 - Uplift Top Search feature to Firefox 62

Status: VERIFIED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Description

[Ed Lee :Mardak]

[Tracking Requested - why for this release]: There's a new feature tracked in bug 1479806 that is implemented across multiple bugs with activity stream (code, images, strings), places, and search changes. They're all related and shouldn't end up on 62 without the others, so I'm combining them into a single patch to uplift. Not all work is done yet as of filing, but we'll attempt to get it ready for a single uplift by testing and revising first in nightly 63.

Comment 1

[Ed Lee :Mardak]

adw, mkaply, are there any other changes that should be included in the uplift? I've included the 2 autocomplete and 4 search changes:

pick 5ad7c7275d75 Bug 1480503 - Create API for setting text (keyword) in and focusing on the awesomebar. r=mak
pick 7b9505783a80 Bug 1480541 - Remove em-dash from search string dropdown text when the search query is empty. r=mak

pick 942309621e01 Bug 1480504 - Add internal aliases for common engines. r=Mardak
pick caee358320a0 Bug 1482125 - Add internal aliases for common search plugins. r=Mardak,adw
pick b0cc8da3a351 Bug 1482235 - Import AppInfo for test. r=Mardak
pick 11fcf58493bc Bug 1482565 - Support and show @Яндекс and @百度 searches for yandex and baidu r=mkaply

Comment 2

[Ed Lee :Mardak]

ursula, nanj, similarly, anything else that needs to be uplifted from outside browser/components/newtab directory? I've included the blocking and telemetry:

pick 456b46a476f8 Bug 1480888 - Implement blocking system for special search shortcut top sites r=k88hudson
pick cbd12a6860ac Bug 1482627 - Switch off test_NewTabUtils.js::getTopFrecentSites_improveSearch for Thunderbird. r=ursula

pick ab243889e496 Bug 1482958 - Add missing events for activity stream UT event telemetry. r=ursula

Comment 3

[Ed Lee :Mardak]

r1cky, ursula, in rebasing some things from master to firefox-62 branch, there were some conflicts. Here's what I found them conflicting and how I resolved:

CONFLICT (content): Merge conflict in content-src/styles/_theme.scss
error: could not apply 2a29885a... Fix Bug 1481677 - persistent/stable search (aka fixed position search bar)

conflicted with 307d9cca - Port Bug 1347204 - Implement Google Chrome New Tab Page color properties
insert +    --newtab-search-header-background-color: $grey-80-95;

CONFLICT (content): Merge conflict in test/functional/mochitest/browser.ini
error: could not apply 616f95fe... fix(tests): Revert #4289 head.js changes to turn off pref from browser.ini

conflicted with b39f7a58 - Backport Bug 1474414 - Move most of browser/extensions/activity-stream to browser/components/newtab
remove +   browser.newtabpage.activity-stream.debug=false

CONFLICT (content): Merge conflict in content-src/components/TopSites/_TopSites.scss
error: could not apply df19f2db... Fix Bug 1480507 - Add/Edit new Top Search Shortcut

conflicted with d9494900 - Fix Bug 1475921 - remove text-transform usage on localized strings (section headers)
move  +      text-transform: none;



Here's the full rebase instructions:
pick 20e90a85 Bug 1479478 - Exclude default search URLs from topsites
pick 5687270d Fix Bug 1480814 - Add a pref to enable/disable top site search shortcuts on the newtab page
pick e7779578 Bug 1479478 - Support alexa top 5 search for top sites filtering Also turns on the search filter experiment be default
pick 76fad507 Fix Bug 1481571 - ship a google icon
pick 388fc7d5 Fix Bug 1480535 - Make in-content search box more prominent
pick 59fa7d21 Fix Bug 1480518 - Replace default and frecent sites with @search when they match given URL pattern
pick 1ca281b5 Fix Bug 1480506 - Top Search Shortcuts should allow user to unpin
pick 725532f6 Fix Bug 1480508 - add topSiteSearchShortcuts for new and existing users (#4289)
pick fd236ced Bug 1481895 - Refactor SEARCH_SHORTCUTS, getSearchProvider to separate file
pick 87c9130d Bug 1480888 - Implement blocking system for special search shortcut top sites - Part 1
pick 2a29885a Fix Bug 1481677 - persistent/stable search (aka fixed position search bar)
pick f31b364b Fix Bug 1481597 - Use the correct awesomebar API
pick aab3771a Fix Bug 1482184 - only apply fixed-search if search section is visible
pick b75c7c64 Fix Bug 1481605 - Update designs for Top Site Search Shortcut tiles
pick 7f409d38 Fix Bug 1482190: don't convert manually pinned sites to search shortcuts (#4305)
pick 9606e15a Fix Bug 1481901: include additional search shortcut providers (#4303)
pick 616f95fe fix(tests): Revert #4289 head.js changes to turn off pref from browser.ini Disable pref until bug 1481680
pick 883d23f7 Fix Bug 1482247: rtl search icon position (#4308)
pick 0ac8bcbe Fix Bug 1482209 - Handle disabling of the search shortcuts experiment
pick 1f2cdf88 Bug 1480513: Add telemetry for search shortcut (#4290)
pick d610b25b Fix Bug 1482255: fix search shortcut pinning race condition (#4310)
pick e3e25540 Fix Bug 1482404 - remove unnecessary search background-color
pick 9ddc1713 Fix Bug 1481903: restrict search shortcuts to the correct regions (#4315)
pick 35cca95b Don't change the status of existing pinned sites
pick ac60c72c Add test
pick bf41a9f0 fix(topsites) - TopSitesFeed would fail if it got more pinned sites than slots (#4312)
pick a7622b30 Fix Bug 1482473 - Top search should be more like a button not a link
pick 29d55131 Fix Bug 1482464: check for e.identifier before trying to match (#4319)
pick df19f2db Fix Bug 1480507 - Add/Edit new Top Search Shortcut
pick b1172990 Fix Bug 1482479 - Update SEARCH_SHORTCUTS and top_sites.json to support the 9 desired search urls
pick cd1ae86f Bug 1482519 - Separate custom search shortcut options from organic ones Also fixed a small bug where null top sites were crashing the add search shortcuts editor.
pick aa2902a6 Fix Bug 1482532 - make search shorcuts draggable again
pick b5f4dc5a Fix Bug 1482853 - white background for all searchys
pick 5fe21cda Fix Bug 1482865 - fix searchy overlap when resizing window
pick 4190e3b8 chore(mc): Port Bug 1482565 - Support and show @Яндекс and @百度 searches for yandex and baidu r=mkaply (#4329)
pick 98b38518 Fix Bug 1482914 - fix width of search bar in fixed header
pick 4c99f05e chore(telemetry) - Rename search shortcuts events for UT event telemtry (#4333)
pick 03d098de Fix Bug 1482854 - Middle clicking an @search Top Site wrongly triggers autoscroll
pick 70111e2a Fix Bug 1482629 - Compare search topsites against search engine _internalAliases (#4326)
pick c9d3c708 Fix Bug 1482895 - scroll the modal on short windows (#4331)
pick 253facc4 Fix Bug 1482871 - remove blue flash when clicking on div.search-wrapper (#4325)

Comment 4

[Ed Lee :Mardak]

ursula, there were some conflicts in rebasing mozilla-central to mozilla-beta:

CONFLICT (modify/delete): browser/components/newtab/lib/SearchShortcuts.jsm deleted in HEAD and modified in 11fcf58493bc
error: could not apply 11fcf58493bc... Bug 1482565 - Support and show @Яндекс and @百度 searches for yandex and baidu r=mkaply

skip the browser/components/newtab changes

CONFLICT (content): Merge conflict in toolkit/modules/tests/xpcshell/test_NewTabUtils.js
CONFLICT (content): Merge conflict in toolkit/modules/NewTabUtils.jsm
CONFLICT (content): Merge conflict in toolkit/components/extensions/test/xpcshell/test_ext_topSites.js
error: could not apply 456b46a476f8... Bug 1480888 - Implement blocking system for special search shortcut top sites r=k88hudson

conflicted with dd7e6cdf5cbc - Bug 1445836 - finalize topSites api on platform APIs that will be stable, r=Mardak,rpl
remove “onePerDomain” related changes

CONFLICT (content): Merge conflict in toolkit/components/telemetry/Events.yaml
error: could not apply ab243889e496... Bug 1482958 - Add missing events for activity stream UT event telemetry. r=ursula

remove all the web console related probes


Also, I needed to insert/rebase an export before the blocking stuff. And that's still failing tests because it's not able to import the jsm from activity stream. I wonder if it's because on beta, it's still a system add-on, so firefox-appdir = browser doesn't help ???

Comment 5

[Nan Jiang [:nanj]]

Yes, I have one more here (bug 1483065) if the merge is still open.

Comment 6

[Mike Kaply [:mkaply]]

Those seem correct from my end.

Comment 7

[Ed Lee :Mardak]

Just noting that for strings, I excluded 	modified:   browser/locales/en-US/chrome/browser/activity-stream/newtab.properties

That was the only file not added as part of the usual activity-stream firefox-62 branch export. So the prerendered strings, etc were updated in the usual way for uplift. Those strings are from activity-stream master branch which were imported as usual from l10n-central and includes cherry-picked search.properties.

https://hg.mozilla.org/try/rev/9d74a24a11672fa60670ba8195cb533a86faecfc
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fad75046bffd16a82a85a9f511938dbb032ddfe3

Comment 8

[Drew Willcoxon :adw]

Looks good to me, Ed

Comment 9

[Ursula Sarracini (:ursula)]

(In reply to Ed Lee :Mardak (PTO Aug 11-26) from comment #2)
> ursula, nanj, similarly, anything else that needs to be uplifted from
> outside browser/components/newtab directory? I've included the blocking and
> telemetry:
> 
> pick 456b46a476f8 Bug 1480888 - Implement blocking system for special search
> shortcut top sites r=k88hudson
> pick cbd12a6860ac Bug 1482627 - Switch off
> test_NewTabUtils.js::getTopFrecentSites_improveSearch for Thunderbird.
> r=ursula
> 
> pick ab243889e496 Bug 1482958 - Add missing events for activity stream UT
> event telemetry. r=ursula


Yup, as Nan mentioned there will be one more UT patch incoming. Everything else is fine I believe.

As far as the NewTabUtils.jsm changes go, as discussed on IRC, we're going to not uplift the test in test_NewTabUtils.jsm that bug 1480888 added, and that should fix the failures, which means we also do not need to uplift bug 1482627.

Thanks Ed!

Comment 10

[Ricky Rosario [:rrosario, :r1cky]]

LGTM. Thank you!

Comment 11

[Ed Lee :Mardak]

Here's what my uplift branch looks like:

* 8e65d5104135 - (HEAD -> as-beta) Bug 1482398 - Uplift Top Search feature to Firefox 62 (16 hours ago) <Ed Lee>
* b01a6b3028e6 - Bug 1483065 - Add search edit UT events for activity stream r=ursula (82 minutes ago) <Nan Jiang>
* 792e8ab2e312 - Bug 1482958 - Add missing events for activity stream UT event telemetry. r=ursula (21 hours ago) <Nan Jiang>
* bc1ed6b2dfaf - Bug 1480888 - Implement blocking system for special search shortcut top sites r=k88hudson (4 days ago) <Ursula Sarracini>
* d9c1705d2c6c - Bug 1482565 - Support and show @Яндекс and @百度 searches for yandex and baidu r=mkaply (3 days ago) <Ed Lee>
* c67a49b6cefb - Bug 1482235 - Import AppInfo for test. r=Mardak (5 days ago) <Michael Kaply>
* 396035493b19 - Bug 1482125 - Add internal aliases for common search plugins. r=Mardak,adw (4 days ago) <Michael Kaply>
* dae9b4fed8e0 - Bug 1480504 - Add internal aliases for common engines. r=Mardak (7 days ago) <Michael Kaply>
* f2b0aca72b33 - Bug 1480541 - Remove em-dash from search string dropdown text when the search query is empty. r=mak (6 days ago) <Drew Willcoxon>
* 6c204f331b53 - Bug 1480503 - Create API for setting text (keyword) in and focusing on the awesomebar. r=mak (6 days ago) <Drew Willcoxon>
* 6cf0734bcb9d - (beta, refs/cinnabar/refs/heads/bookmarks/beta) no bug - Bumping Firefox l10n changesets DONTBUILD r=release a=l10n-bump (5 hours ago) <L10n Bumper Bot>

Hopefully `arc diff` will just work .. ! ?

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

Comment 12

[Ed Lee :Mardak]

Attachment: Bug 1482398 - Uplift Top Search feature to Firefox 62

Bug 1480503 - Create API for setting text (keyword) in and focusing on the awesomebar. r=mak
Bug 1480541 - Remove em-dash from search string dropdown text when the search query is empty. r=mak
Bug 1480504 - Add internal aliases for common engines. r=Mardak
Bug 1482125 - Add internal aliases for common search plugins. r=Mardak,adw
Bug 1482235 - Import AppInfo for test. r=Mardak
Bug 1482565 - Support and show @Яндекс and @百度 searches for yandex and baidu r=mkaply
Bug 1480888 - Implement blocking system for special search shortcut top sites r=k88hudson
Bug 1482958 - Add missing events for activity stream UT event telemetry. r=ursula
Bug 1483065 - Add search edit UT events for activity stream r=ursula

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, after meeting today and some more discussion, sounds like we're going to wait for one more patch, plus another patch to turn this off by default. Then uplift to beta, aiming for tomorrow's beta 18 build.   The two planned experiments will then use pref rollouts to control the rates for turning on the feature(s).

Comment 14

[Ed Lee :Mardak]

Here's the bugs included in the patch now:

Bug 1480503 - Create API for setting text (keyword) in and focusing on the awesomebar. r=mak
Bug 1480541 - Remove em-dash from search string dropdown text when the search query is empty. r=mak
Bug 1482551 - avoid selecting url bar text on searches r=Mardak
Bug 1480504 - Add internal aliases for common engines. r=Mardak
Bug 1482125 - Add internal aliases for common search plugins. r=Mardak,adw
Bug 1482235 - Import AppInfo for test. r=Mardak
Bug 1482565 - Support and show @Яндекс and @百度 searches for yandex and baidu r=mkaply
Bug 1482085 - Disable test_PlacesSearchAutocompleteProvider.js for Thunderbird. r=mkaply
Bug 1480888 - Implement blocking system for special search shortcut top sites r=k88hudson
Bug 1482958 - Add missing events for activity stream UT event telemetry. r=ursula
Bug 1483065 - Add search edit UT events for activity stream r=ursula

Notably bug 1482627 doesn't need to be uplifted as the test in question was removed during rebasing / conflict resolution.

For the commit titled of this bug, it includes these changes: https://github.com/Mardak/activity-stream/compare/firefox-62b18...firefox-62 (will push to mozilla/activity-stream when given the okay)

Comment 15

[Ed Lee :Mardak]

https://github.com/mozilla/activity-stream/compare/firefox-62b18...firefox-62 is updated with the RTL fix! Phabricator is also updated

Comment 16

[Kate Hudson :k88hudson]

Comment on attachment 8999984 [details]
Bug 1482398 - Uplift Top Search feature to Firefox 62

Kate Hudson :k88hudson has approved the revision.

Comment 17

[Ed Lee :Mardak]

Comment on attachment 8999984 [details]
Bug 1482398 - Uplift Top Search feature to Firefox 62

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1479806 new top search feature experiment
[User impact if declined]: Old search experience that might discourage searching
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Most of the changes have landed in Nightly over the last week and 14 have been verified so far
[Needs manual test from QE? If yes, steps to reproduce]: Yes, I believe mcoman and ciprian know what to test. Generally search top site appearing by default (for new and existing users), add search engine section menu, hiding of top search sites from top sites.
[List of other uplifts needed for the feature/fix]: This one patch has all the uplifts combined.
[Is the change risky?]: Somewhat
[Why is the change risky/not risky?]: New functionality in activity stream with relatively small changes to the url bar and search service. There's a lot of files and lines changed, but a good portion of that is the built artifacts generated by activity stream.
[String changes made/needed]: Yes, but not to files that are exposed to localization. The changes here include the localized strings.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8999984 [details]
Bug 1482398 - Uplift Top Search feature to Firefox 62

As discussed today, let's uplift for beta 18.

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

Sorry, failed to land.

Error: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, "applying /tmp/tmpARIVK0\npatching file toolkit/modules/tests/xpcshell/xpcshell.ini\nHunk #1 succeeded at 4 with fuzz 2 (offset 1 lines).\npatching file toolkit/modules/tests/xpcshell/head.js\nHunk #1 FAILED at 9\n1 out of 1 hunks FAILED -- saving rejects to file toolkit/modules/tests/xpcshell/head.js.rej\npatching file toolkit/modules/NewTabUtils.jsm\nHunk #1 succeeded at 24 with fuzz 2 (offset 12 lines).\nHunk #2 FAILED at 1155\n1 out of 2 hunks FAILED -- saving rejects to file toolkit/modules/NewTabUtils.jsm.rej\npatching file toolkit/content/widgets/autocomplete.xml\nHunk #1 FAILED at 1842\nHunk #2 FAILED at 1909\n2 out of 2 hunks FAILED -- saving rejects to file toolkit/content/widgets/autocomplete.xml.rej\npatching file toolkit/components/thumbnails/test/browser_thumbnails_bg_topsites.js\nHunk #1 FAILED at 5\n1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/thumbnails/test/browser_thumbnails_bg_topsites.js.rej\npatching file toolkit/components/telemetry/Events.yaml\nHunk #1 FAILED at 22\n1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/Events.yaml.rej\npatching file toolkit/components/search/nsSearchService.js\nHunk #1 succeeded at 953 with fuzz 1 (offset -41 lines).\nHunk #2 succeeded at 2248 with fuzz 1 (offset -26 lines).\nHunk #3 FAILED at 3876\n1 out of 3 hunks FAILED -- saving rejects to file toolkit/components/search/nsSearchService.js.rej\npatching file toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js\nHunk #1 FAILED at 2\nHunk #2 FAILED at 54\nHunk #3 FAILED at 77\nHunk #4 succeeded at 122 with fuzz 1 (offset 7 lines).\n3 out of 4 hunks FAILED -- saving rejects to file toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js.rej\npatching file toolkit/components/places/PlacesSearchAutocompleteProvider.jsm\nHunk #1 FAILED at 83\nHunk #2 succeeded at 116 with fuzz 1 (offset 6 lines).\nHunk #3 FAILED at 276\n2 out of 3 hunks FAILED -- saving rejects to file toolkit/components/places.....

https://lando.services.mozilla.com/revisions/D3348/8436/

Comment 20

[Ed Lee :Mardak]

Those errors seem to be from applying the patch against mozilla-central. In particular from https://lando.services.mozilla.com/revisions/D3348/8436/

unable to find 'browser/extensions/activity-stream/test/unit/unit-entry.js' for patching\n(use '--prefix' to apply patch relative to the current directory)

That file exists on beta but not on central.

Comment 21

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=9e752b54a20f425449e1f00bf3adefa903f8051a

Comment 22

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e752b54a20f425449e1f00bf3adefa903f8051a

Comment 23

[Stephanie Ouillon [:arroway] (needinfo me)]

r+ for product security

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this fixed since all the work uplifted to beta in this bug has already landed on m-c, shown in the list of dependent bugs just under comment 22.

Comment 25

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

We will assess the verification process for every actionable dependency and then will also mark this as verified.

Comment 26

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

The verification process is done, all the actionable implementations have been successfully verified. Marking the flags accordingly.