688574 - Sync UI should not use Weave.Service.loggedIn flag

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Philipp von Weitershausen [:philikon]]

The Weave.Service.loggedIn flag represents an internal flag at this point. It may have had some meaning before Firefox 4, but that meaning has long disappeared. What we really want to know: is Sync set up or not? If it is, Sync takes care of all the rest (connecting, scheduling syncs, etc.)

Comment 1

[Martijn Wargers (dead)]

Philipp, thanks for the info. I see that flag mentioned here on mobile:
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/sync.js#262
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/sync.js#400
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings.xml#1393

What needs to change for that code?
Or should the flag just be removed or something?

Comment 2

[Philipp von Weitershausen [:philikon]]

I suspect what the UI really wants to know is whether Sync is set up on the device. The easiest way to do that, without even causing Sync to load, is to check whether the 'services.sync.username' pref is set to a non-empty string. If it's empty or (more likely) non-existent, Sync isn't set up.

Comment 3

[Philipp von Weitershausen [:philikon]]

When Sync is already loaded and we have access to the 'Weave' object, we can also just check for Weave.Service.username which is a dynamic getter for that pref.

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> I suspect what the UI really wants to know is whether Sync is set up on the
> device. The easiest way to do that, without even causing Sync to load, is to
> check whether the 'services.sync.username' pref is set to a non-empty
> string. If it's empty or (more likely) non-existent, Sync isn't set up.

Do you mean that we shouldn't be using Weave.Status.checkSetup() either?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> (In reply to Philipp von Weitershausen [:philikon] from comment #2)
> > I suspect what the UI really wants to know is whether Sync is set up on the
> > device. The easiest way to do that, without even causing Sync to load, is to
> > check whether the 'services.sync.username' pref is set to a non-empty
> > string. If it's empty or (more likely) non-existent, Sync isn't set up.
> 
> Do you mean that we shouldn't be using Weave.Status.checkSetup() either?

Right. We can use:

Services.prefs.prefHasUserValue("services.sync.username")

If that returns true, Sync is already configured.

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: (1/4) Don't use Weave.Service.isLoggedIn flag

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: (2/4) Check sync username prefs key instead of using sync's API

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: (3/4) Use Weave constant for locked master password

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: (4/4) Fix indentation in in sync's services.js

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 564281 [details] [diff] [review]
(2/4) Check sync username prefs key instead of using sync's API


>diff --git a/mobile/chrome/content/sync.js b/mobile/chrome/content/sync.js

