Updated users still see new user tour after bug 1367696 lands

VERIFIED FIXED in Firefox 56

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: timdream, Assigned: gasolin)

Tracking

unspecified
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding] )

Attachments

(1 attachment)

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
Comment hidden (mozreview-request)
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.
Assignee

Comment 6

2 years ago
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 7

2 years ago
mozreview-review
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-
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
Thanks for review!

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

Comment 10

2 years ago
mozreview-review
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)
Comment hidden (mozreview-request)
Assignee

Comment 12

2 years ago
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

2 years ago
mozreview-review
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 17

2 years ago
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
Assignee

Updated

2 years ago
See Also: → 1380245

Comment 18

2 years ago
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

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ed0ec2f26de4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
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.