Based on bug 1360474, overlay should able to show tour-set based on conditions

UX said we must have 2 set of tours in 57

Here's the logic I talked through with Fischer.

To decide the new user/update user, I guess only the main version update counts as "update", such as 56 -> 57. The minor version update will treat as the same version. such as * 56.0a1 update to 56.0a2 will be treated as same 56 * 55.0b1 update to 55.0b2 will be treated as same 55 Is that assumption right?

(In reply to Fred Lin [:gasolin] from comment #3) > To decide the new user/update user, I guess only the main version update > counts as "update", such as 56 -> 57. > > The minor version update will treat as the same version. such as > * 56.0a1 update to 56.0a2 will be treated as same 56 > * 55.0b1 update to 55.0b2 will be treated as same 55 > > Is that assumption right? Correct. And to clarify further, an update from 55.01, for example, to 58.02 would trigger the update tour.

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; The patch contains 3 parts: 1. detect the user is a new user in nsBrowserGlue.js. (when user comes with a new profile, we treat him/her as a new user and set a new pref "browser.onboarding.newuser") 2. determine the user is a new user or an update user (Check once in bootstrap.js) 3. handle new user/update user tour in `onboarding.js` The first patch is aiming 1 and 2. I put the use type determine code in bootstrap.js since it only runs once. Another reason is the API `Services.appinfo` is only available in .jsm so put that into onboarding.js takes extra work. The disadvantage is it might not possible to test bootstrap.js with mochitest since the script runs before the content window is ready. Do you have any idea about how to test it?

The PART II patch is simpler: it 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.

Comment on attachment 8879079 [details] Bug 1367696 - PART II add xpcshell structure for onboarding; https://reviewboard.mozilla.org/r/150408/#review155270 ::: browser/extensions/onboarding/content/onboarding.js:245 (Diff revision 2) > - if (this._tourItems.length == 0) { > - // Lazy loading until first toggle. > + // Lazy loading until first toggle. > - this._loadTours(onboardingTours); > + if (this._tourItems.length == 0) { > + let userTours = ["private", "addons", "customize", "search", "default"]; > + let tours = []; > + userTours.forEach( tourId=> { Spacing nit: userTours.forEach(tourId => {

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review155254 This needs more descriptive comments to explain what is supposed to be happening here as I couldn't figure it out. Do we ever intend to update the onboarding content mid-version and what do we expect to happen in that case? ::: browser/app/profile/firefox.js:1703 (Diff revision 5) > pref("browser.suppress_first_window_animation", true); > > // Preferences for Photon onboarding system extension > pref("browser.onboarding.enabled", true); > pref("browser.onboarding.hidden", false); > +pref("browser.onboarding.tour-version", "56"); it seems wrong to set this pref here when the onboarding tour itself is shipping as a system add-on. ::: browser/extensions/onboarding/bootstrap.js:61 (Diff revision 5) > + const TOUR_VERSION = Preferences.get("browser.onboarding.tour-version", ""); > + // The regex is modified from toolkit/components/urlformatter/nsURLFormatter.js and only > + // match the major browser version > + const CURRENT_BROWSER_VERSION = Services.appinfo.version.replace(/^([^\.]+)\.[0-9]+[a-z]*.*/gi, "$1"); > + // browser.onboarding.newuser is used to check if we should show the new user tour or the update user tour. > + let isNewUser = Services.prefs.getBoolPref(PREF_NEW_USER, false); This is never used ::: browser/extensions/onboarding/bootstrap.js:72 (Diff revision 5) > + if (Services.prefs.getBoolPref(PREF_NEW_USER, false)) { > + Services.prefs.setBoolPref(PREF_NEW_USER, false); > + } Too much indenting

(In reply to Fred Lin [:gasolin] from comment #10) > Comment on attachment 8879027 [details] > Bug 1367696 - PART I determine a new user > > The patch contains 3 parts: > > 1. detect the user is a new user in nsBrowserGlue.js. > (when user comes with a new profile, we treat him/her as a new user and set > a new pref "browser.onboarding.newuser") > > 2. determine the user is a new user or an update user (Check once in > bootstrap.js) > > 3. handle new user/update user tour in `onboarding.js` > > The first patch is aiming 1 and 2. I put the use type determine code in > bootstrap.js since it only runs once. Another reason is the API > `Services.appinfo` is only available in .jsm so put that into onboarding.js > takes extra work. > > The disadvantage is it might not possible to test bootstrap.js with > mochitest since the script runs before the content window is ready. Do you > have any idea about how to test it? You could disable then enable the system add-on to have it run its startup script again, setting prefs and checking the results.

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review155388 Thanks for feedback! I'll add some description before `determineUserType` to describe the full detection flow. Then try to write test with AddonManager.getAddonByID and addon.userDisabled ::: browser/app/profile/firefox.js:1703 (Diff revision 5) > pref("browser.suppress_first_window_animation", true); > > // Preferences for Photon onboarding system extension > pref("browser.onboarding.enabled", true); > pref("browser.onboarding.hidden", false); > +pref("browser.onboarding.tour-version", "56"); We have no plan to update onboarding system-addon tour version between browser versions. So I think its fine for us. PM(pdol & cindy) are planning to use shield to change the tour set order pref remotely(not in v56), and we can do that after PART II patch.

I addressed issues and updated the descriptions. Now use `tourset-version` instead of `tour-version` to better describe what we actually saved. The test will come with a separate patch.

To test this issue, 1. use `./mach run -P` to create a new user profile You should see the onboarding new user tour open `about:config` and filter with `onboarding` settings. New profile means a new user, so related prefs should be ``` * browser.onboarding.hidden, false browser.onboarding.newuser: true browser.onboarding.tourset-version: "56" browser.onboarding.watched-tourset-version: "56" ``` 2. Then, set * browser.onboarding.hidden, true * browser.onboarding.newuser: true * browser.onboarding.tourset-version: "53" to mimic we have new tour set and we changed to another tour set version. Close the browser and open again with `./mach run -P` You should see the onboarding new user tour, the watched-toolset-version is updated and `browser.onboarding.hidden` is set to false ``` browser.onboarding.hidden, false browser.onboarding.newuser: true browser.onboarding.tourset-version: "53" browser.onboarding.watched-tourset-version: "53" ``` 3. Then, close the browser and open again with `./mach run -P` You should not see the onboarding new user tour (because watchedTourVersion("53") !== CURRENT_BROWSER_VERSION("56")) ``` browser.onboarding.newuser: false browser.onboarding.tourset-version: "53" browser.onboarding.watched-tourset-version: "53" browser.onboarding.hidden, false ```

> > We have no plan to update onboarding system-addon tour version between > browser versions. So I think its fine for us. > > PM(pdol & cindy) are planning to use shield to change the tour set order > pref remotely(not in v56), and we can do that after PART II patch. Mossop, do you mean we should get the tour set version from the system-addon manifest (addon.version)? It's possible to use the addon.version, but it will be not possible to test the update tour case because we can't programly set the tour set version.

Michael, here's the user case I distilled from the flow chart. QA may able to use these for manual test plan, and reviewer can refer these use case as requirement as well. Can you help double confirm if these cases are correct? New User Tour - when user start with a new profile, we treat him/her as a new user - we will show user the new user tour - user can hide the overlay and fox icon by click the checkbox in the overlay - once overlay is dismissed, the overlay won't show again - if the user have watched the new user tour and not hide it by click the checkbox, we keep showing the fox icon. Update User Tour - when user upgrade from the older browser version, we treat him/her as a upgrade user - we only count on major browser version (ex: 56,57,58,59) upgrade, and neglect minor versions (If user update from 56.0a1 -> 56.0b2, we still treat the current browser version as 56) - we will show the update tour, no matter user prefer to hide the overlay in new user tour - the checkbox should be shown as unchecked in overlay - (in v56) we will hide the overlay and the fox icon. - (after v56) we will show user the update user tour - (after v56)if the user have watched the update user tour and not hide it by click the checkbox, we keep showing the fox icon. - if the user never watched the update user tour, show the update user tour - (after v56) user can hide the overlay and fox icon by click the checkbox in the overlay - (after v56) once overlay is dismissed, the overlay wont show again in this browser version Upgrade to the next major version - if user have watched the old new user tour (ex: 56), but now we have a 'new' new user tour set(ex 57), we will not show the new user tour to the user (because the user is now turned into the update user) - if user have watched the old update user tour (ex: 57), but now we have a new update user tour set(ex 60) - we will show the update user tour again, no matter user prefer to hide the overlay in update user tour, or have watched the old update user tour - the checkbox should be shown as unchecked in overlay

The 3rd section title should be `Upgrade to the next tour set version`.

(In reply to Fred Lin [:gasolin] from comment #22) > > > > We have no plan to update onboarding system-addon tour version between > > browser versions. So I think its fine for us. > > > > PM(pdol & cindy) are planning to use shield to change the tour set order > > pref remotely(not in v56), and we can do that after PART II patch. > > Mossop, do you mean we should get the tour set version from the system-addon > manifest (addon.version)? > It's possible to use the addon.version, but it will be not possible to test > the update tour case because we can't programly set the tour set version. I think I'm asking whether the tourset pref is even needed if it is always going to match the browser version. And if it isn't always going to match the browser version why do we ever compare it to the browser version?

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review155918 ::: browser/app/profile/firefox.js:1701 (Diff revision 7) > pref("browser.suppress_first_window_animation", true); > > // Preferences for Photon onboarding system extension > pref("browser.onboarding.enabled", true); > pref("browser.onboarding.hidden", false); > +// Current onboarding toolset version toolset or tourset? Either way this comment doesn't really say much about what this pref means. ::: browser/app/profile/firefox.js:1702 (Diff revision 7) > > // Preferences for Photon onboarding system extension > pref("browser.onboarding.enabled", true); > pref("browser.onboarding.hidden", false); > +// Current onboarding toolset version > +pref("browser.onboarding.tourset-version", "56"); If we're never changing tours mid-browser version why do we need this pref at all? ::: browser/components/nsBrowserGlue.js:1728 (Diff revision 7) > if (Services.prefs.prefHasUserValue("browser.migration.version")) { > currentUIVersion = Services.prefs.getIntPref("browser.migration.version"); > } else { > // This is a new profile, nothing to migrate. > Services.prefs.setIntPref("browser.migration.version", UI_VERSION); > + // annotate its a firefox new user for onboarding grammar nit: annotate that it's a new firefox user for onboarding ::: browser/extensions/onboarding/bootstrap.js:53 (Diff revision 7) > function install(aData, aReason) {} > > function uninstall(aData, aReason) {} > > +/** > + * Determine the current user type (new user or update user). Is there a third type, regular user? ::: browser/extensions/onboarding/bootstrap.js:54 (Diff revision 7) > > function uninstall(aData, aReason) {} > > +/** > + * Determine the current user type (new user or update user). > + * As the result the function will set the right current user type in the new user pref (PREF_NEW_USER) for later use. From what I can see the function is actually unhiding the tour button for update users and not really controlling the value of PREF_NEW_USER. ::: browser/extensions/onboarding/bootstrap.js:71 (Diff revision 7) > + const TOURSET_VERSION = Preferences.get("browser.onboarding.tourset-version", ""); > + // The regex is modified from toolkit/components/urlformatter/nsURLFormatter.js and only > + // match the major browser version > + const CURRENT_BROWSER_VERSION = Services.appinfo.version.replace(/^([^\.]+)\.[0-9]+[a-z]*.*/gi, "$1"); > + // The last tour version user have been watched. > + let watchedTourVersion = Services.prefs.getStringPref(PREF_WATCHED_TOURSET_VERSION, ""); Why not just default this to TOURSET_VERSION? ::: browser/extensions/onboarding/bootstrap.js:79 (Diff revision 7) > + if (watchedTourVersion !== CURRENT_BROWSER_VERSION) { > + if (Services.prefs.getBoolPref(PREF_NEW_USER, false)) { > + Services.prefs.setBoolPref(PREF_NEW_USER, false); > + } > + } Is this just overly-defensive? It seems like this should never happen. ::: browser/extensions/onboarding/content/onboarding.js:159 (Diff revision 7) > this._window = contentWindow; > this._tourItems = []; > this._tourPages = []; > + > + // we only support the new user tour at this moment > + if (!Services.prefs.getBoolPref("browser.onboarding.newuser", false)) { Shouldn't we also set this to false at some point?

> I think I'm asking whether the tourset pref is even needed if it is always > going to match the browser version. And if it isn't always going to match > the browser version why do we ever compare it to the browser version? The tourset pref store the major tourset version (ex: "57") but not the current browser version, so when browser update to the next version (ex: 58, 59), the tourset pref is still "57" if we didn't do any major tourset update. Refer Comment 25 Use case, ``` Upgrade to the next tour set version: if user have watched the old update user tour (ex: 57), but now we have a new update user tour set(ex 60) - we will show the update user tour again, no matter user prefer to hide the overlay in update user tour, or have watched the old update user tour ```

Comment on attachment 8879079 [details] Bug 1367696 - PART II add xpcshell structure for onboarding; https://reviewboard.mozilla.org/r/150408/#review156012

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review155918 Thanks for the feedback! I know these new/update user detection and conditions to show which tours are pretty complicated. So I map the flow chart to Use case in comment 25 and hope it could help us figure out all the requirements and implement details. And we can have a direct talk after Wed/Thu meeting if you like > If we're never changing tours mid-browser version why do we need this pref at all? refer Comment 29 > Is there a third type, regular user? refer to the flow chart, theres only new and update user. I think the regular user can be categorized as an update user. > From what I can see the function is actually unhiding the tour button for update users and not really controlling the value of PREF_NEW_USER. The function compare the user watched tourset version and the CURRENT_BROWSER_VERSION to set newuser pref to false. Here are examples with state changes: ## new user in v56 (this is the default state to compare with the follow up cases) a new user in v56, the states will be: browser.onboarding.newuser = true, browser.onboarding.tourset-version = '56' CURRENT_BROWSER_VERSION = '56' browser.onboarding.watched-tourset-version = '56' (no previous watchedTourVersion) browser.onboarding.hidden = false ## Update from v56 to v57, v58: When the v56 new user update to v57, the variables will be changed to: browser.onboarding.newuser = false, (watchedTourVersion('56') != CURRENT_BROWSER_VERSION('57')) browser.onboarding.tourset-version = '57' (we will have a major change there, so the version is pumped) CURRENT_BROWSER_VERSION = '57' browser.onboarding.watched-tourset-version = '57' (watchedTourVersion `56` != TOURSET_VERSION `57`) browser.onboarding.hidden = false (watchedTourVersion `56` != TOURSET_VERSION `57`) When the v56 new user update directly to v58 (skip version), the variables will be changed to: browser.onboarding.newuser = false, (watchedTourVersion('56') != CURRENT_BROWSER_VERSION('58')) browser.onboarding.tourset-version = '57' (we do not have a major change there, so the version is stay in `57`) CURRENT_BROWSER_VERSION = '58' browser.onboarding.watched-tourset-version = '57'(watchedTourVersion `56` != TOURSET_VERSION `57`) browser.onboarding.hidden = false (watchedTourVersion `56` != TOURSET_VERSION `57`) ## Update from v53(we don't know the user is a new or update user) to v57, v58 When the v53 user update to v57, the variables will be changed to: browser.onboarding.newuser = '', (we could set false by default when we read the newuser pref) browser.onboarding.tourset-version = '57' CURRENT_BROWSER_VERSION = '57' browser.onboarding.watched-tourset-version = '57' (no previous watchedTourVersion) browser.onboarding.hidden = false (watchedTourVersion `56` != TOURSET_VERSION `57`) When the v53 user update to v58, the variables will be changed to: browser.onboarding.newuser = '', (we could set false by default when we read the newuser pref) browser.onboarding.tourset-version = '57' CURRENT_BROWSER_VERSION = '58' browser.onboarding.watched-tourset-version = '57' (no previous watchedTourVersion) browser.onboarding.hidden = false (watchedTourVersion `56` != TOURSET_VERSION `57`) ## a update user update from v58 to v60 (assume we don't have any new tour set update): an update user in v58, the default state will be: browser.onboarding.newuser = false, browser.onboarding.tourset-version = '57' CURRENT_BROWSER_VERSION = '58' browser.onboarding.watched-tourset-version = '57' browser.onboarding.hidden = true an user from v58 to v60, the variables will be changed to: browser.onboarding.newuser = false, browser.onboarding.tourset-version = '57' CURRENT_BROWSER_VERSION = '60' browser.onboarding.watched-tourset-version = '57' browser.onboarding.hidden = true ## an update user update from v58 to v65 (assume we have new tour set in v62): the variables will be changed to: browser.onboarding.newuser = false, browser.onboarding.tourset-version = '62' CURRENT_BROWSER_VERSION = '65' browser.onboarding.watched-tourset-version = '62' (watchedTourVersion '57' !== TOURSET_VERSION '62') browser.onboarding.hidden = false (watchedTourVersion '57' !== TOURSET_VERSION '62') > Why not just default this to TOURSET_VERSION? we have to turn on "browser.onboarding.hidden" when watchedTourVersion and TOURSET_VERSION version are different, to meet the requirement of `Upgrade to the next tour set version` (in comment 25) > Is this just overly-defensive? It seems like this should never happen. see previous comment > Shouldn't we also set this to false at some point? yes! that will help make less assumption.

Using these examples what do we have when a new user starts Firefox 58? Following the code flow: TOURSET_VERSION = '57' CURRENT_BROWSER_VERSION = '58' nsBrowserGlue sets browser.onboarding.newuser to true. browser.onboarding.watched-tourset-version is undefined so watchedTourVersion is "" This causes us to set browser.onboarding.watched-tourset-version and watchedTourVersion to '57' Since watchedTourVersion !== CURRENT_BROWSER_VERSION we set browser.onboarding.newuser to false. So user is now considered an update user and won't see the new user tour. Looking at the flowchart I think I see the following behaviours: New user installs Firefox -> new user tour User upgrades from an earlier Firefox which had the same tour set -> make no changes to the tour shown User upgrades from an earlier Firefox which had a different tour set -> updated user tour (force shown even if hidden previously) User remains on the same version of Firefox -> make no changes to the tour shown I think that means we need three prefs to track things: newUser, set to true in nsBrowserGlue when a new user starts Firefox seenTourSet, initially empty to say the last seen tour set for the user tourType, initially empty to say what type of tour should be shown. Then the following pseudocode would work: if newUser is true { set tourType to "newuser" set newUser to false } else if seenTourSet != TOURSET_VERSION { set tourType to "update" unhide tour } set seenTourSet to TOURSET_VERSION Either way we desperately need tests for this complex behaviour which will be difficult if one of the variables we test against is the browser version. If you haven't had luck in disabling/enabling the add-on then an alternative is to break out this logic into a JSM or other script that the test can load and call into directly.

Thanks for review! With your suggestion we don't need to depends on current browser version, it seems more testable now. I'd try separate the rules into OnboardingTourType.jsm and setup the xpcshell test.

Comment on attachment 8879079 [details] Bug 1367696 - PART II add xpcshell structure for onboarding; https://reviewboard.mozilla.org/r/150408/#review156600 ::: browser/extensions/onboarding/content/onboarding.js:250 (Diff revision 5) > - if (this._tourItems.length == 0) { > - // Lazy loading until first toggle. > + // Lazy loading until first toggle. > - this._loadTours(onboardingTours); > + if (this._tourItems.length == 0) { > + let tours = []; > + this.userTours.forEach(tourId => { > + if (onboardingTours[tourId]) { 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.

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review155590 ::: browser/extensions/onboarding/bootstrap.js:66 (Diff revision 7) > + */ > +function determineUserType() { > + const PREF_NEW_USER = "browser.onboarding.newuser"; > + const PREF_WATCHED_TOURSET_VERSION = "browser.onboarding.watched-tourset-version"; > + // browser.onboarding.tourset-version is only changed when new onboarding tour version is available. > + const TOURSET_VERSION = Preferences.get("browser.onboarding.tourset-version", ""); Maybe this don't need to be a pref? We can have a const for it. ::: browser/extensions/onboarding/bootstrap.js:71 (Diff revision 8) > + const PREF_WATCHED_TOURSET_VERSION = "browser.onboarding.watched-tourset-version"; > + // Tourset version is only changed when there's a major onboarding tourset update. > + const TOURSET_VERSION = Preferences.get("browser.onboarding.tourset-version", ""); > + // The regex is modified from toolkit/components/urlformatter/nsURLFormatter.js and only > + // match the major browser version > + const CURRENT_BROWSER_VERSION = Services.appinfo.version.replace(/^([^\.]+)\.[0-9]+[a-z]*.*/gi, "$1"); I'm still not very sure if we really need to get the current browser version involved.. If we have an tourset that doesn't update between version 59~61, and a fresh user who installed version 59 and saw the "new user set" will see "updated use set" of the same tour version after updating to version 60 sounds a little bit weird to me. If we can confirm "Update tour target" maps to our TOURSET_VERSION then we may be able to get rid of the CURRENT_BROWSER_VERSION. ::: browser/extensions/onboarding/content/onboarding.js:159 (Diff revision 8) > this._window = contentWindow; > this._tourItems = []; > this._tourPages = []; > + > + // we only support the new user tour at this moment > + if (!Services.prefs.getBoolPref("browser.onboarding.newuser", false)) { I guess we don't even need to load Onboarding module, so you can move the check to before creating it, together with browser.onboarding.hidden and browser.onboarding.enabled.

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review155590 > Maybe this don't need to be a pref? We can have a const for it. I forgot to clear this out. Since this has been discussed before please just ignore it. sorry for the confusion.

> 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. Thanks. Since PART II can be done in 57. I'll move `PART II allow reuse existing tours` into a separate bug and focus on tour type detection and related tests in this bug.

The PR PART I is based on comment 34 suggestion and move the logic into OnboardTourType.jsm PART II add the xpcshelll test infrastructure for onboarding PART III add OnboardTourType.jsm test Please kindly review again

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review156678 ::: browser/components/nsBrowserGlue.js:1729 (Diff revision 10) > currentUIVersion = Services.prefs.getIntPref("browser.migration.version"); > } else { > // This is a new profile, nothing to migrate. > Services.prefs.setIntPref("browser.migration.version", UI_VERSION); > + // Annotate that its a new firefox user for onboarding > + Services.prefs.setBoolPref("browser.onboarding.newuser", true); I wonder we could tidy up `_migrateUI()` by simply bump the UI_VERSION, instead of adding this pref in this new profile block? We could set `browser.onboarding.seen-tourset-version` to `"0"`, to distinguish the unset value. The logic in `OnboardingTourType.jsm` can then tidy up to ``` if (seen-userset unset) { - show new user tour - set seen-userset to "1" } else if (seen-userset !== current tour) { - show updated user tour - set seen-userset to "1" } ``` and the same logic will hold for future versions. BTW I wonder if `browser.onboarding.seen-tourset-version` should be a Int perf instead, and instead of `!==` the above code should use `<`.

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review156894 Let's take Tim's suggested change then this will be good to go ::: browser/components/nsBrowserGlue.js:1729 (Diff revision 10) > currentUIVersion = Services.prefs.getIntPref("browser.migration.version"); > } else { > // This is a new profile, nothing to migrate. > Services.prefs.setIntPref("browser.migration.version", UI_VERSION); > + // Annotate that its a new firefox user for onboarding > + Services.prefs.setBoolPref("browser.onboarding.newuser", true); This is a good thought.

Comment on attachment 8880318 [details] Bug 1367696 - PART III add OnboardingTourType tests; https://reviewboard.mozilla.org/r/151668/#review156896 Excellent!

(In reply to Fred Lin [:gasolin] from comment #25) > Michael, here's the user case I distilled from the flow chart. QA may able > to use these for manual test plan, and reviewer can refer these use case as > requirement as well. Can you help double confirm if these cases are correct? > > > New User Tour > > - when user start with a new profile, we treat him/her as a new user > - we will show user the new user tour > - user can hide the overlay and fox icon by click the checkbox in the > overlay > - once overlay is dismissed, the overlay won't show again > - if the user have watched the new user tour and not hide it by click the > checkbox, we keep showing the fox icon. > > Update User Tour > > - when user upgrade from the older browser version, we treat him/her as a > upgrade user > - we only count on major browser version (ex: 56,57,58,59) upgrade, and > neglect minor versions (If user update from 56.0a1 -> 56.0b2, we still treat > the current browser version as 56) > - we will show the update tour, no matter user prefer to hide the overlay > in new user tour > - the checkbox should be shown as unchecked in overlay > - (in v56) we will hide the overlay and the fox icon. > - (after v56) we will show user the update user tour > - (after v56)if the user have watched the update user tour and not hide it > by click the checkbox, we keep showing the fox icon. > - if the user never watched the update user tour, show the update user tour > - (after v56) user can hide the overlay and fox icon by click the checkbox > in the overlay > - (after v56) once overlay is dismissed, the overlay wont show again in > this browser version > > Upgrade to the next major version > > > - if user have watched the old new user tour (ex: 56), but now we have a > 'new' new user tour set(ex 57), we will not show the new user tour to the > user (because the user is now turned into the update user) > - if user have watched the old update user tour (ex: 57), but now we have > a new update user tour set(ex 60) > - we will show the update user tour again, no matter user prefer to hide > the overlay in update user tour, or have watched the old update user tour > - the checkbox should be shown as unchecked in overlay This sounds correct. The main point is that the checkbox removes that particular tour. So I can say I'm done with the 57 update tour and it will go away. But when we make a new update tour in, let's say, Firefox 70 then we'll show that one to updating users regardless of whether or not they checked the box to get rid of the 57 update tour.

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review157004 ::: browser/components/nsBrowserGlue.js:1729 (Diff revision 11) > currentUIVersion = Services.prefs.getIntPref("browser.migration.version"); > } else { > // This is a new profile, nothing to migrate. > Services.prefs.setIntPref("browser.migration.version", UI_VERSION); > + // Annotate that a user haven't seen any onboarding tour > + Services.prefs.setIntPref("browser.onboarding.seen-tourset-version", 0); This is not what I talked about. I asked the verision to be set on existing profiles, not on new profiles, so we will know a new profile is new profile whenever we find the pref is unset.

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review157028 ::: browser/extensions/onboarding/OnboardingTourType.jsm:21 (Diff revision 11) > +var OnboardingTourType = { > + /** > + * Determine the current tour type (new user tour or update user tour). > + * As the result the function will set the right current tour type in the tour type pref (PREF_TOUR_TYPE) for later use. > + * The function checks 3 criterias > + * - PREF_NEW_USER_PROFILE: if is a new user profile Comment needs to be updated. ::: browser/extensions/onboarding/OnboardingTourType.jsm:36 (Diff revision 11) > + let seenTourSet = Services.prefs.getIntPref(PREF_SEEN_TOURSET, 0); > + if (Services.prefs.prefHasUserValue(PREF_SEEN_TOURSET) && > + Services.prefs.getIntPref(PREF_SEEN_TOURSET, 0) === 0) { > + Services.prefs.setStringPref(PREF_TOUR_TYPE, "new"); > + } else if (seenTourSet < TOURSET_VERSION) { > + // show update tour when tour set version does not matched the seen tourset version, ditto.

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review157042 ::: browser/components/nsBrowserGlue.js:1719 (Diff revision 14) > AlertsService.showAlertNotification(null, title, body, true, null, clickCallback); > }, > > // eslint-disable-next-line complexity > _migrateUI: function BG__migrateUI() { > - const UI_VERSION = 48; > + const UI_VERSION = 56; There is no need to bump UI_VERSION to Gecko version. If you look at the commit log you will know that. ::: browser/components/nsBrowserGlue.js:2057 (Diff revision 14) > // The situation was only happening for a few nightlies in 56, so this migration can > // be removed in version 58. > xulStore.removeValue(BROWSER_DOCURL, "sidebar-box", "checked"); > } > > + if (currentUIVersion < 56) { s/56/49/ ::: browser/extensions/onboarding/OnboardingTourType.jsm:27 (Diff revision 14) > + * As the result the function will set the right current tour type in the tour type pref (PREF_TOUR_TYPE) for later use. > + */ > + check() { > + const PREF_TOUR_TYPE = "browser.onboarding.tour-type"; > + const PREF_SEEN_TOURSET_VERSION = "browser.onboarding.seen-tourset-version"; > + // Tourset version is only changed when there's a major onboarding tourset update. Remove this redundant comment (it's in firefox.js already). ::: browser/extensions/onboarding/OnboardingTourType.jsm:31 (Diff revision 14) > + const PREF_SEEN_TOURSET_VERSION = "browser.onboarding.seen-tourset-version"; > + // Tourset version is only changed when there's a major onboarding tourset update. > + const TOURSET_VERSION = Services.prefs.getIntPref("browser.onboarding.tourset-version"); > + > + if (!Services.prefs.prefHasUserValue(PREF_SEEN_TOURSET_VERSION)) { > + Services.prefs.setStringPref(PREF_TOUR_TYPE, "new"); Comment above ``` // User has never seen an onboarding tour, present the user with the new user tourset. ```

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; Sorry I was meant to r+ with comments addressed. Let me know if mozreview didn't record this.

Issue addressed, thanks for the nice suggestion! Please help set the review+ flag in mozreview to let the autoland work

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review157060 ::: browser/components/nsBrowserGlue.js:2058 (Diff revision 15) > // be removed in version 58. > xulStore.removeValue(BROWSER_DOCURL, "sidebar-box", "checked"); > } > > + if (currentUIVersion < 49) { > + // Annotate that a user haven't seen any onboarding tour `// Mark this as an upgraded profile so we don't offer the initial new user onboarding tour.`

Comment on attachment 8879027 [details] Bug 1367696 - PART I determine the tour type; https://reviewboard.mozilla.org/r/150328/#review157394 ::: browser/extensions/onboarding/README.md:19 (Diff revision 15) > * All styles and ids should be formated as `onboarding-*` to avoid conflict with the origin page. > * All strings in `locales` should be formated as `onboarding.*` for consistency. > > +## How to pump tour set version after update tours > + > +The tourset version is used to track the last major tourset change version. The `tourset-version` pref store the major tourset version (ex: `1`) but not the current browser ersion. When browser update to the next version (ex: 58, 59) the tourset pref is still `1` if we didn't do any major tourset update. s/ersion/version ::: browser/extensions/onboarding/content/onboarding.js:159 (Diff revision 15) > + if (Services.prefs.getStringPref("browser.onboarding.tour-type", "update") !== "new") { > + return; > + } This needs an extra space of indent

Comment on attachment 8879079 [details] Bug 1367696 - PART II add xpcshell structure for onboarding; https://reviewboard.mozilla.org/r/150408/#review157396 ::: browser/extensions/onboarding/test/unit/head.js:43 (Diff revision 12) > +const require = Loader.Require(loader, {id: ""}); > +const sinon = require("sinon-2.3.2"); Where is this used? The SDK is going to be removed in a few versions so I'm not sure adding new dependencies on its loader is a good idea.

(In reply to Dave Townsend [:mossop] from comment #73) > Comment on attachment 8879079 [details] > Bug 1367696 - PART II add xpcshell structure for onboarding; > > https://reviewboard.mozilla.org/r/150408/#review157396 > > ::: browser/extensions/onboarding/test/unit/head.js:43 > (Diff revision 12) > > +const require = Loader.Require(loader, {id: ""}); > > +const sinon = require("sinon-2.3.2"); > > Where is this used? > > The SDK is going to be removed in a few versions so I'm not sure adding new > dependencies on its loader is a good idea. I really like sinon but its not been used in onboarding test yet, I'll remove that.

Comment on attachment 8879079 [details] Bug 1367696 - PART II add xpcshell structure for onboarding; https://reviewboard.mozilla.org/r/150408/#review157738 ::: browser/extensions/onboarding/test/unit/head.js:22 (Diff revision 14) > + extensionDir = extensionDir.parent; > + extensionDir.append(EXTENSION_ID + ".xpi"); You can just do this: extensionDir.leafName += ".xpi"

Comment on attachment 8879079 [details] Bug 1367696 - PART II add xpcshell structure for onboarding; https://reviewboard.mozilla.org/r/150408/#review157738 > You can just do this: > > extensionDir.leafName += ".xpi" thanks, I also updated test in formautofill and shield-receipe-client system add-on, xpcshell-test works fine

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4cd7f4883692 PART I determine the tour type;r=mossop,timdream https://hg.mozilla.org/integration/autoland/rev/f031600d142d PART II add xpcshell structure for onboarding;r=mossop,rexboy https://hg.mozilla.org/integration/autoland/rev/09814ac870d3 PART III add OnboardingTourType tests;r=mossop

Backed out for failing formautofill tests, e.g. browser/extensions/formautofill/test/unit/heuristics/test_basic.js: https://hg.mozilla.org/integration/autoland/rev/b57b55397f70fa3286f89a56eb186456a3d3ddce https://hg.mozilla.org/integration/autoland/rev/7aa2c6582fda1482f6ebb7b9c5e16907645d8a61 https://hg.mozilla.org/integration/autoland/rev/910dd9259059f1724910efeb963b471093cf5635 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=09814ac870d3132d28a8d2e0b531b54379d2b516&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110219219&repo=autoland [task 2017-06-28T00:27:36.277355Z] 00:27:36 INFO - TEST-START | browser/extensions/formautofill/test/unit/heuristics/test_basic.js [task 2017-06-28T00:27:37.108503Z] 00:27:37 WARNING - TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/unit/heuristics/test_basic.js | xpcshell return code: 0 [task 2017-06-28T00:27:37.111179Z] 00:27:37 INFO - TEST-INFO took 836ms [task 2017-06-28T00:27:37.113040Z] 00:27:37 INFO - >>>>>>> [task 2017-06-28T00:27:37.115485Z] 00:27:37 INFO - PID 8994 | JavaScript strict warning: resource://gre/modules/commonjs/toolkit/loader.js -> resource://testing-common/sinon-2.3.2.js, line 8941: ReferenceError: reference to undefined property "iso-8859-8-i" [task 2017-06-28T00:27:37.117940Z] 00:27:37 INFO - NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXPCComponents_Utils.import] [task 2017-06-28T00:27:37.121069Z] 00:27:37 INFO - runHeuristicsTest@/home/worker/workspace/build/tests/xpcshell/tests/browser/extensions/formautofill/test/unit/head.js:120:3 [task 2017-06-28T00:27:37.123133Z] 00:27:37 INFO - @/home/worker/workspace/build/tests/xpcshell/tests/browser/extensions/formautofill/test/unit/heuristics/test_basic.js:5:1 [task 2017-06-28T00:27:37.125034Z] 00:27:37 INFO - load_file@/home/worker/workspace/build/tests/xpcshell/head.js:653:7 [task 2017-06-28T00:27:37.126975Z] 00:27:37 INFO - _load_files@/home/worker/workspace/build/tests/xpcshell/head.js:665:3 [task 2017-06-28T00:27:37.128951Z] 00:27:37 INFO - _execute_test@/home/worker/workspace/build/tests/xpcshell/head.js:523:3 [task 2017-06-28T00:27:37.131057Z] 00:27:37 INFO - @-e:1:1 [task 2017-06-28T00:27:37.133518Z] 00:27:37 INFO - (xpcshell/head.js) | test MAIN run_test pending (1) [task 2017-06-28T00:27:37.135688Z] 00:27:37 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2) [task 2017-06-28T00:27:37.140805Z] 00:27:37 INFO - (xpcshell/head.js) | test MAIN run_test finished (2) [task 2017-06-28T00:27:37.142743Z] 00:27:37 INFO - running event loop [task 2017-06-28T00:27:37.145013Z] 00:27:37 INFO - "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "iso-8859-8-i"" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://testing-common/sinon-2.3.2.js" line: 8941}]" [task 2017-06-28T00:27:37.147093Z] 00:27:37 INFO - browser/extensions/formautofill/test/unit/heuristics/test_basic.js | Starting head_initialize [task 2017-06-28T00:27:37.148993Z] 00:27:37 INFO - (xpcshell/head.js) | test head_initialize pending (2) [task 2017-06-28T00:27:37.152510Z] 00:27:37 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2) [task 2017-06-28T00:27:37.154433Z] 00:27:37 INFO - (xpcshell/head.js) | test run_next_test 1 pending (2) [task 2017-06-28T00:27:37.156375Z] 00:27:37 INFO - (xpcshell/head.js) | test head_initialize finished (2) [task 2017-06-28T00:27:37.158325Z] 00:27:37 INFO - (xpcshell/head.js) | test run_next_test 1 finished (1) [task 2017-06-28T00:27:37.160381Z] 00:27:37 INFO - exiting test [task 2017-06-28T00:27:37.162514Z] 00:27:37 INFO - <<<<<<<

(In reply to Fred Lin [:gasolin] from comment #82) > Comment on attachment 8879079 [details] > Bug 1367696 - PART II add xpcshell structure for onboarding; > > https://reviewboard.mozilla.org/r/150408/#review157738 > > > You can just do this: > > > > extensionDir.leafName += ".xpi" > > thanks, I also updated test in formautofill and shield-receipe-client system > add-on, xpcshell-test works fine Since they are outside of the code under review changes to these other places really should be made as a separate changeset rather than being rolled into this one. I would have also preferred you re-requesting review on that too.

> > Since they are outside of the code under review changes to these other > places really should be made as a separate changeset rather than being > rolled into this one. I would have also preferred you re-requesting review > on that too. I'll pulled out the change ourside of the onboarding extension now so there's no extra review needed. I've update the rebased version and will reland if try run all green. BTW for those test changes, I pulled the latest branch and do the local xpcshell-test again, related extensions xpcshell tests still all pass.

Comment on attachment 8879079 [details] Bug 1367696 - PART II add xpcshell structure for onboarding; https://reviewboard.mozilla.org/r/150408/#review158086 ::: browser/extensions/onboarding/test/unit/head.js:22 (Diff revision 17) > +extensionDir.append("browser"); > +extensionDir.append("features"); > +extensionDir.append(EXTENSION_ID); > +// If the unpacked extension doesn't exist, use the packed version. > +if (!extensionDir.exists()) { > + extensionDir = extensionDir.parent; This line is unnecessary and is what is breaking the tests.

Thanks mossop! Thats the issue. (Sorry for my limited addon-SDK knowledge) Try green now.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7624b5ff791a PART I determine the tour type;r=mossop,timdream https://hg.mozilla.org/integration/autoland/rev/103b93ca7422 PART II add xpcshell structure for onboarding;r=mossop,rexboy https://hg.mozilla.org/integration/autoland/rev/7c5ecb0be10d PART III add OnboardingTourType tests;r=mossop

https://hg.mozilla.org/mozilla-central/rev/7624b5ff791a https://hg.mozilla.org/mozilla-central/rev/103b93ca7422 https://hg.mozilla.org/mozilla-central/rev/7c5ecb0be10d

I have verified this bug on today's nightly.