>     // Generating keypairs is expensive on mobile, so disable it
>-    if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED) {
>+    if (Services.prefs.prefHasUserValue("services.sync.username")) {

Remove the comment above the check. It is very out of date

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 564284 [details] [diff] [review]
(4/4) Fix indentation in in sync's services.js

Passing this review to Philipp. He may want it landed on the services branch.

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 564284 [details] [diff] [review]
(4/4) Fix indentation in in sync's services.js

We generally like to land things on s-c for QA purposes, but this whitespace change doesn't need to go through QA :). So feel free to land wherever.

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 564280 [details] [diff] [review]
(1/4) Don't use Weave.Service.isLoggedIn flag

>+      <field name="_syncObserver"><![CDATA[({
>+        _self: this,
>+
>+        QueryInterface: function(aIID) {
>+          const Ci = Components.interfaces;
>+          if (aIID.equals(Ci.nsIObserver) ||
>+              aIID.equals(Ci.nsISupportsWeakReference) ||
>+              aIID.equals(Ci.nsISupports))
>+            return this;
>+
>+          throw Components.results.NS_ERROR_NO_INTERFACE;
>+        },

You can use XPCOMUtils.generateQI() to avoid all this boilerplate. Also, nsISupportsWeakReference inherits from nsISupports, so you won't have to add nsISupports explicitly. Lastly, you don't actually register the observer as with ownsWeak = true, so it seems like either you want to fix that or not use nsISupportsWeakReference at all.

Note that observers don't actually need a QI. They don't even need to be an object, they can just be functions (thanks to nsIObserver being declared a [function] interface). You could bind this._loadChildren to 'this', save it (so that you can unregister it later), and it as an observer, e.g.:

  this._boundLoadChildren = this._loadChildren.bind(this);
  Services.obs.removeObserver(this._boundLoadChildren, topic);

>+
>+        observe: function(aSubject, aTopic, aPrefName) {
>+          this._self._loadChildren();
>+        }
>+      })]]>
>+      </field>
>+


>     // If user is already logged-in, try to connect straight away
>-    if (Weave.Service.isLoggedIn) {
>+    if (Weave.Status.login == Weave.LOGIN_SUCCEEDED) {
>       this.connect();
>       return;
>     }
...
> 
>-    let loggedIn = Weave.Service.isLoggedIn;
>+    let loggedIn = (Weave.Status.login == Weave.LOGIN_SUCCEEDED);
> 
>     connect.collapsed = loggedIn;
>     connected.collapsed = !loggedIn;

Maybe my point on comment 0 wasn't clear enough: the whole concept of "logging in" is going away. The "logged in" state is an increasingly inaccurately represented state because we are basically doing it as part of the first sync now. Really, there is just this question: do you have Sync set up or not. Sync will take care of "logging in" (this will actually go away as a separate step, too) and all the rest. So I suggest you check for whether Sync is set up (the 'services.sync.username' pref is a good indicator for that).

As for the observer notifications are concerned, it still makes sense to observe 'weave:service:login:*' to update the UI in case there were any authentication errors.

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> Comment on attachment 564280 [details] [diff] [review] [diff] [details] [review]
> (1/4) Don't use Weave.Service.isLoggedIn flag
> 
> >+      <field name="_syncObserver"><![CDATA[({
> >+        _self: this,
> >+
> >+        QueryInterface: function(aIID) {
> >+          const Ci = Components.interfaces;
> >+          if (aIID.equals(Ci.nsIObserver) ||
> >+              aIID.equals(Ci.nsISupportsWeakReference) ||
> >+              aIID.equals(Ci.nsISupports))
> >+            return this;
> >+
> >+          throw Components.results.NS_ERROR_NO_INTERFACE;
> >+        },
> 
> You can use XPCOMUtils.generateQI() to avoid all this boilerplate. Also,
> nsISupportsWeakReference inherits from nsISupports, so you won't have to add
> nsISupports explicitly. Lastly, you don't actually register the observer as
> with ownsWeak = true, so it seems like either you want to fix that or not
> use nsISupportsWeakReference at all.
> 
> Note that observers don't actually need a QI. They don't even need to be an
> object, they can just be functions (thanks to nsIObserver being declared a
> [function] interface). You could bind this._loadChildren to 'this', save it
> (so that you can unregister it later), and it as an observer, e.g.:
> 
>   this._boundLoadChildren = this._loadChildren.bind(this);
>   Services.obs.removeObserver(this._boundLoadChildren, topic);

Ok, will try that.

> >+
> >+        observe: function(aSubject, aTopic, aPrefName) {
> >+          this._self._loadChildren();
> >+        }
> >+      })]]>
> >+      </field>
> >+
> 
> 
> >     // If user is already logged-in, try to connect straight away
> >-    if (Weave.Service.isLoggedIn) {
> >+    if (Weave.Status.login == Weave.LOGIN_SUCCEEDED) {
> >       this.connect();
> >       return;
> >     }
> ...
> > 
> >-    let loggedIn = Weave.Service.isLoggedIn;
> >+    let loggedIn = (Weave.Status.login == Weave.LOGIN_SUCCEEDED);
> > 
> >     connect.collapsed = loggedIn;
> >     connected.collapsed = !loggedIn;
> 
> Maybe my point on comment 0 wasn't clear enough: the whole concept of
> "logging in" is going away. The "logged in" state is an increasingly
> inaccurately represented state because we are basically doing it as part of
> the first sync now. Really, there is just this question: do you have Sync
> set up or not. Sync will take care of "logging in" (this will actually go
> away as a separate step, too) and all the rest. So I suggest you check for
> whether Sync is set up (the 'services.sync.username' pref is a good
> indicator for that).

The UI still needs to account for the case of master password being locked (see bug 624552). If sync password is locked, the UI should show the "Connect" button to prompt for master password. So, login status (as in "got access to sync password") still matters here. It would be a bit misleading to show UI in connected state while sync is actually not really logged-in due to locked password.

Comment 15

[Philipp von Weitershausen [:philikon]]

(In reply to Lucas Rocha (:lucasr) from comment #14)
> The UI still needs to account for the case of master password being locked
> (see bug 624552). If sync password is locked, the UI should show the
> "Connect" button to prompt for master password. So, login status (as in "got
> access to sync password") still matters here. It would be a bit misleading
> to show UI in connected state while sync is actually not really logged-in
> due to locked password.

I disagree. "Connected" isn't the best word here, but for all intents and purposes, you're still "connected to Sync". You're just not syncing because the MP is locked. But Sync is still set up.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Philipp von Weitershausen [:philikon] from comment #13)

Hmm, I blew this review a bit...

> >+      <field name="_syncObserver"><![CDATA[({
> >+        _self: this,
> >+
> >+        QueryInterface: function(aIID) {
> >+          const Ci = Components.interfaces;
> >+          if (aIID.equals(Ci.nsIObserver) ||
> >+              aIID.equals(Ci.nsISupportsWeakReference) ||
> >+              aIID.equals(Ci.nsISupports))
> >+            return this;
> >+
> >+          throw Components.results.NS_ERROR_NO_INTERFACE;
> >+        },
> 
> You can use XPCOMUtils.generateQI() to avoid all this boilerplate. Also,
> nsISupportsWeakReference inherits from nsISupports, so you won't have to add
> nsISupports explicitly. Lastly, you don't actually register the observer as
> with ownsWeak = true, so it seems like either you want to fix that or not
> use nsISupportsWeakReference at all.

The current code does not use a weak reference, and removes the observers in the observe handler. If the view is closed (but still active) the notifications could be fired and caught by the code. There might be checks in loadChildren to avoid trying to load into a closed view.

In any case, you could avoid the QueryInterface altogether. Why did you want to move away from the current way of hooking up observers?

> Note that observers don't actually need a QI. They don't even need to be an
> object, they can just be functions (thanks to nsIObserver being declared a
> [function] interface). You could bind this._loadChildren to 'this', save it
> (so that you can unregister it later), and it as an observer, e.g.:
> 
>   this._boundLoadChildren = this._loadChildren.bind(this);
>   Services.obs.removeObserver(this._boundLoadChildren, topic);

This certainly seems like the easiest way to set up the observers

> >     // If user is already logged-in, try to connect straight away
> >-    if (Weave.Service.isLoggedIn) {
> >+    if (Weave.Status.login == Weave.LOGIN_SUCCEEDED) {
> >       this.connect();
> >       return;
> >     }
> ...
> > 
> >-    let loggedIn = Weave.Service.isLoggedIn;
> >+    let loggedIn = (Weave.Status.login == Weave.LOGIN_SUCCEEDED);
> > 
> >     connect.collapsed = loggedIn;
> >     connected.collapsed = !loggedIn;
> 
> Maybe my point on comment 0 wasn't clear enough: the whole concept of
> "logging in" is going away. The "logged in" state is an increasingly
> inaccurately represented state because we are basically doing it as part of
> the first sync now. Really, there is just this question: do you have Sync
> set up or not. Sync will take care of "logging in" (this will actually go
> away as a separate step, too) and all the rest. So I suggest you check for
> whether Sync is set up (the 'services.sync.username' pref is a good
> indicator for that).

So we really need to re-think every place we use loggedIn (or similar). We need to decide if we just want to assume we _are_ logged in or if we want to check for configuration. In the past, Fennec treated those as separate states.

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to Philipp von Weitershausen [:philikon] from comment #15)
> (In reply to Lucas Rocha (:lucasr) from comment #14)
> > The UI still needs to account for the case of master password being locked
> > (see bug 624552). If sync password is locked, the UI should show the
> > "Connect" button to prompt for master password. So, login status (as in "got
> > access to sync password") still matters here. It would be a bit misleading
> > to show UI in connected state while sync is actually not really logged-in
> > due to locked password.
> 
> I disagree. "Connected" isn't the best word here, but for all intents and
> purposes, you're still "connected to Sync". You're just not syncing because
> the MP is locked. But Sync is still set up.

Ok, I get your point. Maybe this is more of a UI design question. We need to figure out a proper UI for the case where sync is setup but MP is locked.

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #16)
> The current code does not use a weak reference, and removes the observers in
> the observe handler. If the view is closed (but still active) the
> notifications could be fired and caught by the code. There might be checks
> in loadChildren to avoid trying to load into a closed view.
> 
> In any case, you could avoid the QueryInterface altogether. Why did you want
> to move away from the current way of hooking up observers?

Because we're now unconditionally adding observers. We don't check logged-in status anymore to decide if we should load remote tabs straight or wait for login/sync process to finish. So, we now need to remove observers when the remote tabs panel is closed.

> So we really need to re-think every place we use loggedIn (or similar). We
> need to decide if we just want to assume we _are_ logged in or if we want to
> check for configuration. In the past, Fennec treated those as separate
> states.

I think the only pending issue now is what UI we should show when sync is setup but master password is locked. It looks safe remove all other cases where we're checking logged-in status.

Comment 19

[Lucas Rocha (:lucasr)]

Attachment: (1/4) Don't use Weave.Service.isLoggedIn flag

Here's a new patched. I used the simpler way of adding observer and removed all explicit uses of logged-in status.

There's still no clear UI for when sync is setup and master password is locked. When you're in this state, the MP prompt dialog will show up if you click on "Sync now".  Maybe we could show a clear message beside the "Sync now" button about the locked MP?

Comment 20

[Philipp von Weitershausen [:philikon]]

Comment on attachment 564505 [details] [diff] [review]
(1/4) Don't use Weave.Service.isLoggedIn flag

Sorry for the delay. This looks good to me!

Comment 21

[Lucas Rocha (:lucasr)]

Stepping back a bit, I think it's probably fine to not have any special UI when sync is setup and master password is locked. IIUC, the MP prompt will keep showing up until the user enters it correctly anyway. Mark, what do you think?

Comment 22

[Lucas Rocha (:lucasr)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3eb62b784a65
http://hg.mozilla.org/integration/mozilla-inbound/rev/17d34b5fd929
http://hg.mozilla.org/integration/mozilla-inbound/rev/bbf1f86fc5c1
http://hg.mozilla.org/integration/mozilla-inbound/rev/9c903b53932e

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/3eb62b784a65
https://hg.mozilla.org/mozilla-central/rev/17d34b5fd929
https://hg.mozilla.org/mozilla-central/rev/bbf1f86fc5c1
https://hg.mozilla.org/mozilla-central/rev/9c903b53932e