Closed Bug 1357027 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, defect, P3)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- verified
firefox58 --- verified

People

(Reporter: Fischer, Assigned: rexboy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-onboarding] )

Attachments

(1 file)

Bug 1357027 - Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account.
59 bytes, text/x-review-board-request
mossop
: review+
florian
: review+
Details 
Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account
Whiteboard: [photon-onboarding]
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
Priority: -- → P2
Target Milestone: --- → Firefox 57
Flags: qe-verify+
QA Contact: jwilliams
Assignee: nobody → rexboy
Status: NEW → ASSIGNED
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.
Attachment #8884722 - Flags: review?(dtownsend) → review-
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 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.
Attachment #8884722 - Flags: review?(dtownsend) → review+
How is this not for Firefox 56? Please correct me if this is wrong.
Flags: needinfo?(rexboy)
Target Milestone: Firefox 57 → Firefox 56
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.
Flags: needinfo?(rexboy)
So this is not even an P1. Flag set accordingly. Thanks.
Priority: P1 → P3
I think it's triaged as MVP of 57 though. We can discuss that later.
please rebase after bug 1377470 landed
Update: just found the patch causes regression because it loads FxAccounts at too early phase. I'll fix that before landing.
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?
Flags: needinfo?(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.
(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 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 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.
(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?
Flags: needinfo?(florian)
(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 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 :).
(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.
(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 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.
Attachment #8884722 - Flags: review?(florian) → review+
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
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aeb68364f85e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
I have confirmed this fix.
Status: RESOLVED → VERIFIED
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.
status-firefox56: fixed → verified
status-firefox58: --- → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: