Jim Mathies [:jimm] Assignee
	Description • 11 years ago

spun off from grab bag bug 865318:

"Sometimes the "Sync now" button under "Sync" in the "Settings" charm completely stops becoming responsive. Usually when you select the "Sync now" button, it will let you know that its syncing and then a time stamp is added. In some cases, it won't do anything and quickly flash between grayed/ungrayed states without any feedback from Firefox Metro (confirmation or success messages)"

I think this is on purpose, we probably don't force a sync until the default timeout is reached. Ally suggested we remove this button entirely.

	Jim Mathies [:jimm] Assignee
	Comment 1 • 11 years ago

Also - 

"Sometimes the "Sync now" button is grayed out and not selectable but doesn't give you a reason on why. This makes it a little confusing as there is no indication if a sync occurring. Just seems like its disabled and sometimes takes over an hour for the button to be selectable once again."

	Jim Mathies [:jimm] Assignee
	Comment 2 • 11 years ago

This isn't a complete fix for the issues here, but it addresses one problem I was able to reproduce. Extending this timeout out a bit insures sync is actually locked. Sometimes we check before that happens.

	Jim Mathies [:jimm] Assignee
	Comment 3 • 11 years ago

Updating this, the check should also check for a !lastSync.

	Jim Mathies [:jimm] Assignee
	Comment 4 • 11 years ago

sorry for the spam, posted the wrong patch.

	Jim Mathies [:jimm] Assignee
	Comment 5 • 11 years ago

Comment on attachment 744853 [details] [diff] [review]
part1 - delay checking sync state

Bah, I posted the wrong darn patch again.

	Jim Mathies [:jimm] Assignee
	Comment 6 • 11 years ago

Comment on attachment 744818 [details] [diff] [review]
part1 - delay checking sync state

It helps if you're posting the patch to the right darn bug!

	Jim Mathies [:jimm] Assignee
	Comment 7 • 11 years ago

This is an update to the original patch. What I've done is:

- move all ui updating into a _updateUI helper that doesn't rely on observer notifications. We can call this from the observer notify handler and from _init. This insures the ui state is always up to date. Note _init is called on delay load, which usually happens during or after the first sync takes place. This sometimes left the ui in a random state after startup.

- added an error description element below the sync state message. We were setting an error description as a "desc" property of one of the elements, which wasn't getting displayed. I've moved that to this new element. You can see this by turning on airplane mode after setup and hitting Sync Now.

- I'm now updating the last sync status on login start and sync start after we set the last sync date, so it displays "in progress.." more accurately.

	Jim Mathies [:jimm] Assignee
	Comment 8 • 11 years ago

- removed metro.js debug prefs

	Jim Mathies [:jimm] Assignee
	Comment 11 • 11 years ago

Improved ui updated *and* removal of the Sync Now button per discussion with Asa about it's unreliability.

	Jim Mathies [:jimm] Assignee
	Comment 12 • 11 years ago

Yuan requested this move up to the top. It now sits with the last sync date and the error message text.

I do believe that's the final set of changes for this bug.

	Jim Mathies [:jimm] Assignee
	Comment 13 • 11 years ago

final revs

	Allison Naaktgeboren :ally
	Comment 15 • 11 years ago

Comment on attachment 745813 [details] [diff] [review]
account move

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

::: browser/metro/base/content/browser.xul
@@ +401,5 @@
>      </flyoutpanel>
>  
>      <flyoutpanel id="sync-flyoutpanel" headertext="&syncHeader.title;">
>        <description>&sync.setup.description;</description> 
> +      <description id="sync-accountinfo" collapsed="true"></description> 

nit: whitespace at end of line (and bonus points if you get rid of the other 4 trailing whitespaces on the above/below it. :)

	Allison Naaktgeboren :ally
	Comment 16 • 11 years ago

Comment on attachment 745812 [details] [diff] [review]
improved ui updating

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

Looks good to me, but we've moved enough enough of the sync logic that I would like to run it by a sync peer in case there are some assumptions I don't know about.

