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

VERIFIED FIXED in Firefox 56

Status

()

Firefox
General
P3
normal
VERIFIED FIXED
4 months ago
14 days ago

People

(Reporter: Fischer, Assigned: rexboy)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding] )

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
Should mark the Firefox Sync tour as completed if user sign-in a Firefox Account
(Reporter)

Updated

4 months ago
Whiteboard: [photon-onboarding]
(Assignee)

Comment 1

4 months ago
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

Updated

4 months ago
Priority: -- → P2

Updated

4 months ago
Target Milestone: --- → Firefox 57

Updated

3 months ago
Flags: qe-verify+
QA Contact: jwilliams
(Assignee)

Updated

2 months ago
Assignee: nobody → rexboy

Updated

2 months ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 3

a month ago
mozreview-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

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

Comment 5

a month ago
mozreview-review-reply
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

a month ago
mozreview-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/#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
Priority: P2 → P1
(Assignee)

Comment 8

a month ago
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
(Assignee)

Comment 10

a month ago
I think it's triaged as MVP of 57 though. We can discuss that later.
Comment hidden (mozreview-request)

Comment 12

a month ago
please rebase after bug 1377470 landed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a month ago
Update: just found the patch causes regression because it loads FxAccounts at too early phase. I'll fix that before landing.
Comment hidden (mozreview-request)
Blocks: 1378772
(Assignee)

Comment 17

a month ago
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 18

a month ago
mozreview-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/#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.
(Reporter)

Comment 19

a month ago
(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.
(Assignee)

Comment 20

a month ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 22

a month ago
mozreview-review-reply
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 hidden (mozreview-request)
(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)
(Assignee)

Comment 25

a month ago
(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 hidden (mozreview-request)

Comment 27

a month ago
mozreview-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/#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.
(Assignee)

Comment 29

a month ago
(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 hidden (mozreview-request)

Comment 31

a month ago
mozreview-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/#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 hidden (mozreview-request)

Comment 33

a month ago
mozreview-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
Comment hidden (mozreview-request)
(Assignee)

Updated

29 days ago
Keywords: checkin-needed

Comment 35

29 days ago
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
Last Resolved: 29 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
I have confirmed this fix.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.