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

20 files changed, 64 insertions, 15 deletions

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));
   }