::: browser/metro/base/content/browser.xul
@@ +402,5 @@
>  
>      <flyoutpanel id="sync-flyoutpanel" headertext="&syncHeader.title;">
>        <description>&sync.setup.description;</description> 
> +      <description id="sync-lastsync" collapsed="true"></description> 
> +      <description id="sync-errordescription" collapsed="true"></description> 

nit: trailing whitespace

	Richard Newman [:rnewman]
	Comment 17 • 11 years ago

Comment on attachment 745812 [details] [diff] [review]
improved ui updating

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

There looks to be some amount of scary dead Fennec code still breathing here. I strongly advise you to assume that anything lifted from XUL Fennec doesn't work, and to check desktop code instead.

::: browser/metro/base/content/sync.js
@@ +69,5 @@
>      this._addListeners();
>  
>      this.setupData = { account: "", password: "" , synckey: "", serverURL: "" };
>  
> +    if (Weave.Status.login != Weave.LOGIN_FAILED_NO_USERNAME) {

status.login == LOGIN_FAILED_NO_USERNAME can only be true after Service.verifyLogin has been called. Unless you've actually fired off a sync, or have called login, why would that be the case here?

If you're trying to figure out whether an account is set up, desktop's code is evil but works:

https://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/sync.js#90

@@ +429,5 @@
> +
> +    // This gets updated when an error occurs
> +    this._elements.errordescription.collapsed = true;
> +
> +    let isConfigured = (!this._loginError && Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED);

Again, desktop's prefs code achieves this goal.

	Jim Mathies [:jimm] Assignee
	Comment 18 • 11 years ago

> There looks to be some amount of scary dead Fennec code still breathing
> here. I strongly advise you to assume that anything lifted from XUL Fennec
> doesn't work, and to check desktop code instead.

FWIW I've been testing a lot and things seem to be working pretty well.

> If you're trying to figure out whether an account is set up, desktop's code
> is evil but works:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/components/
> preferences/sync.js#90

I didn't use this specific logic because the two browsers don't have the same UI. What I did do is add a prop that checks for all potential client side login error codes. I ran through the whole process a few times to be sure everything works.

	Jim Mathies [:jimm] Assignee
	Comment 19 • 11 years ago

Comment on attachment 746151 [details] [diff] [review]
improved ui updating v.2

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

::: browser/metro/base/content/sync.js
@@ +16,5 @@
> +  get _isSetup() {
> +    // check for issues related to failed logins that do not have anything to
> +    // do with network, server, and other non-client issues. See the login
> +    // failure status codes in sync service.
> +    return (Weave.Status.login != Weave.CLIENT_NOT_CONFIGURED &&

Actually this isn't right, since this isn't a login code. I suppose I should call checkSetup first, and if it claims the client is configured, but there were ligin errors, assume the client is not configured. Will update the patch.

	Jim Mathies [:jimm] Assignee
	Comment 20 • 11 years ago

updated per previous comments.

	Richard Newman [:rnewman]
	Comment 21 • 11 years ago

Comment on attachment 746156 [details] [diff] [review]
improved ui updating v.2

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

::: browser/metro/base/content/browser.xul
@@ +402,5 @@
>  
>      <flyoutpanel id="sync-flyoutpanel" headertext="&syncHeader.title;">
>        <description>&sync.setup.description;</description> 
> +      <description id="sync-lastsync" collapsed="true"></description> 
> +      <description id="sync-errordescription" collapsed="true"></description> 

Nit: trailing whitespace.

::: browser/metro/base/content/sync.js
@@ +23,5 @@
> +    return (Weave.Status.login != Weave.LOGIN_FAILED_NO_USERNAME &&
> +            Weave.Status.login != Weave.LOGIN_FAILED_NO_PASSWORD &&
> +            Weave.Status.login != Weave.LOGIN_FAILED_NO_PASSPHRASE &&
> +            Weave.Status.login != Weave.LOGIN_FAILED_INVALID_PASSPHRASE &&
> +            Weave.Status.login != Weave.LOGIN_FAILED_LOGIN_REJECTED);

I'd remove the check for LOGIN_REJECTED. If the user changes their password elsewhere, or perhaps even in some node reassignment scenarios, you'll hit this case.

As a user, if I change my password on one device, I don't expect my other device to suddenly claim that it's not set up, when all of its internal and external state shows otherwise (e.g., tabs are still on the server).

The same kinda applies to INVALID_PASSPHRASE (user changed sync key), but I'm not 100% confident of that flow. You'd need to do

  Prefs > Sync > My Recovery Key > Generate a new key

on another device and see what happens. This should be part of a QA pass.

(More broadly, this means you should -- eventually -- have a "Sync is set up, but it can't sync" state, and either call this function isSetUpAndWorking, or figure something else out.)

But this will do for now if you're rushing for a release.

@@ +431,5 @@
>    },
>  
> +  _updateUI: function _updateUI() {
> +    if (this._elements == null)
> +      return;

Prefer braces even for single-line conditionals.

	Jim Mathies [:jimm] Assignee
	Comment 22 • 11 years ago

(In reply to Richard Newman [:rnewman] from comment #21)
> ::: browser/metro/base/content/sync.js
> @@ +23,5 @@
> > +    return (Weave.Status.login != Weave.LOGIN_FAILED_NO_USERNAME &&
> > +            Weave.Status.login != Weave.LOGIN_FAILED_NO_PASSWORD &&
> > +            Weave.Status.login != Weave.LOGIN_FAILED_NO_PASSPHRASE &&
> > +            Weave.Status.login != Weave.LOGIN_FAILED_INVALID_PASSPHRASE &&
> > +            Weave.Status.login != Weave.LOGIN_FAILED_LOGIN_REJECTED);
> 
> I'd remove the check for LOGIN_REJECTED. If the user changes their password
> elsewhere, or perhaps even in some node reassignment scenarios, you'll hit
> this case.
> 
> As a user, if I change my password on one device, I don't expect my other
> device to suddenly claim that it's not set up, when all of its internal and
> external state shows otherwise (e.g., tabs are still on the server).
> 
> The same kinda applies to INVALID_PASSPHRASE (user changed sync key), but
> I'm not 100% confident of that flow. You'd need to do
> 
>   Prefs > Sync > My Recovery Key > Generate a new key
> 
> on another device and see what happens. This should be part of a QA pass.
> 
> (More broadly, this means you should -- eventually -- have a "Sync is set
> up, but it can't sync" state, and either call this function
> isSetUpAndWorking, or figure something else out.)

In this case, what information would we need to collect from the user to get things working again? The new recovery key?

	Jim Mathies [:jimm] Assignee
	Comment 23 • 11 years ago

I just tested this by changing my password. We display an invalid login message and the set up sync button. When you click on the setup button, you can either repair, or enter the new information manually. Seems like we have this covered.

	Richard Newman [:rnewman]
	Comment 24 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #23)
> I just tested this by changing my password. We display an invalid login
> message and the set up sync button. When you click on the setup button, you
> can either repair, or enter the new information manually. Seems like we have
> this covered.

That's actually the case I was talking about.

What you just demonstrated is that an incorrect password requires you to set up Sync from scratch, just as if you didn't have an account already configured.

Setting Sync up again will cause a fresh first-start sync and a new client record to appear on the server, complete with duplicate tabs etc.

What it ought to do is prompt you for your changed password (or recovery key if that's what you changed), with an _option_ to set up Sync from scratch (because re-typing a recovery key is no fun).

It's fine to skip this for a v1, because users don't typically change passwords or keys with any frequency (that we know of), but be aware that it's not a solved problem.

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

https://hg.mozilla.org/mozilla-central/rev/6ace0127a85b
https://hg.mozilla.org/mozilla-central/rev/6636d08e4c24

	Jim Mathies [:jimm] Assignee
	Comment 26 • 11 years ago

Hey Juan, there are a few good test scenarios described here. With metrofx curious how we should handle flagging for litmus type tests?

	juan becerra [:juanb]
	Comment 27 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #26)
> Hey Juan, there are a few good test scenarios described here. With metrofx
> curious how we should handle flagging for litmus type tests?

We should be using in-moztrap flags.

Note to self:
-- Test for Sync time stamp
-- Test for visual indication of progress
-- Test for the ability to click it (is it grayed out?)
-- Test for error message (what are common causes? network problems?)
-- Test for password change, login error, and Sync setup
-- Test for duplication of sync'd entries (lack thereof)