diff options
23 files changed, 651 insertions, 158 deletions
diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc index ffce745..fc49f35 100644 --- a/chrome/browser/sync/internal_api/sync_manager.cc +++ b/chrome/browser/sync/internal_api/sync_manager.cc @@ -38,6 +38,7 @@ #include "chrome/browser/sync/syncable/directory_change_delegate.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/syncable/model_type.h" +#include "chrome/browser/sync/syncable/model_type_payload_map.h" #include "chrome/browser/sync/syncable/syncable.h" #include "chrome/browser/sync/util/cryptographer.h" #include "chrome/browser/sync/weak_handle.h" @@ -1599,7 +1600,9 @@ void SyncManager::SyncInternal::OnSyncEngineEvent( if (is_notifiable_commit) { allstatus_.IncrementNotifiableCommits(); if (sync_notifier_.get()) { - sync_notifier_->SendNotification(); + const syncable::ModelTypeSet& changed_types = + syncable::ModelTypePayloadMapToSet(event.snapshot->source.types); + sync_notifier_->SendNotification(changed_types); } else { VLOG(1) << "Not sending notification: sync_notifier_ is NULL"; } diff --git a/chrome/browser/sync/internal_api/syncapi_unittest.cc b/chrome/browser/sync/internal_api/syncapi_unittest.cc index 1dcc478..5e028ec 100644 --- a/chrome/browser/sync/internal_api/syncapi_unittest.cc +++ b/chrome/browser/sync/internal_api/syncapi_unittest.cc @@ -720,7 +720,7 @@ class SyncNotifierMock : public sync_notifier::SyncNotifier { void(const std::string&, const std::string&)); MOCK_METHOD1(UpdateEnabledTypes, void(const syncable::ModelTypeSet&)); - MOCK_METHOD0(SendNotification, void()); + MOCK_METHOD1(SendNotification, void(const syncable::ModelTypeSet&)); }; class SyncManagerTest : public testing::Test, diff --git a/chrome/browser/sync/notifier/invalidation_notifier.cc b/chrome/browser/sync/notifier/invalidation_notifier.cc index c7e68b8..17ae738 100644 --- a/chrome/browser/sync/notifier/invalidation_notifier.cc +++ b/chrome/browser/sync/notifier/invalidation_notifier.cc @@ -83,13 +83,15 @@ void InvalidationNotifier::UpdateCredentials( } void InvalidationNotifier::UpdateEnabledTypes( - const syncable::ModelTypeSet& types) { + const syncable::ModelTypeSet& enabled_types) { DCHECK(non_thread_safe_.CalledOnValidThread()); - invalidation_client_.RegisterTypes(types); + invalidation_client_.RegisterTypes(enabled_types); } -void InvalidationNotifier::SendNotification() { +void InvalidationNotifier::SendNotification( + const syncable::ModelTypeSet& changed_types) { DCHECK(non_thread_safe_.CalledOnValidThread()); + // Do nothing. } void InvalidationNotifier::OnConnect( diff --git a/chrome/browser/sync/notifier/invalidation_notifier.h b/chrome/browser/sync/notifier/invalidation_notifier.h index 78f72e4..c49eeb4 100644 --- a/chrome/browser/sync/notifier/invalidation_notifier.h +++ b/chrome/browser/sync/notifier/invalidation_notifier.h @@ -50,8 +50,9 @@ class InvalidationNotifier virtual void UpdateCredentials( const std::string& email, const std::string& token) OVERRIDE; virtual void UpdateEnabledTypes( - const syncable::ModelTypeSet& types) OVERRIDE; - virtual void SendNotification() OVERRIDE; + const syncable::ModelTypeSet& enabled_types) OVERRIDE; + virtual void SendNotification( + const syncable::ModelTypeSet& changed_types) OVERRIDE; // notifier::LoginDelegate implementation. virtual void OnConnect(base::WeakPtr<talk_base::Task> base_task) OVERRIDE; diff --git a/chrome/browser/sync/notifier/invalidation_notifier_unittest.cc b/chrome/browser/sync/notifier/invalidation_notifier_unittest.cc index 7050afa..bcd724c 100644 --- a/chrome/browser/sync/notifier/invalidation_notifier_unittest.cc +++ b/chrome/browser/sync/notifier/invalidation_notifier_unittest.cc @@ -12,6 +12,7 @@ #include "chrome/test/base/test_url_request_context_getter.h" #include "content/browser/browser_thread.h" #include "jingle/notifier/base/fake_base_task.h" +#include "jingle/notifier/base/notifier_options.h" #include "net/base/cert_verifier.h" #include "net/base/host_resolver.h" #include "testing/gmock/include/gmock/gmock.h" @@ -30,9 +31,10 @@ class InvalidationNotifierTest : public testing::Test { protected: virtual void SetUp() { - request_context_getter_ = new TestURLRequestContextGetter; notifier::NotifierOptions notifier_options; - notifier_options.request_context_getter = request_context_getter_; + // Note: URLRequestContextGetters are ref-counted. + notifier_options.request_context_getter = + new TestURLRequestContextGetter(); invalidation_notifier_.reset(new InvalidationNotifier(notifier_options, "fake_client_info")); invalidation_notifier_->AddObserver(&mock_observer_); @@ -41,11 +43,9 @@ class InvalidationNotifierTest : public testing::Test { virtual void TearDown() { invalidation_notifier_->RemoveObserver(&mock_observer_); invalidation_notifier_.reset(); - request_context_getter_ = NULL; } MessageLoop message_loop_; - scoped_refptr<net::URLRequestContextGetter> request_context_getter_; scoped_ptr<InvalidationNotifier> invalidation_notifier_; StrictMock<MockSyncNotifierObserver> mock_observer_; notifier::FakeBaseTask fake_base_task_; diff --git a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc index 575700f..e6040a3 100644 --- a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc +++ b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc @@ -32,8 +32,7 @@ class NonBlockingInvalidationNotifier::Core void SetUniqueId(const std::string& unique_id); void SetState(const std::string& state); void UpdateCredentials(const std::string& email, const std::string& token); - void UpdateEnabledTypes(const syncable::ModelTypeSet& types); - void SendNotification(); + void UpdateEnabledTypes(const syncable::ModelTypeSet& enabled_types); // SyncNotifierObserver implementation (all called on I/O thread). virtual void OnIncomingNotification( @@ -111,9 +110,9 @@ void NonBlockingInvalidationNotifier::Core::UpdateCredentials( } void NonBlockingInvalidationNotifier::Core::UpdateEnabledTypes( - const syncable::ModelTypeSet& types) { + const syncable::ModelTypeSet& enabled_types) { DCHECK(io_message_loop_proxy_->BelongsToCurrentThread()); - invalidation_notifier_->UpdateEnabledTypes(types); + invalidation_notifier_->UpdateEnabledTypes(enabled_types); } void NonBlockingInvalidationNotifier::Core::OnIncomingNotification( @@ -216,19 +215,20 @@ void NonBlockingInvalidationNotifier::UpdateCredentials( } void NonBlockingInvalidationNotifier::UpdateEnabledTypes( - const syncable::ModelTypeSet& types) { + const syncable::ModelTypeSet& enabled_types) { DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread()); if (!io_message_loop_proxy_->PostTask( FROM_HERE, NewRunnableMethod( core_.get(), &NonBlockingInvalidationNotifier::Core::UpdateEnabledTypes, - types))) { + enabled_types))) { NOTREACHED(); } } -void NonBlockingInvalidationNotifier::SendNotification() { +void NonBlockingInvalidationNotifier::SendNotification( + const syncable::ModelTypeSet& changed_types) { DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread()); // InvalidationClient doesn't implement SendNotification(), so no // need to forward on the call. diff --git a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.h b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.h index 4a49115..7e65158 100644 --- a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.h +++ b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.h @@ -39,8 +39,9 @@ class NonBlockingInvalidationNotifier : public SyncNotifier { virtual void UpdateCredentials( const std::string& email, const std::string& token) OVERRIDE; virtual void UpdateEnabledTypes( - const syncable::ModelTypeSet& types) OVERRIDE; - virtual void SendNotification() OVERRIDE; + const syncable::ModelTypeSet& enabled_types) OVERRIDE; + virtual void SendNotification( + const syncable::ModelTypeSet& changed_types) OVERRIDE; private: // The real guts of NonBlockingInvalidationNotifier, which allows this class diff --git a/chrome/browser/sync/notifier/p2p_notifier.cc b/chrome/browser/sync/notifier/p2p_notifier.cc index 197de34..8f8f4e6 100644 --- a/chrome/browser/sync/notifier/p2p_notifier.cc +++ b/chrome/browser/sync/notifier/p2p_notifier.cc @@ -4,26 +4,146 @@ #include "chrome/browser/sync/notifier/p2p_notifier.h" +#include <algorithm> + +#include "base/logging.h" #include "base/message_loop_proxy.h" +#include "base/json/json_reader.h" +#include "base/json/json_writer.h" +#include "base/values.h" #include "chrome/browser/sync/notifier/sync_notifier_observer.h" #include "chrome/browser/sync/protocol/service_constants.h" #include "chrome/browser/sync/syncable/model_type_payload_map.h" -#include "jingle/notifier/listener/mediator_thread_impl.h" -#include "jingle/notifier/listener/talk_mediator_impl.h" namespace sync_notifier { namespace { + const char kSyncNotificationChannel[] = "http://www.google.com/chrome/sync"; -const char kSyncNotificationData[] = "sync-ping-p2p"; + +const char kNotifySelf[] = "notifySelf"; +const char kNotifyOthers[] = "notifyOthers"; +const char kNotifyAll[] = "notifyAll"; + +const char kSenderIdKey[] = "senderId"; +const char kNotificationTypeKey[] = "notificationType"; +const char kChangedTypesKey[] = "changedTypes"; + } // namespace -P2PNotifier::P2PNotifier( - const notifier::NotifierOptions& notifier_options) - : talk_mediator_( - new notifier::TalkMediatorImpl( - new notifier::MediatorThreadImpl(notifier_options), - notifier_options)), +std::string P2PNotificationTargetToString(P2PNotificationTarget target) { + switch (target) { + case NOTIFY_SELF: + return kNotifySelf; + case NOTIFY_OTHERS: + return kNotifyOthers; + case NOTIFY_ALL: + return kNotifyAll; + default: + NOTREACHED(); + return ""; + } +} + +P2PNotificationTarget P2PNotificationTargetFromString( + const std::string& target_str) { + if (target_str == kNotifySelf) { + return NOTIFY_SELF; + } + if (target_str == kNotifyOthers) { + return NOTIFY_OTHERS; + } + if (target_str == kNotifyAll) { + return NOTIFY_ALL; + } + LOG(WARNING) << "Could not parse " << target_str; + return NOTIFY_SELF; +} + +P2PNotificationData::P2PNotificationData() : target_(NOTIFY_SELF) {} + +P2PNotificationData::P2PNotificationData( + const std::string& sender_id, + P2PNotificationTarget target, + const syncable::ModelTypeSet& changed_types) + : sender_id_(sender_id), + target_(target), + changed_types_(changed_types) {} + +P2PNotificationData::~P2PNotificationData() {} + +bool P2PNotificationData::IsTargeted(const std::string& id) const { + switch (target_) { + case NOTIFY_SELF: + return sender_id_ == id; + case NOTIFY_OTHERS: + return sender_id_ != id; + case NOTIFY_ALL: + return true; + default: + NOTREACHED(); + return false; + } +} + +const syncable::ModelTypeSet& P2PNotificationData::GetChangedTypes() const { + return changed_types_; +} + +bool P2PNotificationData::Equals(const P2PNotificationData& other) const { + return + (sender_id_ == other.sender_id_) && + (target_ == other.target_) && + (changed_types_ == other.changed_types_); +} + +std::string P2PNotificationData::ToString() const { + scoped_ptr<DictionaryValue> dict(new DictionaryValue()); + dict->SetString(kSenderIdKey, sender_id_); + dict->SetString(kNotificationTypeKey, + P2PNotificationTargetToString(target_)); + dict->Set(kChangedTypesKey, syncable::ModelTypeSetToValue(changed_types_)); + std::string json; + base::JSONWriter::Write(dict.get(), false /* pretty_print */, &json); + return json; +} + +bool P2PNotificationData::ResetFromString(const std::string& str) { + scoped_ptr<Value> data_value( + base::JSONReader::Read(str, false /* allow_trailing_comma */)); + if (!data_value.get()) { + LOG(WARNING) << "Could not parse " << str; + return false; + } + if (!data_value->IsType(Value::TYPE_DICTIONARY)) { + LOG(WARNING) << "Could not parse " << str << " as a dictionary"; + return false; + } + // TODO(akalin): Use Values::AsDictionary() when it becomes + // available. + DictionaryValue* data_dict = + static_cast<DictionaryValue*>(data_value.get()); + if (!data_dict->GetString(kSenderIdKey, &sender_id_)) { + LOG(WARNING) << "Could not find string value for " << kSenderIdKey; + } + std::string target_str; + if (!data_dict->GetString(kNotificationTypeKey, &target_str)) { + LOG(WARNING) << "Could not find string value for " + << kNotificationTypeKey; + } + target_ = P2PNotificationTargetFromString(target_str); + ListValue* changed_types_list = NULL; + if (!data_dict->GetList(kChangedTypesKey, &changed_types_list)) { + LOG(WARNING) << "Could not find list value for " + << kChangedTypesKey; + return false; + } + changed_types_ = syncable::ModelTypeSetFromValue(*changed_types_list); + return true; +} + +P2PNotifier::P2PNotifier(notifier::TalkMediator* talk_mediator) + : talk_mediator_(talk_mediator), logged_in_(false), notifications_enabled_(false), parent_message_loop_proxy_( @@ -56,6 +176,7 @@ void P2PNotifier::RemoveObserver(SyncNotifierObserver* observer) { void P2PNotifier::SetUniqueId(const std::string& unique_id) { DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread()); + unique_id_ = unique_id; } void P2PNotifier::SetState(const std::string& state) { @@ -86,43 +207,45 @@ void P2PNotifier::UpdateCredentials( } } -void P2PNotifier::UpdateEnabledTypes(const syncable::ModelTypeSet& types) { +void P2PNotifier::UpdateEnabledTypes( + const syncable::ModelTypeSet& enabled_types) { DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread()); - enabled_types_ = types; - MaybeEmitNotification(); + syncable::ModelTypeSet new_enabled_types; + std::set_difference(enabled_types.begin(), enabled_types.end(), + enabled_types_.begin(), enabled_types_.end(), + std::inserter(new_enabled_types, + new_enabled_types.end())); + enabled_types_ = enabled_types; + const P2PNotificationData notification_data( + unique_id_, NOTIFY_SELF, new_enabled_types); + SendNotificationData(notification_data); } -void P2PNotifier::SendNotification() { +void P2PNotifier::SendNotification( + const syncable::ModelTypeSet& changed_types) { DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread()); - VLOG(1) << "Sending XMPP notification..."; - notifier::Notification notification; - notification.channel = kSyncNotificationChannel; - notification.data = kSyncNotificationData; - talk_mediator_->SendNotification(notification); + const P2PNotificationData notification_data( + unique_id_, NOTIFY_OTHERS, changed_types); + SendNotificationData(notification_data); } void P2PNotifier::OnNotificationStateChange(bool notifications_enabled) { DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread()); + bool disabled_to_enabled = notifications_enabled && !notifications_enabled_; notifications_enabled_ = notifications_enabled; FOR_EACH_OBSERVER(SyncNotifierObserver, observer_list_, OnNotificationStateChange(notifications_enabled_)); - MaybeEmitNotification(); + if (disabled_to_enabled) { + const P2PNotificationData notification_data( + unique_id_, NOTIFY_SELF, enabled_types_); + SendNotificationData(notification_data); + } } void P2PNotifier::OnIncomingNotification( const notifier::Notification& notification) { DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread()); - VLOG(1) << "Sync received P2P notification."; - if (notification.channel != kSyncNotificationChannel) { - LOG(WARNING) << "Notification from unexpected source: " - << notification.channel; - } - MaybeEmitNotification(); -} - -void P2PNotifier::OnOutgoingNotification() {} - -void P2PNotifier::MaybeEmitNotification() { + VLOG(1) << "Received notification " << notification.ToString(); if (!logged_in_) { VLOG(1) << "Not logged in yet -- not emitting notification"; return; @@ -131,15 +254,49 @@ void P2PNotifier::MaybeEmitNotification() { VLOG(1) << "Notifications not enabled -- not emitting notification"; return; } - if (enabled_types_.empty()) { - VLOG(1) << "No enabled types -- not emitting notification"; + if (notification.channel != kSyncNotificationChannel) { + LOG(WARNING) << "Notification from unexpected source " + << notification.channel; + } + P2PNotificationData notification_data; + if (!notification_data.ResetFromString(notification.data)) { + LOG(WARNING) << "Could not parse notification data from " + << notification.data; + notification_data = + P2PNotificationData(unique_id_, NOTIFY_ALL, enabled_types_); + } + if (!notification_data.IsTargeted(unique_id_)) { + VLOG(1) << "Not a target of the notification -- " + << "not emitting notification"; + return; + } + if (notification_data.GetChangedTypes().empty()) { + VLOG(1) << "No changed types -- not emitting notification"; return; } - syncable::ModelTypePayloadMap type_payloads = + const syncable::ModelTypePayloadMap& type_payloads = syncable::ModelTypePayloadMapFromBitSet( - syncable::ModelTypeBitSetFromSet(enabled_types_), std::string()); + syncable::ModelTypeBitSetFromSet( + notification_data.GetChangedTypes()), std::string()); FOR_EACH_OBSERVER(SyncNotifierObserver, observer_list_, OnIncomingNotification(type_payloads)); } +void P2PNotifier::OnOutgoingNotification() {} + +void P2PNotifier::SendNotificationDataForTest( + const P2PNotificationData& notification_data) { + SendNotificationData(notification_data); +} + +void P2PNotifier::SendNotificationData( + const P2PNotificationData& notification_data) { + DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread()); + notifier::Notification notification; + notification.channel = kSyncNotificationChannel; + notification.data = notification_data.ToString(); + VLOG(1) << "Sending XMPP notification: " << notification.ToString(); + talk_mediator_->SendNotification(notification); +} + } // namespace sync_notifier diff --git a/chrome/browser/sync/notifier/p2p_notifier.h b/chrome/browser/sync/notifier/p2p_notifier.h index 0aa415d..90ff99a 100644 --- a/chrome/browser/sync/notifier/p2p_notifier.h +++ b/chrome/browser/sync/notifier/p2p_notifier.h @@ -23,17 +23,66 @@ class MessageLoopProxy; } -namespace notifier { -struct NotifierOptions; -} // namespace - namespace sync_notifier { +// The intended recipient(s) of a P2P notification. +enum P2PNotificationTarget { + NOTIFY_SELF, + FIRST_NOTIFICATION_TARGET = NOTIFY_SELF, + NOTIFY_OTHERS, + NOTIFY_ALL, + LAST_NOTIFICATION_TARGET = NOTIFY_ALL +}; + +std::string P2PNotificationTargetToString( + P2PNotificationTarget target); + +// If |target_str| can't be parsed, assumes NOTIFY_SELF. +P2PNotificationTarget P2PNotificationTargetFromString( + const std::string& target_str); + +// Helper notification data class that can be serialized to and +// deserialized from a string. +class P2PNotificationData { + public: + // Initializes with an empty sender ID, target set to NOTIFY_SELF, + // and empty changed types. + P2PNotificationData(); + P2PNotificationData(const std::string& sender_id, + P2PNotificationTarget target, + const syncable::ModelTypeSet& changed_types); + + ~P2PNotificationData(); + + // Returns true if the given ID is targeted by this notification. + bool IsTargeted(const std::string& id) const; + + const syncable::ModelTypeSet& GetChangedTypes() const; + + bool Equals(const P2PNotificationData& other) const; + + std::string ToString() const; + + // Returns whether parsing |str| was successful. If parsing was + // unsuccessful, the state of the notification is undefined. + bool ResetFromString(const std::string& str); + + private: + // The unique ID of the client that sent the notification. + std::string sender_id_; + // The intendent recipient(s) of the notification. + P2PNotificationTarget target_; + // The types the notification is for. + syncable::ModelTypeSet changed_types_; +}; + class P2PNotifier : public SyncNotifier, public notifier::TalkMediator::Delegate { public: - explicit P2PNotifier(const notifier::NotifierOptions& notifier_options); + // Takes ownership of |talk_mediator|, but it is guaranteed that + // |talk_mediator| is destroyed only when this object is destroyed. + explicit P2PNotifier(notifier::TalkMediator* talk_mediator); virtual ~P2PNotifier(); @@ -45,8 +94,9 @@ class P2PNotifier virtual void UpdateCredentials( const std::string& email, const std::string& token) OVERRIDE; virtual void UpdateEnabledTypes( - const syncable::ModelTypeSet& types) OVERRIDE; - virtual void SendNotification() OVERRIDE; + const syncable::ModelTypeSet& enabled_types) OVERRIDE; + virtual void SendNotification( + const syncable::ModelTypeSet& changed_types) OVERRIDE; // TalkMediator::Delegate implementation. virtual void OnNotificationStateChange(bool notifications_enabled); @@ -54,15 +104,19 @@ class P2PNotifier const notifier::Notification& notification); virtual void OnOutgoingNotification(); + // For testing. + void SendNotificationDataForTest( + const P2PNotificationData& notification_data); + private: - // Call OnIncomingNotification() on observers if we have a non-empty - // set of enabled types. - void MaybeEmitNotification(); + void SendNotificationData(const P2PNotificationData& notification_data); ObserverList<SyncNotifierObserver> observer_list_; // The actual notification listener. scoped_ptr<notifier::TalkMediator> talk_mediator_; + // Our unique ID. + std::string unique_id_; // Whether we called Login() on |talk_mediator_| yet. bool logged_in_; // Whether |talk_mediator_| has notified us that notifications are diff --git a/chrome/browser/sync/notifier/p2p_notifier_unittest.cc b/chrome/browser/sync/notifier/p2p_notifier_unittest.cc new file mode 100644 index 0000000..59f1fb5 --- /dev/null +++ b/chrome/browser/sync/notifier/p2p_notifier_unittest.cc @@ -0,0 +1,259 @@ +// Copyright (c) 2011 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 "chrome/browser/sync/notifier/p2p_notifier.h" + +#include <cstddef> + +#include "base/compiler_specific.h" +#include "base/memory/scoped_ptr.h" +#include "base/message_loop.h" +#include "chrome/browser/sync/notifier/mock_sync_notifier_observer.h" +#include "chrome/browser/sync/syncable/model_type.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace sync_notifier { + +namespace { + +using ::testing::_; +using ::testing::Mock; +using ::testing::StrictMock; + +class FakeTalkMediator : public notifier::TalkMediator { + public: + FakeTalkMediator() : delegate_(NULL) {} + virtual ~FakeTalkMediator() {} + + // notifier::TalkMediator implementation. + virtual void SetDelegate(Delegate* delegate) OVERRIDE { + delegate_ = delegate; + } + virtual void SetAuthToken(const std::string& email, + const std::string& token, + const std::string& token_service) OVERRIDE {} + virtual bool Login() OVERRIDE { + if (delegate_) { + delegate_->OnNotificationStateChange(true /* notifications_enabled */); + } + return true; + } + virtual bool Logout() OVERRIDE { + if (delegate_) { + delegate_->OnNotificationStateChange(false /* notifiations_enabled */); + } + return true; + } + virtual void SendNotification(const notifier::Notification& data) OVERRIDE { + if (delegate_) { + delegate_->OnOutgoingNotification(); + delegate_->OnIncomingNotification(data); + } + } + virtual void AddSubscription( + const notifier::Subscription& subscription) OVERRIDE {} + + private: + Delegate* delegate_; +}; + +class P2PNotifierTest : public testing::Test { + protected: + P2PNotifierTest() : talk_mediator_(NULL) {} + + virtual void SetUp() { + talk_mediator_ = new FakeTalkMediator(); + p2p_notifier_.reset(new P2PNotifier(talk_mediator_)); + p2p_notifier_->AddObserver(&mock_observer_); + } + + virtual void TearDown() { + p2p_notifier_->RemoveObserver(&mock_observer_); + p2p_notifier_.reset(); + talk_mediator_ = NULL; + } + + syncable::ModelTypePayloadMap MakePayloadMap( + const syncable::ModelTypeSet& types) { + return syncable::ModelTypePayloadMapFromBitSet( + syncable::ModelTypeBitSetFromSet(types), ""); + } + + MessageLoop message_loop_; + // Owned by |p2p_notifier_|. + notifier::TalkMediator* talk_mediator_; + scoped_ptr<P2PNotifier> p2p_notifier_; + StrictMock<MockSyncNotifierObserver> mock_observer_; +}; + +TEST_F(P2PNotifierTest, P2PNotificationTarget) { + for (int i = FIRST_NOTIFICATION_TARGET; + i <= LAST_NOTIFICATION_TARGET; ++i) { + P2PNotificationTarget target = static_cast<P2PNotificationTarget>(i); + const std::string& target_str = P2PNotificationTargetToString(target); + EXPECT_FALSE(target_str.empty()); + EXPECT_EQ(target, P2PNotificationTargetFromString(target_str)); + } + EXPECT_EQ(NOTIFY_SELF, P2PNotificationTargetFromString("unknown")); +} + +TEST_F(P2PNotifierTest, P2PNotificationDataIsTargeted) { + { + const P2PNotificationData notification_data( + "sender", NOTIFY_SELF, syncable::ModelTypeSet()); + EXPECT_TRUE(notification_data.IsTargeted("sender")); + EXPECT_FALSE(notification_data.IsTargeted("other1")); + EXPECT_FALSE(notification_data.IsTargeted("other2")); + } + { + const P2PNotificationData notification_data( + "sender", NOTIFY_OTHERS, syncable::ModelTypeSet()); + EXPECT_FALSE(notification_data.IsTargeted("sender")); + EXPECT_TRUE(notification_data.IsTargeted("other1")); + EXPECT_TRUE(notification_data.IsTargeted("other2")); + } + { + const P2PNotificationData notification_data( + "sender", NOTIFY_ALL, syncable::ModelTypeSet()); + EXPECT_TRUE(notification_data.IsTargeted("sender")); + EXPECT_TRUE(notification_data.IsTargeted("other1")); + EXPECT_TRUE(notification_data.IsTargeted("other2")); + } +} + +TEST_F(P2PNotifierTest, P2PNotificationDataDefault) { + const P2PNotificationData notification_data; + EXPECT_TRUE(notification_data.IsTargeted("")); + EXPECT_FALSE(notification_data.IsTargeted("other1")); + EXPECT_FALSE(notification_data.IsTargeted("other2")); + EXPECT_TRUE(notification_data.GetChangedTypes().empty()); + const std::string& notification_data_str = notification_data.ToString(); + EXPECT_EQ( + "{\"changedTypes\":[],\"notificationType\":\"notifySelf\"," + "\"senderId\":\"\"}", notification_data_str); + + P2PNotificationData notification_data_parsed; + EXPECT_TRUE(notification_data_parsed.ResetFromString(notification_data_str)); + EXPECT_TRUE(notification_data.Equals(notification_data_parsed)); +} + +TEST_F(P2PNotifierTest, P2PNotificationDataNonDefault) { + syncable::ModelTypeSet changed_types; + changed_types.insert(syncable::BOOKMARKS); + changed_types.insert(syncable::THEMES); + const P2PNotificationData notification_data( + "sender", NOTIFY_ALL, changed_types); + EXPECT_TRUE(notification_data.IsTargeted("sender")); + EXPECT_TRUE(notification_data.IsTargeted("other1")); + EXPECT_TRUE(notification_data.IsTargeted("other2")); + EXPECT_EQ(changed_types, notification_data.GetChangedTypes()); + const std::string& notification_data_str = notification_data.ToString(); + EXPECT_EQ( + "{\"changedTypes\":[\"Bookmarks\",\"Themes\"]," + "\"notificationType\":\"notifyAll\"," + "\"senderId\":\"sender\"}", notification_data_str); + + P2PNotificationData notification_data_parsed; + EXPECT_TRUE(notification_data_parsed.ResetFromString(notification_data_str)); + EXPECT_TRUE(notification_data.Equals(notification_data_parsed)); +} + +TEST_F(P2PNotifierTest, NotificationsBasic) { + syncable::ModelTypeSet enabled_types; + enabled_types.insert(syncable::BOOKMARKS); + enabled_types.insert(syncable::PREFERENCES); + + EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); + EXPECT_CALL(mock_observer_, + OnIncomingNotification(MakePayloadMap(enabled_types))); + + p2p_notifier_->SetUniqueId("sender"); + p2p_notifier_->UpdateCredentials("foo@bar.com", "fake_token"); + p2p_notifier_->UpdateEnabledTypes(enabled_types); + // Sent with target NOTIFY_OTHERS so should not be propagated to + // |mock_observer_|. + { + syncable::ModelTypeSet changed_types; + enabled_types.insert(syncable::THEMES); + enabled_types.insert(syncable::APPS); + p2p_notifier_->SendNotification(changed_types); + } +} + +TEST_F(P2PNotifierTest, SendNotificationData) { + syncable::ModelTypeSet enabled_types; + enabled_types.insert(syncable::BOOKMARKS); + enabled_types.insert(syncable::PREFERENCES); + + syncable::ModelTypeSet changed_types; + changed_types.insert(syncable::THEMES); + changed_types.insert(syncable::APPS); + + const syncable::ModelTypePayloadMap& changed_payload_map = + MakePayloadMap(changed_types); + + EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); + EXPECT_CALL(mock_observer_, + OnIncomingNotification(MakePayloadMap(enabled_types))); + + p2p_notifier_->SetUniqueId("sender"); + p2p_notifier_->UpdateCredentials("foo@bar.com", "fake_token"); + p2p_notifier_->UpdateEnabledTypes(enabled_types); + + // Should be dropped. + Mock::VerifyAndClearExpectations(&mock_observer_); + p2p_notifier_->SendNotificationDataForTest(P2PNotificationData()); + + // Should be propagated. + Mock::VerifyAndClearExpectations(&mock_observer_); + EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map)); + p2p_notifier_->SendNotificationDataForTest( + P2PNotificationData("sender", NOTIFY_SELF, changed_types)); + + // Should be dropped. + Mock::VerifyAndClearExpectations(&mock_observer_); + p2p_notifier_->SendNotificationDataForTest( + P2PNotificationData("sender2", NOTIFY_SELF, changed_types)); + + // Should be dropped. + Mock::VerifyAndClearExpectations(&mock_observer_); + p2p_notifier_->SendNotificationDataForTest( + P2PNotificationData("sender", NOTIFY_SELF, syncable::ModelTypeSet())); + + // Should be dropped. + p2p_notifier_->SendNotificationDataForTest( + P2PNotificationData("sender", NOTIFY_OTHERS, changed_types)); + + // Should be propagated. + Mock::VerifyAndClearExpectations(&mock_observer_); + EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map)); + p2p_notifier_->SendNotificationDataForTest( + P2PNotificationData("sender2", NOTIFY_OTHERS, changed_types)); + + // Should be dropped. + Mock::VerifyAndClearExpectations(&mock_observer_); + p2p_notifier_->SendNotificationDataForTest( + P2PNotificationData("sender2", NOTIFY_OTHERS, syncable::ModelTypeSet())); + + // Should be propagated. + Mock::VerifyAndClearExpectations(&mock_observer_); + EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map)); + p2p_notifier_->SendNotificationDataForTest( + P2PNotificationData("sender", NOTIFY_ALL, changed_types)); + + // Should be propagated. + Mock::VerifyAndClearExpectations(&mock_observer_); + EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map)); + p2p_notifier_->SendNotificationDataForTest( + P2PNotificationData("sender2", NOTIFY_ALL, changed_types)); + + // Should be dropped. + Mock::VerifyAndClearExpectations(&mock_observer_); + p2p_notifier_->SendNotificationDataForTest( + P2PNotificationData("sender2", NOTIFY_ALL, syncable::ModelTypeSet())); +} + +} // namespace + +} // namespace sync_notifier diff --git a/chrome/browser/sync/notifier/sync_notifier.h b/chrome/browser/sync/notifier/sync_notifier.h index b62542b..db1f703 100644 --- a/chrome/browser/sync/notifier/sync_notifier.h +++ b/chrome/browser/sync/notifier/sync_notifier.h @@ -39,13 +39,15 @@ class SyncNotifier { virtual void UpdateCredentials( const std::string& email, const std::string& token) = 0; - virtual void UpdateEnabledTypes(const syncable::ModelTypeSet& types) = 0; + virtual void UpdateEnabledTypes( + const syncable::ModelTypeSet& enabled_types) = 0; // This is here only to support the old p2p notification implementation, // which is still used by sync integration tests. // TODO(akalin): Remove this once we move the integration tests off p2p // notifications. - virtual void SendNotification() = 0; + virtual void SendNotification( + const syncable::ModelTypeSet& changed_types) = 0; }; } // namespace sync_notifier diff --git a/chrome/browser/sync/notifier/sync_notifier_factory.cc b/chrome/browser/sync/notifier/sync_notifier_factory.cc index d006a94..635d390 100644 --- a/chrome/browser/sync/notifier/sync_notifier_factory.cc +++ b/chrome/browser/sync/notifier/sync_notifier_factory.cc @@ -17,6 +17,8 @@ #include "content/browser/browser_thread.h" #include "jingle/notifier/base/const_communicator.h" #include "jingle/notifier/base/notifier_options.h" +#include "jingle/notifier/listener/mediator_thread_impl.h" +#include "jingle/notifier/listener/talk_mediator_impl.h" #include "net/base/host_port_pair.h" namespace sync_notifier { @@ -90,7 +92,12 @@ SyncNotifier* CreateDefaultSyncNotifier( } if (notifier_options.notification_method == notifier::NOTIFICATION_P2P) { - return new P2PNotifier(notifier_options); + notifier::TalkMediator* const talk_mediator = + new notifier::TalkMediatorImpl( + new notifier::MediatorThreadImpl(notifier_options), + notifier_options); + // Takes ownership of |talk_mediator|. + return new P2PNotifier(talk_mediator); } return new NonBlockingInvalidationNotifier(notifier_options, client_info); diff --git a/chrome/browser/sync/syncable/model_type.cc b/chrome/browser/sync/syncable/model_type.cc index 23cb864..8d4227d 100644 --- a/chrome/browser/sync/syncable/model_type.cc +++ b/chrome/browser/sync/syncable/model_type.cc @@ -314,30 +314,6 @@ std::string ModelTypeBitSetToString(const ModelTypeBitSet& model_types) { return result; } -bool ModelTypeBitSetFromString( - const std::string& model_type_bitset_string, - ModelTypeBitSet* model_types) { - DCHECK(model_types); - ModelTypeBitSet bitset; - if (!model_type_bitset_string.empty()) { - std::vector<std::string> types; - // Parse the comma-delimited list of types. - base::SplitString(model_type_bitset_string, ',', &types); - - // Walk the list of types and set them in our ModelTypeBitSet. - for (std::vector<std::string>::const_iterator it = types.begin(); - it != types.end(); - ++it) { - ModelType type = ModelTypeFromString(*it); - if (type == UNSPECIFIED) - return false; - bitset.set(type); - } - } - *model_types = bitset; - return true; -} - ModelTypeBitSet ModelTypeBitSetFromSet(const ModelTypeSet& set) { ModelTypeBitSet bitset; for (ModelTypeSet::const_iterator iter = set.begin(); iter != set.end(); @@ -375,6 +351,14 @@ ListValue* ModelTypeSetToValue(const ModelTypeSet& model_types) { return value; } +ModelTypeSet ModelTypeSetFromValue(const base::ListValue& value) { + ModelTypeSet result; + for (ListValue::const_iterator i = value.begin(); i != value.end(); ++i) { + result.insert(ModelTypeFromValue(**i)); + } + return result; +} + // TODO(zea): remove all hardcoded tags in model associators and have them use // this instead. std::string ModelTypeToRootTag(ModelType type) { diff --git a/chrome/browser/sync/syncable/model_type.h b/chrome/browser/sync/syncable/model_type.h index 7a97cdc..b4285e5 100644 --- a/chrome/browser/sync/syncable/model_type.h +++ b/chrome/browser/sync/syncable/model_type.h @@ -113,6 +113,8 @@ ModelType GetModelTypeFromExtensionFieldNumber(int field_number); // a model type. int GetExtensionFieldNumberFromModelType(ModelType model_type); +// TODO(sync): The functions below badly need some cleanup. + // Returns a string that represents the name of |model_type|. std::string ModelTypeToString(ModelType model_type); @@ -131,12 +133,6 @@ ModelType ModelTypeFromString(const std::string& model_type_string); std::string ModelTypeBitSetToString(const ModelTypeBitSet& model_types); -// Converts a string into a model type bitset. If successful, returns true. If -// failed to parse string, returns false and model_types is unspecified. -bool ModelTypeBitSetFromString( - const std::string& model_type_bitset_string, - ModelTypeBitSet* model_types); - // Convert a ModelTypeSet to a ModelTypeBitSet. ModelTypeBitSet ModelTypeBitSetFromSet(const ModelTypeSet& set); @@ -148,6 +144,8 @@ ModelTypeBitSet ModelTypeBitSetFromValue(const base::ListValue& value); // Caller takes ownership of returned list. base::ListValue* ModelTypeSetToValue(const ModelTypeSet& model_types); +ModelTypeSet ModelTypeSetFromValue(const base::ListValue& value); + // Returns a string corresponding to the syncable tag for this datatype. std::string ModelTypeToRootTag(ModelType type); diff --git a/chrome/browser/sync/syncable/model_type_payload_map.cc b/chrome/browser/sync/syncable/model_type_payload_map.cc index 545eb4ad..ea87c52 100644 --- a/chrome/browser/sync/syncable/model_type_payload_map.cc +++ b/chrome/browser/sync/syncable/model_type_payload_map.cc @@ -26,6 +26,15 @@ ModelTypePayloadMap ModelTypePayloadMapFromBitSet( return types_with_payloads; } +ModelTypeSet ModelTypePayloadMapToSet(const ModelTypePayloadMap& payload_map) { + ModelTypeSet types; + for (ModelTypePayloadMap::const_iterator it = payload_map.begin(); + it != payload_map.end(); ++it) { + types.insert(it->first); + } + return types; +} + ModelTypePayloadMap ModelTypePayloadMapFromRoutingInfo( const browser_sync::ModelSafeRoutingInfo& routes, const std::string& payload) { diff --git a/chrome/browser/sync/syncable/model_type_payload_map.h b/chrome/browser/sync/syncable/model_type_payload_map.h index 5a4d375..e4b5817 100644 --- a/chrome/browser/sync/syncable/model_type_payload_map.h +++ b/chrome/browser/sync/syncable/model_type_payload_map.h @@ -32,6 +32,8 @@ ModelTypePayloadMap ModelTypePayloadMapFromBitSet( const ModelTypeBitSet& model_types, const std::string& payload); +ModelTypeSet ModelTypePayloadMapToSet(const ModelTypePayloadMap& payload_map); + // Make a TypePayloadMap for all the enabled types in a // ModelSafeRoutingInfo using a default payload. ModelTypePayloadMap ModelTypePayloadMapFromRoutingInfo( diff --git a/chrome/browser/sync/syncable/model_type_payload_map_unittest.cc b/chrome/browser/sync/syncable/model_type_payload_map_unittest.cc index cb9324b..54e54fd 100644 --- a/chrome/browser/sync/syncable/model_type_payload_map_unittest.cc +++ b/chrome/browser/sync/syncable/model_type_payload_map_unittest.cc @@ -18,10 +18,21 @@ using test::ExpectDictStringValue; class ModelTypePayloadMapTest : public testing::Test {}; +TEST_F(ModelTypePayloadMapTest, TypePayloadMapToSet) { + ModelTypePayloadMap payloads; + payloads[BOOKMARKS] = "bookmarkpayload"; + payloads[APPS] = ""; + + ModelTypeSet types; + types.insert(BOOKMARKS); + types.insert(APPS); + EXPECT_EQ(types, ModelTypePayloadMapToSet(payloads)); +} + TEST_F(ModelTypePayloadMapTest, TypePayloadMapToValue) { ModelTypePayloadMap payloads; - payloads[syncable::BOOKMARKS] = "bookmarkpayload"; - payloads[syncable::APPS] = ""; + payloads[BOOKMARKS] = "bookmarkpayload"; + payloads[APPS] = ""; scoped_ptr<DictionaryValue> value(ModelTypePayloadMapToValue(payloads)); EXPECT_EQ(2u, value->size()); diff --git a/chrome/browser/sync/syncable/model_type_unittest.cc b/chrome/browser/sync/syncable/model_type_unittest.cc index 18965b0..5b39a3cb 100644 --- a/chrome/browser/sync/syncable/model_type_unittest.cc +++ b/chrome/browser/sync/syncable/model_type_unittest.cc @@ -63,7 +63,6 @@ TEST_F(ModelTypeTest, ModelTypeBitSetFromValue) { model_types.set(syncable::APPS); value.reset(ModelTypeBitSetToValue(model_types)); EXPECT_EQ(model_types, ModelTypeBitSetFromValue(*value)); - } TEST_F(ModelTypeTest, ModelTypeSetToValue) { @@ -80,23 +79,17 @@ TEST_F(ModelTypeTest, ModelTypeSetToValue) { EXPECT_EQ("Apps", types[1]); } -TEST_F(ModelTypeTest, ModelTypeBitSetFromString) { - ModelTypeBitSet input, output; - input.set(BOOKMARKS); - input.set(AUTOFILL); - input.set(APPS); - std::string input_string = "Bookmarks, Autofill, Apps"; - EXPECT_TRUE(ModelTypeBitSetFromString(input_string, &output)); - EXPECT_EQ(input, output); - - // Check that ModelTypeBitSetFromString(ModelTypeBitSetToString(set)) == set. - std::string set_as_string = ModelTypeBitSetToString(input); - EXPECT_TRUE(ModelTypeBitSetFromString(set_as_string, &output)); - EXPECT_EQ(input, output); - - input_string.clear(); - EXPECT_TRUE(ModelTypeBitSetFromString(input_string, &output)); - EXPECT_EQ(ModelTypeBitSet(), output); +TEST_F(ModelTypeTest, ModelTypeSetFromValue) { + // Try empty set first. + ModelTypeSet model_types; + scoped_ptr<ListValue> value(ModelTypeSetToValue(model_types)); + EXPECT_EQ(model_types, ModelTypeSetFromValue(*value)); + + // Now try with a few random types. + model_types.insert(BOOKMARKS); + model_types.insert(APPS); + value.reset(ModelTypeSetToValue(model_types)); + EXPECT_EQ(model_types, ModelTypeSetFromValue(*value)); } TEST_F(ModelTypeTest, GetAllRealModelTypes) { diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 8270b74..13f8597 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -3115,6 +3115,7 @@ 'browser/sync/notifier/chrome_system_resources_unittest.cc', 'browser/sync/notifier/invalidation_notifier_unittest.cc', 'browser/sync/notifier/non_blocking_invalidation_notifier_unittest.cc', + 'browser/sync/notifier/p2p_notifier_unittest.cc', 'browser/sync/notifier/registration_manager_unittest.cc', 'browser/sync/profile_sync_factory_mock.h', 'browser/sync/protocol/proto_enum_conversions_unittest.cc', diff --git a/chrome/test/live_sync/migration_errors_test.cc b/chrome/test/live_sync/migration_errors_test.cc index fb124a4..792e5bb 100644 --- a/chrome/test/live_sync/migration_errors_test.cc +++ b/chrome/test/live_sync/migration_errors_test.cc @@ -55,8 +55,8 @@ IN_PROC_BROWSER_TEST_F(MigrationCycleTest, PrefsOnly) { ASSERT_TRUE(GetClient(0)->AwaitSyncCycleCompletion("Migration")); } -// TODO(akalin): Fails due to http://crbug.com/92928. -IN_PROC_BROWSER_TEST_F(MigrationCycleTest, FAILS_PrefsNigori) { +// TODO(akalin): Fails (times out) due to http://crbug.com/92928. +IN_PROC_BROWSER_TEST_F(MigrationCycleTest, DISABLED_PrefsNigori) { if (!ServerSupportsErrorTriggering()) { LOG(WARNING) << "Test skipped in this server environment."; return; @@ -87,8 +87,8 @@ IN_PROC_BROWSER_TEST_F(MigrationCycleTest, FAILS_PrefsNigori) { ASSERT_TRUE(GetClient(0)->AwaitSyncCycleCompletion("Migration")); } -// TODO(akalin): Fails due to http://crbug.com/92928. -IN_PROC_BROWSER_TEST_F(MigrationCycleTest, FAILS_BookmarksPrefs) { +// TODO(akalin): Fails (times out) due to http://crbug.com/92928. +IN_PROC_BROWSER_TEST_F(MigrationCycleTest, DISABLED_BookmarksPrefs) { if (!ServerSupportsErrorTriggering()) { LOG(WARNING) << "Test skipped in this server environment."; return; @@ -133,7 +133,9 @@ class MigrationErrorsTest : public LiveSyncTest { // Easiest possible test of migration errors: triggers a server migration on // one datatype, then modifies some other datatype. -IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, MigratePrefsThenModifyBookmark) { +// TODO(akalin): Fails (times out) due to http://crbug.com/92928. +IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, + DISABLED_MigratePrefsThenModifyBookmark) { if (!ServerSupportsErrorTriggering()) { LOG(WARNING) << "Test skipped in this server environment."; return; @@ -165,8 +167,9 @@ IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, MigratePrefsThenModifyBookmark) { // Triggers a server migration on two datatypes, then makes a local // modification to one of them. +// TODO(akalin): Fails (times out) due to http://crbug.com/92928. IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, - MigratePrefsAndBookmarksThenModifyBookmark) { + DISABLED_MigratePrefsAndBookmarksThenModifyBookmark) { if (!ServerSupportsErrorTriggering()) { LOG(WARNING) << "Test skipped in this server environment."; return; @@ -199,7 +202,9 @@ IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, // Migrate every datatype in sequence; the catch being that the server // will only tell the client about the migrations one at a time. -IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, MigrationHellWithoutNigori) { +// TODO(akalin): Fails (times out) due to http://crbug.com/92928. +IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, + DISABLED_MigrationHellWithoutNigori) { if (!ServerSupportsErrorTriggering()) { LOG(WARNING) << "Test skipped in this server environment."; return; @@ -239,7 +244,9 @@ IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, MigrationHellWithoutNigori) { ASSERT_TRUE(BooleanPrefMatches(prefs::kShowHomeButton)); } -IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, MigrationHellWithNigori) { +// TODO(akalin): Fails (times out) due to http://crbug.com/92928. +IN_PROC_BROWSER_TEST_F(MigrationErrorsTest, + DISABLED_MigrationHellWithNigori) { if (!ServerSupportsErrorTriggering()) { LOG(WARNING) << "Test skipped in this server environment."; return; @@ -329,7 +336,9 @@ IN_PROC_BROWSER_TEST_F(MigrationReconfigureTest, SetSyncTabs) { ASSERT_TRUE(GetClient(0)->IsTypePreferred(syncable::SESSIONS)); } -IN_PROC_BROWSER_TEST_F(MigrationReconfigureTest, SetSyncTabsAndMigrate) { +// TODO(akalin): Fails (times out) due to http://crbug.com/92928. +IN_PROC_BROWSER_TEST_F(MigrationReconfigureTest, + DISABLED_SetSyncTabsAndMigrate) { if (!ServerSupportsErrorTriggering()) { LOG(WARNING) << "Test skipped in this server environment."; return; diff --git a/jingle/notifier/listener/push_notifications_listen_task.cc b/jingle/notifier/listener/push_notifications_listen_task.cc index 8617f84..328bbe2 100644 --- a/jingle/notifier/listener/push_notifications_listen_task.cc +++ b/jingle/notifier/listener/push_notifications_listen_task.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -29,16 +29,17 @@ PushNotificationsListenTask::~PushNotificationsListenTask() { } int PushNotificationsListenTask::ProcessStart() { - VLOG(1) << "Push notifications: Listener task started."; return STATE_RESPONSE; } int PushNotificationsListenTask::ProcessResponse() { - VLOG(1) << "Push notifications: Listener response received."; const buzz::XmlElement* stanza = NextStanza(); if (stanza == NULL) { return STATE_BLOCKED; } + + VLOG(1) << "Received stanza " << XmlElementToString(*stanza); + // The push notifications service does not need us to acknowledge receipt of // the notification to the buzz server. @@ -46,36 +47,41 @@ int PushNotificationsListenTask::ProcessResponse() { // Extract the service URL and service-specific data from the stanza. // Note that we treat the channel name as service URL. // The response stanza has the following format. - // <message from=’someservice.google.com’ to={full_jid}> - // <push xmlns=’google:push’ channel={channel name}> - // <recipient to={bare_jid}>{base-64 encoded data}</recipient> - // <data>{base-64 data}</data> + // <message from="{url or bare jid}" to={full jid}> + // <push xmlns="google:push" channel={channel name}> + // <recipient to={bare jid}>{base-64 encoded data}</recipient> + // <data>{base-64 encoded data}</data> // </push> // </message> - const buzz::XmlElement* push_element = - stanza->FirstNamed(buzz::QName(kPushNotificationsNamespace, "push")); + const buzz::QName kQnPush(kPushNotificationsNamespace, "push"); + const buzz::QName kQnChannel(buzz::STR_EMPTY, "channel"); + const buzz::QName kQnData(kPushNotificationsNamespace, "data"); + + const buzz::XmlElement* push_element = stanza->FirstNamed(kQnPush); if (push_element) { Notification notification; - notification.channel = - push_element->Attr(buzz::QName(buzz::STR_EMPTY, "channel")); - const buzz::XmlElement* data_element = push_element->FirstNamed( - buzz::QName(kPushNotificationsNamespace, "data")); + notification.channel = push_element->Attr(kQnChannel); + const buzz::XmlElement* data_element = push_element->FirstNamed(kQnData); if (data_element) { - base::Base64Decode(data_element->BodyText(), - ¬ification.data); + const std::string& base64_encoded_data = data_element->BodyText(); + if (!base::Base64Decode(base64_encoded_data, ¬ification.data)) { + LOG(WARNING) << "Could not base64-decode " << base64_encoded_data; + } + } else { + LOG(WARNING) << "No data element found in push element " + << XmlElementToString(*push_element); } + VLOG(1) << "Received notification " << notification.ToString(); delegate_->OnNotificationReceived(notification); } else { - LOG(WARNING) << - "Push notifications: No push element found in response stanza"; + LOG(WARNING) << "No push element found in stanza " + << XmlElementToString(*stanza); } return STATE_RESPONSE; } bool PushNotificationsListenTask::HandleStanza(const buzz::XmlElement* stanza) { - VLOG(1) << "Push notifications: Stanza received: " - << XmlElementToString(*stanza); if (IsValidNotification(stanza)) { QueueStanza(stanza); return true; @@ -85,13 +91,6 @@ bool PushNotificationsListenTask::HandleStanza(const buzz::XmlElement* stanza) { bool PushNotificationsListenTask::IsValidNotification( const buzz::XmlElement* stanza) { - // An update notification has the following form. - // <message from=’someservice.google.com’ to={full_jid}> - // <push xmlns=’google:push’ channel={channel name}> - // <recipient to={bare_jid}>{base-64 encoded data}</recipient> - // <data>{base-64 data}</data> - // </push> - // </message> // We don't do much validation here, just check if the stanza is a message // stanza. return (stanza->Name() == buzz::QN_MESSAGE); diff --git a/jingle/notifier/listener/push_notifications_send_update_task.cc b/jingle/notifier/listener/push_notifications_send_update_task.cc index 3e05a31..5cef1b5 100644 --- a/jingle/notifier/listener/push_notifications_send_update_task.cc +++ b/jingle/notifier/listener/push_notifications_send_update_task.cc @@ -29,9 +29,10 @@ int PushNotificationsSendUpdateTask::ProcessStart() { scoped_ptr<buzz::XmlElement> stanza( MakeUpdateMessage(notification_, GetClient()->jid().BareJid())); - VLOG(1) << "P2P: Sending notification: " << XmlElementToString(*stanza); + VLOG(1) << "Sending notification " << notification_.ToString() + << " as stanza " << XmlElementToString(*stanza); if (SendStanza(stanza.get()) != buzz::XMPP_RETURN_OK) { - LOG(WARNING) << "Could not send: " << XmlElementToString(*stanza); + LOG(WARNING) << "Could not send stanza " << XmlElementToString(*stanza); } return STATE_DONE; } @@ -42,10 +43,10 @@ buzz::XmlElement* PushNotificationsSendUpdateTask::MakeUpdateMessage( DCHECK(to_jid_bare.IsBare()); const buzz::QName kQnPush(kPushNotificationsNamespace, "push"); const buzz::QName kQnChannel(buzz::STR_EMPTY, "channel"); - const buzz::QName kQnData(buzz::STR_EMPTY, "data"); + const buzz::QName kQnData(kPushNotificationsNamespace, "data"); // Create our update stanza. The message is constructed as: - // <message from='{fullJid}' to='{bareJid}' type='headline'> + // <message from='{full jid}' to='{bare jid}' type='headline'> // <push xmlns='google:push' channel='{channel}'> // <data>{base-64 encoded data}</data> // </push> @@ -62,7 +63,7 @@ buzz::XmlElement* PushNotificationsSendUpdateTask::MakeUpdateMessage( buzz::XmlElement* data = new buzz::XmlElement(kQnData, true); std::string base64_data; if (!base::Base64Encode(notification.data, &base64_data)) { - LOG(WARNING) << "Could not encode data: " << notification.data; + LOG(WARNING) << "Could not encode data " << notification.data; } data->SetBodyText(base64_data); push->AddElement(data); diff --git a/jingle/notifier/listener/push_notifications_send_update_task_unittest.cc b/jingle/notifier/listener/push_notifications_send_update_task_unittest.cc index 811b888..3fa424ee 100644 --- a/jingle/notifier/listener/push_notifications_send_update_task_unittest.cc +++ b/jingle/notifier/listener/push_notifications_send_update_task_unittest.cc @@ -47,7 +47,7 @@ TEST_F(PushNotificationsSendUpdateTaskTest, MakeUpdateMessage) { "<cli:message to=\"%s\" type=\"headline\" " "xmlns:cli=\"jabber:client\">" "<push xmlns=\"google:push\" channel=\"%s\">" - "<data xmlns=\"\">%s</data>" + "<data xmlns=\"google:push\">%s</data>" "</push>" "</cli:message>", to_jid_bare_.Str().c_str(), notification.channel.c_str(), |