Expose sync timestamps from processor to bridges

Processors track sync metadata per sync entity which includes
creation and modification timestamps. We now expose these fields to
sync bridges via ModelTypeChangeProcessor.

The immediate use-case we have in mind is the smarter detection of
likely-duplicate DeviceInfo instances, which is deferred to later
patches.

Bug: 910390
Change-Id: I1066489865a18e56470cbd8e96a41f3a00eeaec8
Reviewed-on: https://chromium-review.googlesource.com/c/1449652
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#628317}

components/sync/model/fake_model_type_change_processor.cc

@@ -48,6 +48,16 @@ bool FakeModelTypeChangeProcessor::IsEntityUnsynced(
   return false;
 }
 
+base::Time FakeModelTypeChangeProcessor::GetEntityCreationTime(
+    const std::string& storage_key) const {
+  return base::Time();
+}
+
+base::Time FakeModelTypeChangeProcessor::GetEntityModificationTime(
+    const std::string& storage_key) const {
+  return base::Time();
+}
+
 void FakeModelTypeChangeProcessor::OnModelStarting(
     ModelTypeSyncBridge* bridge) {}
 

components/sync/model/fake_model_type_change_processor.h

@@ -39,6 +39,10 @@ class FakeModelTypeChangeProcessor : public ModelTypeChangeProcessor {
   void UntrackEntityForClientTagHash(
       const std::string& client_tag_hash) override;
   bool IsEntityUnsynced(const std::string& storage_key) override;
+  base::Time GetEntityCreationTime(
+      const std::string& storage_key) const override;
+  base::Time GetEntityModificationTime(
+      const std::string& storage_key) const override;
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;

components/sync/model/mock_model_type_change_processor.cc

@@ -53,6 +53,16 @@ class ForwardingModelTypeChangeProcessor : public ModelTypeChangeProcessor {
     return other_->IsEntityUnsynced(storage_key);
   }
 
+  base::Time GetEntityCreationTime(
+      const std::string& storage_key) const override {
+    return other_->GetEntityCreationTime(storage_key);
+  }
+
+  base::Time GetEntityModificationTime(
+      const std::string& storage_key) const override {
+    return other_->GetEntityModificationTime(storage_key);
+  }
+
   void OnModelStarting(ModelTypeSyncBridge* bridge) override {
     other_->OnModelStarting(bridge);
   }

components/sync/model/mock_model_type_change_processor.h

@@ -35,6 +35,10 @@ class MockModelTypeChangeProcessor : public ModelTypeChangeProcessor {
   MOCK_METHOD1(UntrackEntityForClientTagHash,
                void(const std::string& client_tag_hash));
   MOCK_METHOD1(IsEntityUnsynced, bool(const std::string& storage_key));
+  MOCK_CONST_METHOD1(GetEntityCreationTime,
+                     base::Time(const std::string& storage_key));
+  MOCK_CONST_METHOD1(GetEntityModificationTime,
+                     base::Time(const std::string& storage_key));
   MOCK_METHOD1(OnModelStarting, void(ModelTypeSyncBridge* bridge));
   MOCK_METHOD1(ModelReadyToSync, void(std::unique_ptr<MetadataBatch> batch));
   MOCK_METHOD0(IsTrackingMetadata, bool());

components/sync/model/model_type_change_processor.h

@@ -11,6 +11,7 @@
 #include "base/location.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
+#include "base/time/time.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/model/entity_data.h"
 #include "components/sync/model/model_error.h"
@@ -74,6 +75,16 @@ class ModelTypeChangeProcessor {
   // USS's ability to delete local data upcon commit completion.
   virtual bool IsEntityUnsynced(const std::string& storage_key) = 0;
 
+  // Returns the creation timestamp of the sync entity, or a null time if the
+  // entity is not tracked.
+  virtual base::Time GetEntityCreationTime(
+      const std::string& storage_key) const = 0;
+
+  // Returns the modification timestamp of the sync entity, or a null time if
+  // the entity is not tracked.
+  virtual base::Time GetEntityModificationTime(
+      const std::string& storage_key) const = 0;
+
   // Pass the pointer to the processor so that the processor can notify the
   // bridge of various events; |bridge| must not be nullptr and must outlive
   // this object.

components/sync/model/model_type_sync_bridge.cc

@@ -64,6 +64,10 @@ ModelTypeChangeProcessor* ModelTypeSyncBridge::change_processor() {
   return change_processor_.get();
 }
 
+const ModelTypeChangeProcessor* ModelTypeSyncBridge::change_processor() const {
+  return change_processor_.get();
+}
+
 base::Optional<ModelError>
 ModelTypeSyncBridge::ApplySyncChangesWithNewEncryptionRequirements(
     std::unique_ptr<MetadataChangeList> metadata_change_list,

components/sync/model/model_type_sync_bridge.h

@@ -178,6 +178,7 @@ class ModelTypeSyncBridge {
   // Put(). The changing metadata should be stored to persistent storage
   // before or atomically with the model changes.
   ModelTypeChangeProcessor* change_processor();
+  const ModelTypeChangeProcessor* change_processor() const;
 
   // Similar to ApplySyncChanges(), but notifies the bridge that the processor
   // is about to recommit all data due to encryption changes.

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -531,6 +531,24 @@ bool ClientTagBasedModelTypeProcessor::IsEntityUnsynced(
   return entity->IsUnsynced();
 }
 
+base::Time ClientTagBasedModelTypeProcessor::GetEntityCreationTime(
+    const std::string& storage_key) const {
+  const ProcessorEntityTracker* entity = GetEntityForStorageKey(storage_key);
+  if (entity == nullptr) {
+    return base::Time();
+  }
+  return ProtoTimeToTime(entity->metadata().creation_time());
+}
+
+base::Time ClientTagBasedModelTypeProcessor::GetEntityModificationTime(
+    const std::string& storage_key) const {
+  const ProcessorEntityTracker* entity = GetEntityForStorageKey(storage_key);
+  if (entity == nullptr) {
+    return base::Time();
+  }
+  return ProtoTimeToTime(entity->metadata().modification_time());
+}
+
 void ClientTagBasedModelTypeProcessor::NudgeForCommitIfNeeded() {
   // Don't bother sending anything if there's no one to send to.
   if (!IsConnected())
@@ -1221,12 +1239,28 @@ ClientTagBasedModelTypeProcessor::GetEntityForStorageKey(
              : GetEntityForTagHash(iter->second);
 }
 
+const ProcessorEntityTracker*
+ClientTagBasedModelTypeProcessor::GetEntityForStorageKey(
+    const std::string& storage_key) const {
+  auto iter = storage_key_to_tag_hash_.find(storage_key);
+  return iter == storage_key_to_tag_hash_.end()
+             ? nullptr
+             : GetEntityForTagHash(iter->second);
+}
+
 ProcessorEntityTracker* ClientTagBasedModelTypeProcessor::GetEntityForTagHash(
     const std::string& tag_hash) {
   auto it = entities_.find(tag_hash);
   return it != entities_.end() ? it->second.get() : nullptr;
 }
 
+const ProcessorEntityTracker*
+ClientTagBasedModelTypeProcessor::GetEntityForTagHash(
+    const std::string& tag_hash) const {
+  auto it = entities_.find(tag_hash);
+  return it != entities_.end() ? it->second.get() : nullptr;
+}
+
 ProcessorEntityTracker* ClientTagBasedModelTypeProcessor::CreateEntity(
     const std::string& storage_key,
     const EntityData& data) {

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -70,6 +70,10 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   void UntrackEntityForClientTagHash(
       const std::string& client_tag_hash) override;
   bool IsEntityUnsynced(const std::string& storage_key) override;
+  base::Time GetEntityCreationTime(
+      const std::string& storage_key) const override;
+  base::Time GetEntityModificationTime(
+      const std::string& storage_key) const override;
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;
@@ -190,9 +194,13 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   // Gets the entity for the given storage key, or null if there isn't one.
   ProcessorEntityTracker* GetEntityForStorageKey(
       const std::string& storage_key);
+  const ProcessorEntityTracker* GetEntityForStorageKey(
+      const std::string& storage_key) const;
 
   // Gets the entity for the given tag hash, or null if there isn't one.
   ProcessorEntityTracker* GetEntityForTagHash(const std::string& tag_hash);
+  const ProcessorEntityTracker* GetEntityForTagHash(
+      const std::string& tag_hash) const;
 
   // Create an entity in the entity map for |storage_key| and return a pointer
   // to it.

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/run_loop.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_task_environment.h"
+#include "base/threading/platform_thread.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/storage_option.h"
 #include "components/sync/base/time.h"
@@ -695,10 +696,16 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
 // Thoroughly tests the data generated by a local item creation.
 TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldCommitLocalCreation) {
   InitializeToReadyState();
-  EXPECT_EQ(0U, worker()->GetNumPendingCommits());
+  ASSERT_EQ(0U, worker()->GetNumPendingCommits());
+  ASSERT_FALSE(type_processor()->IsEntityUnsynced(kKey1));
 
   bridge()->WriteItem(kKey1, kValue1);
 
+  EXPECT_TRUE(type_processor()->IsEntityUnsynced(kKey1));
+  EXPECT_FALSE(type_processor()->GetEntityCreationTime(kKey1).is_null());
+  EXPECT_EQ(type_processor()->GetEntityCreationTime(kKey1),
+            type_processor()->GetEntityModificationTime(kKey1));
+
   // Verify the commit request this operation has triggered.
   worker()->VerifyPendingCommits({{kHash1}});
   const CommitRequestData& tag1_request_data =
@@ -727,6 +734,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldCommitLocalCreation) {
   EXPECT_TRUE(metadata.has_specifics_hash());
 
   worker()->AckOnePendingCommit();
+  EXPECT_FALSE(type_processor()->IsEntityUnsynced(kKey1));
   EXPECT_EQ(1U, db().metadata_count());
   const EntityMetadata acked_metadata = db().GetMetadata(kKey1);
   EXPECT_TRUE(acked_metadata.has_server_id());
@@ -807,7 +815,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   EXPECT_NE(metadata_v1.specifics_hash(), metadata_v2.specifics_hash());
 }
 
-// Creates a new local item then modifies it.
+// Creates a new local item, then modifies it after it has been acked.
 // Thoroughly tests data generated by modification of server-unknown item.
 TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldCommitLocalUpdate) {
   InitializeToReadyState();
@@ -821,10 +829,94 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldCommitLocalUpdate) {
   const EntityData& data_v1 = request_data_v1.entity.value();
   const EntityMetadata metadata_v1 = db().GetMetadata(kKey1);
 
+  worker()->AckOnePendingCommit();
+  ASSERT_FALSE(type_processor()->IsEntityUnsynced(kKey1));
+  const base::Time ctime = type_processor()->GetEntityCreationTime(kKey1);
+  ASSERT_FALSE(ctime.is_null());
+  ASSERT_EQ(ctime, type_processor()->GetEntityModificationTime(kKey1));
+
+  // Make sure the clock advances.
+  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
+  ASSERT_NE(ctime, base::Time::Now());
+
+  bridge()->WriteItem(kKey1, kValue2);
+  EXPECT_EQ(1U, db().metadata_count());
+  worker()->VerifyPendingCommits({{kHash1}});
+
+  EXPECT_TRUE(type_processor()->IsEntityUnsynced(kKey1));
+  EXPECT_EQ(ctime, type_processor()->GetEntityCreationTime(kKey1));
+  const base::Time mtime = type_processor()->GetEntityModificationTime(kKey1);
+  EXPECT_NE(ctime, mtime);
+
+  const CommitRequestData& request_data_v2 =
+      worker()->GetLatestPendingCommitForHash(kHash1);
+  const EntityData& data_v2 = request_data_v2.entity.value();
+  const EntityMetadata metadata_v2 = db().GetMetadata(kKey1);
+
+  // Test some of the relations between old and new commit requests.
+  EXPECT_GT(request_data_v2.sequence_number, request_data_v1.sequence_number);
+  EXPECT_EQ(data_v1.specifics.preference().value(), kValue1);
+
+  // Perform a thorough examination of the update-generated request.
+  EXPECT_NE(kUncommittedVersion, request_data_v2.base_version);
+  EXPECT_FALSE(data_v2.id.empty());
+  EXPECT_EQ(ctime, data_v2.creation_time);
+  EXPECT_EQ(mtime, data_v2.modification_time);
+  EXPECT_EQ(kKey1, data_v2.non_unique_name);
+  EXPECT_FALSE(data_v2.is_deleted());
+  EXPECT_EQ(kKey1, data_v2.specifics.preference().name());
+  EXPECT_EQ(kValue2, data_v2.specifics.preference().value());
+
+  // Perform a thorough examination of the local sync metadata.
+  EXPECT_FALSE(metadata_v1.has_server_id());
+  EXPECT_FALSE(metadata_v1.is_deleted());
+  EXPECT_EQ(1, metadata_v1.sequence_number());
+  EXPECT_EQ(0, metadata_v1.acked_sequence_number());
+  EXPECT_EQ(kUncommittedVersion, metadata_v1.server_version());
+
+  EXPECT_FALSE(metadata_v2.server_id().empty());
+  EXPECT_FALSE(metadata_v2.is_deleted());
+  EXPECT_EQ(2, metadata_v2.sequence_number());
+  EXPECT_EQ(1, metadata_v2.acked_sequence_number());
+  EXPECT_NE(kUncommittedVersion, metadata_v2.server_version());
+
+  EXPECT_EQ(metadata_v1.client_tag_hash(), metadata_v2.client_tag_hash());
+  EXPECT_NE(metadata_v1.specifics_hash(), metadata_v2.specifics_hash());
+}
+
+// Same as above, but modifies the item BEFORE it has been acked.
+// Thoroughly tests data generated by modification of server-unknown item.
+TEST_F(ClientTagBasedModelTypeProcessorTest,
+       ShouldCommitLocalUpdateBeforeCreationAck) {
+  InitializeToReadyState();
+
+  bridge()->WriteItem(kKey1, kValue1);
+  ASSERT_EQ(1U, db().metadata_count());
+  worker()->VerifyPendingCommits({{kHash1}});
+
+  const CommitRequestData& request_data_v1 =
+      worker()->GetLatestPendingCommitForHash(kHash1);
+  const EntityData& data_v1 = request_data_v1.entity.value();
+  const EntityMetadata metadata_v1 = db().GetMetadata(kKey1);
+
+  ASSERT_TRUE(type_processor()->IsEntityUnsynced(kKey1));
+  const base::Time ctime = type_processor()->GetEntityCreationTime(kKey1);
+  ASSERT_EQ(ctime, type_processor()->GetEntityModificationTime(kKey1));
+  ASSERT_FALSE(ctime.is_null());
+
+  // Make sure the clock advances.
+  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
+  ASSERT_NE(ctime, base::Time::Now());
+
   bridge()->WriteItem(kKey1, kValue2);
   EXPECT_EQ(1U, db().metadata_count());
   worker()->VerifyPendingCommits({{kHash1}, {kHash1}});
 
+  EXPECT_TRUE(type_processor()->IsEntityUnsynced(kKey1));
+  EXPECT_EQ(ctime, type_processor()->GetEntityCreationTime(kKey1));
+  const base::Time mtime = type_processor()->GetEntityModificationTime(kKey1);
+  EXPECT_NE(mtime, ctime);
+
   const CommitRequestData& request_data_v2 =
       worker()->GetLatestPendingCommitForHash(kHash1);
   const EntityData& data_v2 = request_data_v2.entity.value();
@@ -837,13 +929,14 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldCommitLocalUpdate) {
   // Perform a thorough examination of the update-generated request.
   EXPECT_EQ(kUncommittedVersion, request_data_v2.base_version);
   EXPECT_TRUE(data_v2.id.empty());
-  EXPECT_FALSE(data_v2.creation_time.is_null());
-  EXPECT_FALSE(data_v2.modification_time.is_null());
+  EXPECT_EQ(ctime, data_v2.creation_time);
+  EXPECT_EQ(mtime, data_v2.modification_time);
   EXPECT_EQ(kKey1, data_v2.non_unique_name);
   EXPECT_FALSE(data_v2.is_deleted());
   EXPECT_EQ(kKey1, data_v2.specifics.preference().name());
   EXPECT_EQ(kValue2, data_v2.specifics.preference().value());
 
+  // Perform a thorough examination of the local sync metadata.
   EXPECT_FALSE(metadata_v1.has_server_id());
   EXPECT_FALSE(metadata_v1.is_deleted());
   EXPECT_EQ(1, metadata_v1.sequence_number());
@@ -865,11 +958,19 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldCommitLocalUpdate) {
 TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldIgnoreRedundantLocalUpdate) {
   InitializeToReadyState();
   bridge()->WriteItem(kKey1, kValue1);
-  EXPECT_EQ(1U, db().metadata_count());
+  ASSERT_EQ(1U, db().metadata_count());
   worker()->VerifyPendingCommits({{kHash1}});
 
+  const base::Time ctime = type_processor()->GetEntityCreationTime(kKey1);
+  const base::Time mtime = type_processor()->GetEntityModificationTime(kKey1);
+  ASSERT_FALSE(ctime.is_null());
+  ASSERT_FALSE(mtime.is_null());
+
   bridge()->WriteItem(kKey1, kValue1);
   worker()->VerifyPendingCommits({{kHash1}});
+
+  EXPECT_EQ(ctime, type_processor()->GetEntityCreationTime(kKey1));
+  EXPECT_EQ(mtime, type_processor()->GetEntityModificationTime(kKey1));
 }
 
 // Thoroughly tests the data generated by a server item creation.