[Sync] Add id field to SharingMessageSpecifics.

Generate unique identifier for each message and store it in a new field
in specifics. This id is used by bridge as storage key and client tag.

Changing tag numbers in the proto is fine because it is not used anywhere
yet.

Bug: 1034932
Change-Id: I5c79fdb70d8897ac300e49e98e78760b8394e52c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2015024
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Alex Chau <alexchau@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734433}

chrome/browser/sharing/sharing_message_bridge.h

@@ -20,6 +20,9 @@ class SharingMessageBridge : public KeyedService {
  public:
   // TODO(crbug.com/1034930): take callbacks once commit error propagation back
   // to the bridge is implemented.
+  // TODO(crbug.com/1034932): take each parameter separately and construct
+  // specifics inside. Currently this method updates given |specifics| and
+  // fills in |message_id| field.
   virtual void SendSharingMessage(
       std::unique_ptr<sync_pb::SharingMessageSpecifics> specifics) = 0;
 

chrome/browser/sharing/sharing_message_bridge_impl.cc

@@ -14,10 +14,7 @@ namespace {
 std::unique_ptr<syncer::EntityData> MoveToEntityData(
     std::unique_ptr<sync_pb::SharingMessageSpecifics> specifics) {
   auto entity_data = std::make_unique<syncer::EntityData>();
-  const std::string guid = base::GenerateGUID();
-  entity_data->client_tag_hash =
-      syncer::ClientTagHash::FromUnhashed(syncer::SHARING_MESSAGE, guid);
-  entity_data->name = guid;
+  entity_data->name = specifics->message_id();
   entity_data->specifics.set_allocated_sharing_message(specifics.release());
   return entity_data;
 }
@@ -39,10 +36,12 @@ void SharingMessageBridgeImpl::SendSharingMessage(
     std::unique_ptr<sync_pb::SharingMessageSpecifics> specifics) {
   std::unique_ptr<syncer::MetadataChangeList> metadata_change_list =
       CreateMetadataChangeList();
+  // Fill in the internal message id with unique generated identifier.
+  const std::string message_id = base::GenerateGUID();
+  specifics->set_message_id(message_id);
   std::unique_ptr<syncer::EntityData> entity_data =
       MoveToEntityData(std::move(specifics));
-  const std::string empty_storage_key;
-  change_processor()->Put(empty_storage_key, std::move(entity_data),
+  change_processor()->Put(message_id, std::move(entity_data),
                           metadata_change_list.get());
 }
 
@@ -96,16 +95,6 @@ std::string SharingMessageBridgeImpl::GetClientTag(
 
 std::string SharingMessageBridgeImpl::GetStorageKey(
     const syncer::EntityData& entity_data) {
-  NOTREACHED();
-  return "";
-}
-
-// This is commit-only data type without storing any data on persistent storage.
-// We do not need keys here.
-bool SharingMessageBridgeImpl::SupportsGetClientTag() const {
-  return false;
-}
-
-bool SharingMessageBridgeImpl::SupportsGetStorageKey() const {
-  return false;
+  DCHECK(entity_data.specifics.has_sharing_message());
+  return entity_data.specifics.sharing_message().message_id();
 }

chrome/browser/sharing/sharing_message_bridge_impl.h

@@ -42,8 +42,6 @@ class SharingMessageBridgeImpl : public SharingMessageBridge,
   void GetAllDataForDebugging(DataCallback callback) override;
   std::string GetClientTag(const syncer::EntityData& entity_data) override;
   std::string GetStorageKey(const syncer::EntityData& entity_data) override;
-  bool SupportsGetClientTag() const override;
-  bool SupportsGetStorageKey() const override;
 };
 
 #endif  // CHROME_BROWSER_SHARING_SHARING_MESSAGE_BRIDGE_IMPL_H_

chrome/browser/sharing/sharing_message_bridge_impl_unittest.cc

@@ -67,15 +67,21 @@ TEST_F(SharingMessageBridgeTest, ShouldWriteMessagesToProcessor) {
   EXPECT_TRUE(entity_data.specifics.has_sharing_message());
   EXPECT_EQ(entity_data.specifics.sharing_message().payload(),
             "another_payload");
+  EXPECT_FALSE(bridge()->GetStorageKey(entity_data).empty());
 }
 
-TEST_F(SharingMessageBridgeTest, ShouldNotGenerateStorageKey) {
-  std::string storage_key;
-  EXPECT_CALL(*processor(), Put(_, _, _)).WillOnce(SaveArg<0>(&storage_key));
-  bridge()->SendSharingMessage(
-      std::make_unique<sync_pb::SharingMessageSpecifics>());
+TEST_F(SharingMessageBridgeTest, ShouldGenerateUniqueStorageKey) {
+  std::string first_storage_key;
+  std::string second_storage_key;
+  EXPECT_CALL(*processor(), Put(_, _, _))
+      .WillOnce(SaveArg<0>(&first_storage_key))
+      .WillOnce(SaveArg<0>(&second_storage_key));
+  bridge()->SendSharingMessage(CreateSpecifics("payload"));
+  bridge()->SendSharingMessage(CreateSpecifics("another_payload"));
 
-  EXPECT_TRUE(storage_key.empty());
+  EXPECT_FALSE(first_storage_key.empty());
+  EXPECT_FALSE(second_storage_key.empty());
+  EXPECT_NE(first_storage_key, second_storage_key);
 }
 
 }  // namespace

components/sync/protocol/proto_visitors.h

@@ -795,6 +795,7 @@ VISIT_PROTO_FIELDS(const sync_pb::SessionWindow& proto) {
 }
 
 VISIT_PROTO_FIELDS(const sync_pb::SharingMessageSpecifics& proto) {
+  VISIT(message_id);
   VISIT(channel_configuration);
   VISIT_BYTES(payload);
 }

components/sync/protocol/sharing_message_specifics.proto

@@ -17,6 +17,9 @@ option optimize_for = LITE_RUNTIME;
 package sync_pb;
 
 message SharingMessageSpecifics {
+  // Unique identifier of message.
+  optional string message_id = 1;
+
   message ChannelConfiguration {
     message ServerChannelConfiguration {
       // Where to send the message to.
@@ -47,13 +50,13 @@ message SharingMessageSpecifics {
     }
   }
 
-  optional ChannelConfiguration channel_configuration = 1;
+  optional ChannelConfiguration channel_configuration = 2;
 
   // Payload encrypted using the target user keys according to WebPush
   // encryption scheme. The payload has to be a valid
   // chrome/browser/sharing/proto/sharing_message.proto serialized using
   // SerializeToString.
-  optional bytes payload = 2;
+  optional bytes payload = 3;
 }
 
 // Used for the server to return fine grained commit errors back to the client.