Moving across to migration, since that's where the code will need to be to drive this.

Should the toolbar also only be displayed if bookmarks were imported from the toolbar of the other application? This should be detectable.

I also wondered this, and the answer is "No", because we put a "Bookmarks from Opera" folder in the toolbar containing all the bookmarks from the other browser, and it's that folder that we want to guide people to…

(Also, I believe we would like this to be behind a pref so that we can test it as part of the Shirley Omnibus experiment.)

(In reply to Blake Winton (:bwinton) (:☕️) from comment #2)

I also wondered this, and the answer is "No", because we put a "Bookmarks from Opera" folder in the toolbar containing all the bookmarks from the other browser, and it's that folder that we want to guide people to…

I commented based on a brief look at the code, but looking at an actual import from Chrome, I think we are both right.

There's a folder "From Google Chrome" that appears on the toolbar, but there is also one in the Bookmarks Menu - they're both different according to the contents that were in Chrome.

Safari import is also the same.

It might be that the default profiles for Safari and Chrome have bookmarks in for the toolbar anyway - so it might not be worth having a check before displaying the toolbar, but still worth considering I think.

this is not the first time we do this, we have code in BrowserGlue that already shows the bookmarks toolbar if import happened on startup
https://searchfox.org/mozilla-central/rev/56bb74ea8e04bdac57c33cbe9b54d889b9262ade/browser/components/BrowserGlue.jsm#3025

Right, the main issue is that we no longer use the startup migration flow (other than --migration)… we should consider changing the code that determines whether to import in folders or not e.g. if this is the first subsession (or some other metric indicating this is a new user) then don't make the folders and import directly to the menu and toolbar. i.e. the subfolders weren't the desired UX for new users, this is more fallout from not using the wizard at first install.

(In reply to Matthew N. [:MattN] from comment #5)

Right, the main issue is that we no longer use the startup migration flow (other than --migration)… we should consider changing the code that determines whether to import in folders or not e.g. if this is the first subsession (or some other metric indicating this is a new user) then don't make the folders and import directly to the menu and toolbar. i.e. the subfolders weren't the desired UX for new users, this is more fallout from not using the wizard at first install.

Agreed that putting toolbar bookmarks from 3rd party import into a folder is not great for new profiles.
I'm now updating the user story with this as a P2 item - this is something we want for 81 and I'm conscious of not making the scope too large.

Moving to P1 given we want this for 81. RT, I assume you'll find an engineer for this. You may also want to request tracking for 81.

We may want to consider using the existence of other bookmarks (rather than the profile age) when deciding whether to create folders.

[Tracking Requested - why for this release]: Part of Shirley Omnibus per comment #2

This is disabled by default behind a pref so we can run a study on it. There is also a TODO to get the correct sourceID for the migrator so we can add some extra telemetry.

Romain, could we change:

P2 If the profile is new (profile creation date is today) the third party bookmark toolbar import should not be placed in a "From $ORIGINATING_BROWSER" folder

to either this:

P2 If the number of bookmarks is below some number (10 for example) they should not be placed in a "From $ORIGINATING_BROWSER" folder

or this:

P2 If the number of bookmarks is below some number (10 for example) or if the profile is new, the new bookmarks should not be placed in a "From $ORIGINATING_BROWSER" folder

It seems like bookmarks counts for new users are strange (I would have expected some kind of linear distribution of users with a given bookmark count but this is not really happening: https://sql.telemetry.mozilla.org/queries/73789/source#184505
I although agree that you suggestion below is betteer as it should help catch existing users who could benefit from the change:
P2 If the number of bookmarks is below some number (10 for example) or if the profile is new, the new bookmarks should not be placed in a "From $ORIGINATING_BROWSER" folder

Updating the US field now

Backed out changeset c2c4b8d6b915 (Bug 1656277) for causing build bustages in nsDocShell.cpp CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=XvP9yA7iR1em5VjFwv96pw.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=c2c4b8d6b9152bc840e9909352c906b9fb805740

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312962650&repo=autoland&lineNumber=9082

Backout: https://hg.mozilla.org/integration/autoland/rev/edbbba3538f7dadf9c3018105ce33448cdea5714

This was relanded https://hg.mozilla.org/integration/autoland/rev/4fc26e678c18a8cdf308d0336430861518fa5a2c

Say, should the browser.migrate.showBookmarksToolbarAfterMigration pref only be true in EARLY_BETA_OR_EARLIER, like the rest of the Shirley Prefs, so that it can be part of the omnibus experiment?

(I checked the code, and I think it's just always true currently…)

Backed out for failures on test_Edge_db_migration.js

backout: https://hg.mozilla.org/integration/autoland/rev/e93466500f6b40181d208517267b05b6191d590a

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=windows%2C7%2Copt%2Cxpcshell%2Ctests%2Ctest-windows7-32%2Fopt-xpcshell-e10s%2Cx2&revision=4e6b4ef959560877408e46d85db19011b1ca5efe&selectedTaskRun=LJj7sbkSQqC9iOHO3V8prQ.0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312977809&repo=autoland&lineNumber=8817

[task 2020-08-14T01:09:03.641Z] 01:09:03 INFO - TEST-START | browser/components/migration/tests/unit/test_Edge_db_migration.js
[task 2020-08-14T01:09:04.744Z] 01:09:04 WARNING - TEST-UNEXPECTED-FAIL | browser/components/migration/tests/unit/test_Edge_db_migration.js | xpcshell return code: 0
[task 2020-08-14T01:09:04.744Z] 01:09:04 INFO - TEST-INFO took 1105ms
[task 2020-08-14T01:09:04.744Z] 01:09:04 INFO - >>>>>>>
[task 2020-08-14T01:09:04.745Z] 01:09:04 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2020-08-14T01:09:04.745Z] 01:09:04 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2020-08-14T01:09:04.746Z] 01:09:04 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2020-08-14T01:09:04.746Z] 01:09:04 INFO - running event loop
[task 2020-08-14T01:09:04.746Z] 01:09:04 INFO - browser/components/migration/tests/unit/test_Edge_db_migration.js | Starting
[task 2020-08-14T01:09:04.746Z] 01:09:04 INFO - (xpcshell/head.js) | test pending (2)
[task 2020-08-14T01:09:04.746Z] 01:09:04 INFO - TEST-PASS | browser/components/migration/tests/unit/test_Edge_db_migration.js | - Should recognize db we just created - true == true

(In reply to Blake Winton (:bwinton) (:☕️) from comment #17)

Say, should the browser.migrate.showBookmarksToolbarAfterMigration pref only be true in EARLY_BETA_OR_EARLIER, like the rest of the Shirley Prefs, so that it can be part of the omnibus experiment?

(I checked the code, and I think it's just always true currently…)

Yes, we can do that. I wasn't sure of the omnibus plans, and was planning to manually disable it via another patch when needed.

Backed out changeset 8b917683232d (Bug 1656277) for causing xpcshell failures in test_IE_bookmarks.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/044646689ae157d7033dff0cd0a5e3503e096ba9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=8b917683232d97369e1183ac0386b4118ed25103&selectedTaskRun=OdUNGrdYR1usad1NJIMETQ.0

Failure logs:

• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312997110&repo=autoland&lineNumber=5632
• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312997221&repo=autoland&lineNumber=6020

(Update) Also seeing marionette failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=84e4f72a38b04ed11efc6bea6674137a4cab323c&searchStr=windows%2C10%2Cx64%2Cdebug%2Ctest-windows10-64%2Fdebug-marionette-e10s%2Cmn&tochange=4fb5e934d1b9bb7433e046525709bc4538d61c20&selectedTaskRun=RZa1zFCjSxSW3YqjsLpnRA.0

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)

(In reply to Blake Winton (:bwinton) (:☕️) from comment #17)

Say, should the browser.migrate.showBookmarksToolbarAfterMigration pref only be true in EARLY_BETA_OR_EARLIER, like the rest of the Shirley Prefs, so that it can be part of the omnibus experiment?

(I checked the code, and I think it's just always true currently…)

Yes, we can do that. I wasn't sure of the omnibus plans, and was planning to manually disable it via another patch when needed.

Those are the plans as I understand them (so that QA can test the with-everything-on case in the first couple of betas, and the with-everything-off case in the second couple), so let's do that either in this patch, if you need to change it to get it re-landed, or in a followup patch if you'd rather. 🙂

(In reply to Blake Winton (:bwinton) (:☕️) from comment #22)

Those are the plans as I understand them (so that QA can test the with-everything-on case in the first couple of betas, and the with-everything-off case in the second couple), so let's do that either in this patch, if you need to change it to get it re-landed, or in a followup patch if you'd rather. 🙂

I've updated this patch to use EARLY_BETA_OR_EARLIER.

https://hg.mozilla.org/mozilla-central/rev/0b3d15cd21bc
https://hg.mozilla.org/mozilla-central/rev/2d312d483810

Given bug 1659970, should we expand browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js to test the case where we do unhide the bookmarks toolbar?