1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

[Fischer [:Fischer]]

Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account

Comment 1

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Observe fxaccounts:onverified for push message of successful login:
os.addObserver(this, "fxaccounts:onverified");

http://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#251

Comment 2

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

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

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

https://reviewboard.mozilla.org/r/155596/#review160848

::: browser/extensions/onboarding/bootstrap.js:67
(Diff revision 1)
> +function initFxALoginChecker() {
> +  function setSyncTourComplete() {
> +    Preferences.set("browser.onboarding.tour.onboarding-tour-sync.completed", true);
> +  }
> +  // Observer for login action.
> +  Services.obs.addObserver(setSyncTourComplete, "fxaccounts:onverified");

We don't need to do this if the user is already signed in.

Also you need to remove this observer on shutdown.

Comment 4

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

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

Comment 5

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

https://reviewboard.mozilla.org/r/155596/#review160848

> We don't need to do this if the user is already signed in.
> 
> Also you need to remove this observer on shutdown.

That makes sense. Thanks for pointing that out.
To perform more detailed check and remember whether the observer is being registered, I made the observer an object.
Please take a look again. Thank you!

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

https://reviewboard.mozilla.org/r/155596/#review161350

::: browser/extensions/onboarding/bootstrap.js:67
(Diff revision 2)
> +let syncTourChecker = {
> +  registered: false,
> +  observe() {
> +    this.setComplete();
> +  },
> +  init() {

Add an extra line space between each property for readability please.

Comment 7

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

How is this not for Firefox 56? Please correct me if this is wrong.

Comment 8

[KM Lee [:rexboy] (inactivate after Sep. 22)]

If I remember correctly, we decided to check Firefox sync completed only as users click on the form submit button (and that has been landed already) as our MVP, and postpone the more advanced login check to version 57.

As the patch has been completed we may rejudge if that's okay to include it in version 56.

Comment 9

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

So this is not even an P1. Flag set accordingly. Thanks.

Comment 10

[KM Lee [:rexboy] (inactivate after Sep. 22)]

I think it's triaged as MVP of 57 though. We can discuss that later.

Comment 11

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

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

Comment 12

[[e-mail]]

please rebase after bug 1377470 landed

Comment 13

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

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

Comment 14

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

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

Comment 15

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Update: just found the patch causes regression because it loads FxAccounts at too early phase. I'll fix that before landing.

Comment 16

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

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

Comment 17

[KM Lee [:rexboy] (inactivate after Sep. 22)]

I got a test error upon browser_startup.js saying that I can't load FxAccount.jsm before handling user events. 
It is quit okay to postpone the loading but I didn't found a certain event for "after handling user events". Seems the last event I can observe for application start is sessionstore-windows-restored, which doesn't satisfy the test's requirement. Florian could you suggest how to postpone the module loading timing to after handling user events?

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

https://reviewboard.mozilla.org/r/155596/#review162422

::: browser/extensions/onboarding/bootstrap.js:53
(Diff revision 6)
>   *   - {*} value the value to set
>   **/
>  function setPrefs(prefs) {
>    prefs.forEach(pref => {
>      if (PREF_WHITELIST.includes(pref.name)) {
>        Preferences.set(pref.name, pref.value);

Could this avoid using Preferences.jsm?

Also, could this just use http://searchfox.org/mozilla-central/source/toolkit/modules/AsyncPrefs.jsm instead of re-implementing a pref setting mechanism to cross the process boundary?

::: browser/extensions/onboarding/bootstrap.js:129
(Diff revision 6)
>   */
>  function onBrowserReady() {
>    waitingForBrowserReady = false;
>  
>    OnboardingTourType.check();
>    Services.mm.loadFrameScript("resource://onboarding/onboarding.js", true);

Looks like you are loading this unconditionally. Is there a pref that we could read to return early when the user is already done with the onboarding?

::: browser/extensions/onboarding/bootstrap.js:142
(Diff revision 6)
>  function observe(subject, topic, data) {
>    switch (topic) {
>      case BROWSER_READY_NOTIFICATION:
>        Services.obs.removeObserver(observe, BROWSER_READY_NOTIFICATION);
>        // Avoid running synchronously during this event that's used for timing
>        setTimeout(() => onBrowserReady());

I think replacing this setTimeout with Services.tm.idleDispatchToMainThread should do what you need to pass the test.

Comment 19

[Fischer [:Fischer]]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> Comment on attachment 8884722 [details]
> Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in
> a Firefox Account.
> 
> https://reviewboard.mozilla.org/r/155596/#review162422
> 
> ::: browser/extensions/onboarding/bootstrap.js:53
> (Diff revision 6)
> >   *   - {*} value the value to set
> >   **/
> >  function setPrefs(prefs) {
> >    prefs.forEach(pref => {
> >      if (PREF_WHITELIST.includes(pref.name)) {
> >        Preferences.set(pref.name, pref.value);
> 
> Could this avoid using Preferences.jsm?
> 
> Also, could this just use
> http://searchfox.org/mozilla-central/source/toolkit/modules/AsyncPrefs.jsm instead of re-implementing a pref setting mechanism to cross the process boundary?
> 
Thanks for pointing out there is an AsyncPrefs.jsm to utilize. Open a bug 1381368 for it.

Comment 20

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

https://reviewboard.mozilla.org/r/155596/#review162422

> Could this avoid using Preferences.jsm?
> 
> Also, could this just use http://searchfox.org/mozilla-central/source/toolkit/modules/AsyncPrefs.jsm instead of re-implementing a pref setting mechanism to cross the process boundary?

Thanks for the suggestion and thanks Fischer to opened the bug. Just one thing to clearify that fxaccounts:onverified event is not an event for pref-setting things; it's being used elsewhere. And it happened that in onboarding module we just need this event to set a pref.

> Looks like you are loading this unconditionally. Is there a pref that we could read to return early when the user is already done with the onboarding?

We are now doing the pref-check inside this file for preventing further initialization
(namely browser.onboarding.enabled and browser.onboarding.hidden)
I can open a bug and investigate if we can drag them outside.
opened bug 1381377 for it.

Comment 21

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

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

Comment 22

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

https://reviewboard.mozilla.org/r/155596/#review162422

> I think replacing this setTimeout with Services.tm.idleDispatchToMainThread should do what you need to pass the test.

That works, thank you!
It turns out that I need to postpone only the syncTourChecker.init() by Services.tm.idleDispatchToMainThread after sessionstore-windows-restored. Other things in onBrowserReady need to be in the final-ui-startup phase otherwise they'd miss the onload event of about:home. I added these notices in comments.

Comment 23

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155596/diff/7-8/

Comment 24

[Florian Quèze [:florian]]

(In reply to KM Lee [:rexboy] from comment #22)

> Other things in onBrowserReady need to be in the final-ui-startup phase
> otherwise they'd miss the onload event of about:home. I added these notices
> in comments.

final-ui-startup is very early during startup, would browser-delayed-startup-finished work here?

Comment 25

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> (In reply to KM Lee [:rexboy] from comment #22)
> 
> > Other things in onBrowserReady need to be in the final-ui-startup phase
> > otherwise they'd miss the onload event of about:home. I added these notices
> > in comments.
> 
> final-ui-startup is very early during startup, would
> browser-delayed-startup-finished work here?

Works good. Changes as well.

Comment 26

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155596/diff/8-9/

Comment 27

[Florian Quèze [:florian]]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

https://reviewboard.mozilla.org/r/155596/#review162832

::: browser/extensions/onboarding/bootstrap.js:82
(Diff revision 9)
> +  observe() {
> +    this.setComplete();
> +  },
> +
> +  init() {
> +    if (Preferences.get("browser.onboarding.tour.onboarding-tour-sync.completed") == true) {

Can this use Services.prefs.getBoolPref instead of Preferences.jsm? It now supports default values too.

::: browser/extensions/onboarding/bootstrap.js:105
(Diff revision 9)
> +    Services.obs.addObserver(this, "fxaccounts:onverified");
> +    this.registered = true;
> +  },
> +
> +  setComplete() {
> +    Preferences.set("browser.onboarding.tour.onboarding-tour-sync.completed", true);

Same here, can this use Services.prefs directly?

::: browser/extensions/onboarding/bootstrap.js:141
(Diff revision 9)
>  function observe(subject, topic, data) {
>    switch (topic) {
>      case BROWSER_READY_NOTIFICATION:
>        Services.obs.removeObserver(observe, BROWSER_READY_NOTIFICATION);
>        // Avoid running synchronously during this event that's used for timing
>        setTimeout(() => onBrowserReady());

Do you still need this setTimeout? If not, we can remove the Timer.jsm import :).

Comment 28

[Florian Quèze [:florian]]

(In reply to KM Lee [:rexboy] from comment #20)

> > Also, could this just use http://searchfox.org/mozilla-central/source/toolkit/modules/AsyncPrefs.jsm instead of re-implementing a pref setting mechanism to cross the process boundary?
> 
> Thanks for the suggestion and thanks Fischer to opened the bug. Just one
> thing to clearify that fxaccounts:onverified event is not an event for
> pref-setting things; it's being used elsewhere. And it happened that in
> onboarding module we just need this event to set a pref.

My comment wasn't about the fxaccounts:onverified message, but specifically about Onboarding:OnContentMessage, which seems to only be used to set preferences from the content process.

Comment 29

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Florian Quèze [:florian] [:flo] from comment #28)

> My comment wasn't about the fxaccounts:onverified message, but specifically
> about Onboarding:OnContentMessage, which seems to only be used to set
> preferences from the content process.
Got you. Seems that would be a bigger patch so let's leave removing Onboarding:OnContentMessage to bug 1381368 and I can clean up the one you mentioned in mozReview first here.

Comment 30

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155596/diff/9-10/

Comment 31

[Florian Quèze [:florian]]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

https://reviewboard.mozilla.org/r/155596/#review164250

Looks OK, thanks for addressing my previous comments.

::: browser/extensions/onboarding/bootstrap.js:80
(Diff revision 10)
> +  observe() {
> +    this.setComplete();
> +  },
> +
> +  init() {
> +    if (Services.prefs.getBoolPref("browser.onboarding.tour.onboarding-tour-sync.completed", false) == true) {

You can remove the ' == true' here.

::: browser/extensions/onboarding/bootstrap.js:85
(Diff revision 10)
> +    if (Services.prefs.getBoolPref("browser.onboarding.tour.onboarding-tour-sync.completed", false) == true) {
> +      return;
> +    }
> +    // Check if we've already logged in at startup.
> +    fxAccounts.getSignedInUser().then(user => {
> +      if (user != null) {

You don't need the ' != null' here.

Comment 32

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155596/diff/10-11/

Comment 33

[[e-mail]]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

https://reviewboard.mozilla.org/r/155596/#review164526

::: browser/extensions/onboarding/bootstrap.js:161
(Diff revision 10)
>    // Only start Onboarding when the browser UI is ready
>    if (aReason === APP_STARTUP || aReason === ADDON_INSTALL) {
>      Services.obs.addObserver(observe, BROWSER_READY_NOTIFICATION);
> +    Services.obs.addObserver(observe, BROWSER_SESSION_STORE_NOTIFICATION);
>    } else {
>      onBrowserReady();

need to do the syncTourChecker.init() here as well

Comment 34

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8884722 [details]
Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155596/diff/11-12/

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aeb68364f85e
Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account. r=florian,mossop

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 37

[Justin [:JW_SoftvisionQA]]

I have confirmed this fix.

Comment 38

[Justin [:JW_SoftvisionQA]]

I have verified that the Sync tour responds differently to a signed in user on Win 10 x64, Win 7 x32, Ubuntu 16.04 x32, and Mac 10.12 with Firefox 58.