Closed Bug 1377470 Opened 3 years ago Closed 3 years ago

Updated users still see new user tour after bug 1367696 lands

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: timdream, Assigned: gasolin)

References

Details

(Whiteboard: [photon-onboarding] )

Attachments

(1 file)

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?
Flags: needinfo?(gasolin)
It works after a restart. Looks like we're checking the prefs before the migration code is running in final-ui-startup.
(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?
Summary: Updated users still sees new user tour after bug 1367696 lands → Updated users still see new user tour after bug 1367696 lands
(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.
Flags: qe-verify+
QA Contact: jwilliams
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.
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.
Flags: needinfo?(gasolin)
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.
Attachment #8883813 - Flags: review?(dtownsend) → review-
Thanks for review!

After double check it seems Activity Stream does handle the similar issue, so borrow the bootstrap check from them.
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
Attachment #8883813 - Flags: review?(dtownsend)
Issue addressed and update README for 1. how to show the onboarding tour, 2. update the overall architecture

Please kindly review it again
Comment on attachment 8883813 [details]
Bug 1377470 - run onboarding scripts after browser UI is ready;

https://reviewboard.mozilla.org/r/154770/#review161360
Attachment #8883813 - Flags: review?(dtownsend) → review+
Target Milestone: --- → Firefox 56
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.
Keywords: checkin-needed
See Also: → 1380245
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed0ec2f26de4
run onboarding scripts after browser UI is ready;r=mossop
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed0ec2f26de4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Blocks: 1366056
I have verified this issue is fixed on today's nightly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.