1377470 - Updated users still see new user tour after bug 1367696 lands

Status: VERIFIED FIXED

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I updated to 2017-06-30 today which is the first Nightly build that contains bug 1367696, it turned out the logic didn't hold for me, and I still sees new user tours.

Here is the about:config value I saw:

browser.onboarding.tourset-version;1 (default)
browser.onboarding.tour-type;new
browser.onboarding.seen-tourset-version;0
browser.onboarding.notification.lastPrompted;onboarding-tour-addons
browser.onboarding.notification.finished;false (default)
browser.onboarding.hidden;false (default)
browser.onboarding.enabled;true (default)

Fred, could you check this?

Comment 1

[Dave Townsend [:mossop]]

It works after a restart. Looks like we're checking the prefs before the migration code is running in final-ui-startup.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Dave Townsend [:mossop] from comment #1)
> It works after a restart. Looks like we're checking the prefs before the
> migration code is running in final-ui-startup.

Yeah I figured that's the case when I see the about:config values... can we make startup() run after migration instead?

Comment 3

[Dave Townsend [:mossop]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> (In reply to Dave Townsend [:mossop] from comment #1)
> > It works after a restart. Looks like we're checking the prefs before the
> > migration code is running in final-ui-startup.
> 
> Yeah I figured that's the case when I see the about:config values... can we
> make startup() run after migration instead?

Not startup itself that always runs sooner. We'll have to add an observer notification for final-ui-startup then defer to let the migration code to run then do the checking.

Comment 4

[[e-mail]]

Attachment: Bug 1377470 - run onboarding scripts after browser UI is ready;

Review commit: https://reviewboard.mozilla.org/r/154770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154770/

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Please manually test your path by migrate a profile from before bug 1367696 to after your WIP. You can use `mozdownload -t daily --date` to find the correct Nightly version to test on.

Comment 6

[[e-mail]]

I found ngBrowserGlue will send a `browser-ui-startup-complete` notification when all browser-ui related work (include _migrateUI) is done. 

For our case it's good for the onboarding script to be started as late as possible. so I add an observer to listen on `browser-ui-startup-complete` and run all onboarding scripts after received the notification.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8883813 [details]
Bug 1377470 - run onboarding scripts after browser UI is ready;

https://reviewboard.mozilla.org/r/154770/#review160328

::: browser/extensions/onboarding/bootstrap.js:66
(Diff revision 1)
>  
>  function uninstall(aData, aReason) {}
>  
>  function startup(aData, reason) {
> +  // wait until all browser-ui preparation work (including _migrateUI) is done
> +  Services.obs.addObserver(listener, "browser-ui-startup-complete");

Given that this is a system add-on it could get updated and restarted while the application is already running in which case this notification will never happen. You need to verify the reason for the startup and act accordingly.

::: browser/extensions/onboarding/bootstrap.js:72
(Diff revision 1)
> +}
> +
> +function shutdown(aData, reason) {}
> +
> +function listener() {
> +  Services.obs.removeObserver(this, "browser-ui-startup-complete");

this is null here, you need to remove listener as the observer.

Comment 8

[[e-mail]]

Comment on attachment 8883813 [details]
Bug 1377470 - run onboarding scripts after browser UI is ready;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154770/diff/1-2/

Comment 9

[[e-mail]]

Thanks for review!

After double check it seems Activity Stream does handle the similar issue, so borrow the bootstrap check from them.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8883813 [details]
Bug 1377470 - run onboarding scripts after browser UI is ready;

https://reviewboard.mozilla.org/r/154770/#review160882

::: browser/extensions/onboarding/bootstrap.js:18
(Diff revision 2)
>  XPCOMUtils.defineLazyModuleGetter(this, "Services",
>    "resource://gre/modules/Services.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "setTimeout",
> +  "resource://gre/modules/Timer.jsm");
>  
> +const BROWSER_READY_NOTIFICATION = "sessionstore-windows-restored";

Why this notification rather than final-ui-startup? I think with this notification you might miss restored tabs

::: browser/extensions/onboarding/bootstrap.js:98
(Diff revision 2)
>  
>  function uninstall(aData, aReason) {}
>  
>  function startup(aData, reason) {
> -  OnboardingTourType.check();
> -  Services.mm.loadFrameScript("resource://onboarding/onboarding.js", true);
> +  // Only start Onboarding up when the browser UI is ready
> +  if (Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup).startingUp) {

Just compare reason to APP_STARTUP

Comment 11

[[e-mail]]

Comment on attachment 8883813 [details]
Bug 1377470 - run onboarding scripts after browser UI is ready;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154770/diff/2-3/

Comment 12

[[e-mail]]

Issue addressed and update README for 1. how to show the onboarding tour, 2. update the overall architecture

Please kindly review it again

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8883813 [details]
Bug 1377470 - run onboarding scripts after browser UI is ready;

https://reviewboard.mozilla.org/r/154770/#review161360

Comment 14

[[e-mail]]

Comment on attachment 8883813 [details]
Bug 1377470 - run onboarding scripts after browser UI is ready;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154770/diff/3-4/

Comment 15

[[e-mail]]

Comment on attachment 8883813 [details]
Bug 1377470 - run onboarding scripts after browser UI is ready;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154770/diff/4-5/

Comment 16

[[e-mail]]

Comment on attachment 8883813 [details]
Bug 1377470 - run onboarding scripts after browser UI is ready;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154770/diff/5-6/

Comment 17

[[e-mail]]

Test works well by update from release version to patched nightly (after version update, the system addon startup aReadon is `ADDON_INSTALL` though)

But the test from nightly(2017-06-28) to patched nightly does not work since ALL system addons does not load at first start (can't see pocket, screenshots). It does not affect onboarding behavior and only happens while manual select the same profile, but I'll file another bug for that issue.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed0ec2f26de4
run onboarding scripts after browser UI is ready;r=mossop

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed0ec2f26de4

Comment 20

[Justin [:JW_SoftvisionQA]]

I have verified this issue is fixed on today's nightly.