I've confirmed with both :callahad and :fmarier on IRC that watch should onready() after it calls either onlogin() or onlogout(). Firefox Accounts does not have the concept of the loggedInUser that BrowserID supports, so we don't have the third possible response pattern, namely just onready() when there is no state to work with.

- stylistic fixes to test head.js - FXA watch() will call onlogout or onlogin before onready - In case of error, FXA watch() just calls onerror (no onready) - Tests updated

Fernando, could you give us your thoughts on this approach?

We're now having this conversation in two places :) See also Bug 945363 Comment 17 and subsequent discussion. I'm trying to follow the BrowserID spec: https://developer.mozilla.org/en-US/docs/Web/API/navigator.id.watch "The onready callback will be invoked immediately after any automatic invocations of onlogin, onlogout, or onmatch. By waiting to display UI until onready is called, relying parties can avoid UI flicker in cases where the user agent's preferred state is out of sync with the site's session."

:ggp, if you have patches for bug 947374 and bug 971379 applied, try this - it's rebased over those two

I've replied at https://bugzilla.mozilla.org/show_bug.cgi?id=945363#c20

Comment on attachment 8384864 [details] [diff] [review] watch() should cause onlogin() or onlogout() to fire before onready() Review of attachment 8384864 [details] [diff] [review]: ----------------------------------------------------------------- Clearing the feedback flag for now as I tried this patch a few days ago and already discussed it on IRC with :jedp. This patch is buggy: it causes the Firefox Accounts login flow to appear spontaneously, even before my call to request().

Gaia test app for FxA watch() and request()

(In reply to Guilherme Gonçalves [:ggp] from comment #7) > Comment on attachment 8384864 [details] [diff] [review] > watch() should cause onlogin() or onlogout() to fire before onready() Hi, Guilherme, and thanks for checking it out! > Review of attachment 8384864 [details] [diff] [review]: > ----------------------------------------------------------------- > > Clearing the feedback flag for now as I tried this patch a few days ago and > already discussed it on IRC with :jedp. This patch is buggy: it causes the > Firefox Accounts login flow to appear spontaneously, even before my call to > request(). Yup, it does not work as anticipated. Sorry. I've created a gaia UI Test app to confirm this (attachment 8388951 [details] in comment 8). We can iterate with that to solve the problem. Cheers, j

This patch needs to be applied over the latest for bug 947374 and bug 971379. For a gaia test app, see bug 929917 attachment 8389549 [details]. ggp, want to take it for a spin? Thanks!

Bug 947374 has landed, so you just need to apply bug 971379 if you are running today's build.

Comment on attachment 8389553 [details] [diff] [review] watch() should cause onlogin() or onlogout() to fire before onready() Hi, Fernando, I've tested this with a gaia ui test app and feel confident that it works as it should. Hence I've canceled f=ggp. Do you have a moment to review? Thanks! j

Comment on attachment 8389553 [details] [diff] [review] watch() should cause onlogin() or onlogout() to fire before onready() Review of attachment 8389553 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thanks Jed! ::: services/fxaccounts/FxAccountsManager.jsm @@ +363,5 @@ > + * > + * refreshAuthentication - (bool) Force re-auth. > + * > + * silent - (bool) Prevent any UI interaction. > + * I.e., try to get an automatic assertion. nit: trailing whitespaces. ::: toolkit/identity/FirefoxAccounts.jsm @@ +177,5 @@ > > // Call logout() on the next tick > let runnable = { > run: () => { > + this.fxAccountsManager.signOut().then(() => { nit: trailing whitespace

Thanks for your review, Fernando! Sorry about the trailing whitespace. Fixed. (I seem to have got something wrong with my .vimrc; it gives up on highlighting trailing whitespace after a while.) r=ferjm

Looks like some tests broke. Removing checkin-needed and fixing.

Tests needed a tweak. Should be all good now.