diff options
24 files changed, 271 insertions, 57 deletions
diff --git a/chrome/browser/sync/engine/nudge_source.cc b/chrome/browser/sync/engine/nudge_source.cc index c8942f7..b8e4bc8 100644 --- a/chrome/browser/sync/engine/nudge_source.cc +++ b/chrome/browser/sync/engine/nudge_source.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -16,6 +16,7 @@ const char* GetNudgeSourceString(NudgeSource nudge_source) { ENUM_CASE(NUDGE_SOURCE_NOTIFICATION); ENUM_CASE(NUDGE_SOURCE_LOCAL); ENUM_CASE(NUDGE_SOURCE_CONTINUATION); + ENUM_CASE(NUDGE_SOURCE_LOCAL_REFRESH); }; NOTREACHED(); return ""; diff --git a/chrome/browser/sync/engine/nudge_source.h b/chrome/browser/sync/engine/nudge_source.h index 4b92ee2..9968b8a 100644 --- a/chrome/browser/sync/engine/nudge_source.h +++ b/chrome/browser/sync/engine/nudge_source.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -16,6 +16,8 @@ enum NudgeSource { NUDGE_SOURCE_LOCAL, // A previous sync cycle did not fully complete (e.g. HTTP error). NUDGE_SOURCE_CONTINUATION, + // A local event is triggering an optimistic datatype refresh. + NUDGE_SOURCE_LOCAL_REFRESH, }; const char* GetNudgeSourceString(NudgeSource nudge_source); diff --git a/chrome/browser/sync/engine/sync_scheduler.cc b/chrome/browser/sync/engine/sync_scheduler.cc index 09e6668..13c822d 100644 --- a/chrome/browser/sync/engine/sync_scheduler.cc +++ b/chrome/browser/sync/engine/sync_scheduler.cc @@ -133,6 +133,8 @@ GetUpdatesCallerInfo::GetUpdatesSource GetUpdatesFromNudgeSource( return GetUpdatesCallerInfo::LOCAL; case NUDGE_SOURCE_CONTINUATION: return GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION; + case NUDGE_SOURCE_LOCAL_REFRESH: + return GetUpdatesCallerInfo::DATATYPE_REFRESH; case NUDGE_SOURCE_UNKNOWN: return GetUpdatesCallerInfo::UNKNOWN; default: @@ -584,6 +586,8 @@ void SyncScheduler::ScheduleNudgeImpl( pending_nudge_.reset(); } + // TODO(zea): Consider adding separate throttling/backoff for datatype + // refresh requests. ScheduleSyncSessionJob(job); } diff --git a/chrome/browser/sync/glue/session_change_processor.cc b/chrome/browser/sync/glue/session_change_processor.cc index f08b4d3..34f850b 100644 --- a/chrome/browser/sync/glue/session_change_processor.cc +++ b/chrome/browser/sync/glue/session_change_processor.cc @@ -20,6 +20,7 @@ #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h" #include "chrome/common/chrome_notification_types.h" #include "content/public/browser/navigation_controller.h" +#include "content/public/browser/navigation_entry.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/notification_details.h" #include "content/public/browser/notification_source.h" @@ -33,6 +34,10 @@ namespace browser_sync { namespace { +// The URL at which the set of synced tabs is displayed. We treat it differently +// from all other URL's as accessing it triggers a sync refresh of Sessions. +static const char kNTPOpenTabSyncURL[] = "chrome://newtab/#opentabs"; + // Extract the source SyncedTabDelegate from a NotificationSource originating // from a NavigationController, if it exists. Returns |NULL| otherwise. SyncedTabDelegate* ExtractSyncedTabDelegate( @@ -186,6 +191,25 @@ void SessionChangeProcessor::Observe( break; } + // Check if this tab should trigger a session sync refresh. By virtue of + // it being a modified tab, we know the tab is active (so we won't do + // refreshes just because the refresh page is open in a background tab). + if (!modified_tabs.empty()) { + SyncedTabDelegate* tab = modified_tabs.front(); + const content::NavigationEntry* entry = tab->GetActiveEntry(); + if (!tab->IsBeingDestroyed() && + entry && + entry->GetVirtualURL().is_valid() && + entry->GetVirtualURL().spec() == kNTPOpenTabSyncURL) { + DVLOG(1) << "Triggering sync refresh for sessions datatype."; + const syncable::ModelType type = syncable::SESSIONS; + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_SYNC_REFRESH, + content::Source<Profile>(profile_), + content::Details<const syncable::ModelType>(&type)); + } + } + // Associate tabs first so the synced session tracker is aware of them. // Note that if we fail to associate, it means something has gone wrong, // such as our local session being deleted, so we disassociate and associate diff --git a/chrome/browser/sync/glue/session_change_processor.h b/chrome/browser/sync/glue/session_change_processor.h index d6cc011..392e839 100644 --- a/chrome/browser/sync/glue/session_change_processor.h +++ b/chrome/browser/sync/glue/session_change_processor.h @@ -31,6 +31,7 @@ class SessionChangeProcessor : public ChangeProcessor, SessionChangeProcessor( UnrecoverableErrorHandler* error_handler, SessionModelAssociator* session_model_associator); + // For testing only. SessionChangeProcessor( UnrecoverableErrorHandler* error_handler, SessionModelAssociator* session_model_associator, @@ -56,12 +57,14 @@ class SessionChangeProcessor : public ChangeProcessor, private: friend class ScopedStopObserving<SessionChangeProcessor>; + void StartObserving(); void StopObserving(); + SessionModelAssociator* session_model_associator_; content::NotificationRegistrar notification_registrar_; - // Owner of the SessionService. Non-NULL iff |running()| is true. + // Profile being synced. Non-null if |running()| is true. Profile* profile_; // To bypass some checks/codepaths not applicable in tests. diff --git a/chrome/browser/sync/glue/session_model_associator.cc b/chrome/browser/sync/glue/session_model_associator.cc index 1ce4f84..f8aa61d 100644 --- a/chrome/browser/sync/glue/session_model_associator.cc +++ b/chrome/browser/sync/glue/session_model_associator.cc @@ -65,9 +65,11 @@ SessionModelAssociator::SessionModelAssociator(ProfileSyncService* sync_service) stale_session_threshold_days_(kDefaultStaleSessionThresholdDays), setup_for_test_(false), waiting_for_change_(false), - ALLOW_THIS_IN_INITIALIZER_LIST(test_weak_factory_(this)) { + ALLOW_THIS_IN_INITIALIZER_LIST(test_weak_factory_(this)), + profile_(sync_service->profile()) { DCHECK(CalledOnValidThread()); DCHECK(sync_service_); + DCHECK(profile_); } SessionModelAssociator::SessionModelAssociator(ProfileSyncService* sync_service, @@ -78,8 +80,11 @@ SessionModelAssociator::SessionModelAssociator(ProfileSyncService* sync_service, stale_session_threshold_days_(kDefaultStaleSessionThresholdDays), setup_for_test_(setup_for_test), waiting_for_change_(false), - ALLOW_THIS_IN_INITIALIZER_LIST(test_weak_factory_(this)) { + ALLOW_THIS_IN_INITIALIZER_LIST(test_weak_factory_(this)), + profile_(sync_service->profile()) { DCHECK(CalledOnValidThread()); + DCHECK(sync_service_); + DCHECK(profile_); } SessionModelAssociator::~SessionModelAssociator() { @@ -953,6 +958,15 @@ void SessionModelAssociator::TabNodePool::FreeTabNode(int64 sync_id) { tab_syncid_pool_[static_cast<size_t>(++tab_pool_fp_)] = sync_id; } +void SessionModelAssociator::AttemptSessionsDataRefresh() const { + DVLOG(1) << "Triggering sync refresh for sessions datatype."; + const syncable::ModelType type = syncable::SESSIONS; + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_SYNC_REFRESH, + content::Source<Profile>(profile_), + content::Details<const syncable::ModelType>(&type)); +} + bool SessionModelAssociator::GetLocalSession( const SyncedSession* * local_session) { DCHECK(CalledOnValidThread()); diff --git a/chrome/browser/sync/glue/session_model_associator.h b/chrome/browser/sync/glue/session_model_associator.h index 76b2345..9237626 100644 --- a/chrome/browser/sync/glue/session_model_associator.h +++ b/chrome/browser/sync/glue/session_model_associator.h @@ -153,6 +153,12 @@ class SessionModelAssociator // found for that session. bool DisassociateForeignSession(const std::string& foreign_session_tag); + // Attempts to asynchronously refresh the sessions sync data. If new data is + // received, the FOREIGN_SESSIONS_UPDATED notification is sent. No + // notification will be sent otherwise. This method is not guaranteed to + // trigger a sync cycle. + void AttemptSessionsDataRefresh() const; + // Sets |*local_session| to point to the associator's representation of the // local machine. Used primarily for testing. bool GetLocalSession(const SyncedSession* * local_session); @@ -211,8 +217,7 @@ class SessionModelAssociator WriteForeignSessionToNode); FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceSessionTest, TabNodePoolEmpty); FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceSessionTest, TabNodePoolNonEmpty); - FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceSessionTest, - ValidTabs); + FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceSessionTest, ValidTabs); FRIEND_TEST_ALL_PREFIXES(SessionModelAssociatorTest, PopulateSessionHeader); FRIEND_TEST_ALL_PREFIXES(SessionModelAssociatorTest, PopulateSessionWindow); FRIEND_TEST_ALL_PREFIXES(SessionModelAssociatorTest, PopulateSessionTab); @@ -449,6 +454,9 @@ class SessionModelAssociator bool waiting_for_change_; base::WeakPtrFactory<SessionModelAssociator> test_weak_factory_; + // Profile being synced. + const Profile* const profile_; + DISALLOW_COPY_AND_ASSIGN(SessionModelAssociator); }; diff --git a/chrome/browser/sync/glue/session_model_associator_unittest.cc b/chrome/browser/sync/glue/session_model_associator_unittest.cc index e22e3a5..fa951bd 100644 --- a/chrome/browser/sync/glue/session_model_associator_unittest.cc +++ b/chrome/browser/sync/glue/session_model_associator_unittest.cc @@ -11,7 +11,13 @@ #include "chrome/browser/sync/glue/session_model_associator.h" #include "chrome/browser/sync/glue/synced_tab_delegate.h" #include "chrome/browser/sync/protocol/session_specifics.pb.h" +#include "chrome/browser/sync/profile_sync_service_mock.h" +#include "chrome/common/chrome_notification_types.h" #include "chrome/common/url_constants.h" +#include "chrome/test/base/profile_mock.h" +#include "content/public/browser/navigation_entry.h" +#include "content/public/browser/notification_service.h" +#include "content/public/browser/notification_details.h" #include "content/public/browser/navigation_entry.h" #include "content/public/common/page_transition_types.h" #include "content/test/test_browser_thread.h" @@ -24,7 +30,25 @@ using testing::Return; namespace browser_sync { -typedef testing::Test SessionModelAssociatorTest; +class SessionModelAssociatorTest : public testing::Test { + public: + SessionModelAssociatorTest() + : ui_thread_(BrowserThread::UI, &message_loop_), + sync_service_(&profile_) {} + virtual void SetUp() OVERRIDE { + model_associator_.reset(new SessionModelAssociator(&sync_service_, true)); + } + virtual void TearDown() OVERRIDE { + model_associator_.reset(); + } + + protected: + MessageLoopForUI message_loop_; + content::TestBrowserThread ui_thread_; + NiceMock<ProfileMock> profile_; + NiceMock<ProfileSyncServiceMock> sync_service_; + scoped_ptr<SessionModelAssociator> model_associator_; +}; TEST_F(SessionModelAssociatorTest, SessionWindowHasNoTabsToSync) { SessionWindow win; @@ -208,13 +232,43 @@ class SyncedTabDelegateMock : public SyncedTabDelegate { MOCK_CONST_METHOD0(GetActiveEntry, content::NavigationEntry*()); }; +class SyncRefreshListener : public content::NotificationObserver { + public: + SyncRefreshListener() : notified_of_refresh_(false) { + registrar_.Add(this, chrome::NOTIFICATION_SYNC_REFRESH, + content::NotificationService::AllSources()); + } + + void Observe(int type, + const content::NotificationSource& source, + const content::NotificationDetails& details) { + if (type == chrome::NOTIFICATION_SYNC_REFRESH) { + notified_of_refresh_ = true; + } + } + + bool notified_of_refresh() const { return notified_of_refresh_; } + + private: + bool notified_of_refresh_; + content::NotificationRegistrar registrar_; +}; + } // namespace. -TEST_F(SessionModelAssociatorTest, ValidTabs) { - MessageLoopForUI message_loop; - content::TestBrowserThread ui_thread(BrowserThread::UI, &message_loop); - SessionModelAssociator model_associator(NULL, true); +// Test that AttemptSessionsDataRefresh() triggers the NOTIFICATION_SYNC_REFRESH +// notification. +TEST_F(SessionModelAssociatorTest, TriggerSessionRefresh) { + SyncRefreshListener refresh_listener; + EXPECT_FALSE(refresh_listener.notified_of_refresh()); + model_associator_->AttemptSessionsDataRefresh(); + EXPECT_TRUE(refresh_listener.notified_of_refresh()); +} + +// Test that we exclude tabs with only chrome:// and file:// schemed navigations +// from ShouldSyncTab(..). +TEST_F(SessionModelAssociatorTest, ValidTabs) { NiceMock<SyncedTabDelegateMock> tab_mock; // A null entry shouldn't crash. @@ -223,7 +277,7 @@ TEST_F(SessionModelAssociatorTest, ValidTabs) { Return((content::NavigationEntry *)NULL)); EXPECT_CALL(tab_mock, GetEntryCount()).WillRepeatedly(Return(1)); EXPECT_CALL(tab_mock, GetPendingEntryIndex()).WillRepeatedly(Return(-1)); - EXPECT_FALSE(model_associator.ShouldSyncTab(tab_mock)); + EXPECT_FALSE(model_associator_->ShouldSyncTab(tab_mock)); // A chrome:// entry isn't valid. scoped_ptr<content::NavigationEntry> entry( @@ -234,7 +288,7 @@ TEST_F(SessionModelAssociatorTest, ValidTabs) { EXPECT_CALL(tab_mock, GetEntryAtIndex(0)).WillRepeatedly(Return(entry.get())); EXPECT_CALL(tab_mock, GetEntryCount()).WillRepeatedly(Return(1)); EXPECT_CALL(tab_mock, GetPendingEntryIndex()).WillRepeatedly(Return(-1)); - EXPECT_FALSE(model_associator.ShouldSyncTab(tab_mock)); + EXPECT_FALSE(model_associator_->ShouldSyncTab(tab_mock)); // A file:// entry isn't valid, even in addition to another entry. scoped_ptr<content::NavigationEntry> entry2( @@ -247,7 +301,7 @@ TEST_F(SessionModelAssociatorTest, ValidTabs) { Return(entry2.get())); EXPECT_CALL(tab_mock, GetEntryCount()).WillRepeatedly(Return(2)); EXPECT_CALL(tab_mock, GetPendingEntryIndex()).WillRepeatedly(Return(-1)); - EXPECT_FALSE(model_associator.ShouldSyncTab(tab_mock)); + EXPECT_FALSE(model_associator_->ShouldSyncTab(tab_mock)); // Add a valid scheme entry to tab, making the tab valid. scoped_ptr<content::NavigationEntry> entry3( @@ -263,7 +317,7 @@ TEST_F(SessionModelAssociatorTest, ValidTabs) { Return(entry3.get())); EXPECT_CALL(tab_mock, GetEntryCount()).WillRepeatedly(Return(3)); EXPECT_CALL(tab_mock, GetPendingEntryIndex()).WillRepeatedly(Return(-1)); - EXPECT_TRUE(model_associator.ShouldSyncTab(tab_mock)); + EXPECT_TRUE(model_associator_->ShouldSyncTab(tab_mock)); } } // namespace browser_sync diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc index c7c9485..09739d2 100644 --- a/chrome/browser/sync/internal_api/sync_manager.cc +++ b/chrome/browser/sync/internal_api/sync_manager.cc @@ -95,6 +95,8 @@ typedef GoogleServiceAuthError AuthError; namespace { +// Delays for syncer nudges. +static const int kSyncRefreshDelayMsec = 500; static const int kSyncSchedulerDelayMsec = 250; #if defined(OS_CHROMEOS) @@ -277,7 +279,8 @@ class SyncManager::SyncInternal bool notifications_enabled) OVERRIDE; virtual void OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads) OVERRIDE; + const syncable::ModelTypePayloadMap& type_payloads, + sync_notifier::IncomingNotificationSource source) OVERRIDE; virtual void StoreState(const std::string& cookie) OVERRIDE; @@ -1716,10 +1719,11 @@ SyncManager::Status SyncManager::SyncInternal::GetStatus() { void SyncManager::SyncInternal::RequestNudge( const tracked_objects::Location& location) { - if (scheduler()) + if (scheduler()) { scheduler()->ScheduleNudge( TimeDelta::FromMilliseconds(0), browser_sync::NUDGE_SOURCE_LOCAL, ModelTypeSet(), location); + } } TimeDelta SyncManager::SyncInternal::GetNudgeDelayTimeDelta( @@ -2070,8 +2074,17 @@ void SyncManager::SyncInternal::UpdateNotificationInfo( } void SyncManager::SyncInternal::OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads) { - if (!type_payloads.empty()) { + const syncable::ModelTypePayloadMap& type_payloads, + sync_notifier::IncomingNotificationSource source) { + DCHECK(thread_checker_.CalledOnValidThread()); + if (source == sync_notifier::LOCAL_NOTIFICATION) { + if (scheduler()) { + scheduler()->ScheduleNudgeWithPayloads( + TimeDelta::FromMilliseconds(kSyncRefreshDelayMsec), + browser_sync::NUDGE_SOURCE_LOCAL_REFRESH, + type_payloads, FROM_HERE); + } + } else if (!type_payloads.empty()) { if (scheduler()) { scheduler()->ScheduleNudgeWithPayloads( TimeDelta::FromMilliseconds(kSyncSchedulerDelayMsec), @@ -2095,6 +2108,8 @@ void SyncManager::SyncInternal::OnIncomingNotification( syncable::ModelTypeToString(it->first); changed_types->Append(Value::CreateStringValue(model_type_str)); } + details.SetString("source", (source == sync_notifier::LOCAL_NOTIFICATION) ? + "LOCAL_NOTIFICATION" : "REMOTE_NOTIFICATION"); js_event_handler_.Call(FROM_HERE, &JsEventHandler::HandleJsEvent, "onIncomingNotification", @@ -2214,7 +2229,8 @@ void SyncManager::TriggerOnIncomingNotificationForTest( syncable::ModelTypePayloadMapFromEnumSet(model_types, std::string()); - data_->OnIncomingNotification(model_types_with_payloads); + data_->OnIncomingNotification(model_types_with_payloads, + sync_notifier::REMOTE_NOTIFICATION); } // Helper function that converts a PassphraseRequiredReason value to a string. diff --git a/chrome/browser/sync/internal_api/syncapi_unittest.cc b/chrome/browser/sync/internal_api/syncapi_unittest.cc index 8d50baf..0fb6e4a 100644 --- a/chrome/browser/sync/internal_api/syncapi_unittest.cc +++ b/chrome/browser/sync/internal_api/syncapi_unittest.cc @@ -1253,6 +1253,7 @@ TEST_F(SyncManagerTest, OnIncomingNotification) { DictionaryValue expected_details; { ListValue* model_type_list = new ListValue(); + expected_details.SetString("source", "REMOTE_NOTIFICATION"); expected_details.Set("changedTypes", model_type_list); for (syncable::ModelTypeSet::Iterator it = model_types.First(); it.Good(); it.Inc()) { @@ -1264,7 +1265,7 @@ TEST_F(SyncManagerTest, OnIncomingNotification) { EXPECT_CALL(event_handler, HandleJsEvent("onIncomingNotification", - HasDetailsAsDictionary(expected_details))); + HasDetailsAsDictionary(expected_details))); sync_manager_.TriggerOnIncomingNotificationForTest(empty_model_types); sync_manager_.TriggerOnIncomingNotificationForTest(model_types); diff --git a/chrome/browser/sync/notifier/invalidation_notifier.cc b/chrome/browser/sync/notifier/invalidation_notifier.cc index 9a69048..be7acb9 100644 --- a/chrome/browser/sync/notifier/invalidation_notifier.cc +++ b/chrome/browser/sync/notifier/invalidation_notifier.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -133,7 +133,8 @@ void InvalidationNotifier::OnInvalidate( const syncable::ModelTypePayloadMap& type_payloads) { DCHECK(non_thread_safe_.CalledOnValidThread()); FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, - OnIncomingNotification(type_payloads)); + OnIncomingNotification(type_payloads, + sync_notifier::REMOTE_NOTIFICATION)); } void InvalidationNotifier::OnSessionStatusChanged(bool has_session) { diff --git a/chrome/browser/sync/notifier/invalidation_notifier_unittest.cc b/chrome/browser/sync/notifier/invalidation_notifier_unittest.cc index 4ec2d65..93b123a 100644 --- a/chrome/browser/sync/notifier/invalidation_notifier_unittest.cc +++ b/chrome/browser/sync/notifier/invalidation_notifier_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -72,7 +72,9 @@ TEST_F(InvalidationNotifierTest, Basic) { EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); EXPECT_CALL(mock_observer_, StoreState("new_fake_state")); - EXPECT_CALL(mock_observer_, OnIncomingNotification(type_payloads)); + EXPECT_CALL(mock_observer_, + OnIncomingNotification(type_payloads, + REMOTE_NOTIFICATION)); EXPECT_CALL(mock_observer_, OnNotificationStateChange(false)); invalidation_notifier_->SetState("fake_state"); diff --git a/chrome/browser/sync/notifier/mock_sync_notifier_observer.h b/chrome/browser/sync/notifier/mock_sync_notifier_observer.h index a9e141f..8c53329 100644 --- a/chrome/browser/sync/notifier/mock_sync_notifier_observer.h +++ b/chrome/browser/sync/notifier/mock_sync_notifier_observer.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -18,8 +18,9 @@ class MockSyncNotifierObserver : public SyncNotifierObserver { MockSyncNotifierObserver(); virtual ~MockSyncNotifierObserver(); - MOCK_METHOD1(OnIncomingNotification, - void(const syncable::ModelTypePayloadMap&)); + MOCK_METHOD2(OnIncomingNotification, + void(const syncable::ModelTypePayloadMap&, + IncomingNotificationSource)); MOCK_METHOD1(OnNotificationStateChange, void(bool)); MOCK_METHOD1(StoreState, void(const std::string&)); }; diff --git a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc index 6aff10c..0f441a4 100644 --- a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc +++ b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -37,7 +37,8 @@ class NonBlockingInvalidationNotifier::Core // SyncNotifierObserver implementation (all called on I/O thread). virtual void OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads); + const syncable::ModelTypePayloadMap& type_payloads, + IncomingNotificationSource source); virtual void OnNotificationStateChange(bool notifications_enabled); virtual void StoreState(const std::string& state); @@ -119,11 +120,13 @@ void NonBlockingInvalidationNotifier::Core::UpdateEnabledTypes( } void NonBlockingInvalidationNotifier::Core::OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads) { + const syncable::ModelTypePayloadMap& type_payloads, + IncomingNotificationSource source) { DCHECK(io_message_loop_proxy_->BelongsToCurrentThread()); delegate_observer_.Call(FROM_HERE, &SyncNotifierObserver::OnIncomingNotification, - type_payloads); + type_payloads, + source); } void NonBlockingInvalidationNotifier::Core::OnNotificationStateChange( @@ -241,10 +244,11 @@ void NonBlockingInvalidationNotifier::SendNotification( } void NonBlockingInvalidationNotifier::OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads) { + const syncable::ModelTypePayloadMap& type_payloads, + IncomingNotificationSource source) { DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread()); FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, - OnIncomingNotification(type_payloads)); + OnIncomingNotification(type_payloads, source)); } void NonBlockingInvalidationNotifier::OnNotificationStateChange( diff --git a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.h b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.h index 9299b3b..7fa736d 100644 --- a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.h +++ b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. // @@ -56,7 +56,8 @@ class NonBlockingInvalidationNotifier // SyncNotifierObserver implementation. virtual void OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads) OVERRIDE; + const syncable::ModelTypePayloadMap& type_payloads, + IncomingNotificationSource source) OVERRIDE; virtual void OnNotificationStateChange(bool notifications_enabled) OVERRIDE; virtual void StoreState(const std::string& state) OVERRIDE; diff --git a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier_unittest.cc b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier_unittest.cc index 3d81713..c293bdb 100644 --- a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier_unittest.cc +++ b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -76,7 +76,9 @@ TEST_F(NonBlockingInvalidationNotifierTest, Basic) { EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); EXPECT_CALL(mock_observer_, StoreState("new_fake_state")); - EXPECT_CALL(mock_observer_, OnIncomingNotification(type_payloads)); + EXPECT_CALL(mock_observer_, + OnIncomingNotification(type_payloads, + REMOTE_NOTIFICATION)); EXPECT_CALL(mock_observer_, OnNotificationStateChange(false)); invalidation_notifier_->SetState("fake_state"); @@ -85,7 +87,8 @@ TEST_F(NonBlockingInvalidationNotifierTest, Basic) { invalidation_notifier_->OnNotificationStateChange(true); invalidation_notifier_->StoreState("new_fake_state"); - invalidation_notifier_->OnIncomingNotification(type_payloads); + invalidation_notifier_->OnIncomingNotification(type_payloads, + REMOTE_NOTIFICATION); invalidation_notifier_->OnNotificationStateChange(false); ui_loop_.RunAllPending(); diff --git a/chrome/browser/sync/notifier/p2p_notifier.cc b/chrome/browser/sync/notifier/p2p_notifier.cc index 60555a3..1cc50ca 100644 --- a/chrome/browser/sync/notifier/p2p_notifier.cc +++ b/chrome/browser/sync/notifier/p2p_notifier.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -279,7 +279,7 @@ void P2PNotifier::OnIncomingNotification( syncable::ModelTypePayloadMapFromEnumSet( notification_data.GetChangedTypes(), std::string()); FOR_EACH_OBSERVER(SyncNotifierObserver, observer_list_, - OnIncomingNotification(type_payloads)); + OnIncomingNotification(type_payloads, REMOTE_NOTIFICATION)); } void P2PNotifier::OnOutgoingNotification() {} diff --git a/chrome/browser/sync/notifier/p2p_notifier_unittest.cc b/chrome/browser/sync/notifier/p2p_notifier_unittest.cc index 51344ba..a7c8ad0 100644 --- a/chrome/browser/sync/notifier/p2p_notifier_unittest.cc +++ b/chrome/browser/sync/notifier/p2p_notifier_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -163,7 +163,8 @@ TEST_F(P2PNotifierTest, NotificationsBasic) { EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); EXPECT_CALL(mock_observer_, - OnIncomingNotification(MakePayloadMap(enabled_types))); + OnIncomingNotification(MakePayloadMap(enabled_types), + REMOTE_NOTIFICATION)); p2p_notifier_->SetUniqueId("sender"); p2p_notifier_->UpdateCredentials("foo@bar.com", "fake_token"); @@ -189,7 +190,8 @@ TEST_F(P2PNotifierTest, SendNotificationData) { EXPECT_CALL(mock_observer_, OnNotificationStateChange(true)); EXPECT_CALL(mock_observer_, - OnIncomingNotification(MakePayloadMap(enabled_types))); + OnIncomingNotification(MakePayloadMap(enabled_types), + REMOTE_NOTIFICATION)); p2p_notifier_->SetUniqueId("sender"); p2p_notifier_->UpdateCredentials("foo@bar.com", "fake_token"); @@ -201,7 +203,8 @@ TEST_F(P2PNotifierTest, SendNotificationData) { // Should be propagated. Mock::VerifyAndClearExpectations(&mock_observer_); - EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map)); + EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map, + REMOTE_NOTIFICATION)); p2p_notifier_->SendNotificationDataForTest( P2PNotificationData("sender", NOTIFY_SELF, changed_types)); @@ -221,7 +224,8 @@ TEST_F(P2PNotifierTest, SendNotificationData) { // Should be propagated. Mock::VerifyAndClearExpectations(&mock_observer_); - EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map)); + EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map, + REMOTE_NOTIFICATION)); p2p_notifier_->SendNotificationDataForTest( P2PNotificationData("sender2", NOTIFY_OTHERS, changed_types)); @@ -232,13 +236,15 @@ TEST_F(P2PNotifierTest, SendNotificationData) { // Should be propagated. Mock::VerifyAndClearExpectations(&mock_observer_); - EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map)); + EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map, + REMOTE_NOTIFICATION)); p2p_notifier_->SendNotificationDataForTest( P2PNotificationData("sender", NOTIFY_ALL, changed_types)); // Should be propagated. Mock::VerifyAndClearExpectations(&mock_observer_); - EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map)); + EXPECT_CALL(mock_observer_, OnIncomingNotification(changed_payload_map, + REMOTE_NOTIFICATION)); p2p_notifier_->SendNotificationDataForTest( P2PNotificationData("sender2", NOTIFY_ALL, changed_types)); diff --git a/chrome/browser/sync/notifier/sync_notifier_observer.h b/chrome/browser/sync/notifier/sync_notifier_observer.h index f2d7cda..103655c2 100644 --- a/chrome/browser/sync/notifier/sync_notifier_observer.h +++ b/chrome/browser/sync/notifier/sync_notifier_observer.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -12,13 +12,21 @@ namespace sync_notifier { +enum IncomingNotificationSource { + // The server is notifying us that one or more datatypes have stale data. + REMOTE_NOTIFICATION, + // A chrome datatype is requesting an optimistic refresh of its data. + LOCAL_NOTIFICATION, +}; + class SyncNotifierObserver { public: SyncNotifierObserver() {} virtual ~SyncNotifierObserver() {} virtual void OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads) = 0; + const syncable::ModelTypePayloadMap& type_payloads, + IncomingNotificationSource source) = 0; virtual void OnNotificationStateChange(bool notifications_enabled) = 0; // TODO(nileshagrawal): Find a way to hide state handling inside the diff --git a/chrome/browser/sync/profile_sync_service_session_unittest.cc b/chrome/browser/sync/profile_sync_service_session_unittest.cc index 7e082f2..4cd3e0e 100644 --- a/chrome/browser/sync/profile_sync_service_session_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_session_unittest.cc @@ -158,7 +158,8 @@ class ProfileSyncServiceSessionTest ProfileSyncServiceSessionTest() : io_thread_(BrowserThread::IO), window_bounds_(0, 1, 2, 3), - notified_of_update_(false) {} + notified_of_update_(false), + notified_of_refresh_(false) {} ProfileSyncService* sync_service() { return sync_service_.get(); } TestIdFactory* ids() { return sync_service_->id_factory(); } @@ -172,6 +173,8 @@ class ProfileSyncServiceSessionTest ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); registrar_.Add(this, chrome::NOTIFICATION_FOREIGN_SESSION_UPDATED, content::NotificationService::AllSources()); + registrar_.Add(this, chrome::NOTIFICATION_SYNC_REFRESH, + content::NotificationService::AllSources()); } void Observe(int type, @@ -181,6 +184,9 @@ class ProfileSyncServiceSessionTest case chrome::NOTIFICATION_FOREIGN_SESSION_UPDATED: notified_of_update_ = true; break; + case chrome::NOTIFICATION_SYNC_REFRESH: + notified_of_refresh_ = true; + break; default: NOTREACHED(); break; @@ -253,6 +259,7 @@ class ProfileSyncServiceSessionTest scoped_ptr<TestProfileSyncService> sync_service_; const gfx::Rect window_bounds_; bool notified_of_update_; + bool notified_of_refresh_; content::NotificationRegistrar registrar_; }; @@ -899,4 +906,47 @@ TEST_F(ProfileSyncServiceSessionTest, StaleSessionRefresh) { VerifySyncedSession(tag, session_reference, *(foreign_sessions[0])); } +// Test that tabs with nothing but "chrome://*" and "file://*" navigations are +// not be synced. +TEST_F(ProfileSyncServiceSessionTest, ValidTabs) { + CreateRootHelper create_root(this); + ASSERT_TRUE(StartSyncService(create_root.callback(), false)); + ASSERT_TRUE(create_root.success()); + + AddTab(browser(), GURL("chrome://bla1/")); + NavigateAndCommitActiveTab(GURL("chrome://bla2")); + AddTab(browser(), GURL("file://bla3/")); + AddTab(browser(), GURL("bla://bla")); + // Note: chrome://newtab has special handling which crashes in unit tests. + + // Get the tabs for this machine. Only the bla:// url should be synced. + SessionModelAssociator::TabLinksMap tab_map = model_associator_->tab_map_; + ASSERT_EQ(1U, tab_map.size()); + SessionModelAssociator::TabLinksMap::iterator iter = tab_map.begin(); + ASSERT_EQ(1, iter->second.tab()->GetEntryCount()); + ASSERT_EQ(GURL("bla://bla"), iter->second.tab()-> + GetEntryAtIndex(0)->GetVirtualURL()); +} + +// Verify that AttemptSessionsDataRefresh triggers the NOTIFICATION_SYNC_REFRESH +// notification. +// TODO(zea): Once we can have unit tests that are able to open to the NTP, +// test that the NTP/#opentabs URL triggers a refresh as well (but only when +// it is the active tab). +TEST_F(ProfileSyncServiceSessionTest, SessionsRefresh) { + CreateRootHelper create_root(this); + ASSERT_TRUE(StartSyncService(create_root.callback(), false)); + ASSERT_TRUE(create_root.success()); + + // Empty, so returns false. + std::vector<const SyncedSession*> foreign_sessions; + ASSERT_FALSE(model_associator_->GetAllForeignSessions(&foreign_sessions)); + ASSERT_FALSE(notified_of_refresh_); + model_associator_->AttemptSessionsDataRefresh(); + ASSERT_TRUE(notified_of_refresh_); + + // Nothing should have changed since we don't have unapplied data. + ASSERT_FALSE(model_associator_->GetAllForeignSessions(&foreign_sessions)); +} + } // namespace browser_sync diff --git a/chrome/browser/sync/protocol/proto_enum_conversions.cc b/chrome/browser/sync/protocol/proto_enum_conversions.cc index 7cb95ad..4832b85 100644 --- a/chrome/browser/sync/protocol/proto_enum_conversions.cc +++ b/chrome/browser/sync/protocol/proto_enum_conversions.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -71,7 +71,7 @@ const char* GetPageTransitionQualifierString( const char* GetUpdatesSourceString( sync_pb::GetUpdatesCallerInfo::GetUpdatesSource updates_source) { ASSERT_ENUM_BOUNDS(sync_pb::GetUpdatesCallerInfo, GetUpdatesSource, - UNKNOWN, RECONFIGURATION); + UNKNOWN, DATATYPE_REFRESH); switch (updates_source) { ENUM_CASE(sync_pb::GetUpdatesCallerInfo, UNKNOWN); ENUM_CASE(sync_pb::GetUpdatesCallerInfo, FIRST_UPDATE); @@ -84,6 +84,7 @@ const char* GetUpdatesSourceString( ENUM_CASE(sync_pb::GetUpdatesCallerInfo, MIGRATION); ENUM_CASE(sync_pb::GetUpdatesCallerInfo, NEW_CLIENT); ENUM_CASE(sync_pb::GetUpdatesCallerInfo, RECONFIGURATION); + ENUM_CASE(sync_pb::GetUpdatesCallerInfo, DATATYPE_REFRESH); } NOTREACHED(); return ""; diff --git a/chrome/browser/sync/protocol/sync.proto b/chrome/browser/sync/protocol/sync.proto index 1c497ef..41ef933 100644 --- a/chrome/browser/sync/protocol/sync.proto +++ b/chrome/browser/sync/protocol/sync.proto @@ -304,6 +304,9 @@ message GetUpdatesCallerInfo { // confused with FIRST_UPDATE. RECONFIGURATION = 10; // The client is in configuration mode because the // user opted to sync a different set of datatypes. + DATATYPE_REFRESH = 11; // A datatype has requested a refresh. This is + // typically used when datatype's have custom + // sync UI, e.g. sessions. } required GetUpdatesSource source = 1; diff --git a/chrome/browser/sync/tools/sync_listen_notifications.cc b/chrome/browser/sync/tools/sync_listen_notifications.cc index fa1c099..32055af 100644 --- a/chrome/browser/sync/tools/sync_listen_notifications.cc +++ b/chrome/browser/sync/tools/sync_listen_notifications.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -39,10 +39,13 @@ class NotificationPrinter : public sync_notifier::SyncNotifierObserver { virtual ~NotificationPrinter() {} virtual void OnIncomingNotification( - const syncable::ModelTypePayloadMap& type_payloads) OVERRIDE { + const syncable::ModelTypePayloadMap& type_payloads, + sync_notifier::IncomingNotificationSource source) OVERRIDE { for (syncable::ModelTypePayloadMap::const_iterator it = type_payloads.begin(); it != type_payloads.end(); ++it) { - LOG(INFO) << "Notification: type = " + LOG(INFO) << (source == sync_notifier::REMOTE_NOTIFICATION ? + "Remote" : "Local") + << " Notification: type = " << syncable::ModelTypeToString(it->first) << ", payload = " << it->second; } diff --git a/chrome/common/chrome_notification_types.h b/chrome/common/chrome_notification_types.h index 90f06c3..02c625d 100644 --- a/chrome/common/chrome_notification_types.h +++ b/chrome/common/chrome_notification_types.h @@ -698,6 +698,10 @@ enum NotificationType { // The sync service is finished the configuration process. NOTIFICATION_SYNC_CONFIGURE_DONE, + // A service is requesting a sync datatype refresh for the current profile. + // The details value is a const syncable::ModelType. + NOTIFICATION_SYNC_REFRESH, + // The session service has been saved. This notification type is only sent // if there were new SessionService commands to save, and not for no-op save // operations. |