diff options
author | haitaol@chromium.org <haitaol@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-10-26 05:06:45 +0000 |
---|---|---|
committer | haitaol@chromium.org <haitaol@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-10-26 05:06:45 +0000 |
commit | 1858410f3beb10255da673b90f7dabbd97fbc708 (patch) | |
tree | 5be2f5b3a0a150adcaf14f9106f957cc0d78346f | |
parent | 947e5d8f893f7ac7ca6f9be8c41e2d1c516cddc1 (diff) | |
download | chromium_src-1858410f3beb10255da673b90f7dabbd97fbc708.zip chromium_src-1858410f3beb10255da673b90f7dabbd97fbc708.tar.gz chromium_src-1858410f3beb10255da673b90f7dabbd97fbc708.tar.bz2 |
Transaction version is used to detect out-of-sync between sync model and native model. The values in sync model and native model should be equal. If not, there're some changes that are applied in one but not the other. This change updates the transaction version of a model type in sync when changes to its native model are found. And implement native transaction version for bookmark model.
BUG=154858
Review URL: https://chromiumcodereview.appspot.com/11028146
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@164263 0039d316-1c4b-4281-b951-d872f2087c98
37 files changed, 319 insertions, 21 deletions
diff --git a/chrome/browser/bookmarks/bookmark_codec.cc b/chrome/browser/bookmarks/bookmark_codec.cc index f8e4652..796580d 100644 --- a/chrome/browser/bookmarks/bookmark_codec.cc +++ b/chrome/browser/bookmarks/bookmark_codec.cc @@ -30,6 +30,7 @@ const char* BookmarkCodec::kDateAddedKey = "date_added"; const char* BookmarkCodec::kURLKey = "url"; const char* BookmarkCodec::kDateModifiedKey = "date_modified"; const char* BookmarkCodec::kChildrenKey = "children"; +const char* BookmarkCodec::kMetaInfo = "meta_info"; const char* BookmarkCodec::kTypeURL = "url"; const char* BookmarkCodec::kTypeFolder = "folder"; @@ -47,19 +48,22 @@ BookmarkCodec::~BookmarkCodec() {} Value* BookmarkCodec::Encode(BookmarkModel* model) { return Encode(model->bookmark_bar_node(), model->other_node(), - model->mobile_node()); + model->mobile_node(), + model->root_node()->meta_info_str()); } Value* BookmarkCodec::Encode(const BookmarkNode* bookmark_bar_node, const BookmarkNode* other_folder_node, - const BookmarkNode* mobile_folder_node) { + const BookmarkNode* mobile_folder_node, + const std::string& model_meta_info) { ids_reassigned_ = false; InitializeChecksum(); DictionaryValue* roots = new DictionaryValue(); roots->Set(kRootFolderNameKey, EncodeNode(bookmark_bar_node)); roots->Set(kOtherBookmarkFolderNameKey, EncodeNode(other_folder_node)); roots->Set(kMobileBookmarkFolderNameKey, EncodeNode(mobile_folder_node)); - + if (!model_meta_info.empty()) + roots->SetString(kMetaInfo, model_meta_info); DictionaryValue* main = new DictionaryValue(); main->SetInteger(kVersionKey, kCurrentVersion); FinalizeChecksum(); @@ -118,6 +122,8 @@ Value* BookmarkCodec::EncodeNode(const BookmarkNode* node) { for (int i = 0; i < node->child_count(); ++i) child_values->Append(EncodeNode(node->GetChild(i))); } + if (!node->meta_info_str().empty()) + value->SetString(kMetaInfo, node->meta_info_str()); return value; } @@ -185,6 +191,8 @@ bool BookmarkCodec::DecodeHelper(BookmarkNode* bb_node, ReassignIDsHelper(mobile_folder_node); } + roots_d_value->GetString(kMetaInfo, &model_meta_info_); + // Need to reset the type as decoding resets the type to FOLDER. Similarly // we need to reset the title as the title is persisted and restored from // the file. @@ -319,6 +327,11 @@ bool BookmarkCodec::DecodeNode(const DictionaryValue& value, node->SetTitle(title); node->set_date_added(date_added); + + std::string meta_info; + if (value.GetString(kMetaInfo, &meta_info)) + node->set_meta_info_str(meta_info); + return true; } diff --git a/chrome/browser/bookmarks/bookmark_codec.h b/chrome/browser/bookmarks/bookmark_codec.h index 347abb7..3368acf2eb 100644 --- a/chrome/browser/bookmarks/bookmark_codec.h +++ b/chrome/browser/bookmarks/bookmark_codec.h @@ -43,7 +43,8 @@ class BookmarkCodec { // up to the caller to delete the returned object. base::Value* Encode(const BookmarkNode* bookmark_bar_node, const BookmarkNode* other_folder_node, - const BookmarkNode* mobile_folder_node); + const BookmarkNode* mobile_folder_node, + const std::string& model_meta_info); // Decodes the previously encoded value to the specified nodes as well as // setting |max_node_id| to the greatest node id. Returns true on success, @@ -66,6 +67,9 @@ class BookmarkCodec { // user. const std::string& stored_checksum() const { return stored_checksum_; } + // Return meta info of bookmark model root. + const std::string& model_meta_info() const { return model_meta_info_; } + // Returns whether the IDs were reassigned during decoding. Always returns // false after encoding. bool ids_reassigned() const { return ids_reassigned_; } @@ -84,6 +88,7 @@ class BookmarkCodec { static const char* kURLKey; static const char* kDateModifiedKey; static const char* kChildrenKey; + static const char* kMetaInfo; // Possible values for kTypeKey. static const char* kTypeURL; @@ -161,6 +166,9 @@ class BookmarkCodec { // Maximum ID assigned when decoding data. int64 maximum_id_; + // Meta info set on bookmark model root. + std::string model_meta_info_; + DISALLOW_COPY_AND_ASSIGN(BookmarkCodec); }; diff --git a/chrome/browser/bookmarks/bookmark_codec_unittest.cc b/chrome/browser/bookmarks/bookmark_codec_unittest.cc index dbc1681..aba1347 100644 --- a/chrome/browser/bookmarks/bookmark_codec_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_codec_unittest.cc @@ -119,6 +119,7 @@ class BookmarkCodecTest : public testing::Test { AsMutable(model->mobile_node()), &max_id, value); model->set_next_node_id(max_id); + AsMutable(model->root_node())->set_meta_info_str(codec->model_meta_info()); return result; } @@ -336,3 +337,27 @@ TEST_F(BookmarkCodecTest, CanDecodeModelWithoutMobileBookmarks) { ASSERT_TRUE(decoded_model.mobile_node() != NULL); } + +TEST_F(BookmarkCodecTest, EncodeAndDecodeMetaInfo) { + // Add meta info and encode. + scoped_ptr<BookmarkModel> model(CreateTestModel1()); + model->SetNodeMetaInfo(model->root_node(), "model_info", "value1"); + model->SetNodeMetaInfo(model->bookmark_bar_node()->GetChild(0), + "node_info", "value2"); + std::string checksum; + scoped_ptr<Value> value(EncodeHelper(model.get(), &checksum)); + ASSERT_TRUE(value.get() != NULL); + + // Decode and check for meta info. + model.reset(DecodeHelper(*value, checksum, &checksum, false)); + std::string meta_value; + EXPECT_TRUE(model->root_node()->GetMetaInfo("model_info", &meta_value)); + EXPECT_EQ("value1", meta_value); + EXPECT_FALSE(model->root_node()->GetMetaInfo("other_key", &meta_value)); + const BookmarkNode* bbn = model->bookmark_bar_node(); + ASSERT_EQ(1, bbn->child_count()); + const BookmarkNode* child = bbn->GetChild(0); + EXPECT_TRUE(child->GetMetaInfo("node_info", &meta_value)); + EXPECT_EQ("value2", meta_value); + EXPECT_FALSE(child->GetMetaInfo("other_key", &meta_value)); +} diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc index d4bdb2c..2bcef3e 100644 --- a/chrome/browser/bookmarks/bookmark_model.cc +++ b/chrome/browser/bookmarks/bookmark_model.cc @@ -9,8 +9,10 @@ #include "base/bind.h" #include "base/bind_helpers.h" +#include "base/json/json_string_value_serializer.h" #include "base/memory/scoped_vector.h" #include "base/string_util.h" +#include "base/values.h" #include "build/build_config.h" #include "chrome/browser/bookmarks/bookmark_expanded_state_tracker.h" #include "chrome/browser/bookmarks/bookmark_index.h" @@ -76,12 +78,68 @@ bool BookmarkNode::IsVisible() const { return true; } +bool BookmarkNode::GetMetaInfo(const std::string& key, + std::string* value) const { + if (meta_info_str_.empty()) + return false; + + JSONStringValueSerializer serializer(meta_info_str_); + scoped_ptr<DictionaryValue> meta_dict( + static_cast<DictionaryValue*>(serializer.Deserialize(NULL, NULL))); + return meta_dict.get() ? meta_dict->GetString(key, value) : false; +} + +bool BookmarkNode::SetMetaInfo(const std::string& key, + const std::string& value) { + JSONStringValueSerializer serializer(&meta_info_str_); + scoped_ptr<DictionaryValue> meta_dict; + if (!meta_info_str_.empty()) { + meta_dict.reset( + static_cast<DictionaryValue*>(serializer.Deserialize(NULL, NULL))); + } + if (!meta_dict.get()) { + meta_dict.reset(new DictionaryValue); + } else { + std::string old_value; + if (meta_dict->GetString(key, &old_value) && old_value == value) + return false; + } + meta_dict->SetString(key, value); + serializer.Serialize(*meta_dict); + std::string(meta_info_str_.data(), meta_info_str_.size()).swap( + meta_info_str_); + return true; +} + +bool BookmarkNode::DeleteMetaInfo(const std::string& key) { + if (meta_info_str_.empty()) + return false; + + JSONStringValueSerializer serializer(&meta_info_str_); + scoped_ptr<DictionaryValue> meta_dict( + static_cast<DictionaryValue*>(serializer.Deserialize(NULL, NULL))); + if (meta_dict.get() && meta_dict->HasKey(key)) { + meta_dict->Remove(key, NULL); + if (meta_dict->empty()) { + meta_info_str_.clear(); + } else { + serializer.Serialize(*meta_dict); + std::string(meta_info_str_.data(), meta_info_str_.size()).swap( + meta_info_str_); + } + return true; + } else { + return false; + } +} + void BookmarkNode::Initialize(int64 id) { id_ = id; type_ = url_.is_empty() ? FOLDER : URL; date_added_ = Time::Now(); favicon_state_ = INVALID_FAVICON; favicon_load_handle_ = 0; + meta_info_str_.clear(); } void BookmarkNode::InvalidateFavicon() { @@ -378,6 +436,19 @@ void BookmarkModel::SetURL(const BookmarkNode* node, const GURL& url) { BookmarkNodeChanged(this, node)); } +void BookmarkModel::SetNodeMetaInfo(const BookmarkNode* node, + const std::string& key, + const std::string& value) { + if (AsMutable(node)->SetMetaInfo(key, value) && store_.get()) + store_->ScheduleSave(); +} + +void BookmarkModel::DeleteNodeMetaInfo(const BookmarkNode* node, + const std::string& key) { + if (AsMutable(node)->DeleteMetaInfo(key) && store_.get()) + store_->ScheduleSave(); +} + void BookmarkModel::GetNodesByURL(const GURL& url, std::vector<const BookmarkNode*>* nodes) { base::AutoLock url_lock(url_lock_); @@ -634,6 +705,8 @@ void BookmarkModel::DoneLoading(BookmarkLoadDetails* details_delete_me) { root_.Add(other_node_, 1); root_.Add(mobile_node_, 2); + root_.set_meta_info_str(details->model_meta_info()); + { base::AutoLock url_lock(url_lock_); // Update nodes_ordered_by_url_set_ from the nodes. @@ -884,5 +957,6 @@ BookmarkLoadDetails* BookmarkModel::CreateLoadDetails() { BookmarkPermanentNode* mobile_node = CreatePermanentNode(BookmarkNode::MOBILE); return new BookmarkLoadDetails(bb_node, other_node, mobile_node, - new BookmarkIndex(profile_), next_node_id_); + new BookmarkIndex(profile_), + next_node_id_); } diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h index fe9783a..4b354b5 100644 --- a/chrome/browser/bookmarks/bookmark_model.h +++ b/chrome/browser/bookmarks/bookmark_model.h @@ -109,6 +109,18 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> { // representation but we may want to suppress some nodes. virtual bool IsVisible() const; + // Gets/sets/deletes value of |key| in the meta info represented by + // |meta_info_str_|. Return true if key is found in meta info for gets or + // meta info is changed indeed for sets/deletes. + bool GetMetaInfo(const std::string& key, std::string* value) const; + bool SetMetaInfo(const std::string& key, const std::string& value); + bool DeleteMetaInfo(const std::string& key); + void set_meta_info_str(const std::string& meta_info_str) { + meta_info_str_.reserve(meta_info_str.size()); + meta_info_str_ = meta_info_str.substr(0); + } + const std::string& meta_info_str() const { return meta_info_str_; } + // TODO(sky): Consider adding last visit time here, it'll greatly simplify // HistoryContentsProvider. @@ -160,6 +172,10 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> { // from the HistoryService. HistoryService::Handle favicon_load_handle_; + // A JSON string representing a DictionaryValue that stores arbitrary meta + // information about the node. Use serialized format to save memory. + std::string meta_info_str_; + DISALLOW_COPY_AND_ASSIGN(BookmarkNode); }; @@ -364,6 +380,13 @@ class BookmarkModel : public content::NotificationObserver, // Sets the visibility of one of the permanent nodes. This is set by sync. void SetPermanentNodeVisible(BookmarkNode::Type type, bool value); + // Sets/deletes meta info of |node|. + void SetNodeMetaInfo(const BookmarkNode* node, + const std::string& key, + const std::string& value); + void DeleteNodeMetaInfo(const BookmarkNode* node, + const std::string& key); + private: friend class BookmarkCodecTest; friend class BookmarkModelTest; diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc index 0d783ae..e499850 100644 --- a/chrome/browser/bookmarks/bookmark_model_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc @@ -34,6 +34,7 @@ #include "content/public/browser/notification_registrar.h" #include "content/public/browser/notification_source.h" #include "content/public/test/test_browser_thread.h" +#include "googleurl/src/gurl.h" #include "testing/gtest/include/gtest/gtest.h" #include "ui/base/models/tree_node_iterator.h" #include "ui/base/models/tree_node_model.h" @@ -1079,4 +1080,28 @@ TEST_F(BookmarkModelTest, MultipleExtensiveChangesObserver) { AssertExtensiveChangesObserverCount(1, 1); } +TEST(BookmarkNodeTest, NodeMetaInfo) { + GURL url; + BookmarkNode node(url); + EXPECT_TRUE(node.meta_info_str().empty()); + + EXPECT_TRUE(node.SetMetaInfo("key1", "value1")); + std::string out_value; + EXPECT_TRUE(node.GetMetaInfo("key1", &out_value)); + EXPECT_EQ("value1", out_value); + EXPECT_FALSE(node.SetMetaInfo("key1", "value1")); + + EXPECT_FALSE(node.GetMetaInfo("key2", &out_value)); + EXPECT_TRUE(node.SetMetaInfo("key2", "value2")); + EXPECT_TRUE(node.GetMetaInfo("key2", &out_value)); + EXPECT_EQ("value2", out_value); + + EXPECT_TRUE(node.DeleteMetaInfo("key1")); + EXPECT_TRUE(node.DeleteMetaInfo("key2")); + EXPECT_FALSE(node.DeleteMetaInfo("key3")); + EXPECT_FALSE(node.GetMetaInfo("key1", &out_value)); + EXPECT_FALSE(node.GetMetaInfo("key2", &out_value)); + EXPECT_TRUE(node.meta_info_str().empty()); +} + } // namespace diff --git a/chrome/browser/bookmarks/bookmark_storage.cc b/chrome/browser/bookmarks/bookmark_storage.cc index e44f2e9..92f19e4 100644 --- a/chrome/browser/bookmarks/bookmark_storage.cc +++ b/chrome/browser/bookmarks/bookmark_storage.cc @@ -66,6 +66,7 @@ void LoadCallback(const FilePath& path, details->set_computed_checksum(codec.computed_checksum()); details->set_stored_checksum(codec.stored_checksum()); details->set_ids_reassigned(codec.ids_reassigned()); + details->set_model_meta_info(codec.model_meta_info()); UMA_HISTOGRAM_TIMES("Bookmarks.DecodeTime", TimeTicks::Now() - start_time); diff --git a/chrome/browser/bookmarks/bookmark_storage.h b/chrome/browser/bookmarks/bookmark_storage.h index b4f0d9d..ac780d5 100644 --- a/chrome/browser/bookmarks/bookmark_storage.h +++ b/chrome/browser/bookmarks/bookmark_storage.h @@ -51,6 +51,11 @@ class BookmarkLoadDetails { BookmarkIndex* index() { return index_.get(); } BookmarkIndex* release_index() { return index_.release(); } + const std::string& model_meta_info() { return model_meta_info_; } + void set_model_meta_info(const std::string& meta_info) { + model_meta_info_ = meta_info; + } + // Max id of the nodes. void set_max_id(int64 max_id) { max_id_ = max_id; } int64 max_id() const { return max_id_; } @@ -79,6 +84,7 @@ class BookmarkLoadDetails { scoped_ptr<BookmarkPermanentNode> other_folder_node_; scoped_ptr<BookmarkPermanentNode> mobile_folder_node_; scoped_ptr<BookmarkIndex> index_; + std::string model_meta_info_; int64 max_id_; std::string computed_checksum_; std::string stored_checksum_; diff --git a/chrome/browser/sync/glue/bookmark_change_processor.cc b/chrome/browser/sync/glue/bookmark_change_processor.cc index 33876da..b81fde1 100644 --- a/chrome/browser/sync/glue/bookmark_change_processor.cc +++ b/chrome/browser/sync/glue/bookmark_change_processor.cc @@ -9,6 +9,7 @@ #include "base/location.h" #include "base/string16.h" +#include "base/string_number_conversions.h" #include "base/string_util.h" #include "base/utf_string_conversions.h" #include "chrome/browser/bookmarks/bookmark_model.h" @@ -34,6 +35,9 @@ namespace browser_sync { static const char kMobileBookmarksTag[] = "synced_bookmarks"; +// Key for sync transaction version in bookmark node meta info. +const char kBookmarkTransactionVersionKey[] = "sync.transaction_version"; + BookmarkChangeProcessor::BookmarkChangeProcessor( BookmarkModelAssociator* model_associator, DataTypeErrorHandler* error_handler) @@ -413,6 +417,7 @@ int BookmarkChangeProcessor::CalculateBookmarkModelInsertionIndex( // model. void BookmarkChangeProcessor::ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); // A note about ordering. Sync backend is responsible for ordering the change @@ -535,6 +540,11 @@ void BookmarkChangeProcessor::ApplyChangesFromSyncModel( // We are now ready to hear about bookmarks changes again. model->AddObserver(this); + + // All changes are applied in bookmark model. Set transaction version on + // bookmark model to mark as synced. + model->SetNodeMetaInfo(model->root_node(), kBookmarkTransactionVersionKey, + base::Int64ToString(model_version)); } // Create a bookmark node corresponding to |src| if one is not already diff --git a/chrome/browser/sync/glue/bookmark_change_processor.h b/chrome/browser/sync/glue/bookmark_change_processor.h index a6d2f4e6..4f71668 100644 --- a/chrome/browser/sync/glue/bookmark_change_processor.h +++ b/chrome/browser/sync/glue/bookmark_change_processor.h @@ -21,6 +21,8 @@ class WriteTransaction; namespace browser_sync { +extern const char kBookmarkTransactionVersionKey[]; + // This class is responsible for taking changes from the BookmarkModel // and applying them to the sync API 'syncable' model, and vice versa. // All operations and use of this class are from the UI thread. @@ -59,6 +61,7 @@ class BookmarkChangeProcessor : public BookmarkModelObserver, // the sync model to the bookmarks model. virtual void ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) OVERRIDE; // The following methods are static and hence may be invoked at any time, diff --git a/chrome/browser/sync/glue/bookmark_model_associator.cc b/chrome/browser/sync/glue/bookmark_model_associator.cc index db0caf3..cbb067e 100644 --- a/chrome/browser/sync/glue/bookmark_model_associator.cc +++ b/chrome/browser/sync/glue/bookmark_model_associator.cc @@ -11,6 +11,7 @@ #include "base/hash_tables.h" #include "base/location.h" #include "base/message_loop.h" +#include "base/string_number_conversions.h" #include "base/utf_string_conversions.h" #include "chrome/browser/bookmarks/bookmark_model.h" #include "chrome/browser/profiles/profile.h" @@ -22,6 +23,7 @@ #include "sync/internal_api/public/write_node.h" #include "sync/internal_api/public/write_transaction.h" #include "sync/util/cryptographer.h" +#include "sync/util/data_type_histogram.h" using content::BrowserThread; @@ -355,6 +357,8 @@ bool BookmarkModelAssociator::GetSyncIdForTaggedNode(const std::string& tag, } syncer::SyncError BookmarkModelAssociator::AssociateModels() { + CheckModelSyncState(); + scoped_ptr<ScopedAssociationUpdater> association_updater( new ScopedAssociationUpdater(bookmark_model_)); // Try to load model associations from persisted associations first. If that @@ -666,4 +670,21 @@ bool BookmarkModelAssociator::CryptoReadyIfNecessary() { trans.GetCryptographer()->is_ready(); } +void BookmarkModelAssociator::CheckModelSyncState() const { + std::string version_str; + if (bookmark_model_->root_node()->GetMetaInfo(kBookmarkTransactionVersionKey, + &version_str)) { + syncer::ReadTransaction trans(FROM_HERE, user_share_); + int64 native_version; + if (base::StringToInt64(version_str, &native_version) && + native_version != trans.GetModelVersion(syncer::BOOKMARKS)) { + UMA_HISTOGRAM_ENUMERATION("Sync.LocalModelOutOfSync", + syncer::BOOKMARKS, syncer::MODEL_TYPE_COUNT); + // Clear version on bookmark model so that we only report error once. + bookmark_model_->DeleteNodeMetaInfo(bookmark_model_->root_node(), + kBookmarkTransactionVersionKey); + } + } +} + } // namespace browser_sync diff --git a/chrome/browser/sync/glue/bookmark_model_associator.h b/chrome/browser/sync/glue/bookmark_model_associator.h index 21c3710..c5623bc 100644 --- a/chrome/browser/sync/glue/bookmark_model_associator.h +++ b/chrome/browser/sync/glue/bookmark_model_associator.h @@ -133,6 +133,10 @@ class BookmarkModelAssociator bool NodesMatch(const BookmarkNode* bookmark, const syncer::BaseNode* sync_node) const; + // Check whether bookmark model and sync model are synced by comparing + // their transaction versions. + void CheckModelSyncState() const; + BookmarkModel* bookmark_model_; syncer::UserShare* user_share_; DataTypeErrorHandler* unrecoverable_error_handler_; diff --git a/chrome/browser/sync/glue/change_processor.h b/chrome/browser/sync/glue/change_processor.h index dcf8bb5..f2da744 100644 --- a/chrome/browser/sync/glue/change_processor.h +++ b/chrome/browser/sync/glue/change_processor.h @@ -40,6 +40,7 @@ class ChangeProcessor { // how to interpret and process |changes|. virtual void ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) = 0; // The changes found in ApplyChangesFromSyncModel may be too slow to be diff --git a/chrome/browser/sync/glue/change_processor_mock.h b/chrome/browser/sync/glue/change_processor_mock.h index 284f834..d2cb6c57 100644 --- a/chrome/browser/sync/glue/change_processor_mock.h +++ b/chrome/browser/sync/glue/change_processor_mock.h @@ -17,8 +17,8 @@ class ChangeProcessorMock public: ChangeProcessorMock(); virtual ~ChangeProcessorMock(); - MOCK_METHOD2(ApplyChangesFromSyncModel, - void(const syncer::BaseTransaction*, + MOCK_METHOD3(ApplyChangesFromSyncModel, + void(const syncer::BaseTransaction*, int64, const syncer::ImmutableChangeRecordList&)); MOCK_METHOD0(CommitChangesFromSyncModel, void()); MOCK_METHOD1(StartImpl, void(Profile*)); diff --git a/chrome/browser/sync/glue/generic_change_processor.cc b/chrome/browser/sync/glue/generic_change_processor.cc index 4f63230..6c5f5e2 100644 --- a/chrome/browser/sync/glue/generic_change_processor.cc +++ b/chrome/browser/sync/glue/generic_change_processor.cc @@ -40,6 +40,7 @@ GenericChangeProcessor::~GenericChangeProcessor() { void GenericChangeProcessor::ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) { DCHECK(CalledOnValidThread()); DCHECK(syncer_changes_.empty()); diff --git a/chrome/browser/sync/glue/generic_change_processor.h b/chrome/browser/sync/glue/generic_change_processor.h index aa3b2bc..d326fcd 100644 --- a/chrome/browser/sync/glue/generic_change_processor.h +++ b/chrome/browser/sync/glue/generic_change_processor.h @@ -49,6 +49,7 @@ class GenericChangeProcessor : public ChangeProcessor, // Build and store a list of all changes into |syncer_changes_|. virtual void ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 version, const syncer::ImmutableChangeRecordList& changes) OVERRIDE; // Passes |syncer_changes_|, built in ApplyChangesFromSyncModel, onto // |local_service_| by way of its ProcessSyncChanges method. diff --git a/chrome/browser/sync/glue/password_change_processor.cc b/chrome/browser/sync/glue/password_change_processor.cc index 5a6e456..5da17df 100644 --- a/chrome/browser/sync/glue/password_change_processor.cc +++ b/chrome/browser/sync/glue/password_change_processor.cc @@ -160,6 +160,7 @@ void PasswordChangeProcessor::Observe( void PasswordChangeProcessor::ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) { DCHECK(expected_loop_ == MessageLoop::current()); diff --git a/chrome/browser/sync/glue/password_change_processor.h b/chrome/browser/sync/glue/password_change_processor.h index 77fddc9..54430bd 100644 --- a/chrome/browser/sync/glue/password_change_processor.h +++ b/chrome/browser/sync/glue/password_change_processor.h @@ -44,6 +44,7 @@ class PasswordChangeProcessor : public ChangeProcessor, // sync API model -> WebDataService change application. virtual void ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) OVERRIDE; // Commit changes buffered during ApplyChanges. We must commit them to the diff --git a/chrome/browser/sync/glue/session_change_processor.cc b/chrome/browser/sync/glue/session_change_processor.cc index 5a5efc2d..6b859b5 100644 --- a/chrome/browser/sync/glue/session_change_processor.cc +++ b/chrome/browser/sync/glue/session_change_processor.cc @@ -247,6 +247,7 @@ void SessionChangeProcessor::Observe( void SessionChangeProcessor::ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); diff --git a/chrome/browser/sync/glue/session_change_processor.h b/chrome/browser/sync/glue/session_change_processor.h index 5f8a726..4d541c2 100644 --- a/chrome/browser/sync/glue/session_change_processor.h +++ b/chrome/browser/sync/glue/session_change_processor.h @@ -48,6 +48,7 @@ class SessionChangeProcessor : public ChangeProcessor, // sync API model -> BrowserSessionProvider change application. virtual void ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) OVERRIDE; protected: diff --git a/chrome/browser/sync/glue/sync_backend_registrar.cc b/chrome/browser/sync/glue/sync_backend_registrar.cc index 3ff6446..e3fa3bd 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar.cc +++ b/chrome/browser/sync/glue/sync_backend_registrar.cc @@ -219,13 +219,14 @@ bool SyncBackendRegistrar::IsTypeActivatedForTest( void SyncBackendRegistrar::OnChangesApplied( syncer::ModelType model_type, + int64 model_version, const syncer::BaseTransaction* trans, const syncer::ImmutableChangeRecordList& changes) { ChangeProcessor* processor = GetProcessor(model_type); if (!processor) return; - processor->ApplyChangesFromSyncModel(trans, changes); + processor->ApplyChangesFromSyncModel(trans, model_version, changes); } void SyncBackendRegistrar::OnChangesComplete(syncer::ModelType model_type) { diff --git a/chrome/browser/sync/glue/sync_backend_registrar.h b/chrome/browser/sync/glue/sync_backend_registrar.h index 4f18397..a02c5d8 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar.h +++ b/chrome/browser/sync/glue/sync_backend_registrar.h @@ -97,6 +97,7 @@ class SyncBackendRegistrar : public syncer::SyncManager::ChangeDelegate { // any thread. virtual void OnChangesApplied( syncer::ModelType model_type, + int64 model_version, const syncer::BaseTransaction* trans, const syncer::ImmutableChangeRecordList& changes) OVERRIDE; virtual void OnChangesComplete(syncer::ModelType model_type) OVERRIDE; diff --git a/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc b/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc index cb19c6b..a628f77 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc +++ b/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc @@ -150,7 +150,7 @@ TEST_F(SyncBackendRegistrarTest, ConfigureDataTypes) { } void TriggerChanges(SyncBackendRegistrar* registrar, ModelType type) { - registrar->OnChangesApplied(type, NULL, + registrar->OnChangesApplied(type, 0, NULL, syncer::ImmutableChangeRecordList()); registrar->OnChangesComplete(type); } @@ -168,7 +168,7 @@ TEST_F(SyncBackendRegistrarTest, ActivateDeactivateUIDataType) { EXPECT_CALL(change_processor_mock, StartImpl(&profile)); EXPECT_CALL(change_processor_mock, IsRunning()) .WillRepeatedly(Return(true)); - EXPECT_CALL(change_processor_mock, ApplyChangesFromSyncModel(NULL, _)); + EXPECT_CALL(change_processor_mock, ApplyChangesFromSyncModel(NULL, _, _)); EXPECT_CALL(change_processor_mock, IsRunning()) .WillRepeatedly(Return(true)); EXPECT_CALL(change_processor_mock, CommitChangesFromSyncModel()); @@ -215,7 +215,7 @@ TEST_F(SyncBackendRegistrarTest, ActivateDeactivateNonUIDataType) { EXPECT_CALL(change_processor_mock, StartImpl(&profile)); EXPECT_CALL(change_processor_mock, IsRunning()) .WillRepeatedly(Return(true)); - EXPECT_CALL(change_processor_mock, ApplyChangesFromSyncModel(NULL, _)); + EXPECT_CALL(change_processor_mock, ApplyChangesFromSyncModel(NULL, _, _)); EXPECT_CALL(change_processor_mock, IsRunning()) .WillRepeatedly(Return(true)); EXPECT_CALL(change_processor_mock, CommitChangesFromSyncModel()); diff --git a/chrome/browser/sync/glue/typed_url_change_processor.cc b/chrome/browser/sync/glue/typed_url_change_processor.cc index e0574e1..d8664ad 100644 --- a/chrome/browser/sync/glue/typed_url_change_processor.cc +++ b/chrome/browser/sync/glue/typed_url_change_processor.cc @@ -243,6 +243,7 @@ bool TypedUrlChangeProcessor::ShouldSyncVisit( void TypedUrlChangeProcessor::ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) { DCHECK(expected_loop_ == MessageLoop::current()); diff --git a/chrome/browser/sync/glue/typed_url_change_processor.h b/chrome/browser/sync/glue/typed_url_change_processor.h index e5568f8..619a8eb 100644 --- a/chrome/browser/sync/glue/typed_url_change_processor.h +++ b/chrome/browser/sync/glue/typed_url_change_processor.h @@ -57,6 +57,7 @@ class TypedUrlChangeProcessor : public ChangeProcessor, // sync API model -> WebDataService change application. virtual void ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, + int64 model_version, const syncer::ImmutableChangeRecordList& changes) OVERRIDE; // Commit changes here, after we've released the transaction lock to avoid diff --git a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc index 377d6ba..9749d5c 100644 --- a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc @@ -251,7 +251,7 @@ class FakeServerChange { // Pass the fake change list to |service|. void ApplyPendingChanges(ChangeProcessor* processor) { processor->ApplyChangesFromSyncModel( - trans_, syncer::ImmutableChangeRecordList(&changes_)); + trans_, 0, syncer::ImmutableChangeRecordList(&changes_)); } const syncer::ChangeRecordList& changes() { diff --git a/chrome/browser/sync/profile_sync_service_preference_unittest.cc b/chrome/browser/sync/profile_sync_service_preference_unittest.cc index 1188b40..ab101f4 100644 --- a/chrome/browser/sync/profile_sync_service_preference_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_preference_unittest.cc @@ -398,7 +398,7 @@ TEST_F(ProfileSyncServicePreferenceTest, UpdatedSyncNodeActionUpdate) { { syncer::WriteTransaction trans(FROM_HERE, service_->GetUserShare()); change_processor_->ApplyChangesFromSyncModel( - &trans, + &trans, 0, ProfileSyncServiceTestHelper::MakeSingletonChangeRecordList( node_id, ChangeRecord::ACTION_UPDATE)); } @@ -419,7 +419,7 @@ TEST_F(ProfileSyncServicePreferenceTest, UpdatedSyncNodeActionAdd) { { syncer::WriteTransaction trans(FROM_HERE, service_->GetUserShare()); change_processor_->ApplyChangesFromSyncModel( - &trans, + &trans, 0, ProfileSyncServiceTestHelper::MakeSingletonChangeRecordList( node_id, ChangeRecord::ACTION_ADD)); } @@ -442,7 +442,7 @@ TEST_F(ProfileSyncServicePreferenceTest, UpdatedSyncNodeUnknownPreference) { { syncer::WriteTransaction trans(FROM_HERE, service_->GetUserShare()); change_processor_->ApplyChangesFromSyncModel( - &trans, + &trans, 0, ProfileSyncServiceTestHelper::MakeSingletonChangeRecordList( node_id, ChangeRecord::ACTION_UPDATE)); } @@ -477,7 +477,7 @@ TEST_F(ProfileSyncServicePreferenceTest, ManagedPreferences) { { syncer::WriteTransaction trans(FROM_HERE, service_->GetUserShare()); change_processor_->ApplyChangesFromSyncModel( - &trans, + &trans, 0, ProfileSyncServiceTestHelper::MakeSingletonChangeRecordList( node_id, ChangeRecord::ACTION_UPDATE)); } @@ -540,7 +540,7 @@ TEST_F(ProfileSyncServicePreferenceTest, { syncer::WriteTransaction trans(FROM_HERE, service_->GetUserShare()); change_processor_->ApplyChangesFromSyncModel( - &trans, + &trans, 0, ProfileSyncServiceTestHelper::MakeSingletonChangeRecordList( node_id, ChangeRecord::ACTION_ADD)); } diff --git a/chrome/browser/sync/profile_sync_service_session_unittest.cc b/chrome/browser/sync/profile_sync_service_session_unittest.cc index 96c9f91..c97b70c 100644 --- a/chrome/browser/sync/profile_sync_service_session_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_session_unittest.cc @@ -651,7 +651,7 @@ TEST_F(ProfileSyncServiceSessionTest, UpdatedSyncNodeActionUpdate) { { syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare()); change_processor_->ApplyChangesFromSyncModel( - &trans, + &trans, 0, ProfileSyncServiceTestHelper::MakeSingletonChangeRecordList( node_id, ChangeRecord::ACTION_UPDATE)); } @@ -670,7 +670,7 @@ TEST_F(ProfileSyncServiceSessionTest, UpdatedSyncNodeActionAdd) { { syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare()); change_processor_->ApplyChangesFromSyncModel( - &trans, + &trans, 0, ProfileSyncServiceTestHelper::MakeSingletonChangeRecordList( node_id, ChangeRecord::ACTION_ADD)); } @@ -691,7 +691,7 @@ TEST_F(ProfileSyncServiceSessionTest, UpdatedSyncNodeActionDelete) { { syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare()); change_processor_->ApplyChangesFromSyncModel( - &trans, + &trans, 0, ProfileSyncServiceTestHelper::MakeSingletonDeletionChangeRecordList( node_id, deleted_specifics)); } diff --git a/sync/internal_api/public/base_node.h b/sync/internal_api/public/base_node.h index 4b229cd..4a2fb9e 100644 --- a/sync/internal_api/public/base_node.h +++ b/sync/internal_api/public/base_node.h @@ -247,6 +247,7 @@ class BaseNode { FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, SetNonBookmarkTitle); FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, SetNonBookmarkTitleWithEncryption); FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, SetPreviouslyEncryptedSpecifics); + FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, IncrementTransactionVersion); void* operator new(size_t size); // Node is meant for stack use only. diff --git a/sync/internal_api/public/read_transaction.h b/sync/internal_api/public/read_transaction.h index b7bcff3..6619bb0 100644 --- a/sync/internal_api/public/read_transaction.h +++ b/sync/internal_api/public/read_transaction.h @@ -31,6 +31,11 @@ class ReadTransaction : public BaseTransaction { // BaseTransaction override. virtual syncable::BaseTransaction* GetWrappedTrans() const OVERRIDE; + + // Return |transaction_version| of |type| stored in sync directory's + // persisted info. + int64 GetModelVersion(ModelType type); + private: void* operator new(size_t size); // Transaction is meant for stack use only. diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h index 654eb7e..15b91e2 100644 --- a/sync/internal_api/public/sync_manager.h +++ b/sync/internal_api/public/sync_manager.h @@ -105,6 +105,7 @@ class SyncManager { // updated later in the list. virtual void OnChangesApplied( ModelType model_type, + int64 model_version, const BaseTransaction* trans, const ImmutableChangeRecordList& changes) = 0; diff --git a/sync/internal_api/read_transaction.cc b/sync/internal_api/read_transaction.cc index 32296f7..6de2f42 100644 --- a/sync/internal_api/read_transaction.cc +++ b/sync/internal_api/read_transaction.cc @@ -4,6 +4,7 @@ #include "sync/internal_api/public/read_transaction.h" +#include "sync/syncable/directory.h" #include "sync/syncable/read_transaction.h" namespace syncer { @@ -35,4 +36,8 @@ syncable::BaseTransaction* ReadTransaction::GetWrappedTrans() const { return transaction_; } +int64 ReadTransaction::GetModelVersion(ModelType type) { + return transaction_->directory()->GetTransactionVersion(type); +} + } // namespace syncer diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc index d035cb7..af1f6fa 100644 --- a/sync/internal_api/sync_manager_impl.cc +++ b/sync/internal_api/sync_manager_impl.cc @@ -795,8 +795,14 @@ SyncManagerImpl::HandleTransactionEndingChangeEvent( CHECK(change_buffers_[type].GetAllChangesInTreeOrder(&read_trans, &ordered_changes)); if (!ordered_changes.Get().empty()) { + // Increment transaction version so that change processor can read + // updated value and set it in native model after changes are applied. + trans->directory()->IncrementTransactionVersion(type); + change_delegate_-> - OnChangesApplied(type, &read_trans, ordered_changes); + OnChangesApplied(type, + trans->directory()->GetTransactionVersion(type), + &read_trans, ordered_changes); change_observer_.Call(FROM_HERE, &SyncManager::ChangeObserver::OnChangesApplied, type, write_transaction_info.Get().id, ordered_changes); diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index 2eb5112..d0b3793 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -801,6 +801,7 @@ class SyncManagerTest : public testing::Test, virtual void OnChangesApplied( ModelType model_type, + int64 model_version, const BaseTransaction* trans, const ImmutableChangeRecordList& changes) OVERRIDE {} @@ -2618,6 +2619,45 @@ TEST_F(SyncManagerTest, SetPreviouslyEncryptedSpecifics) { } } +// Verify transaction version of a model type is incremented when node of +// that type is updated. +TEST_F(SyncManagerTest, IncrementTransactionVersion) { + ModelSafeRoutingInfo routing_info; + GetModelSafeRoutingInfo(&routing_info); + + { + ReadTransaction read_trans(FROM_HERE, sync_manager_.GetUserShare()); + for (ModelSafeRoutingInfo::iterator i = routing_info.begin(); + i != routing_info.end(); ++i) { + // Transaction version is incremented when SyncManagerTest::SetUp() + // creates a node of each type. + EXPECT_EQ(1, + sync_manager_.GetUserShare()->directory-> + GetTransactionVersion(i->first)); + } + } + + // Create bookmark node to increment transaction version of bookmark model. + std::string client_tag = "title"; + sync_pb::EntitySpecifics entity_specifics; + entity_specifics.mutable_bookmark()->set_url("url"); + entity_specifics.mutable_bookmark()->set_title("title"); + MakeServerNode(sync_manager_.GetUserShare(), BOOKMARKS, client_tag, + BaseNode::GenerateSyncableHash(BOOKMARKS, + client_tag), + entity_specifics); + + { + ReadTransaction read_trans(FROM_HERE, sync_manager_.GetUserShare()); + for (ModelSafeRoutingInfo::iterator i = routing_info.begin(); + i != routing_info.end(); ++i) { + EXPECT_EQ(i->first == BOOKMARKS ? 2 : 1, + sync_manager_.GetUserShare()->directory-> + GetTransactionVersion(i->first)); + } + } +} + class MockSyncScheduler : public FakeSyncScheduler { public: MockSyncScheduler() : FakeSyncScheduler() {} diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc index a0a4e24..1eaaf13 100644 --- a/sync/syncable/directory.cc +++ b/sync/syncable/directory.cc @@ -657,6 +657,16 @@ void Directory::SetDownloadProgress( kernel_->info_status = KERNEL_SHARE_INFO_DIRTY; } +int64 Directory::GetTransactionVersion(ModelType type) const { + kernel_->transaction_mutex.AssertAcquired(); + return kernel_->persisted_info.transaction_version[type]; +} + +void Directory::IncrementTransactionVersion(ModelType type) { + kernel_->transaction_mutex.AssertAcquired(); + kernel_->persisted_info.transaction_version[type]++; +} + ModelTypeSet Directory::initial_sync_ended_types() const { ScopedKernelLock lock(this); return kernel_->persisted_info.initial_sync_ended; diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h index c123e36..5ce498b 100644 --- a/sync/syncable/directory.h +++ b/sync/syncable/directory.h @@ -259,6 +259,11 @@ class Directory { ModelType type, const sync_pb::DataTypeProgressMarker& value); + // Gets/Increments transaction version of a model type. Must be called when + // holding kernel mutex. + int64 GetTransactionVersion(ModelType type) const; + void IncrementTransactionVersion(ModelType type); + ModelTypeSet initial_sync_ended_types() const; bool initial_sync_ended_for_type(ModelType type) const; void set_initial_sync_ended_for_type(ModelType type, bool value); diff --git a/sync/tools/sync_client.cc b/sync/tools/sync_client.cc index 96be8a3..78db3b4 100644 --- a/sync/tools/sync_client.cc +++ b/sync/tools/sync_client.cc @@ -164,6 +164,7 @@ class LoggingChangeDelegate : public SyncManager::ChangeDelegate { virtual void OnChangesApplied( ModelType model_type, + int64 model_version, const BaseTransaction* trans, const ImmutableChangeRecordList& changes) OVERRIDE { LOG(INFO) << "Changes applied for " |