[Sync] Propagate XMPP auth errors to SyncNotifierObservers

[Sync] Propagate XMPP auth errors to SyncNotifierObservers

Detect XMPP auth errors and add new notifications to SingleLoginAttempt
and Login.

Replace PushClientObserver::OnNotificationStateChange with
OnNotifications{Enabled,Disabled} notifications.

Change SyncNotifierObserver similarly.

Handle InvalidationClient errors and propagate auth errors from that,
too.

Propagate XMPP auth errors all the way up to SyncManager.  It will be
handled in a future CL.

BUG=38091
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142806

[akalin, 2012-06-14 02:57:06]

+ghc for invalidation stuff (only InformError in chrome_invalidation_client.*)
+gene for cloudprint stuff
+rlarocque for everything else

[gene, 2012-06-14 06:14:41]

lgtm for cloudprint

[ghc, 2012-06-14 17:47:32]

lgtm

[rlarocque, 2012-06-14 20:01:06]

The code mostly looks good, though I still need to read through it in more
detail.  

However, I'm not convinced that CREDENTIALS_REJECTED is a state distinct from
NOTIFICATIONS_OFF.  It looks to me like the notifier can be on or off, and that
it emits notifications to indicate a transition along with the reason for the
change.  

In other words, I'm wondering why you decided to make NOTIFICATIONS_ON,
NOTIFICATIONS_OFF and CREDENTIALS_REJECTED into states rather than state
transitions.  Is there a situation (or will in the future exist a situation)
where the notifier behaves differently based on it having a NOTIFICATIONS_OFF
vs. a CREDENTIALS_REJECTED state?

https://chromiumcodereview.appspot.com/10545170/diff/7002/sync/notifier/p2p_n...
File sync/notifier/p2p_notifier.cc (right):

https://chromiumcodereview.appspot.com/10545170/diff/7002/sync/notifier/p2p_n...
sync/notifier/p2p_notifier.cc:220: bool just_turned_on =
This protects against spurious notifications, right?  Do we really expect to
receive spurious notifications?

[akalin, 2012-06-15 03:54:14]

Hmm, are you saying it would be better to have a bool for notificiations on/off,
and maybe a reason for what made the notifications go to 'off'?

I'd have to think about it.  Hmm...

On 2012/06/14 20:01:06, rlarocque wrote:
> The code mostly looks good, though I still need to read through it in more
> detail.  
> 
> However, I'm not convinced that CREDENTIALS_REJECTED is a state distinct from
> NOTIFICATIONS_OFF.  It looks to me like the notifier can be on or off, and
that
> it emits notifications to indicate a transition along with the reason for the
> change.  
> 
> In other words, I'm wondering why you decided to make NOTIFICATIONS_ON,
> NOTIFICATIONS_OFF and CREDENTIALS_REJECTED into states rather than state
> transitions.  Is there a situation (or will in the future exist a situation)
> where the notifier behaves differently based on it having a NOTIFICATIONS_OFF
> vs. a CREDENTIALS_REJECTED state?
> 
>
https://chromiumcodereview.appspot.com/10545170/diff/7002/sync/notifier/p2p_n...
> File sync/notifier/p2p_notifier.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10545170/diff/7002/sync/notifier/p2p_n...
> sync/notifier/p2p_notifier.cc:220: bool just_turned_on =
> This protects against spurious notifications, right?  Do we really expect to
> receive spurious notifications?

[akalin, 2012-06-16 01:06:55]

rlarocque: Addressed your comments, PTAL.

gene: you may want to take another look, since I made some less trivial
modifications to cloud-print code.

http://codereview.chromium.org/10545170/diff/7002/sync/notifier/p2p_notifier.cc
File sync/notifier/p2p_notifier.cc (right):

http://codereview.chromium.org/10545170/diff/7002/sync/notifier/p2p_notifier....
sync/notifier/p2p_notifier.cc:220: bool just_turned_on =
On 2012/06/14 20:01:07, rlarocque wrote:
> This protects against spurious notifications, right?  Do we really expect to
> receive spurious notifications?

I think it's okay to be defensive here since it could conceivably be used with a
real XMPP server (as opposed to our test one).

But yeah, it's not likely that we'd get spurious notifications, unless the XMPP
server sends out multiple subscription replies or something.

[akalin, 2012-06-18 17:36:32]

ping!

On 2012/06/16 01:06:55, akalin wrote:
> rlarocque: Addressed your comments, PTAL.
> 
> gene: you may want to take another look, since I made some less trivial
> modifications to cloud-print code.
> 
>
http://codereview.chromium.org/10545170/diff/7002/sync/notifier/p2p_notifier.cc
> File sync/notifier/p2p_notifier.cc (right):
> 
>
http://codereview.chromium.org/10545170/diff/7002/sync/notifier/p2p_notifier....
> sync/notifier/p2p_notifier.cc:220: bool just_turned_on =
> On 2012/06/14 20:01:07, rlarocque wrote:
> > This protects against spurious notifications, right?  Do we really expect to
> > receive spurious notifications?
> 
> I think it's okay to be defensive here since it could conceivably be used with
a
> real XMPP server (as opposed to our test one).
> 
> But yeah, it's not likely that we'd get spurious notifications, unless the
XMPP
> server sends out multiple subscription replies or something.

[rlarocque, 2012-06-18 18:14:14]

LGTM!

I have one additional nit, but since it only affects the internals of one class,
I'm not too concerned about it.

http://codereview.chromium.org/10545170/diff/19088/sync/notifier/chrome_inval...
File sync/notifier/chrome_invalidation_client.h (right):

http://codereview.chromium.org/10545170/diff/19088/sync/notifier/chrome_inval...
sync/notifier/chrome_invalidation_client.h:153: NotificationsDisabledReason
ticl_state_;
optional: Could you remove these?  A common pattern is to set them then
EmitStateChange(), could you just add a parameter to EmitStateChange() instead?

[akalin, 2012-06-18 18:33:37]

On 2012/06/18 18:14:14, rlarocque wrote:
> LGTM!
> 
> I have one additional nit, but since it only affects the internals of one
class,
> I'm not too concerned about it.
> 
>
http://codereview.chromium.org/10545170/diff/19088/sync/notifier/chrome_inval...
> File sync/notifier/chrome_invalidation_client.h (right):
> 
>
http://codereview.chromium.org/10545170/diff/19088/sync/notifier/chrome_inval...
> sync/notifier/chrome_invalidation_client.h:153: NotificationsDisabledReason
> ticl_state_;
> optional: Could you remove these?  A common pattern is to set them then
> EmitStateChange(), could you just add a parameter to EmitStateChange()
instead?

Did you mean "a common pattern ... to set them before calling
EmitStateChange()"?  If so, I don't think I can just add a parameter to
EmitStateChange().  EmitStateChange() would have to take two parameters, since
it depends on both the ticl and the push client state.  But it's called from the
callbacks for both the ticl and the push client, so if it's called from a ticl
callback it has to know the push client state, and vice versa.  So you'd end up
having to store the state anyway.

Committing!

[commit-bot: I haz the power, 2012-06-18 18:33:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10545170/19088

[commit-bot: I haz the power, 2012-06-18 20:50:31]

Change committed as 142806