Fernando Jiménez Moreno [:ferjm] Assignee
	Description • 11 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=905637#c11

	Sam Penrose
	Comment 1 • 11 years ago

Fernando, should we also think about two onlogout events here? Where's My Fox needs to know when the user controlling the device has successfully logged out with authentication:

  https://bugzilla.mozilla.org/show_bug.cgi?id=952355

and of course, the user can be logged out because they failed authentication or because of some other error state.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 2 • 11 years ago

This bug is supposed to trigger events only for FxA manager apps (FTU and Settings so far), not for RPs.

WMF is a RP and so we will need a different mechanism to notify about onlogout events as described in bug 952355. 

We talked about this during Madrid work week, but AFAICT we didn't decide how to notify RPs about FxA logouts. We mentioned different alternatives, like removing the authentication cookie or the possibility of using system messages. We'll need to work on this, but probably not in this bug.

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please)
	Comment 6 • 11 years ago

:ferjm great to see the WIP code. Some questions about it:

* how should settings app code listen for changes?
* what will the format of the data packet be? will you just send down the event name, or will you send down data with it?
* what will be the event names?

For an example of some client code, take a look at the event listener (lines 21-22) and the event handler (lines 27-48) here:
https://github.com/6a68/gaia/blob/641c913/apps/settings/js/fxa_menu.js

Thanks!

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 7 • 11 years ago

(In reply to jhirsch from comment #6)
> :ferjm great to see the WIP code. Some questions about it:
> 
> * how should settings app code listen for changes?

Via FxAccountsIACHelper.addEventListener(eventName, listener)

> * what will the format of the data packet be? will you just send down the
> event name, or will you send down data with it?

There won't be any data passed to the event handlers.

> * what will be the event names?
> 

onlogin, onverifiedlogin and onlogout.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 9 • 11 years ago

This PR enables users of the FxAccountsIACHelper to subscribe/unsubscribe to events like 'onlogin', 'onverified' and 'onlogout' coming from the FxA platform implementation.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 10 • 11 years ago

I am also adding the diff, so you can use github or bugzilla for the review.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 11 • 11 years ago

Mark, this patch does a s/fxaccounts:onlogin/fxaccounts:onverified for the notification that is triggered when the verification status changes from false to true and introduces a new fxaccounts:onlogin notification that is sent right after setting the signed in user even if it is an unverified account.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 12 • 11 years ago

Fabrice, this patch introduces the observers for the fxa notifications in the B2G fxa management service and does a s/mozFxAccountsRPChromeEvent/mozFxAccountsUnsolChromeEvent/g where "RP" used to mean Relying Party and "Unsol" stands for "unsolicited".

	Mark Hammond [:markh] [:mhammond]
	Comment 13 • 11 years ago

Comment on attachment 8361701 [details] [diff] [review]
Part 1: Add onverified/onlogin (WIP)

Review of attachment 8361701 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM - I added the same notification in bug 956605, but this is a better place to fire it than I had in that bug.  It's also going to make a couple other of my patches stale, but I can live with that.

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please)
	Comment 14 • 11 years ago

ferjm - It just occurred to me that it might not be possible to tell, from the settings app, whether the user is logged out, or whether there is just no information locally. Is that right? getAccounts() would return `null` in either case?

It might be worthwhile to differentiate the two states, to avoid showing users a logged-out state when they're already logged into the server, but the cache is cold. I'll know more after talking with UX next week about cold-cache edge cases.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 15 • 11 years ago

Comment on attachment 8361696 [details]
Part 3: Gaia PR

r+ but please see github comments.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 16 • 11 years ago

Thanks Mark and Alive for the review!

(In reply to jhirsch from comment #14)
> ferjm - It just occurred to me that it might not be possible to tell, from
> the settings app, whether the user is logged out, or whether there is just
> no information locally. Is that right? getAccounts() would return `null` in
> either case?
> 
> It might be worthwhile to differentiate the two states, to avoid showing
> users a logged-out state when they're already logged into the server, but
> the cache is cold. I'll know more after talking with UX next week about
> cold-cache edge cases.

IMHO the concept of "logged-out account" does not make sense until we consider multi-account support.

The platform does not save any information about previously logged accounts. When a logout is requested, we remove the server session and the local account information.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 17 • 11 years ago

Try build looks good: https://tbpl.mozilla.org/?tree=Try&rev=9776f6ede91d

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 18 • 11 years ago

Thanks Fabrice!

https://hg.mozilla.org/integration/b2g-inbound/rev/dea1e5ca965d
https://hg.mozilla.org/integration/b2g-inbound/rev/27083f8936a7

https://github.com/mozilla-b2g/gaia/commit/e87b269b7b38cf2df894f51ac4498c4697c2f8d2

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/dea1e5ca965d
https://hg.mozilla.org/mozilla-central/rev/27083f8936a7