convert existing onboardingTours from list to dictionary. And form the actual tour from a predefined id list. So we can load userTours as new user tour or update user tour based on the predefined list. The patch is r+'ed in bug 1367696 https://reviewboard.mozilla.org/r/150408/diff/5/ but we need to think twice on this due to the newly introduced notification also share the same list. from fischer's comment ``` Please switch the usage between userTours and onboardingTours. That is, let userTours be the tours map and let onboardingTours still be the tours array as originally. This is for backward compatibilty, because we have other codes which is using onboardingTours. This code compatibility handling is a bit tricky, maybe we could handle when rebasing code and have a discussion about how to approach then. And because the tour notification would need the tours to prompt notification, we need to decide tours in the init stage or the tour notification couldn't find any tours to prompt. ```

In this issue we can also save the new/update tour order in pref, so PM could change the pref later with shield and analysis for the best combination.

The patch is based on Bug 1357641 - Add onboarding tour notification and Bug 1367696 - determine the tour type, so please test with these 2 patches. You can change the tour order by change "browser.onboarding.newtour" pref and test updating tour via add tour id in "browser.onboarding.updatetour" and turn "browser.onboarding.tour-type" to "update". I'd like do some refactor to remove the global `onboardingTours` and add tests in the PART II patch.

Comment on attachment 8881547 [details] Bug 1375775 - Allow reuse existing tours; https://reviewboard.mozilla.org/r/152718/#review157842 ::: browser/extensions/onboarding/content/onboarding.js:199 (Diff revision 2) > async init(contentWindow) { > this._window = contentWindow; > this._tourItems = []; > this._tourPages = []; > > + let userTours = []; Please rename `userTours` to such as `tourIds` or `shortTourIds` to align with what it carrys. ::: browser/extensions/onboarding/content/onboarding.js:201 (Diff revision 2) > this._tourItems = []; > this._tourPages = []; > > + let userTours = []; > // we only support the new user tour at this moment > - if (Services.prefs.getStringPref("browser.onboarding.tour-type", "update") !== "new") { > + if (Services.prefs.getStringPref("browser.onboarding.tour-type", "update") === "new") { You could call a shorter of `Preferences.get("browser.onboarding.tour-type", "update")` since Preferences.jsm has been imprted already. Up to you. ::: browser/extensions/onboarding/content/onboarding.js:242 (Diff revision 2) > this._initNotification(); > } > > + _getTourList(tourType) { > + let tours = Services.prefs.getStringPref(`browser.onboarding.${tourType}tour`, ""); > + return tours.split(",").filter(word => word !== "" && word.trim()); Since we are calling `trim` here, looks like we are assuming there could be extra whitespace there. Please also return trimmed words to outside in case of "browser.onboarding.newtour" = "private, addons, customize, search, default".

Hi Fred, Would you like to postpone this bug to Thursday? Because the bug 1357021 which is going to be checked-in today also need code compatibility handling. And the tests should also need code compatibility handling. Thanks.

Sure, I'll also make the change depends on bug 1357021

Comment on attachment 8881547 [details] Bug 1375775 - Allow reuse existing tours; https://reviewboard.mozilla.org/r/152718/#review158060 ::: browser/extensions/onboarding/content/onboarding.js:180 (Diff revision 5) > <button id="onboarding-tour-default-browser-button" data-l10n-id="${defaultBrowserButtonId}"></button> > </aside> > `; > return div; > }, > }, Just remember to rebase and do the same change with the sync tour before landing. If you're going to postpone it to Thursday, sync tour should be in central.

Comment on attachment 8881547 [details] Bug 1375775 - Allow reuse existing tours; https://reviewboard.mozilla.org/r/152718/#review158294 ::: browser/app/profile/firefox.js:1712 (Diff revision 5) > pref("browser.onboarding.hidden", false); > // On the Activity-Stream page, the snippet's position overlaps with our notification. > // So use `browser.onboarding.notification.finished` to let the AS page know > // if our notification is finished and safe to show their snippet. > pref("browser.onboarding.notification.finished", false); > +pref("browser.onboarding.newtour", "private,addons,customize,search,default"); Why does this need to be a preference? ::: browser/extensions/onboarding/content/onboarding.js:199 (Diff revision 5) > this._tourItems = []; > this._tourPages = []; > + this._tours = []; > > + let tourIds = []; > // we only support the new user tour at this moment This comment is outdated ::: browser/extensions/onboarding/content/onboarding.js:200 (Diff revision 5) > - if (Services.prefs.getStringPref("browser.onboarding.tour-type", "update") !== "new") { > + if (Services.prefs.getStringPref("browser.onboarding.tour-type", "update") === "new") { > + tourIds = this._getTourList("new"); > + } else { > + tourIds = this._getTourList("update"); > + } The list you get always matches the pref value so maybe just use that instead of the if statement. ::: browser/extensions/onboarding/content/onboarding.js:240 (Diff revision 5) > > this._initPrefObserver(); > this._initNotification(); > } > > + _getTourList(tourType) { Call this _getTourIDList

Comment on attachment 8881547 [details] Bug 1375775 - Allow reuse existing tours; https://reviewboard.mozilla.org/r/152718/#review158304 ::: browser/app/profile/firefox.js:1712 (Diff revision 5) > pref("browser.onboarding.hidden", false); > // On the Activity-Stream page, the snippet's position overlaps with our notification. > // So use `browser.onboarding.notification.finished` to let the AS page know > // if our notification is finished and safe to show their snippet. > pref("browser.onboarding.notification.finished", false); > +pref("browser.onboarding.newtour", "private,addons,customize,search,default"); 1. PM want to change the order of tours remotely with Shield study and like to change the different combinations from the tourset. 2. We can enable/disable/change order of update tours when we set the `updatetour` pref. So put the tours order into preference might be a good idea.

Comment on attachment 8881547 [details] Bug 1375775 - Allow reuse existing tours; https://reviewboard.mozilla.org/r/152718/#review158802 ::: browser/extensions/onboarding/content/onboarding.js:268 (Diff revision 6) > this._initNotification(); > } > > + _getTourIDList(tourType) { > + let tours = Services.prefs.getStringPref(`browser.onboarding.${tourType}tour`, ""); > + return tours.split(",").filter(word => word !== ""); Let's do the trimming here with a .map(word => word.trim())

Comment on attachment 8884157 [details] Bug 1375775 - test cases for reusable tourset; https://reviewboard.mozilla.org/r/155072/#review160898

thanks!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e3e948f381b5 Allow reuse existing tours;r=mossop https://hg.mozilla.org/integration/autoland/rev/3915337ce78e test cases for reusable tourset;r=mossop

https://hg.mozilla.org/mozilla-central/rev/e3e948f381b5 https://hg.mozilla.org/mozilla-central/rev/3915337ce78e

I can confirm that this pref change has been made.