diff options
| -rw-r--r-- | chrome/browser/sync/engine/syncapi.cc | 164 | ||||
| -rw-r--r-- | chrome/browser/sync/engine/syncer_types.h | 1 | ||||
| -rw-r--r-- | chrome/browser/sync/profile_sync_service_autofill_unittest.cc | 21 | ||||
| -rw-r--r-- | chrome/browser/sync/syncable/directory_change_listener.h | 42 | ||||
| -rw-r--r-- | chrome/browser/sync/syncable/syncable.cc | 84 | ||||
| -rw-r--r-- | chrome/browser/sync/syncable/syncable.h | 48 | ||||
| -rw-r--r-- | chrome/browser/sync/util/channel.h | 145 | ||||
| -rw-r--r-- | chrome/browser/sync/util/channel_unittest.cc | 32 | ||||
| -rw-r--r-- | chrome/chrome.gyp | 2 | ||||
| -rw-r--r-- | chrome/chrome_tests.gypi | 1 | ||||
| -rw-r--r-- | tools/valgrind/tsan/suppressions.txt | 4 |
11 files changed, 170 insertions, 374 deletions
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc index d52326f..9ad111d 100644 --- a/chrome/browser/sync/engine/syncapi.cc +++ b/chrome/browser/sync/engine/syncapi.cc @@ -55,6 +55,7 @@ #include "chrome/browser/sync/sessions/sync_session.h" #include "chrome/browser/sync/sessions/sync_session_context.h" #include "chrome/browser/sync/syncable/autofill_migration.h" +#include "chrome/browser/sync/syncable/directory_change_listener.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/syncable/model_type_payload_map.h" #include "chrome/browser/sync/syncable/model_type.h" @@ -91,6 +92,8 @@ using syncable::Directory; using syncable::DirectoryManager; using syncable::Entry; using syncable::ModelTypeBitSet; +using syncable::OriginalEntries; +using syncable::WriterTag; using syncable::SPECIFICS; using sync_pb::AutofillProfileSpecifics; @@ -1170,10 +1173,10 @@ DictionaryValue* NotificationInfoToValue( class SyncManager::SyncInternal : public net::NetworkChangeNotifier::IPAddressObserver, public sync_notifier::SyncNotifierObserver, - public browser_sync::ChannelEventHandler<syncable::DirectoryChangeEvent>, public browser_sync::JsBackend, public SyncEngineEventListener, - public ServerConnectionEventListener { + public ServerConnectionEventListener, + public syncable::DirectoryChangeListener { static const int kDefaultNudgeDelayMilliseconds; static const int kPreferencesNudgeDelayMilliseconds; public: @@ -1234,18 +1237,22 @@ class SyncManager::SyncInternal // to the syncapi model. void SaveChanges(); + // DirectoryChangeListener implementation. // This listener is called upon completion of a syncable transaction, and // builds the list of sync-engine initiated changes that will be forwarded to // the SyncManager's Observers. - virtual void HandleChannelEvent(const syncable::DirectoryChangeEvent& event); - void HandleTransactionCompleteChangeEvent( - const syncable::DirectoryChangeEvent& event); - void HandleTransactionEndingChangeEvent( - const syncable::DirectoryChangeEvent& event); - void HandleCalculateChangesChangeEventFromSyncApi( - const syncable::DirectoryChangeEvent& event); - void HandleCalculateChangesChangeEventFromSyncer( - const syncable::DirectoryChangeEvent& event); + virtual void HandleTransactionCompleteChangeEvent( + const ModelTypeBitSet& models_with_changes); + virtual ModelTypeBitSet HandleTransactionEndingChangeEvent( + syncable::BaseTransaction* trans); + virtual void HandleCalculateChangesChangeEventFromSyncApi( + const OriginalEntries& originals, + const WriterTag& writer, + syncable::BaseTransaction* trans); + virtual void HandleCalculateChangesChangeEventFromSyncer( + const OriginalEntries& originals, + const WriterTag& writer, + syncable::BaseTransaction* trans); // Listens for notifications from the ServerConnectionManager void HandleServerConnectionEvent(const ServerConnectionEvent& event); @@ -1541,17 +1548,6 @@ class SyncManager::SyncInternal // TRANSACTION_COMPLETE step by HandleTransactionCompleteChangeEvent. ChangeReorderBuffer change_buffers_[syncable::MODEL_TYPE_COUNT]; - // Bit vector keeping track of which models need to have their - // OnChangesComplete observer set. - // - // Set by HandleTransactionEndingChangeEvent, cleared in - // HandleTransactionCompleteChangeEvent. - std::bitset<syncable::MODEL_TYPE_COUNT> model_has_change_; - - // The event listener hookup that is registered for HandleChangeEvent. - scoped_ptr<browser_sync::ChannelHookup<syncable::DirectoryChangeEvent> > - dir_change_hookup_; - // Event listener hookup for the ServerConnectionManager. scoped_ptr<EventListenerHookup> connection_manager_hookup_; @@ -1809,10 +1805,10 @@ void SyncManager::SyncInternal::BootstrapEncryption( } void SyncManager::SyncInternal::StartSyncing() { - // Start the syncer thread. This won't actually - // result in any syncing until at least the - // DirectoryManager broadcasts the OPENED event, - // and a valid server connection is detected. + // Start the syncer thread. This won't actually + // result in any syncing until at least the + // DirectoryManager broadcasts the OPENED event, + // and a valid server connection is detected. if (syncer_thread()) // NULL during certain unittests. syncer_thread()->Start(SyncerThread::NORMAL_MODE, NULL); } @@ -1867,7 +1863,7 @@ bool SyncManager::SyncInternal::OpenDirectory() { connection_manager()->set_client_id(lookup->cache_guid()); - dir_change_hookup_.reset(lookup->AddChangeObserver(this)); + lookup->SetChangeListener(this); return true; } @@ -2229,9 +2225,6 @@ void SyncManager::SyncInternal::Shutdown() { // handles to backing files. share_.dir_manager.reset(); - // We don't want to process any more events. - dir_change_hookup_.reset(); - core_message_loop_ = NULL; } @@ -2256,49 +2249,6 @@ void SyncManager::SyncInternal::OnIPAddressChangedImpl() { RequestNudge(FROM_HERE); } -// Listen to model changes, filter out ones initiated by the sync API, and -// saves the rest (hopefully just backend Syncer changes resulting from -// ApplyUpdates) to data_->changelist. -void SyncManager::SyncInternal::HandleChannelEvent( - const syncable::DirectoryChangeEvent& event) { - if (event.todo == syncable::DirectoryChangeEvent::TRANSACTION_COMPLETE) { - // Safe to perform slow I/O operations now, go ahead and commit. - HandleTransactionCompleteChangeEvent(event); - return; - } else if (event.todo == syncable::DirectoryChangeEvent::TRANSACTION_ENDING) { - HandleTransactionEndingChangeEvent(event); - return; - } else if (event.todo == syncable::DirectoryChangeEvent::CALCULATE_CHANGES) { - if (event.writer == syncable::SYNCAPI) { - HandleCalculateChangesChangeEventFromSyncApi(event); - return; - } - HandleCalculateChangesChangeEventFromSyncer(event); - return; - } else if (event.todo == syncable::DirectoryChangeEvent::SHUTDOWN) { - dir_change_hookup_.reset(); - } -} - -void SyncManager::SyncInternal::HandleTransactionCompleteChangeEvent( - const syncable::DirectoryChangeEvent& event) { - // This notification happens immediately after the channel mutex is released - // This allows work to be performed without holding the WriteTransaction lock - // but before the transaction is finished. - DCHECK_EQ(event.todo, syncable::DirectoryChangeEvent::TRANSACTION_COMPLETE); - if (observers_.size() <= 0) - return; - - // Call commit - for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) { - if (model_has_change_.test(i)) { - FOR_EACH_OBSERVER(SyncManager::Observer, observers_, - OnChangesComplete(syncable::ModelTypeFromInt(i))); - model_has_change_.reset(i); - } - } -} - void SyncManager::SyncInternal::OnServerConnectionEvent( const ServerConnectionEvent2& event) { ServerConnectionEvent legacy; @@ -2326,55 +2276,74 @@ void SyncManager::SyncInternal::HandleServerConnectionEvent( } } -void SyncManager::SyncInternal::HandleTransactionEndingChangeEvent( - const syncable::DirectoryChangeEvent& event) { +void SyncManager::SyncInternal::HandleTransactionCompleteChangeEvent( + const syncable::ModelTypeBitSet& models_with_changes) { + // This notification happens immediately after the transaction mutex is + // released. This allows work to be performed without blocking other threads + // from acquiring a transaction. + if (observers_.size() <= 0) + return; + + // Call commit. + for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) { + if (models_with_changes.test(i)) { + FOR_EACH_OBSERVER(SyncManager::Observer, observers_, + OnChangesComplete(syncable::ModelTypeFromInt(i))); + } + } +} + +ModelTypeBitSet SyncManager::SyncInternal::HandleTransactionEndingChangeEvent( + syncable::BaseTransaction* trans) { // This notification happens immediately before a syncable WriteTransaction // falls out of scope. It happens while the channel mutex is still held, // and while the transaction mutex is held, so it cannot be re-entrant. - DCHECK_EQ(event.todo, syncable::DirectoryChangeEvent::TRANSACTION_ENDING); if (observers_.size() <= 0 || ChangeBuffersAreEmpty()) - return; + return ModelTypeBitSet(); // This will continue the WriteTransaction using a read only wrapper. // This is the last chance for read to occur in the WriteTransaction // that's closing. This special ReadTransaction will not close the // underlying transaction. - ReadTransaction trans(GetUserShare(), event.trans); + ReadTransaction read_trans(GetUserShare(), trans); + syncable::ModelTypeBitSet models_with_changes; for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) { if (change_buffers_[i].IsEmpty()) continue; vector<ChangeRecord> ordered_changes; - change_buffers_[i].GetAllChangesInTreeOrder(&trans, &ordered_changes); + change_buffers_[i].GetAllChangesInTreeOrder(&read_trans, &ordered_changes); if (!ordered_changes.empty()) { FOR_EACH_OBSERVER( SyncManager::Observer, observers_, - OnChangesApplied(syncable::ModelTypeFromInt(i), &trans, + OnChangesApplied(syncable::ModelTypeFromInt(i), &read_trans, &ordered_changes[0], ordered_changes.size())); - model_has_change_.set(i, true); + models_with_changes.set(i, true); } change_buffers_[i].Clear(); } + return models_with_changes; } void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncApi( - const syncable::DirectoryChangeEvent& event) { - // We have been notified about a user action changing the bookmark model. - DCHECK_EQ(event.todo, syncable::DirectoryChangeEvent::CALCULATE_CHANGES); - DCHECK(event.writer == syncable::SYNCAPI || - event.writer == syncable::UNITTEST); + const OriginalEntries& originals, + const WriterTag& writer, + syncable::BaseTransaction* trans) { + // We have been notified about a user action changing a sync model. + DCHECK(writer == syncable::SYNCAPI || + writer == syncable::UNITTEST); LOG_IF(WARNING, !ChangeBuffersAreEmpty()) << "CALCULATE_CHANGES called with unapplied old changes."; bool exists_unsynced_items = false; bool only_preference_changes = true; syncable::ModelTypeBitSet model_types; - for (syncable::OriginalEntries::const_iterator i = event.originals->begin(); - i != event.originals->end() && !exists_unsynced_items; + for (syncable::OriginalEntries::const_iterator i = originals.begin(); + i != originals.end() && !exists_unsynced_items; ++i) { int64 id = i->ref(syncable::META_HANDLE); - syncable::Entry e(event.trans, syncable::GET_BY_HANDLE, id); + syncable::Entry e(trans, syncable::GET_BY_HANDLE, id); DCHECK(e.good()); syncable::ModelType model_type = e.GetModelType(); @@ -2436,20 +2405,21 @@ void SyncManager::SyncInternal::SetExtraChangeRecordData(int64 id, } void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncer( - const syncable::DirectoryChangeEvent& event) { + const OriginalEntries& originals, + const WriterTag& writer, + syncable::BaseTransaction* trans) { // We only expect one notification per sync step, so change_buffers_ should // contain no pending entries. - DCHECK_EQ(event.todo, syncable::DirectoryChangeEvent::CALCULATE_CHANGES); - DCHECK(event.writer == syncable::SYNCER || - event.writer == syncable::UNITTEST); + DCHECK(writer == syncable::SYNCER || + writer == syncable::UNITTEST); LOG_IF(WARNING, !ChangeBuffersAreEmpty()) << "CALCULATE_CHANGES called with unapplied old changes."; - Cryptographer* crypto = dir_manager()->GetCryptographer(event.trans); - for (syncable::OriginalEntries::const_iterator i = event.originals->begin(); - i != event.originals->end(); ++i) { + Cryptographer* crypto = dir_manager()->GetCryptographer(trans); + for (syncable::OriginalEntries::const_iterator i = originals.begin(); + i != originals.end(); ++i) { int64 id = i->ref(syncable::META_HANDLE); - syncable::Entry e(event.trans, syncable::GET_BY_HANDLE, id); + syncable::Entry e(trans, syncable::GET_BY_HANDLE, id); bool existed_before = !i->ref(syncable::IS_DEL); bool exists_now = e.good() && !e.Get(syncable::IS_DEL); DCHECK(e.good()); diff --git a/chrome/browser/sync/engine/syncer_types.h b/chrome/browser/sync/engine/syncer_types.h index bcb59e0..57e8855 100644 --- a/chrome/browser/sync/engine/syncer_types.h +++ b/chrome/browser/sync/engine/syncer_types.h @@ -10,7 +10,6 @@ #include <vector> #include "base/observer_list.h" -#include "chrome/browser/sync/util/channel.h" #include "chrome/browser/sync/syncable/model_type.h" namespace syncable { diff --git a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc index 095bb6e..4437e88 100644 --- a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc @@ -65,7 +65,6 @@ using syncable::CREATE_NEW_UPDATE_ITEM; using syncable::AUTOFILL; using syncable::BASE_VERSION; using syncable::CREATE; -using syncable::DirectoryChangeEvent; using syncable::GET_BY_SERVER_TAG; using syncable::INVALID; using syncable::MutableEntry; @@ -227,7 +226,7 @@ class AutofillProfileFactory : public AbstractAutofillFactory { ProfileSyncService* service) { return new AutofillProfileDataTypeController(factory, profile); - } + } void SetExpectation(ProfileSyncFactoryMock* factory, ProfileSyncService* service, @@ -299,14 +298,14 @@ class ProfileSyncServiceAutofillTest : public AbstractProfileSyncServiceTest { factory->CreateDataTypeController(&factory_, &profile_, service_.get()); - SyncBackendHostForProfileSyncTest:: - SetDefaultExpectationsForWorkerCreation(&profile_); + SyncBackendHostForProfileSyncTest:: + SetDefaultExpectationsForWorkerCreation(&profile_); - factory->SetExpectation(&factory_, - service_.get(), - web_database_.get(), - personal_data_manager_.get(), - data_type_controller); + factory->SetExpectation(&factory_, + service_.get(), + web_database_.get(), + personal_data_manager_.get(), + data_type_controller); EXPECT_CALL(factory_, CreateDataTypeManager(_, _)). WillOnce(ReturnNewDataTypeManager()); @@ -498,12 +497,12 @@ class WriteTransactionTest: public WriteTransaction { : WriteTransaction(directory, writer, source_file, line), wait_for_syncapi_(wait_for_syncapi) { } - virtual void NotifyTransactionComplete() { + virtual void NotifyTransactionComplete(syncable::ModelTypeBitSet types) { // This is where we differ. Force a thread change here, giving another // thread a chance to create a WriteTransaction (*wait_for_syncapi_)->Wait(); - WriteTransaction::NotifyTransactionComplete(); + WriteTransaction::NotifyTransactionComplete(types); } private: diff --git a/chrome/browser/sync/syncable/directory_change_listener.h b/chrome/browser/sync/syncable/directory_change_listener.h new file mode 100644 index 0000000..0867cb7 --- /dev/null +++ b/chrome/browser/sync/syncable/directory_change_listener.h @@ -0,0 +1,42 @@ +// 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. + +#ifndef CHROME_BROWSER_SYNC_SYNCABLE_DIRECTORY_CHANGE_LISTENER_H_ +#define CHROME_BROWSER_SYNC_SYNCABLE_DIRECTORY_CHANGE_LISTENER_H_ +#pragma once + +#include "chrome/browser/sync/syncable/model_type.h" +#include "chrome/browser/sync/syncable/syncable.h" + +namespace syncable { + +// This is an interface for listening to directory change events, triggered by +// the releasing of the syncable transaction. The listener performs work to +// 1. Calculate changes, depending on the source of the transaction +// (HandleCalculateChangesChangeEventFromSyncer/Syncapi). +// 2. Perform final work while the transaction is held +// (HandleTransactionEndingChangeEvent). +// 3. Perform any work that should be done after the transaction is released. +// (HandleTransactionCompleteChangeEvent). +class DirectoryChangeListener { + public: + virtual void HandleCalculateChangesChangeEventFromSyncApi( + const OriginalEntries& originals, + const WriterTag& writer, + BaseTransaction* trans) = 0; + virtual void HandleCalculateChangesChangeEventFromSyncer( + const OriginalEntries& originals, + const WriterTag& writer, + BaseTransaction* trans) = 0; + virtual ModelTypeBitSet HandleTransactionEndingChangeEvent( + BaseTransaction* trans) = 0; + virtual void HandleTransactionCompleteChangeEvent( + const ModelTypeBitSet& models_with_changes) = 0; + protected: + virtual ~DirectoryChangeListener() {} +}; + +} // namespace syncable + +#endif // CHROME_BROWSER_SYNC_SYNCABLE_DIRECTORY_CHANGE_LISTENER_H_ diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc index 81b3ad5..0674457 100644 --- a/chrome/browser/sync/syncable/syncable.cc +++ b/chrome/browser/sync/syncable/syncable.cc @@ -42,6 +42,7 @@ #include "chrome/browser/sync/protocol/proto_value_conversions.h" #include "chrome/browser/sync/protocol/service_constants.h" #include "chrome/browser/sync/syncable/directory_backing_store.h" +#include "chrome/browser/sync/syncable/directory_change_listener.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/syncable/model_type.h" #include "chrome/browser/sync/syncable/syncable-inl.h" @@ -269,9 +270,6 @@ DictionaryValue* EntryKernel::ToValue() const { /////////////////////////////////////////////////////////////////////////// // Directory -static const DirectoryChangeEvent kShutdownChangesEvent = - { DirectoryChangeEvent::SHUTDOWN, 0, 0 }; - void Directory::init_kernel(const std::string& name) { DCHECK(kernel_ == NULL); kernel_ = new Kernel(FilePath(), name, KernelLoadInfo()); @@ -318,6 +316,7 @@ Directory::Kernel::Kernel(const FilePath& db_path, dirty_metahandles(new MetahandleSet), metahandles_to_purge(new MetahandleSet), channel(new Directory::Channel(syncable::DIRECTORY_DESTROYED)), + change_listener_(NULL), info_status(Directory::KERNEL_SHARE_INFO_VALID), persisted_info(info.kernel_info), cache_guid(info.cache_guid), @@ -336,7 +335,6 @@ void Directory::Kernel::Release() { Directory::Kernel::~Kernel() { CHECK_EQ(0, refcount); delete channel; - changes_channel.Notify(kShutdownChangesEvent); delete unsynced_metahandles; delete unapplied_update_metahandles; delete dirty_metahandles; @@ -1088,9 +1086,9 @@ void Directory::CheckTreeInvariants(syncable::BaseTransaction* trans, } } -browser_sync::ChannelHookup<DirectoryChangeEvent>* Directory::AddChangeObserver( - browser_sync::ChannelEventHandler<DirectoryChangeEvent>* observer) { - return kernel_->changes_channel.AddObserver(observer); +void Directory::SetChangeListener(DirectoryChangeListener* listener) { + DCHECK(!kernel_->change_listener_); + kernel_->change_listener_ = listener; } /////////////////////////////////////////////////////////////////////////////// @@ -1138,23 +1136,22 @@ BaseTransaction::BaseTransaction(Directory* directory) BaseTransaction::~BaseTransaction() {} -void BaseTransaction::UnlockAndLog(OriginalEntries* originals_arg) { - // Triggers the CALCULATE_CHANGES and TRANSACTION_ENDING events while - // holding dir_kernel_'s transaction_mutex and changes_channel mutex. - // Releases all mutexes upon completion. - if (!NotifyTransactionChangingAndEnding(originals_arg)) { +void BaseTransaction::UnlockAndLog(OriginalEntries* entries) { + // Work while trasnaction mutex is held + ModelTypeBitSet models_with_changes; + if (!NotifyTransactionChangingAndEnding(entries, &models_with_changes)) return; - } - // Triggers the TRANSACTION_COMPLETE event (and does not hold any mutexes). - NotifyTransactionComplete(); + // Work after mutex is relased. + NotifyTransactionComplete(models_with_changes); } bool BaseTransaction::NotifyTransactionChangingAndEnding( - OriginalEntries* originals_arg) { + OriginalEntries* entries, + ModelTypeBitSet* models_with_changes) { dirkernel_->transaction_mutex.AssertAcquired(); - scoped_ptr<OriginalEntries> originals(originals_arg); + scoped_ptr<OriginalEntries> originals(entries); const base::TimeDelta elapsed = base::TimeTicks::Now() - time_acquired_; if (LOG_IS_ON(INFO) && (1 <= logging::GetVlogLevelHelper( @@ -1165,43 +1162,40 @@ bool BaseTransaction::NotifyTransactionChangingAndEnding( << " seconds."; } - if (NULL == originals.get() || originals->empty()) { + if (NULL == originals.get() || originals->empty() || + !dirkernel_->change_listener_) { dirkernel_->transaction_mutex.Release(); return false; } - - { - // Scoped_lock is only active through the calculate_changes and - // transaction_ending events. - base::AutoLock scoped_lock(dirkernel_->changes_channel_mutex); - - // Tell listeners to calculate changes while we still have the mutex. - DirectoryChangeEvent event = { DirectoryChangeEvent::CALCULATE_CHANGES, - originals.get(), this, writer_ }; - dirkernel_->changes_channel.Notify(event); - - // Necessary for reads to be performed prior to transaction mutex release. - // Allows the listener to use the current transaction to perform reads. - DirectoryChangeEvent ending_event = - { DirectoryChangeEvent::TRANSACTION_ENDING, - NULL, this, INVALID }; - dirkernel_->changes_channel.Notify(ending_event); - - dirkernel_->transaction_mutex.Release(); + if (writer_ == syncable::SYNCAPI) { + dirkernel_->change_listener_->HandleCalculateChangesChangeEventFromSyncApi( + *originals.get(), + writer_, + this); + } else { + dirkernel_->change_listener_->HandleCalculateChangesChangeEventFromSyncer( + *originals.get(), + writer_, + this); } + *models_with_changes = dirkernel_->change_listener_-> + HandleTransactionEndingChangeEvent(this); + + // Release the transaction. Note, once the transaction is released this thread + // can be interrupted by another that was waiting for the transaction, + // resulting in this code possibly being interrupted with another thread + // performing following the same code path. From this point foward, only + // local state can be touched. + dirkernel_->transaction_mutex.Release(); return true; } -void BaseTransaction::NotifyTransactionComplete() { - // Transaction is no longer holding any locks/mutexes, notify that we're - // complete (and commit any outstanding changes that should not be performed - // while holding mutexes). - DirectoryChangeEvent complete_event = - { DirectoryChangeEvent::TRANSACTION_COMPLETE, - NULL, NULL, INVALID }; - dirkernel_->changes_channel.Notify(complete_event); +void BaseTransaction::NotifyTransactionComplete( + ModelTypeBitSet models_with_changes) { + dirkernel_->change_listener_->HandleTransactionCompleteChangeEvent( + models_with_changes); } ReadTransaction::ReadTransaction(Directory* directory, const char* file, diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h index 55f76bb..c97da92 100644 --- a/chrome/browser/sync/syncable/syncable.h +++ b/chrome/browser/sync/syncable/syncable.h @@ -27,7 +27,6 @@ #include "chrome/browser/sync/syncable/directory_event.h" #include "chrome/browser/sync/syncable/syncable_id.h" #include "chrome/browser/sync/syncable/model_type.h" -#include "chrome/browser/sync/util/channel.h" #include "chrome/browser/sync/util/dbgq.h" #include "chrome/common/deprecated/event_sys.h" @@ -41,6 +40,7 @@ class ReadNode; } namespace syncable { +class DirectoryChangeListener; class Entry; std::ostream& operator<<(std::ostream& s, const Entry& e); @@ -633,35 +633,6 @@ enum WriterTag { SYNCAPI }; -// A separate Event type and channel for very frequent changes, caused -// by anything, not just the user. -struct DirectoryChangeEvent { - enum { - // Means listener should go through list of original entries and - // calculate what it needs to notify. It should *not* call any - // callbacks or attempt to lock anything because a - // WriteTransaction is being held until the listener returns. - CALCULATE_CHANGES, - // Means the WriteTransaction is ending, and this is the absolute - // last chance to perform any read operations in the current transaction. - // It is not recommended that the listener perform any writes. - TRANSACTION_ENDING, - // Means the WriteTransaction has been released and the listener - // can now take action on the changes it calculated. - TRANSACTION_COMPLETE, - // Channel is closing. - SHUTDOWN - } todo; - // These members are only valid for CALCULATE_CHANGES. - const OriginalEntries* originals; - BaseTransaction* trans; // This is valid also for TRANSACTION_ENDING - WriterTag writer; - typedef DirectoryChangeEvent EventType; - static inline bool IsChannelShutdownEvent(const EventType& e) { - return SHUTDOWN == e.todo; - } -}; - // The name Directory in this case means the entire directory // structure within a single user account. // @@ -822,8 +793,7 @@ class Directory { // Unique to each account / client pair. std::string cache_guid() const; - browser_sync::ChannelHookup<DirectoryChangeEvent>* AddChangeObserver( - browser_sync::ChannelEventHandler<DirectoryChangeEvent>* observer); + void SetChangeListener(DirectoryChangeListener* listener); protected: // for friends, mainly used by Entry constructors virtual EntryKernel* GetEntryByHandle(int64 handle); @@ -1045,12 +1015,10 @@ class Directory { // TODO(ncarter): Figure out what the hell this is, and comment it. Channel* const channel; - // The changes channel mutex is explicit because it must be locked - // while holding the transaction mutex and released after - // releasing the transaction mutex. - browser_sync::Channel<DirectoryChangeEvent> changes_channel; + // The listener for directory change events, triggered when the transaction + // is ending. + DirectoryChangeListener* change_listener_; - base::Lock changes_channel_mutex; KernelShareInfoStatus info_status; // These 3 members are backed in the share_info table, and @@ -1127,8 +1095,10 @@ class BaseTransaction { explicit BaseTransaction(Directory* directory); void UnlockAndLog(OriginalEntries* entries); - bool NotifyTransactionChangingAndEnding(OriginalEntries* entries); - virtual void NotifyTransactionComplete(); + virtual bool NotifyTransactionChangingAndEnding( + OriginalEntries* entries, + ModelTypeBitSet* models_with_changes); + virtual void NotifyTransactionComplete(ModelTypeBitSet models_with_changes); Directory* const directory_; Directory::Kernel* const dirkernel_; // for brevity diff --git a/chrome/browser/sync/util/channel.h b/chrome/browser/sync/util/channel.h deleted file mode 100644 index 839d653..0000000 --- a/chrome/browser/sync/util/channel.h +++ /dev/null @@ -1,145 +0,0 @@ -// Copyright (c) 2010 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 CHROME_BROWSER_SYNC_UTIL_CHANNEL_H_ -#define CHROME_BROWSER_SYNC_UTIL_CHANNEL_H_ -#pragma once - -/////////////////////////////////////////////////////////////////////////////// -// -// OVERVIEW: -// -// A threadsafe container for a list of observers. Observers are able to -// remove themselves during iteration, and can be added on any thread. This -// allows observers to safely remove themselves during notifications. It -// also provides a handler when an observer is added that will remove the -// observer on destruction. -// -// It is expected that all observers are removed before destruction. -// The channel owner should notify all observers to disconnect on shutdown if -// needed to ensure this. -// -// TYPICAL USAGE: -// -// class MyWidget { -// public: -// ... -// -// class Observer : public ChannelEventHandler<FooEvent> { -// public: -// virtual void HandleChannelEvent(const FooEvent& w) = 0; -// }; -// -// ChannelHookup<MyEvent>* AddObserver(Observer* obs) { -// return channel_.AddObserver(obs); -// } -// -// void RemoveObserver(Observer* obs) { -// channel_.RemoveObserver(obs); -// } -// -// void NotifyFoo(FooEvent& event) { -// channel_.Notify(event); -// } -// -// private: -// Channel<FooEvent> channel_; -// }; -// -// -/////////////////////////////////////////////////////////////////////////////// - -#include "base/observer_list.h" -#include "base/synchronization/lock.h" -#include "base/threading/platform_thread.h" - -namespace browser_sync { - -template <typename EventType> -class Channel; - -class EventHandler { -}; - -template <typename EventType> -class ChannelEventHandler : public EventHandler { - public: - virtual void HandleChannelEvent(const EventType& event) = 0; - - protected: - virtual ~ChannelEventHandler() {} -}; - -// This class manages a connection to a channel. When it is destroyed, it -// will remove the listener from the channel observer list. -template <typename EventType> -class ChannelHookup { - public: - ChannelHookup(Channel<EventType>* channel, - browser_sync::ChannelEventHandler<EventType>* handler) - : channel_(channel), - handler_(handler) {} - ~ChannelHookup() { - channel_->RemoveObserver(handler_); - } - - private: - Channel<EventType>* channel_; - browser_sync::ChannelEventHandler<EventType>* handler_; -}; - -template <typename EventType> -class Channel { - public: - typedef ObserverListBase<EventHandler> ChannelObserverList; - - Channel() : locking_thread_(0) {} - - ChannelHookup<EventType>* AddObserver( - ChannelEventHandler<EventType>* observer) { - base::AutoLock scoped_lock(event_handlers_mutex_); - event_handlers_.AddObserver(observer); - return new ChannelHookup<EventType>(this, observer); - } - - void RemoveObserver(ChannelEventHandler<EventType>* observer) { - // This can be called in response to a notification, so we may already have - // locked this channel on this thread. - bool need_lock = (locking_thread_ != base::PlatformThread::CurrentId()); - if (need_lock) - event_handlers_mutex_.Acquire(); - - event_handlers_mutex_.AssertAcquired(); - event_handlers_.RemoveObserver(observer); - if (need_lock) - event_handlers_mutex_.Release(); - } - - void Notify(const EventType& event) { - base::AutoLock scoped_lock(event_handlers_mutex_); - - // This may result in an observer trying to remove itself, so keep track - // of the thread we're locked on. - locking_thread_ = base::PlatformThread::CurrentId(); - - ChannelObserverList::Iterator it(event_handlers_); - EventHandler* obs; - while ((obs = it.GetNext()) != NULL) { - static_cast<ChannelEventHandler<EventType>* >(obs)-> - HandleChannelEvent(event); - } - - // Set back to an invalid thread id. - locking_thread_ = 0; - } - - private: - base::Lock event_handlers_mutex_; - base::PlatformThreadId locking_thread_; - ObserverList<EventHandler> event_handlers_; -}; - -} // namespace browser_sync - -#endif // CHROME_BROWSER_SYNC_UTIL_CHANNEL_H_ diff --git a/chrome/browser/sync/util/channel_unittest.cc b/chrome/browser/sync/util/channel_unittest.cc deleted file mode 100644 index f2317dc..0000000 --- a/chrome/browser/sync/util/channel_unittest.cc +++ /dev/null @@ -1,32 +0,0 @@ -// Copyright (c) 2010 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/util/channel.h" -#include "testing/gtest/include/gtest/gtest.h" - -struct TestEvent { - explicit TestEvent(int foo) : data(foo) {} - int data; -}; - -class TestObserver : public browser_sync::ChannelEventHandler<TestEvent> { - public: - virtual void HandleChannelEvent(const TestEvent& event) { - delete hookup; - hookup = 0; - } - - browser_sync::ChannelHookup<TestEvent>* hookup; -}; - -TEST(ChannelTest, RemoveOnNotify) { - browser_sync::Channel<TestEvent> channel; - TestObserver observer; - - observer.hookup = channel.AddObserver(&observer); - - ASSERT_TRUE(0 != observer.hookup); - channel.Notify(TestEvent(1)); - ASSERT_EQ(0, observer.hookup); -} diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp index 5dca742..73aeb1e 100644 --- a/chrome/chrome.gyp +++ b/chrome/chrome.gyp @@ -607,6 +607,7 @@ 'browser/sync/syncable/dir_open_result.h', 'browser/sync/syncable/directory_backing_store.cc', 'browser/sync/syncable/directory_backing_store.h', + 'browser/sync/syncable/directory_change_listener.h', 'browser/sync/syncable/directory_event.h', 'browser/sync/syncable/directory_manager.cc', 'browser/sync/syncable/directory_manager.h', @@ -625,7 +626,6 @@ 'browser/sync/syncable/syncable_id.h', 'browser/sync/syncable/syncable_enum_conversions.cc', 'browser/sync/syncable/syncable_enum_conversions.h', - 'browser/sync/util/channel.h', 'browser/sync/util/crypto_helpers.cc', 'browser/sync/util/crypto_helpers.h', 'browser/sync/util/cryptographer.cc', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index d3dbcfd..3c74006 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -2972,7 +2972,6 @@ 'browser/sync/syncable/syncable_enum_conversions_unittest.cc', 'browser/sync/syncable/syncable_id_unittest.cc', 'browser/sync/syncable/syncable_unittest.cc', - 'browser/sync/util/channel_unittest.cc', 'browser/sync/util/crypto_helpers_unittest.cc', 'browser/sync/util/data_encryption_unittest.cc', 'browser/sync/util/extensions_activity_monitor_unittest.cc', diff --git a/tools/valgrind/tsan/suppressions.txt b/tools/valgrind/tsan/suppressions.txt index 9af8fa0..fd04a6d 100644 --- a/tools/valgrind/tsan/suppressions.txt +++ b/tools/valgrind/tsan/suppressions.txt @@ -132,8 +132,8 @@ fun:ObserverListBase* ... fun:sync_api::SyncManager::SyncInternal::* - fun:sync_api::SyncManager::* - fun:browser_sync::* + fun:syncable::BaseTransaction::* + fun:syncable::BaseTransaction::* } ############################ |