diff options
43 files changed, 711 insertions, 238 deletions
diff --git a/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc b/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc index 89d07293..1cc6034 100644 --- a/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc +++ b/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc @@ -70,7 +70,11 @@ class FakeSyncNotifierObserverIO received_improper_notification_ = true; } } - virtual void OnNotificationStateChange(bool notifications_enabled) { + virtual void OnNotificationsEnabled() OVERRIDE { + NOTREACHED(); + } + virtual void OnNotificationsDisabled( + sync_notifier::NotificationsDisabledReason reason) OVERRIDE { NOTREACHED(); } diff --git a/chrome/service/cloud_print/cloud_print_proxy_backend.cc b/chrome/service/cloud_print/cloud_print_proxy_backend.cc index e14e33b..4b2cfa6 100644 --- a/chrome/service/cloud_print/cloud_print_proxy_backend.cc +++ b/chrome/service/cloud_print/cloud_print_proxy_backend.cc @@ -88,8 +88,9 @@ class CloudPrintProxyBackend::Core virtual void OnAuthFailed() OVERRIDE; // notifier::PushClientObserver implementation. - virtual void OnNotificationStateChange( - bool notifications_enabled) OVERRIDE; + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + notifier::NotificationsDisabledReason reason) OVERRIDE; virtual void OnIncomingNotification( const notifier::Notification& notification) OVERRIDE; @@ -500,27 +501,29 @@ void CloudPrintProxyBackend::Core::NotifyUnregisterPrinters( backend_->frontend_->OnUnregisterPrinters(auth_token, printer_ids); } -void CloudPrintProxyBackend::Core::OnNotificationStateChange( - bool notification_enabled) { +void CloudPrintProxyBackend::Core::OnNotificationsEnabled() { DCHECK(MessageLoop::current() == backend_->core_thread_.message_loop()); - notifications_enabled_ = notification_enabled; - if (notifications_enabled_) { - notifications_enabled_since_ = base::TimeTicks::Now(); - VLOG(1) << "Notifications for connector " << proxy_id_ - << " were enabled at " - << notifications_enabled_since_.ToInternalValue(); - } else { - LOG(ERROR) << "Notifications for connector " << proxy_id_ << " disabled."; - notifications_enabled_since_ = base::TimeTicks(); - } - // A state change means one of two cases. - // Case 1: We just lost notifications. This this case we want to schedule a - // job poll if enable_job_poll_ is true. - // Case 2: Notifications just got re-enabled. In this case we want to schedule + notifications_enabled_ = true; + notifications_enabled_since_ = base::TimeTicks::Now(); + VLOG(1) << "Notifications for connector " << proxy_id_ + << " were enabled at " + << notifications_enabled_since_.ToInternalValue(); + // Notifications just got re-enabled. In this case we want to schedule // a poll once for jobs we might have missed when we were dark. // Note that ScheduleJobPoll will not schedule again if a job poll task is // already scheduled. - if (enable_job_poll_ || notifications_enabled_) + ScheduleJobPoll(); +} + +void CloudPrintProxyBackend::Core::OnNotificationsDisabled( + notifier::NotificationsDisabledReason reason) { + DCHECK(MessageLoop::current() == backend_->core_thread_.message_loop()); + notifications_enabled_ = false; + LOG(ERROR) << "Notifications for connector " << proxy_id_ << " disabled."; + notifications_enabled_since_ = base::TimeTicks(); + // We just lost notifications. This this case we want to schedule a + // job poll if enable_job_poll_ is true. + if (enable_job_poll_) ScheduleJobPoll(); } diff --git a/jingle/notifier/communicator/login.cc b/jingle/notifier/communicator/login.cc index 886ea84..db4813b 100644 --- a/jingle/notifier/communicator/login.cc +++ b/jingle/notifier/communicator/login.cc @@ -27,6 +27,8 @@ namespace notifier { // Redirect valid for 5 minutes. static const int kRedirectTimeoutMinutes = 5; +Login::Delegate::~Delegate() {} + Login::Login(Delegate* delegate, const buzz::XmppClientSettings& user_settings, const scoped_refptr<net::URLRequestContextGetter>& @@ -57,19 +59,31 @@ void Login::UpdateXmppSettings(const buzz::XmppClientSettings& user_settings) { login_settings_.set_user_settings(user_settings); } +// In the code below, we assume that calling a delegate method may end +// up in ourselves being deleted, so we always call it last. +// +// TODO(akalin): Add unit tests to enforce the behavior above. + void Login::OnConnect(base::WeakPtr<buzz::XmppTaskParentInterface> base_task) { ResetReconnectState(); delegate_->OnConnect(base_task); } -void Login::OnNeedReconnect() { - TryReconnect(); -} - void Login::OnRedirect(const ServerInformation& redirect_server) { login_settings_.SetRedirectServer(redirect_server); // Drop the current connection, and start the login process again. StartConnection(); + delegate_->OnTransientDisconnection(); +} + +void Login::OnCredentialsRejected() { + TryReconnect(); + delegate_->OnCredentialsRejected(); +} + +void Login::OnSettingsExhausted() { + TryReconnect(); + delegate_->OnTransientDisconnection(); } void Login::OnIPAddressChanged() { @@ -78,6 +92,7 @@ void Login::OnIPAddressChanged() { // avoid spikey behavior on network hiccups). reconnect_interval_ = base::TimeDelta::FromSeconds(base::RandInt(1, 9)); TryReconnect(); + delegate_->OnTransientDisconnection(); } void Login::ResetReconnectState() { @@ -94,7 +109,6 @@ void Login::TryReconnect() { << reconnect_interval_.InSeconds() << " seconds"; reconnect_timer_.Start( FROM_HERE, reconnect_interval_, this, &Login::DoReconnect); - delegate_->OnDisconnect(); } void Login::DoReconnect() { diff --git a/jingle/notifier/communicator/login.h b/jingle/notifier/communicator/login.h index d089cb6d6..29be403 100644 --- a/jingle/notifier/communicator/login.h +++ b/jingle/notifier/communicator/login.h @@ -33,24 +33,31 @@ namespace notifier { class LoginSettings; -// Workaround for MSVS 2005 bug that fails to handle inheritance from a nested -// class properly if it comes directly on a base class list. -typedef SingleLoginAttempt::Delegate SingleLoginAttemptDelegate; - -// Does the login, keeps it alive (with refreshing cookies and reattempting -// login when disconnected), figures out what actions to take on the various -// errors that may occur. +// Does the login, keeps it alive (with refreshing cookies and +// reattempting login when disconnected), and figures out what actions +// to take on the various errors that may occur. class Login : public net::NetworkChangeNotifier::IPAddressObserver, - public SingleLoginAttemptDelegate { + public SingleLoginAttempt::Delegate { public: class Delegate { public: + // Called when a connection has been successfully established. virtual void OnConnect( base::WeakPtr<buzz::XmppTaskParentInterface> base_task) = 0; - virtual void OnDisconnect() = 0; + + // Called when there's no connection to the server but we expect + // it to come back come back eventually. The connection will be + // retried with exponential backoff. + virtual void OnTransientDisconnection() = 0; + + // Called when the current login credentials have been rejected. + // The connection will still be retried with exponential backoff; + // it's up to the delegate to stop connecting and/or prompt for + // new credentials. + virtual void OnCredentialsRejected() = 0; protected: - virtual ~Delegate() {} + virtual ~Delegate(); }; // Does not take ownership of |delegate|, which must not be NULL. @@ -63,9 +70,12 @@ class Login : public net::NetworkChangeNotifier::IPAddressObserver, const std::string& auth_mechanism); virtual ~Login(); + // Starts connecting (or forces a reconnection if we're backed off). void StartConnection(); - // The updated settings only take effect the next time StartConnection - // is called. + + // The updated settings take effect only the next time when a + // connection is attempted (either via reconnection or a call to + // StartConnection()). void UpdateXmppSettings(const buzz::XmppClientSettings& user_settings); // net::NetworkChangeNotifier::IPAddressObserver implementation. @@ -74,8 +84,9 @@ class Login : public net::NetworkChangeNotifier::IPAddressObserver, // SingleLoginAttempt::Delegate implementation. virtual void OnConnect( base::WeakPtr<buzz::XmppTaskParentInterface> base_task) OVERRIDE; - virtual void OnNeedReconnect() OVERRIDE; virtual void OnRedirect(const ServerInformation& redirect_server) OVERRIDE; + virtual void OnCredentialsRejected() OVERRIDE; + virtual void OnSettingsExhausted() OVERRIDE; private: void OnLogoff(); @@ -103,10 +114,6 @@ class Login : public net::NetworkChangeNotifier::IPAddressObserver, DISALLOW_COPY_AND_ASSIGN(Login); }; -// Workaround for MSVS 2005 bug that fails to handle inheritance from a nested -// class properly if it comes directly on a base class list. -typedef Login::Delegate LoginDelegate; - } // namespace notifier #endif // JINGLE_NOTIFIER_COMMUNICATOR_LOGIN_H_ diff --git a/jingle/notifier/communicator/single_login_attempt.cc b/jingle/notifier/communicator/single_login_attempt.cc index 0c789ee..c2405fe 100644 --- a/jingle/notifier/communicator/single_login_attempt.cc +++ b/jingle/notifier/communicator/single_login_attempt.cc @@ -39,6 +39,11 @@ SingleLoginAttempt::SingleLoginAttempt(const LoginSettings& login_settings, SingleLoginAttempt::~SingleLoginAttempt() {} +// In the code below, we assume that calling a delegate method may end +// up in ourselves being deleted, so we always call it last. +// +// TODO(akalin): Add unit tests to enforce the behavior above. + void SingleLoginAttempt::OnConnect( base::WeakPtr<buzz::XmppTaskParentInterface> base_task) { DVLOG(1) << "Connected to " << current_settings_->ToString(); @@ -131,6 +136,11 @@ void SingleLoginAttempt::OnError(buzz::XmppEngine::Error error, int subcode, } } + if (error == buzz::XmppEngine::ERROR_UNAUTHORIZED) { + delegate_->OnCredentialsRejected(); + return; + } + if (current_settings_ == settings_list_.end()) { NOTREACHED(); return; @@ -139,7 +149,7 @@ void SingleLoginAttempt::OnError(buzz::XmppEngine::Error error, int subcode, ++current_settings_; if (current_settings_ == settings_list_.end()) { VLOG(1) << "Could not connect to any XMPP server"; - delegate_->OnNeedReconnect(); + delegate_->OnSettingsExhausted(); return; } diff --git a/jingle/notifier/communicator/single_login_attempt.h b/jingle/notifier/communicator/single_login_attempt.h index 1e0332d..baf45e1 100644 --- a/jingle/notifier/communicator/single_login_attempt.h +++ b/jingle/notifier/communicator/single_login_attempt.h @@ -26,13 +26,30 @@ struct ServerInformation; // login attempt. class SingleLoginAttempt : public XmppConnection::Delegate { public: + // At most one delegate method will be called, depending on the + // result of the login attempt. After the delegate method is + // called, this class won't do anything anymore until it is + // destroyed, at which point it will disconnect if necessary. class Delegate { public: + // Called when the login attempt is successful. virtual void OnConnect( base::WeakPtr<buzz::XmppTaskParentInterface> base_task) = 0; - virtual void OnNeedReconnect() = 0; + + // Called when the server responds with a redirect. A new login + // attempt should be made to the given redirect server. virtual void OnRedirect(const ServerInformation& redirect_server) = 0; + // Called when a server rejects the client's login credentials. A + // new login attempt should be made once the client provides new + // credentials. + virtual void OnCredentialsRejected() = 0; + + // Called when no server could be logged into for reasons other + // than redirection or rejected credentials. A new login attempt + // may be created, but it should be done with exponential backoff. + virtual void OnSettingsExhausted() = 0; + protected: virtual ~Delegate(); }; diff --git a/jingle/notifier/communicator/single_login_attempt_unittest.cc b/jingle/notifier/communicator/single_login_attempt_unittest.cc index 700b117..9076015 100644 --- a/jingle/notifier/communicator/single_login_attempt_unittest.cc +++ b/jingle/notifier/communicator/single_login_attempt_unittest.cc @@ -14,10 +14,10 @@ #include "jingle/notifier/communicator/login_settings.h" #include "net/base/mock_host_resolver.h" #include "net/url_request/url_request_test_util.h" -#include "testing/gtest/include/gtest/gtest.h" #include "talk/xmllite/xmlelement.h" #include "talk/xmpp/constants.h" #include "talk/xmpp/xmppengine.h" +#include "testing/gtest/include/gtest/gtest.h" namespace buzz { class XmppTaskParentInterface; @@ -27,26 +27,33 @@ namespace notifier { namespace { -enum DelegateState { IDLE, CONNECTED, NEED_RECONNECT, REDIRECTED }; +enum DelegateState { + IDLE, CONNECTED, REDIRECTED, CREDENTIALS_REJECTED, SETTINGS_EXHAUSTED +}; class FakeDelegate : public SingleLoginAttempt::Delegate { public: FakeDelegate() : state_(IDLE) {} - void OnConnect(base::WeakPtr<buzz::XmppTaskParentInterface> base_task) { + void OnConnect( + base::WeakPtr<buzz::XmppTaskParentInterface> base_task) OVERRIDE { state_ = CONNECTED; base_task_ = base_task; } - virtual void OnNeedReconnect() { - state_ = NEED_RECONNECT; - } - virtual void OnRedirect(const ServerInformation& redirect_server) OVERRIDE { state_ = REDIRECTED; redirect_server_ = redirect_server; } + virtual void OnCredentialsRejected() OVERRIDE { + state_ = CREDENTIALS_REJECTED; + } + + virtual void OnSettingsExhausted() OVERRIDE { + state_ = SETTINGS_EXHAUSTED; + } + DelegateState state() const { return state_; } base::WeakPtr<buzz::XmppTaskParentInterface> base_task() const { @@ -115,14 +122,14 @@ TEST_F(SingleLoginAttemptTest, Basic) { fake_delegate_.base_task().get()); } -// Fire OnErrors and make sure the delegate gets the OnNeedReconnect() -// event. +// Fire OnErrors and make sure the delegate gets the +// OnSettingsExhausted() event. TEST_F(SingleLoginAttemptTest, Error) { for (int i = 0; i < 2; ++i) { EXPECT_EQ(IDLE, fake_delegate_.state()); attempt_.OnError(buzz::XmppEngine::ERROR_NONE, 0, NULL); } - EXPECT_EQ(NEED_RECONNECT, fake_delegate_.state()); + EXPECT_EQ(SETTINGS_EXHAUSTED, fake_delegate_.state()); } // Fire OnErrors but replace the last one with OnConnect, and make @@ -233,6 +240,13 @@ TEST_F(SingleLoginAttemptTest, RedirectMissingSeeOtherHost) { EXPECT_EQ(IDLE, fake_delegate_.state()); } +// Fire 'Unauthorized' errors and make sure the delegate gets the +// OnCredentialsRejected() event. +TEST_F(SingleLoginAttemptTest, CredentialsRejected) { + attempt_.OnError(buzz::XmppEngine::ERROR_UNAUTHORIZED, 0, NULL); + EXPECT_EQ(CREDENTIALS_REJECTED, fake_delegate_.state()); +} + } // namespace } // namespace notifier diff --git a/jingle/notifier/listener/fake_push_client.cc b/jingle/notifier/listener/fake_push_client.cc index cfe4a99..0565c3c 100644 --- a/jingle/notifier/listener/fake_push_client.cc +++ b/jingle/notifier/listener/fake_push_client.cc @@ -35,10 +35,15 @@ void FakePushClient::SendNotification(const Notification& notification) { sent_notifications_.push_back(notification); } -void FakePushClient::SimulateNotificationStateChange( - bool notifications_enabled) { +void FakePushClient::EnableNotifications() { FOR_EACH_OBSERVER(PushClientObserver, observers_, - OnNotificationStateChange(notifications_enabled)); + OnNotificationsEnabled()); +} + +void FakePushClient::DisableNotifications( + NotificationsDisabledReason reason) { + FOR_EACH_OBSERVER(PushClientObserver, observers_, + OnNotificationsDisabled(reason)); } void FakePushClient::SimulateIncomingNotification( diff --git a/jingle/notifier/listener/fake_push_client.h b/jingle/notifier/listener/fake_push_client.h index 61ba72b..90e086c 100644 --- a/jingle/notifier/listener/fake_push_client.h +++ b/jingle/notifier/listener/fake_push_client.h @@ -11,6 +11,7 @@ #include "base/compiler_specific.h" #include "base/observer_list.h" #include "jingle/notifier/listener/push_client.h" +#include "jingle/notifier/listener/push_client_observer.h" namespace notifier { @@ -29,8 +30,11 @@ class FakePushClient : public PushClient { const std::string& email, const std::string& token) OVERRIDE; virtual void SendNotification(const Notification& notification) OVERRIDE; - // Triggers OnNotificationStateChange on all observers. - void SimulateNotificationStateChange(bool notifications_enabled); + // Triggers OnNotificationsEnabled on all observers. + void EnableNotifications(); + + // Triggers OnNotificationsDisabled on all observers. + void DisableNotifications(NotificationsDisabledReason reason); // Triggers OnIncomingNotification on all observers. void SimulateIncomingNotification(const Notification& notification); diff --git a/jingle/notifier/listener/fake_push_client_observer.cc b/jingle/notifier/listener/fake_push_client_observer.cc index ec1b9e1..7b907d0 100644 --- a/jingle/notifier/listener/fake_push_client_observer.cc +++ b/jingle/notifier/listener/fake_push_client_observer.cc @@ -7,13 +7,17 @@ namespace notifier { FakePushClientObserver::FakePushClientObserver() - : notifications_enabled_(false) {} + :last_notifications_disabled_reason_(DEFAULT_NOTIFICATION_ERROR) {} FakePushClientObserver::~FakePushClientObserver() {} -void FakePushClientObserver::OnNotificationStateChange( - bool notifications_enabled) { - notifications_enabled_ = notifications_enabled; +void FakePushClientObserver::OnNotificationsEnabled() { + last_notifications_disabled_reason_ = NO_NOTIFICATION_ERROR; +} + +void FakePushClientObserver::OnNotificationsDisabled( + NotificationsDisabledReason reason) { + last_notifications_disabled_reason_ = reason; } void FakePushClientObserver::OnIncomingNotification( @@ -21,8 +25,9 @@ void FakePushClientObserver::OnIncomingNotification( last_incoming_notification_ = notification; } -bool FakePushClientObserver::notifications_enabled() const { - return notifications_enabled_; +NotificationsDisabledReason +FakePushClientObserver::last_notifications_disabled_reason() const { + return last_notifications_disabled_reason_; } const Notification& diff --git a/jingle/notifier/listener/fake_push_client_observer.h b/jingle/notifier/listener/fake_push_client_observer.h index b8ee6d6..d350021 100644 --- a/jingle/notifier/listener/fake_push_client_observer.h +++ b/jingle/notifier/listener/fake_push_client_observer.h @@ -17,16 +17,17 @@ class FakePushClientObserver : public PushClientObserver { virtual ~FakePushClientObserver(); // PushClientObserver implementation. - virtual void OnNotificationStateChange( - bool notifications_enabled) OVERRIDE; + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + NotificationsDisabledReason reason) OVERRIDE; virtual void OnIncomingNotification( const Notification& notification) OVERRIDE; - bool notifications_enabled() const; + NotificationsDisabledReason last_notifications_disabled_reason() const; const Notification& last_incoming_notification() const; private: - bool notifications_enabled_; + NotificationsDisabledReason last_notifications_disabled_reason_; Notification last_incoming_notification_; }; diff --git a/jingle/notifier/listener/non_blocking_push_client.cc b/jingle/notifier/listener/non_blocking_push_client.cc index 23a8402..57ec240 100644 --- a/jingle/notifier/listener/non_blocking_push_client.cc +++ b/jingle/notifier/listener/non_blocking_push_client.cc @@ -5,9 +5,9 @@ #include "jingle/notifier/listener/non_blocking_push_client.h" #include "base/bind.h" -#include "base/message_loop_proxy.h" #include "base/location.h" #include "base/logging.h" +#include "base/message_loop_proxy.h" #include "jingle/notifier/listener/push_client_observer.h" namespace notifier { @@ -39,8 +39,9 @@ class NonBlockingPushClient::Core void UpdateCredentials(const std::string& email, const std::string& token); void SendNotification(const Notification& data); - virtual void OnNotificationStateChange( - bool notifications_enabled) OVERRIDE; + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + NotificationsDisabledReason reason) OVERRIDE; virtual void OnIncomingNotification( const Notification& notification) OVERRIDE; @@ -109,13 +110,21 @@ void NonBlockingPushClient::Core::SendNotification( delegate_push_client_->SendNotification(notification); } -void NonBlockingPushClient::Core::OnNotificationStateChange( - bool notifications_enabled) { +void NonBlockingPushClient::Core::OnNotificationsEnabled() { DCHECK(delegate_task_runner_->BelongsToCurrentThread()); parent_task_runner_->PostTask( FROM_HERE, - base::Bind(&NonBlockingPushClient::OnNotificationStateChange, - parent_push_client_, notifications_enabled)); + base::Bind(&NonBlockingPushClient::OnNotificationsEnabled, + parent_push_client_)); +} + +void NonBlockingPushClient::Core::OnNotificationsDisabled( + NotificationsDisabledReason reason) { + DCHECK(delegate_task_runner_->BelongsToCurrentThread()); + parent_task_runner_->PostTask( + FROM_HERE, + base::Bind(&NonBlockingPushClient::OnNotificationsDisabled, + parent_push_client_, reason)); } void NonBlockingPushClient::Core::OnIncomingNotification( @@ -186,11 +195,17 @@ void NonBlockingPushClient::SendNotification( notification)); } -void NonBlockingPushClient::OnNotificationStateChange( - bool notifications_enabled) { +void NonBlockingPushClient::OnNotificationsEnabled() { + DCHECK(non_thread_safe_.CalledOnValidThread()); + FOR_EACH_OBSERVER(PushClientObserver, observers_, + OnNotificationsEnabled()); +} + +void NonBlockingPushClient::OnNotificationsDisabled( + NotificationsDisabledReason reason) { DCHECK(non_thread_safe_.CalledOnValidThread()); FOR_EACH_OBSERVER(PushClientObserver, observers_, - OnNotificationStateChange(notifications_enabled)); + OnNotificationsDisabled(reason)); } void NonBlockingPushClient::OnIncomingNotification( diff --git a/jingle/notifier/listener/non_blocking_push_client.h b/jingle/notifier/listener/non_blocking_push_client.h index ca57229..9205d71 100644 --- a/jingle/notifier/listener/non_blocking_push_client.h +++ b/jingle/notifier/listener/non_blocking_push_client.h @@ -14,6 +14,7 @@ #include "base/observer_list.h" #include "base/threading/non_thread_safe.h" #include "jingle/notifier/listener/push_client.h" +#include "jingle/notifier/listener/push_client_observer.h" namespace base { class SingleThreadTaskRunner; @@ -52,7 +53,8 @@ class NonBlockingPushClient : public PushClient { private: class Core; - void OnNotificationStateChange(bool notifications_enabled); + void OnNotificationsEnabled(); + void OnNotificationsDisabled(NotificationsDisabledReason reason); void OnIncomingNotification(const Notification& notification); base::NonThreadSafe non_thread_safe_; diff --git a/jingle/notifier/listener/non_blocking_push_client_unittest.cc b/jingle/notifier/listener/non_blocking_push_client_unittest.cc index c415225..f9e0282 100644 --- a/jingle/notifier/listener/non_blocking_push_client_unittest.cc +++ b/jingle/notifier/listener/non_blocking_push_client_unittest.cc @@ -115,13 +115,17 @@ TEST_F(NonBlockingPushClientTest, SendNotification) { // Make sure notification state changes get propagated back to the // parent. TEST_F(NonBlockingPushClientTest, NotificationStateChange) { - EXPECT_FALSE(fake_observer_.notifications_enabled()); - fake_push_client_->SimulateNotificationStateChange(true); + EXPECT_EQ(DEFAULT_NOTIFICATION_ERROR, + fake_observer_.last_notifications_disabled_reason()); + fake_push_client_->EnableNotifications(); message_loop_.RunAllPending(); - EXPECT_TRUE(fake_observer_.notifications_enabled()); - fake_push_client_->SimulateNotificationStateChange(false); + EXPECT_EQ(NO_NOTIFICATION_ERROR, + fake_observer_.last_notifications_disabled_reason()); + fake_push_client_->DisableNotifications( + NOTIFICATION_CREDENTIALS_REJECTED); message_loop_.RunAllPending(); - EXPECT_FALSE(fake_observer_.notifications_enabled()); + EXPECT_EQ(NOTIFICATION_CREDENTIALS_REJECTED, + fake_observer_.last_notifications_disabled_reason()); } // Make sure incoming notifications get propagated back to the parent. diff --git a/jingle/notifier/listener/push_client_observer.h b/jingle/notifier/listener/push_client_observer.h index 0fe7d46..d51deb2 100644 --- a/jingle/notifier/listener/push_client_observer.h +++ b/jingle/notifier/listener/push_client_observer.h @@ -9,18 +9,32 @@ namespace notifier { -// A PushClientObserver is notified whenever an incoming notification -// is received or when the state of the push client changes. +enum NotificationsDisabledReason { + // There is an underlying transient problem (e.g., network- or + // XMPP-related). + TRANSIENT_NOTIFICATION_ERROR, + DEFAULT_NOTIFICATION_ERROR = TRANSIENT_NOTIFICATION_ERROR, + // Our credentials have been rejected. + NOTIFICATION_CREDENTIALS_REJECTED, + // No error (useful for avoiding keeping a separate bool for + // notifications enabled/disabled). + NO_NOTIFICATION_ERROR +}; + +// A PushClientObserver is notified when notifications are enabled or +// disabled, and when a notification is received. class PushClientObserver { protected: virtual ~PushClientObserver(); public: - // Called when the state of the push client changes. If - // |notifications_enabled| is true, that means notifications can be - // sent and received freely. If it is false, that means no - // notifications can be sent or received. - virtual void OnNotificationStateChange(bool notifications_enabled) = 0; + // Called when notifications are enabled. + virtual void OnNotificationsEnabled() = 0; + + // Called when notifications are disabled, with the reason (not + // equal to NO_ERROR) in |reason|. + virtual void OnNotificationsDisabled( + NotificationsDisabledReason reason) = 0; // Called when a notification is received. The details of the // notification are in |notification|. diff --git a/jingle/notifier/listener/xmpp_push_client.cc b/jingle/notifier/listener/xmpp_push_client.cc index b1272f3..66521f2 100644 --- a/jingle/notifier/listener/xmpp_push_client.cc +++ b/jingle/notifier/listener/xmpp_push_client.cc @@ -58,11 +58,19 @@ void XmppPushClient::OnConnect( } } -void XmppPushClient::OnDisconnect() { +void XmppPushClient::OnTransientDisconnection() { DCHECK(non_thread_safe_.CalledOnValidThread()); base_task_.reset(); FOR_EACH_OBSERVER(PushClientObserver, observers_, - OnNotificationStateChange(false)); + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); +} + +void XmppPushClient::OnCredentialsRejected() { + DCHECK(non_thread_safe_.CalledOnValidThread()); + base_task_.reset(); + FOR_EACH_OBSERVER( + PushClientObserver, observers_, + OnNotificationsDisabled(NOTIFICATION_CREDENTIALS_REJECTED)); } void XmppPushClient::OnNotificationReceived( @@ -75,13 +83,13 @@ void XmppPushClient::OnNotificationReceived( void XmppPushClient::OnSubscribed() { DCHECK(non_thread_safe_.CalledOnValidThread()); FOR_EACH_OBSERVER(PushClientObserver, observers_, - OnNotificationStateChange(true)); + OnNotificationsEnabled()); } void XmppPushClient::OnSubscriptionError() { DCHECK(non_thread_safe_.CalledOnValidThread()); FOR_EACH_OBSERVER(PushClientObserver, observers_, - OnNotificationStateChange(false)); + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); } void XmppPushClient::AddObserver(PushClientObserver* observer) { diff --git a/jingle/notifier/listener/xmpp_push_client.h b/jingle/notifier/listener/xmpp_push_client.h index 9c4a6f3..34cede5 100644 --- a/jingle/notifier/listener/xmpp_push_client.h +++ b/jingle/notifier/listener/xmpp_push_client.h @@ -10,8 +10,8 @@ #include "base/basictypes.h" #include "base/compiler_specific.h" -#include "base/memory/scoped_ptr.h" #include "base/memory/ref_counted.h" +#include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "base/observer_list.h" #include "base/threading/non_thread_safe.h" @@ -34,7 +34,7 @@ namespace notifier { // This class must be used on a single thread. class XmppPushClient : public PushClient, - public LoginDelegate, + public Login::Delegate, public PushNotificationsListenTaskDelegate, public PushNotificationsSubscribeTaskDelegate { public: @@ -53,7 +53,8 @@ class XmppPushClient : // Login::Delegate implementation. virtual void OnConnect( base::WeakPtr<buzz::XmppTaskParentInterface> base_task) OVERRIDE; - virtual void OnDisconnect() OVERRIDE; + virtual void OnTransientDisconnection() OVERRIDE; + virtual void OnCredentialsRejected() OVERRIDE; // PushNotificationsListenTaskDelegate implementation. virtual void OnNotificationReceived( diff --git a/jingle/notifier/listener/xmpp_push_client_unittest.cc b/jingle/notifier/listener/xmpp_push_client_unittest.cc index 6a92bcf..3487479 100644 --- a/jingle/notifier/listener/xmpp_push_client_unittest.cc +++ b/jingle/notifier/listener/xmpp_push_client_unittest.cc @@ -24,7 +24,8 @@ using ::testing::StrictMock; class MockObserver : public PushClientObserver { public: - MOCK_METHOD1(OnNotificationStateChange, void(bool)); + MOCK_METHOD0(OnNotificationsEnabled, void()); + MOCK_METHOD1(OnNotificationsDisabled, void(NotificationsDisabledReason)); MOCK_METHOD1(OnIncomingNotification, void(const Notification&)); }; @@ -67,7 +68,7 @@ TEST_F(XmppPushClientTest, OnIncomingNotification) { // Make sure the XMPP push client notifies its observers of a // successful connection properly. TEST_F(XmppPushClientTest, ConnectAndSubscribe) { - EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); + EXPECT_CALL(mock_observer_, OnNotificationsEnabled()); xmpp_push_client_->OnConnect(fake_base_task_.AsWeakPtr()); xmpp_push_client_->OnSubscribed(); } @@ -75,14 +76,24 @@ TEST_F(XmppPushClientTest, ConnectAndSubscribe) { // Make sure the XMPP push client notifies its observers of a // terminated connection properly. TEST_F(XmppPushClientTest, Disconnect) { - EXPECT_CALL(mock_observer_, OnNotificationStateChange(false)); - xmpp_push_client_->OnDisconnect(); + EXPECT_CALL(mock_observer_, + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); + xmpp_push_client_->OnTransientDisconnection(); +} + +// Make sure the XMPP push client notifies its observers of +// rejected credentials properly. +TEST_F(XmppPushClientTest, RejectCredentials) { + EXPECT_CALL(mock_observer_, + OnNotificationsDisabled(NOTIFICATION_CREDENTIALS_REJECTED)); + xmpp_push_client_->OnCredentialsRejected(); } // Make sure the XMPP push client notifies its observers of a // subscription error properly. TEST_F(XmppPushClientTest, SubscriptionError) { - EXPECT_CALL(mock_observer_, OnNotificationStateChange(false)); + EXPECT_CALL(mock_observer_, + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); xmpp_push_client_->OnSubscriptionError(); } @@ -92,7 +103,7 @@ TEST_F(XmppPushClientTest, SubscriptionError) { // TODO(akalin): Figure out how to test that the notification was // actually sent. TEST_F(XmppPushClientTest, SendNotification) { - EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); + EXPECT_CALL(mock_observer_, OnNotificationsEnabled()); xmpp_push_client_->OnConnect(fake_base_task_.AsWeakPtr()); xmpp_push_client_->OnSubscribed(); @@ -109,7 +120,7 @@ TEST_F(XmppPushClientTest, SendNotificationPending) { Mock::VerifyAndClearExpectations(&mock_observer_); - EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); + EXPECT_CALL(mock_observer_, OnNotificationsEnabled()); xmpp_push_client_->OnConnect(fake_base_task_.AsWeakPtr()); xmpp_push_client_->OnSubscribed(); diff --git a/sync/internal_api/public/engine/sync_status.h b/sync/internal_api/public/engine/sync_status.h index 1c9ba1a..f05a5bc 100644 --- a/sync/internal_api/public/engine/sync_status.h +++ b/sync/internal_api/public/engine/sync_status.h @@ -22,6 +22,8 @@ struct SyncStatus { SyncStatus(); ~SyncStatus(); + // TODO(akalin): Replace this with a NotificationsDisabledReason + // variable. bool notifications_enabled; // True only if subscribed for notifications. // Notifications counters updated by the actions in synapi. diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h index f4a42e3..a03e2d7 100644 --- a/sync/internal_api/public/sync_manager.h +++ b/sync/internal_api/public/sync_manager.h @@ -532,8 +532,9 @@ class SyncManager { // Functions used for testing. - void TriggerOnNotificationStateChangeForTest( - bool notifications_enabled); + void SimulateEnableNotificationsForTest(); + + void SimulateDisableNotificationsForTest(int reason); void TriggerOnIncomingNotificationForTest( syncable::ModelTypeSet model_types); diff --git a/sync/internal_api/sync_manager.cc b/sync/internal_api/sync_manager.cc index 8c95292..52a9c76 100644 --- a/sync/internal_api/sync_manager.cc +++ b/sync/internal_api/sync_manager.cc @@ -46,6 +46,7 @@ #include "sync/js/js_event_details.h" #include "sync/js/js_event_handler.h" #include "sync/js/js_reply_handler.h" +#include "sync/notifier/notifications_disabled_reason.h" #include "sync/notifier/sync_notifier.h" #include "sync/notifier/sync_notifier_observer.h" #include "sync/protocol/encryption.pb.h" @@ -321,9 +322,9 @@ class SyncManager::SyncInternal bool encrypt_everything) OVERRIDE; // SyncNotifierObserver implementation. - virtual void OnNotificationStateChange( - bool notifications_enabled) OVERRIDE; - + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + sync_notifier::NotificationsDisabledReason reason) OVERRIDE; virtual void OnIncomingNotification( const syncable::ModelTypePayloadMap& type_payloads, sync_notifier::IncomingNotificationSource source) OVERRIDE; @@ -2274,17 +2275,27 @@ void SyncManager::SyncInternal::OnEncryptedTypesChanged( OnEncryptedTypesChanged(encrypted_types, encrypt_everything)); } -void SyncManager::SyncInternal::OnNotificationStateChange( - bool notifications_enabled) { - DVLOG(1) << "P2P: Notifications enabled = " - << (notifications_enabled ? "true" : "false"); - allstatus_.SetNotificationsEnabled(notifications_enabled); +void SyncManager::SyncInternal::UpdateNotificationInfo( + const syncable::ModelTypePayloadMap& type_payloads) { + for (syncable::ModelTypePayloadMap::const_iterator it = type_payloads.begin(); + it != type_payloads.end(); ++it) { + NotificationInfo* info = ¬ification_info_map_[it->first]; + info->total_count++; + info->payload = it->second; + } +} + +void SyncManager::SyncInternal::OnNotificationsEnabled() { + DVLOG(1) << "Notifications enabled"; + allstatus_.SetNotificationsEnabled(true); if (scheduler()) { - scheduler()->set_notifications_enabled(notifications_enabled); + scheduler()->set_notifications_enabled(true); } + // TODO(akalin): Separate onNotificationStateChange into + // enabled/disabled events. if (js_event_handler_.IsInitialized()) { DictionaryValue details; - details.Set("enabled", Value::CreateBooleanValue(notifications_enabled)); + details.Set("enabled", Value::CreateBooleanValue(true)); js_event_handler_.Call(FROM_HERE, &JsEventHandler::HandleJsEvent, "onNotificationStateChange", @@ -2292,14 +2303,24 @@ void SyncManager::SyncInternal::OnNotificationStateChange( } } -void SyncManager::SyncInternal::UpdateNotificationInfo( - const syncable::ModelTypePayloadMap& type_payloads) { - for (syncable::ModelTypePayloadMap::const_iterator it = type_payloads.begin(); - it != type_payloads.end(); ++it) { - NotificationInfo* info = ¬ification_info_map_[it->first]; - info->total_count++; - info->payload = it->second; +void SyncManager::SyncInternal::OnNotificationsDisabled( + sync_notifier::NotificationsDisabledReason reason) { + DVLOG(1) << "Notifications disabled with reason " + << sync_notifier::NotificationsDisabledReasonToString(reason); + allstatus_.SetNotificationsEnabled(false); + if (scheduler()) { + scheduler()->set_notifications_enabled(false); + } + if (js_event_handler_.IsInitialized()) { + DictionaryValue details; + details.Set("enabled", Value::CreateBooleanValue(false)); + js_event_handler_.Call(FROM_HERE, + &JsEventHandler::HandleJsEvent, + "onNotificationStateChange", + JsEventDetails(&details)); } + // TODO(akalin): Treat a CREDENTIALS_REJECTED state as an auth + // error. } void SyncManager::SyncInternal::OnIncomingNotification( @@ -2422,10 +2443,15 @@ bool SyncManager::HasUnsyncedItems() const { return (trans.GetWrappedTrans()->directory()->unsynced_entity_count() != 0); } -void SyncManager::TriggerOnNotificationStateChangeForTest( - bool notifications_enabled) { +void SyncManager::SimulateEnableNotificationsForTest() { + DCHECK(thread_checker_.CalledOnValidThread()); + data_->OnNotificationsEnabled(); +} + +void SyncManager::SimulateDisableNotificationsForTest(int reason) { DCHECK(thread_checker_.CalledOnValidThread()); - data_->OnNotificationStateChange(notifications_enabled); + data_->OnNotificationsDisabled( + static_cast<sync_notifier::NotificationsDisabledReason>(reason)); } void SyncManager::TriggerOnIncomingNotificationForTest( diff --git a/sync/internal_api/syncapi_unittest.cc b/sync/internal_api/syncapi_unittest.cc index 1f1079c..460ee92 100644 --- a/sync/internal_api/syncapi_unittest.cc +++ b/sync/internal_api/syncapi_unittest.cc @@ -1275,16 +1275,19 @@ TEST_F(SyncManagerTest, OnNotificationStateChange) { HandleJsEvent("onNotificationStateChange", HasDetailsAsDictionary(false_details))); - sync_manager_.TriggerOnNotificationStateChangeForTest(true); - sync_manager_.TriggerOnNotificationStateChangeForTest(false); + sync_manager_.SimulateEnableNotificationsForTest(); + sync_manager_.SimulateDisableNotificationsForTest( + sync_notifier::TRANSIENT_NOTIFICATION_ERROR); SetJsEventHandler(event_handler.AsWeakHandle()); - sync_manager_.TriggerOnNotificationStateChangeForTest(true); - sync_manager_.TriggerOnNotificationStateChangeForTest(false); + sync_manager_.SimulateEnableNotificationsForTest(); + sync_manager_.SimulateDisableNotificationsForTest( + sync_notifier::TRANSIENT_NOTIFICATION_ERROR); SetJsEventHandler(WeakHandle<JsEventHandler>()); - sync_manager_.TriggerOnNotificationStateChangeForTest(true); - sync_manager_.TriggerOnNotificationStateChangeForTest(false); + sync_manager_.SimulateEnableNotificationsForTest(); + sync_manager_.SimulateDisableNotificationsForTest( + sync_notifier::TRANSIENT_NOTIFICATION_ERROR); // Should trigger the replies. PumpLoop(); diff --git a/sync/notifier/chrome_invalidation_client.cc b/sync/notifier/chrome_invalidation_client.cc index 278076f..030d7e5 100644 --- a/sync/notifier/chrome_invalidation_client.cc +++ b/sync/notifier/chrome_invalidation_client.cc @@ -31,15 +31,19 @@ ChromeInvalidationClient::Listener::~Listener() {} ChromeInvalidationClient::ChromeInvalidationClient( scoped_ptr<notifier::PushClient> push_client) - : chrome_system_resources_(push_client.Pass(), + : push_client_(push_client.get()), + chrome_system_resources_(push_client.Pass(), ALLOW_THIS_IN_INITIALIZER_LIST(this)), listener_(NULL), - ticl_ready_(false) { + ticl_state_(DEFAULT_NOTIFICATION_ERROR), + push_client_state_(DEFAULT_NOTIFICATION_ERROR) { DCHECK(CalledOnValidThread()); + push_client_->AddObserver(this); } ChromeInvalidationClient::~ChromeInvalidationClient() { DCHECK(CalledOnValidThread()); + push_client_->RemoveObserver(this); Stop(); DCHECK(!listener_); } @@ -98,27 +102,10 @@ void ChromeInvalidationClient::UpdateCredentials( chrome_system_resources_.network()->UpdateCredentials(email, token); } -void ChromeInvalidationClient::Stop() { - DCHECK(CalledOnValidThread()); - if (!invalidation_client_.get()) { - return; - } - - registration_manager_.reset(); - chrome_system_resources_.Stop(); - invalidation_client_->Stop(); - - invalidation_client_.reset(); - listener_ = NULL; - - invalidation_state_tracker_.Reset(); - max_invalidation_versions_.clear(); -} - void ChromeInvalidationClient::RegisterTypes(syncable::ModelTypeSet types) { DCHECK(CalledOnValidThread()); registered_types_ = types; - if (ticl_ready_ && registration_manager_.get()) { + if (GetState() == NO_NOTIFICATION_ERROR && registration_manager_.get()) { registration_manager_->SetRegisteredTypes(registered_types_); } // TODO(akalin): Clear invalidation versions for unregistered types. @@ -126,8 +113,8 @@ void ChromeInvalidationClient::RegisterTypes(syncable::ModelTypeSet types) { void ChromeInvalidationClient::Ready( invalidation::InvalidationClient* client) { - ticl_ready_ = true; - listener_->OnSessionStatusChanged(true); + ticl_state_ = NO_NOTIFICATION_ERROR; + EmitStateChange(); registration_manager_->SetRegisteredTypes(registered_types_); } @@ -286,9 +273,15 @@ void ChromeInvalidationClient::ReissueRegistrations( void ChromeInvalidationClient::InformError( invalidation::InvalidationClient* client, const invalidation::ErrorInfo& error_info) { - listener_->OnSessionStatusChanged(false); - LOG(ERROR) << "Invalidation client encountered an error"; - // TODO(ghc): handle the error. + LOG(ERROR) << "Ticl error " << error_info.error_reason() << ": " + << error_info.error_message() + << " (transient = " << error_info.is_transient() << ")"; + if (error_info.error_reason() == invalidation::ErrorReason::AUTH_FAILURE) { + ticl_state_ = NOTIFICATION_CREDENTIALS_REJECTED; + } else { + ticl_state_ = TRANSIENT_NOTIFICATION_ERROR; + } + EmitStateChange(); } void ChromeInvalidationClient::WriteState(const std::string& state) { @@ -298,4 +291,69 @@ void ChromeInvalidationClient::WriteState(const std::string& state) { FROM_HERE, &InvalidationStateTracker::SetInvalidationState, state); } +void ChromeInvalidationClient::Stop() { + DCHECK(CalledOnValidThread()); + if (!invalidation_client_.get()) { + return; + } + + registration_manager_.reset(); + chrome_system_resources_.Stop(); + invalidation_client_->Stop(); + + invalidation_client_.reset(); + listener_ = NULL; + + invalidation_state_tracker_.Reset(); + max_invalidation_versions_.clear(); + ticl_state_ = DEFAULT_NOTIFICATION_ERROR; + push_client_state_ = DEFAULT_NOTIFICATION_ERROR; +} + +NotificationsDisabledReason ChromeInvalidationClient::GetState() const { + DCHECK(CalledOnValidThread()); + if (ticl_state_ == NOTIFICATION_CREDENTIALS_REJECTED || + push_client_state_ == NOTIFICATION_CREDENTIALS_REJECTED) { + // If either the ticl or the push client rejected our credentials, + // return NOTIFICATION_CREDENTIALS_REJECTED. + return NOTIFICATION_CREDENTIALS_REJECTED; + } + if (ticl_state_ == NO_NOTIFICATION_ERROR && + push_client_state_ == NO_NOTIFICATION_ERROR) { + // If the ticl is ready and the push client notifications are + // enabled, return NO_NOTIFICATION_ERROR. + return NO_NOTIFICATION_ERROR; + } + // Otherwise, we have a transient error. + return TRANSIENT_NOTIFICATION_ERROR; +} + +void ChromeInvalidationClient::EmitStateChange() { + DCHECK(CalledOnValidThread()); + if (GetState() == NO_NOTIFICATION_ERROR) { + listener_->OnNotificationsEnabled(); + } else { + listener_->OnNotificationsDisabled(GetState()); + } +} + +void ChromeInvalidationClient::OnNotificationsEnabled() { + DCHECK(CalledOnValidThread()); + push_client_state_ = NO_NOTIFICATION_ERROR; + EmitStateChange(); +} + +void ChromeInvalidationClient::OnNotificationsDisabled( + notifier::NotificationsDisabledReason reason) { + DCHECK(CalledOnValidThread()); + push_client_state_ = FromNotifierReason(reason); + EmitStateChange(); +} + +void ChromeInvalidationClient::OnIncomingNotification( + const notifier::Notification& notification) { + DCHECK(CalledOnValidThread()); + // Do nothing, since this is already handled by |invalidation_client_|. +} + } // namespace sync_notifier diff --git a/sync/notifier/chrome_invalidation_client.h b/sync/notifier/chrome_invalidation_client.h index fe25dc3..c423c8d 100644 --- a/sync/notifier/chrome_invalidation_client.h +++ b/sync/notifier/chrome_invalidation_client.h @@ -17,11 +17,13 @@ #include "base/memory/weak_ptr.h" #include "base/threading/non_thread_safe.h" #include "google/cacheinvalidation/include/invalidation-listener.h" +#include "jingle/notifier/listener/push_client_observer.h" #include "sync/internal_api/public/syncable/model_type.h" #include "sync/internal_api/public/syncable/model_type_payload_map.h" #include "sync/internal_api/public/util/weak_handle.h" #include "sync/notifier/chrome_system_resources.h" #include "sync/notifier/invalidation_state_tracker.h" +#include "sync/notifier/notifications_disabled_reason.h" #include "sync/notifier/state_writer.h" namespace buzz { @@ -43,6 +45,7 @@ class RegistrationManager; class ChromeInvalidationClient : public InvalidationListener, public StateWriter, + public notifier::PushClientObserver, public base::NonThreadSafe { public: class Listener { @@ -52,7 +55,10 @@ class ChromeInvalidationClient virtual void OnInvalidate( const syncable::ModelTypePayloadMap& type_payloads) = 0; - virtual void OnSessionStatusChanged(bool has_session) = 0; + virtual void OnNotificationsEnabled() = 0; + + virtual void OnNotificationsDisabled( + NotificationsDisabledReason reason) = 0; }; explicit ChromeInvalidationClient( @@ -77,8 +83,6 @@ class ChromeInvalidationClient // notifications for. May be called at any time. void RegisterTypes(syncable::ModelTypeSet types); - virtual void WriteState(const std::string& state) OVERRIDE; - // invalidation::InvalidationListener implementation. virtual void Ready( invalidation::InvalidationClient* client) OVERRIDE; @@ -110,14 +114,30 @@ class ChromeInvalidationClient invalidation::InvalidationClient* client, const invalidation::ErrorInfo& error_info) OVERRIDE; + // StateWriter implementation. + virtual void WriteState(const std::string& state) OVERRIDE; + + // notifier::PushClientObserver implementation. + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + notifier::NotificationsDisabledReason reason) OVERRIDE; + virtual void OnIncomingNotification( + const notifier::Notification& notification) OVERRIDE; + private: friend class ChromeInvalidationClientTest; void Stop(); + NotificationsDisabledReason GetState() const; + + void EmitStateChange(); + void EmitInvalidation( syncable::ModelTypeSet types, const std::string& payload); + // Owned by |chrome_system_resources_|. + notifier::PushClient* const push_client_; ChromeSystemResources chrome_system_resources_; InvalidationVersionMap max_invalidation_versions_; browser_sync::WeakHandle<InvalidationStateTracker> @@ -127,7 +147,11 @@ class ChromeInvalidationClient scoped_ptr<RegistrationManager> registration_manager_; // Stored to pass to |registration_manager_| on start. syncable::ModelTypeSet registered_types_; - bool ticl_ready_; + + // The states of the ticl and the push client (with + // NO_NOTIFICATION_ERROR meaning notifications are enabled). + NotificationsDisabledReason ticl_state_; + NotificationsDisabledReason push_client_state_; DISALLOW_COPY_AND_ASSIGN(ChromeInvalidationClient); }; diff --git a/sync/notifier/chrome_invalidation_client_unittest.cc b/sync/notifier/chrome_invalidation_client_unittest.cc index f1470ac..7fa94ff 100644 --- a/sync/notifier/chrome_invalidation_client_unittest.cc +++ b/sync/notifier/chrome_invalidation_client_unittest.cc @@ -20,6 +20,7 @@ namespace sync_notifier { using ::testing::_; +using ::testing::InSequence; using ::testing::Return; using ::testing::StrictMock; @@ -44,7 +45,8 @@ class MockInvalidationClient : public invalidation::InvalidationClient { class MockListener : public ChromeInvalidationClient::Listener { public: MOCK_METHOD1(OnInvalidate, void(const syncable::ModelTypePayloadMap&)); - MOCK_METHOD1(OnSessionStatusChanged, void(bool)); + MOCK_METHOD0(OnNotificationsEnabled, void()); + MOCK_METHOD1(OnNotificationsDisabled, void(NotificationsDisabledReason)); }; } // namespace @@ -52,9 +54,8 @@ class MockListener : public ChromeInvalidationClient::Listener { class ChromeInvalidationClientTest : public testing::Test { protected: ChromeInvalidationClientTest() - : client_( - scoped_ptr<notifier::PushClient>( - new notifier::FakePushClient())) {} + : fake_push_client_(new notifier::FakePushClient()), + client_(scoped_ptr<notifier::PushClient>(fake_push_client_)) {} virtual void SetUp() { client_.Start(kClientId, kClientInfo, kState, @@ -116,6 +117,7 @@ class ChromeInvalidationClientTest : public testing::Test { StrictMock<MockInvalidationStateTracker> mock_invalidation_state_tracker_; StrictMock<MockInvalidationClient> mock_invalidation_client_; + notifier::FakePushClient* const fake_push_client_; ChromeInvalidationClient client_; }; @@ -159,12 +161,6 @@ TEST_F(ChromeInvalidationClientTest, InvalidateWithPayload) { FireInvalidate("PREFERENCE", 1, "payload"); } -TEST_F(ChromeInvalidationClientTest, WriteState) { - EXPECT_CALL(mock_invalidation_state_tracker_, - SetInvalidationState(kNewState)); - client_.WriteState(kNewState); -} - TEST_F(ChromeInvalidationClientTest, InvalidateVersion) { using ::testing::Mock; @@ -266,6 +262,74 @@ TEST_F(ChromeInvalidationClientTest, RegisterTypes) { FireInvalidateAll(); } -// TODO(akalin): Flesh out unit tests. +TEST_F(ChromeInvalidationClientTest, WriteState) { + EXPECT_CALL(mock_invalidation_state_tracker_, + SetInvalidationState(kNewState)); + client_.WriteState(kNewState); +} + +TEST_F(ChromeInvalidationClientTest, StateChangesNotReady) { + InSequence dummy; + EXPECT_CALL(mock_listener_, + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); + EXPECT_CALL(mock_listener_, + OnNotificationsDisabled(NOTIFICATION_CREDENTIALS_REJECTED)); + EXPECT_CALL(mock_listener_, + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); + + fake_push_client_->DisableNotifications( + notifier::TRANSIENT_NOTIFICATION_ERROR); + fake_push_client_->DisableNotifications( + notifier::NOTIFICATION_CREDENTIALS_REJECTED); + fake_push_client_->EnableNotifications(); +} + +TEST_F(ChromeInvalidationClientTest, StateChangesReady) { + InSequence dummy; + EXPECT_CALL(mock_listener_, + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); + EXPECT_CALL(mock_listener_, OnNotificationsEnabled()); + EXPECT_CALL(mock_listener_, + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); + EXPECT_CALL(mock_listener_, + OnNotificationsDisabled(NOTIFICATION_CREDENTIALS_REJECTED)); + EXPECT_CALL(mock_listener_, OnNotificationsEnabled()); + + fake_push_client_->EnableNotifications(); + client_.Ready(NULL); + fake_push_client_->DisableNotifications( + notifier::TRANSIENT_NOTIFICATION_ERROR); + fake_push_client_->DisableNotifications( + notifier::NOTIFICATION_CREDENTIALS_REJECTED); + fake_push_client_->EnableNotifications(); +} + +TEST_F(ChromeInvalidationClientTest, StateChangesAuthError) { + InSequence dummy; + EXPECT_CALL(mock_listener_, + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); + EXPECT_CALL(mock_listener_, OnNotificationsEnabled()); + EXPECT_CALL(mock_listener_, + OnNotificationsDisabled(NOTIFICATION_CREDENTIALS_REJECTED)) + .Times(4); + EXPECT_CALL(mock_listener_, OnNotificationsEnabled()); + + fake_push_client_->EnableNotifications(); + client_.Ready(NULL); + + client_.InformError( + NULL, + invalidation::ErrorInfo( + invalidation::ErrorReason::AUTH_FAILURE, + false /* is_transient */, + "auth error", + invalidation::ErrorContext())); + fake_push_client_->DisableNotifications( + notifier::TRANSIENT_NOTIFICATION_ERROR); + fake_push_client_->DisableNotifications( + notifier::NOTIFICATION_CREDENTIALS_REJECTED); + fake_push_client_->EnableNotifications(); + client_.Ready(NULL); +} } // namespace sync_notifier diff --git a/sync/notifier/invalidation_notifier.cc b/sync/notifier/invalidation_notifier.cc index 485e969..9925041 100644 --- a/sync/notifier/invalidation_notifier.cc +++ b/sync/notifier/invalidation_notifier.cc @@ -102,14 +102,23 @@ void InvalidationNotifier::SendNotification( void InvalidationNotifier::OnInvalidate( const syncable::ModelTypePayloadMap& type_payloads) { DCHECK(CalledOnValidThread()); + FOR_EACH_OBSERVER( + SyncNotifierObserver, observers_, + OnIncomingNotification(type_payloads, + sync_notifier::REMOTE_NOTIFICATION)); +} + +void InvalidationNotifier::OnNotificationsEnabled() { + DCHECK(CalledOnValidThread()); FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, - OnIncomingNotification(type_payloads, - sync_notifier::REMOTE_NOTIFICATION)); + OnNotificationsEnabled()); } -void InvalidationNotifier::OnSessionStatusChanged(bool has_session) { +void InvalidationNotifier::OnNotificationsDisabled( + NotificationsDisabledReason reason) { + DCHECK(CalledOnValidThread()); FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, - OnNotificationStateChange(has_session)); + OnNotificationsDisabled(reason)); } } // namespace sync_notifier diff --git a/sync/notifier/invalidation_notifier.h b/sync/notifier/invalidation_notifier.h index 4ac715e..5441094 100644 --- a/sync/notifier/invalidation_notifier.h +++ b/sync/notifier/invalidation_notifier.h @@ -64,7 +64,9 @@ class InvalidationNotifier // ChromeInvalidationClient::Listener implementation. virtual void OnInvalidate( const syncable::ModelTypePayloadMap& type_payloads) OVERRIDE; - virtual void OnSessionStatusChanged(bool has_session) OVERRIDE; + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + NotificationsDisabledReason reason) OVERRIDE; private: // We start off in the STOPPED state. When we get our initial diff --git a/sync/notifier/invalidation_notifier_unittest.cc b/sync/notifier/invalidation_notifier_unittest.cc index 16b70c1..9bd1840 100644 --- a/sync/notifier/invalidation_notifier_unittest.cc +++ b/sync/notifier/invalidation_notifier_unittest.cc @@ -80,11 +80,14 @@ TEST_F(InvalidationNotifierTest, Basic) { type_payloads[syncable::BOOKMARKS] = ""; type_payloads[syncable::AUTOFILL] = ""; - EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); + EXPECT_CALL(mock_observer_, OnNotificationsEnabled()); EXPECT_CALL(mock_observer_, OnIncomingNotification(type_payloads, REMOTE_NOTIFICATION)); - EXPECT_CALL(mock_observer_, OnNotificationStateChange(false)); + EXPECT_CALL(mock_observer_, + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); + EXPECT_CALL(mock_observer_, + OnNotificationsDisabled(NOTIFICATION_CREDENTIALS_REJECTED)); // Note no expectation on mock_tracker_, as we initialized with // non-empty initial_invalidation_state above. @@ -94,11 +97,14 @@ TEST_F(InvalidationNotifierTest, Basic) { invalidation_notifier_->SetUniqueId("fake_id"); invalidation_notifier_->UpdateCredentials("foo@bar.com", "fake_token"); - invalidation_notifier_->OnSessionStatusChanged(true); + invalidation_notifier_->OnNotificationsEnabled(); invalidation_notifier_->OnInvalidate(type_payloads); - invalidation_notifier_->OnSessionStatusChanged(false); + invalidation_notifier_->OnNotificationsDisabled( + TRANSIENT_NOTIFICATION_ERROR); + invalidation_notifier_->OnNotificationsDisabled( + NOTIFICATION_CREDENTIALS_REJECTED); } TEST_F(InvalidationNotifierTest, MigrateState) { diff --git a/sync/notifier/mock_sync_notifier_observer.h b/sync/notifier/mock_sync_notifier_observer.h index 426f065..95a5f06 100644 --- a/sync/notifier/mock_sync_notifier_observer.h +++ b/sync/notifier/mock_sync_notifier_observer.h @@ -18,10 +18,11 @@ class MockSyncNotifierObserver : public SyncNotifierObserver { MockSyncNotifierObserver(); virtual ~MockSyncNotifierObserver(); + MOCK_METHOD0(OnNotificationsEnabled, void()); + MOCK_METHOD1(OnNotificationsDisabled, void(NotificationsDisabledReason)); MOCK_METHOD2(OnIncomingNotification, void(const syncable::ModelTypePayloadMap&, IncomingNotificationSource)); - MOCK_METHOD1(OnNotificationStateChange, void(bool)); }; } // namespace sync_notifier diff --git a/sync/notifier/non_blocking_invalidation_notifier.cc b/sync/notifier/non_blocking_invalidation_notifier.cc index f9a7bd1..a92bd66 100644 --- a/sync/notifier/non_blocking_invalidation_notifier.cc +++ b/sync/notifier/non_blocking_invalidation_notifier.cc @@ -42,10 +42,12 @@ class NonBlockingInvalidationNotifier::Core // SyncNotifierObserver implementation (all called on I/O thread by // InvalidationNotifier). + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + NotificationsDisabledReason reason) OVERRIDE; virtual void OnIncomingNotification( const syncable::ModelTypePayloadMap& type_payloads, - IncomingNotificationSource source); - virtual void OnNotificationStateChange(bool notifications_enabled); + IncomingNotificationSource source) OVERRIDE; private: friend class @@ -126,22 +128,27 @@ void NonBlockingInvalidationNotifier::Core::UpdateEnabledTypes( invalidation_notifier_->UpdateEnabledTypes(enabled_types); } -void NonBlockingInvalidationNotifier::Core::OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads, - IncomingNotificationSource source) { +void NonBlockingInvalidationNotifier::Core::OnNotificationsEnabled() { DCHECK(network_task_runner_->BelongsToCurrentThread()); delegate_observer_.Call(FROM_HERE, - &SyncNotifierObserver::OnIncomingNotification, - type_payloads, - source); + &SyncNotifierObserver::OnNotificationsEnabled); } -void NonBlockingInvalidationNotifier::Core::OnNotificationStateChange( - bool notifications_enabled) { +void NonBlockingInvalidationNotifier::Core::OnNotificationsDisabled( + NotificationsDisabledReason reason) { + DCHECK(network_task_runner_->BelongsToCurrentThread()); + delegate_observer_.Call( + FROM_HERE, &SyncNotifierObserver::OnNotificationsDisabled, reason); +} + +void NonBlockingInvalidationNotifier::Core::OnIncomingNotification( + const syncable::ModelTypePayloadMap& type_payloads, + IncomingNotificationSource source) { DCHECK(network_task_runner_->BelongsToCurrentThread()); delegate_observer_.Call(FROM_HERE, - &SyncNotifierObserver::OnNotificationStateChange, - notifications_enabled); + &SyncNotifierObserver::OnIncomingNotification, + type_payloads, + source); } NonBlockingInvalidationNotifier::NonBlockingInvalidationNotifier( @@ -247,19 +254,25 @@ void NonBlockingInvalidationNotifier::SendNotification( // need to forward on the call. } -void NonBlockingInvalidationNotifier::OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads, - IncomingNotificationSource source) { +void NonBlockingInvalidationNotifier::OnNotificationsEnabled() { DCHECK(parent_task_runner_->BelongsToCurrentThread()); FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, - OnIncomingNotification(type_payloads, source)); + OnNotificationsEnabled()); +} + +void NonBlockingInvalidationNotifier::OnNotificationsDisabled( + NotificationsDisabledReason reason) { + DCHECK(parent_task_runner_->BelongsToCurrentThread()); + FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, + OnNotificationsDisabled(reason)); } -void NonBlockingInvalidationNotifier::OnNotificationStateChange( - bool notifications_enabled) { +void NonBlockingInvalidationNotifier::OnIncomingNotification( + const syncable::ModelTypePayloadMap& type_payloads, + IncomingNotificationSource source) { DCHECK(parent_task_runner_->BelongsToCurrentThread()); FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, - OnNotificationStateChange(notifications_enabled)); + OnIncomingNotification(type_payloads, source)); } } // namespace sync_notifier diff --git a/sync/notifier/non_blocking_invalidation_notifier.h b/sync/notifier/non_blocking_invalidation_notifier.h index dfcf63f..ff59c43 100644 --- a/sync/notifier/non_blocking_invalidation_notifier.h +++ b/sync/notifier/non_blocking_invalidation_notifier.h @@ -57,10 +57,12 @@ class NonBlockingInvalidationNotifier syncable::ModelTypeSet changed_types) OVERRIDE; // SyncNotifierObserver implementation. + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + NotificationsDisabledReason reason) OVERRIDE; virtual void OnIncomingNotification( const syncable::ModelTypePayloadMap& type_payloads, IncomingNotificationSource source) OVERRIDE; - virtual void OnNotificationStateChange(bool notifications_enabled) OVERRIDE; private: class Core; diff --git a/sync/notifier/non_blocking_invalidation_notifier_unittest.cc b/sync/notifier/non_blocking_invalidation_notifier_unittest.cc index 42cc5d1..831b443 100644 --- a/sync/notifier/non_blocking_invalidation_notifier_unittest.cc +++ b/sync/notifier/non_blocking_invalidation_notifier_unittest.cc @@ -73,20 +73,26 @@ TEST_F(NonBlockingInvalidationNotifierTest, Basic) { type_payloads[syncable::BOOKMARKS] = ""; type_payloads[syncable::AUTOFILL] = ""; - EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); + EXPECT_CALL(mock_observer_, OnNotificationsEnabled()); EXPECT_CALL(mock_observer_, OnIncomingNotification(type_payloads, REMOTE_NOTIFICATION)); - EXPECT_CALL(mock_observer_, OnNotificationStateChange(false)); + EXPECT_CALL(mock_observer_, + OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR)); + EXPECT_CALL(mock_observer_, + OnNotificationsDisabled(NOTIFICATION_CREDENTIALS_REJECTED)); invalidation_notifier_->SetStateDeprecated("fake_state"); invalidation_notifier_->SetUniqueId("fake_id"); invalidation_notifier_->UpdateCredentials("foo@bar.com", "fake_token"); - invalidation_notifier_->OnNotificationStateChange(true); + invalidation_notifier_->OnNotificationsEnabled(); invalidation_notifier_->OnIncomingNotification(type_payloads, REMOTE_NOTIFICATION); - invalidation_notifier_->OnNotificationStateChange(false); + invalidation_notifier_->OnNotificationsDisabled( + TRANSIENT_NOTIFICATION_ERROR); + invalidation_notifier_->OnNotificationsDisabled( + NOTIFICATION_CREDENTIALS_REJECTED); ui_loop_.RunAllPending(); } diff --git a/sync/notifier/notifications_disabled_reason.cc b/sync/notifier/notifications_disabled_reason.cc new file mode 100644 index 0000000..9503140 --- /dev/null +++ b/sync/notifier/notifications_disabled_reason.cc @@ -0,0 +1,41 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "sync/notifier/notifications_disabled_reason.h" + +#include "base/logging.h" + +namespace sync_notifier { + +const char* NotificationsDisabledReasonToString( + NotificationsDisabledReason reason) { + switch (reason) { + case NO_NOTIFICATION_ERROR: + return "NO_NOTIFICATION_ERROR"; + case TRANSIENT_NOTIFICATION_ERROR: + return "TRANSIENT_NOTIFICATION_ERROR"; + case NOTIFICATION_CREDENTIALS_REJECTED: + return "NOTIFICATION_CREDENTIALS_REJECTED"; + default: + NOTREACHED(); + return "UNKNOWN"; + } +} + +NotificationsDisabledReason FromNotifierReason( + notifier::NotificationsDisabledReason reason) { + switch (reason) { + case notifier::NO_NOTIFICATION_ERROR: + return NO_NOTIFICATION_ERROR; + case notifier::TRANSIENT_NOTIFICATION_ERROR: + return TRANSIENT_NOTIFICATION_ERROR; + case notifier::NOTIFICATION_CREDENTIALS_REJECTED: + return NOTIFICATION_CREDENTIALS_REJECTED; + default: + NOTREACHED(); + return TRANSIENT_NOTIFICATION_ERROR; + } +} + +} // sync_notifier diff --git a/sync/notifier/notifications_disabled_reason.h b/sync/notifier/notifications_disabled_reason.h new file mode 100644 index 0000000..75239cd --- /dev/null +++ b/sync/notifier/notifications_disabled_reason.h @@ -0,0 +1,33 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef SYNC_NOTIFIER_NOTIFICATIONS_DISABLED_REASON_H_ +#define SYNC_NOTIFIER_NOTIFICATIONS_DISABLED_REASON_H_ +#pragma once + +#include "jingle/notifier/listener/push_client_observer.h" + +namespace sync_notifier { + +enum NotificationsDisabledReason { + // There is an underlying transient problem (e.g., network- or + // XMPP-related). + TRANSIENT_NOTIFICATION_ERROR, + DEFAULT_NOTIFICATION_ERROR = TRANSIENT_NOTIFICATION_ERROR, + // Our credentials have been rejected. + NOTIFICATION_CREDENTIALS_REJECTED, + // No error (useful as a default value or to avoid keeping a + // separate bool for notifications enabled/disabled). + NO_NOTIFICATION_ERROR +}; + +const char* NotificationsDisabledReasonToString( + NotificationsDisabledReason reason); + +NotificationsDisabledReason FromNotifierReason( + notifier::NotificationsDisabledReason reason); + +} // namespace sync_notifier + +#endif // SYNC_NOTIFIER_NOTIFICATIONS_DISABLED_REASON_H_ diff --git a/sync/notifier/p2p_notifier.cc b/sync/notifier/p2p_notifier.cc index 2eae246..211cb2a 100644 --- a/sync/notifier/p2p_notifier.cc +++ b/sync/notifier/p2p_notifier.cc @@ -212,19 +212,28 @@ void P2PNotifier::SendNotification( SendNotificationData(notification_data); } -void P2PNotifier::OnNotificationStateChange(bool notifications_enabled) { +void P2PNotifier::OnNotificationsEnabled() { DCHECK(non_thread_safe_.CalledOnValidThread()); - bool disabled_to_enabled = notifications_enabled && !notifications_enabled_; - notifications_enabled_ = notifications_enabled; - FOR_EACH_OBSERVER(SyncNotifierObserver, observer_list_, - OnNotificationStateChange(notifications_enabled_)); - if (disabled_to_enabled) { + bool just_turned_on = (notifications_enabled_ == false); + notifications_enabled_ = true; + FOR_EACH_OBSERVER( + SyncNotifierObserver, observer_list_, + OnNotificationsEnabled()); + if (just_turned_on) { const P2PNotificationData notification_data( unique_id_, NOTIFY_SELF, enabled_types_); SendNotificationData(notification_data); } } +void P2PNotifier::OnNotificationsDisabled( + notifier::NotificationsDisabledReason reason) { + DCHECK(non_thread_safe_.CalledOnValidThread()); + FOR_EACH_OBSERVER( + SyncNotifierObserver, observer_list_, + OnNotificationsDisabled(FromNotifierReason(reason))); +} + void P2PNotifier::OnIncomingNotification( const notifier::Notification& notification) { DCHECK(non_thread_safe_.CalledOnValidThread()); @@ -234,7 +243,7 @@ void P2PNotifier::OnIncomingNotification( return; } if (!notifications_enabled_) { - DVLOG(1) << "Notifications not enabled -- not emitting notification"; + DVLOG(1) << "Notifications not on -- not emitting notification"; return; } if (notification.channel != kSyncP2PNotificationChannel) { diff --git a/sync/notifier/p2p_notifier.h b/sync/notifier/p2p_notifier.h index 341bc8b..2243ff4 100644 --- a/sync/notifier/p2p_notifier.h +++ b/sync/notifier/p2p_notifier.h @@ -18,6 +18,7 @@ #include "base/threading/non_thread_safe.h" #include "jingle/notifier/listener/push_client_observer.h" #include "sync/internal_api/public/syncable/model_type.h" +#include "sync/notifier/notifications_disabled_reason.h" #include "sync/notifier/sync_notifier.h" namespace notifier { @@ -107,7 +108,9 @@ class P2PNotifier syncable::ModelTypeSet changed_types) OVERRIDE; // PushClientObserver implementation. - virtual void OnNotificationStateChange(bool notifications_enabled) OVERRIDE; + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + notifier::NotificationsDisabledReason reason) OVERRIDE; virtual void OnIncomingNotification( const notifier::Notification& notification) OVERRIDE; @@ -127,8 +130,6 @@ class P2PNotifier std::string unique_id_; // Whether we have called UpdateCredentials() yet. bool logged_in_; - // Whether |push_client_| has notified us that notifications are - // enabled. bool notifications_enabled_; // Which set of clients should be sent notifications. P2PNotificationTarget send_notification_target_; diff --git a/sync/notifier/p2p_notifier_unittest.cc b/sync/notifier/p2p_notifier_unittest.cc index 28ec600..d1ed20d 100644 --- a/sync/notifier/p2p_notifier_unittest.cc +++ b/sync/notifier/p2p_notifier_unittest.cc @@ -145,7 +145,7 @@ TEST_F(P2PNotifierTest, NotificationsBasic) { syncable::ModelTypeSet enabled_types( syncable::BOOKMARKS, syncable::PREFERENCES); - EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); + EXPECT_CALL(mock_observer_, OnNotificationsEnabled()); EXPECT_CALL(mock_observer_, OnIncomingNotification(MakePayloadMap(enabled_types), REMOTE_NOTIFICATION)); @@ -169,7 +169,7 @@ TEST_F(P2PNotifierTest, NotificationsBasic) { p2p_notifier_.UpdateEnabledTypes(enabled_types); ReflectSentNotifications(); - fake_push_client_->SimulateNotificationStateChange(true); + fake_push_client_->EnableNotifications(); // Sent with target NOTIFY_OTHERS so should not be propagated to // |mock_observer_|. @@ -195,7 +195,7 @@ TEST_F(P2PNotifierTest, SendNotificationData) { const syncable::ModelTypePayloadMap& changed_payload_map = MakePayloadMap(changed_types); - EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); + EXPECT_CALL(mock_observer_, OnNotificationsEnabled()); EXPECT_CALL(mock_observer_, OnIncomingNotification(MakePayloadMap(enabled_types), REMOTE_NOTIFICATION)); @@ -205,7 +205,7 @@ TEST_F(P2PNotifierTest, SendNotificationData) { p2p_notifier_.UpdateEnabledTypes(enabled_types); ReflectSentNotifications(); - fake_push_client_->SimulateNotificationStateChange(true); + fake_push_client_->EnableNotifications(); ReflectSentNotifications(); diff --git a/sync/notifier/push_client_channel.cc b/sync/notifier/push_client_channel.cc index cfd3129..4ef60bc 100644 --- a/sync/notifier/push_client_channel.cc +++ b/sync/notifier/push_client_channel.cc @@ -62,12 +62,20 @@ void PushClientChannel::SetSystemResources( // Do nothing. } -void PushClientChannel::OnNotificationStateChange( - bool notifications_enabled) { +void PushClientChannel::OnNotificationsEnabled() { for (NetworkStatusReceiverList::const_iterator it = network_status_receivers_.begin(); it != network_status_receivers_.end(); ++it) { - (*it)->Run(notifications_enabled); + (*it)->Run(true); + } +} + +void PushClientChannel::OnNotificationsDisabled( + notifier::NotificationsDisabledReason reason) { + for (NetworkStatusReceiverList::const_iterator it = + network_status_receivers_.begin(); + it != network_status_receivers_.end(); ++it) { + (*it)->Run(false); } } diff --git a/sync/notifier/push_client_channel.h b/sync/notifier/push_client_channel.h index 1721def..8ad1853 100644 --- a/sync/notifier/push_client_channel.h +++ b/sync/notifier/push_client_channel.h @@ -27,6 +27,8 @@ class PushClientChannel : public invalidation::NetworkChannel, public notifier::PushClientObserver { public: + // |push_client| is guaranteed to be destroyed only when this object + // is destroyed. explicit PushClientChannel(scoped_ptr<notifier::PushClient> push_client); virtual ~PushClientChannel(); @@ -46,7 +48,9 @@ class PushClientChannel invalidation::SystemResources* resources) OVERRIDE; // notifier::PushClient::Observer implementation. - virtual void OnNotificationStateChange(bool notifications_enabled) OVERRIDE; + virtual void OnNotificationsEnabled() OVERRIDE; + virtual void OnNotificationsDisabled( + notifier::NotificationsDisabledReason reason) OVERRIDE; virtual void OnIncomingNotification( const notifier::Notification& notification) OVERRIDE; diff --git a/sync/notifier/push_client_channel_unittest.cc b/sync/notifier/push_client_channel_unittest.cc index e179109..f63cb86 100644 --- a/sync/notifier/push_client_channel_unittest.cc +++ b/sync/notifier/push_client_channel_unittest.cc @@ -151,13 +151,19 @@ TEST_F(PushClientChannelTest, SendMessage) { expected_notification)); } -// Simulate notification state changes on the push client. It should +// Simulate push client state changes on the push client. It should // propagate to the channel. -TEST_F(PushClientChannelTest, OnNotificationStateChange) { +TEST_F(PushClientChannelTest, OnPushClientStateChange) { EXPECT_FALSE(connected_); - fake_push_client_->SimulateNotificationStateChange(true); + fake_push_client_->EnableNotifications(); EXPECT_TRUE(connected_); - fake_push_client_->SimulateNotificationStateChange(false); + fake_push_client_->DisableNotifications( + notifier::TRANSIENT_NOTIFICATION_ERROR); + EXPECT_FALSE(connected_); + fake_push_client_->EnableNotifications(); + EXPECT_TRUE(connected_); + fake_push_client_->DisableNotifications( + notifier::NOTIFICATION_CREDENTIALS_REJECTED); EXPECT_FALSE(connected_); } diff --git a/sync/notifier/sync_notifier_observer.h b/sync/notifier/sync_notifier_observer.h index 0685528..4ca43dc 100644 --- a/sync/notifier/sync_notifier_observer.h +++ b/sync/notifier/sync_notifier_observer.h @@ -6,9 +6,8 @@ #define SYNC_NOTIFIER_SYNC_NOTIFIER_OBSERVER_H_ #pragma once -#include <string> - #include "sync/internal_api/public/syncable/model_type_payload_map.h" +#include "sync/notifier/notifications_disabled_reason.h" namespace sync_notifier { @@ -21,10 +20,19 @@ enum IncomingNotificationSource { class SyncNotifierObserver { public: + // Called when notifications are enabled. + virtual void OnNotificationsEnabled() = 0; + + // Called when notifications are disabled, with the reason in + // |reason|. + virtual void OnNotificationsDisabled( + NotificationsDisabledReason reason) = 0; + + // Called when a notification is received. The per-type payloads + // are in |type_payloads| and the source is in |source|. virtual void OnIncomingNotification( const syncable::ModelTypePayloadMap& type_payloads, IncomingNotificationSource source) = 0; - virtual void OnNotificationStateChange(bool notifications_enabled) = 0; protected: virtual ~SyncNotifierObserver() {} diff --git a/sync/sync.gyp b/sync/sync.gyp index 934d318..5413055 100644 --- a/sync/sync.gyp +++ b/sync/sync.gyp @@ -219,6 +219,8 @@ '../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation', ], 'sources': [ + 'notifier/notifications_disabled_reason.h', + 'notifier/notifications_disabled_reason.cc', 'notifier/sync_notifier.h', 'notifier/sync_notifier_factory.h', 'notifier/sync_notifier_factory.cc', diff --git a/sync/tools/sync_listen_notifications.cc b/sync/tools/sync_listen_notifications.cc index 52e2efa..d160285 100644 --- a/sync/tools/sync_listen_notifications.cc +++ b/sync/tools/sync_listen_notifications.cc @@ -38,6 +38,16 @@ class NotificationPrinter : public sync_notifier::SyncNotifierObserver { NotificationPrinter() {} virtual ~NotificationPrinter() {} + virtual void OnNotificationsEnabled() OVERRIDE { + LOG(INFO) << "Notifications enabled"; + } + + virtual void OnNotificationsDisabled( + sync_notifier::NotificationsDisabledReason reason) OVERRIDE { + LOG(INFO) << "Notifications disabled with reason " + << sync_notifier::NotificationsDisabledReasonToString(reason); + } + virtual void OnIncomingNotification( const syncable::ModelTypePayloadMap& type_payloads, sync_notifier::IncomingNotificationSource source) OVERRIDE { @@ -51,11 +61,6 @@ class NotificationPrinter : public sync_notifier::SyncNotifierObserver { } } - virtual void OnNotificationStateChange( - bool notifications_enabled) OVERRIDE { - LOG(INFO) << "Notifications enabled: " << notifications_enabled; - } - private: DISALLOW_COPY_AND_ASSIGN(NotificationPrinter); }; |