Per bug 1357027 comment 18, we can consider doing preference check outside the onboarding.js to decrease loading overhead. Given we do the version check (and preference reset) right before loading onboarding.js now, I think that can be possible without interrupting necessary steps.

We probably could do at [1] like ``` function onBrowserReady() { waitingForBrowserReady = false; OnboardingTourType.check(); if (Services.prefs.getBoolPref("browser.onboarding.enabled") && !Services.prefs.getBoolPref("browser.onboarding.hidden")) { Services.mm.loadFrameScript("resource://onboarding/onboarding.js", true); initContentMessageListener(); } } ``` This would even save on loading the framescript and doing `addMessageListener`. Just we might need to consider 1. after the prefs updated, it would requires a restart to see the onboarding tours loaded or 2. adding one pref observer to make dynamically change possible. The choice between the approach #1 or #2 may depend on how much degree we want to save resource and how important the onboarding tours are. [1] https://dxr.mozilla.org/mozilla-central/rev/5f44d10bacca2d693413b529e0caadc73e634e1e/browser/extensions/onboarding/bootstrap.js#78

(In reply to Fischer [:Fischer] from comment #1) Could we completely avoid the Services.obs.addObserver(observe, BROWSER_READY_NOTIFICATION) call when we are not going to show any tour? Probably something like: if (Services.prefs.getBoolPref("browser.onboarding.enabled") && (!Services.prefs.getBoolPref("browser.onboarding.hidden" || Services.prefs.getIntPref(PREF_SEEN_TOURSET_VERSION, 0) < Services.prefs.getIntPref("browser.onboarding.tourset-version"))) Services.obs.addObserver(observe, BROWSER_READY_NOTIFICATION) From reading the code, it's not obvious why OnboardingTourType needs to be a separate module. It contains a single function that's only called from this bootstrap.js file.

OnboardingTourType is written as a separate module for unit test

(In reply to Florian Quèze [:florian] [:flo] from comment #2) > (In reply to Fischer [:Fischer] from comment #1) > > Could we completely avoid the Services.obs.addObserver(observe, > BROWSER_READY_NOTIFICATION) call when we are not going to show any tour? > > Probably something like: > if (Services.prefs.getBoolPref("browser.onboarding.enabled") && > (!Services.prefs.getBoolPref("browser.onboarding.hidden" || > Services.prefs.getIntPref(PREF_SEEN_TOURSET_VERSION, 0) < > Services.prefs.getIntPref("browser.onboarding.tourset-version"))) > Services.obs.addObserver(observe, BROWSER_READY_NOTIFICATION) > Because the onboarding tours will have different versions (such as the tours for 56 and the tours for 57) and different types (such as the tours for new users and the tours for updating users), we have to check out the different user type and the different tour version to decide what tours to display. Also assume if user hidden the tours in 56, then we would re-show the new version tours in 57 because of another new tour version there. The job of the OnboardingTourType.jsm is doing this checking. And why we are doing `Services.obs.addObserver(observe, BROWSER_READY_NOTIFICATION)` is because of the bug 1377470. That is to make sure we are doing `OnboardingTourType.check` in the right timing if the misjuding could happen. I am going to take this bug because this should be a good improvement and probably will fix it after the major tasks of the onboarding are done and stable.

Originally Onboarding tours could be hidden and we could stop the initialization works in that hidden state. With the bug 1392474, Onboarding tours will not be hidden any more. There is no longer "hidden" state, which is "browser.onboarding.hidden" has been invalid so mark this bug as INVALID.

remove whiteboard tag due to its invalid