[Sync] Add callbacks for sharing messages.

Extend the SharingMessageBridge interface with callbacks on each commit
event (successful or failed).

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

chrome/browser/sharing/sharing_message_bridge.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/sync/protocol/sharing_message_specifics.pb.h"
@@ -18,13 +19,17 @@ class ModelTypeControllerDelegate;
 // Class to provide an interface to send sharing messages using Sync.
 class SharingMessageBridge : public KeyedService {
  public:
-  // TODO(crbug.com/1034930): take callbacks once commit error propagation back
-  // to the bridge is implemented.
+  using CommitFinishedCallback =
+      base::OnceCallback<void(const sync_pb::SharingMessageCommitError&)>;
+
+  // Sends Sharing Message to Sync server. |on_commit_callback| will be called
+  // when commit attempt finishes (either successfully or unsuccessfully).
   // 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;
+      std::unique_ptr<sync_pb::SharingMessageSpecifics> specifics,
+      CommitFinishedCallback on_commit_callback) = 0;
 
   // Returns the delegate for the controller, i.e. sync integration point.
   virtual base::WeakPtr<syncer::ModelTypeControllerDelegate>

chrome/browser/sharing/sharing_message_bridge_impl.cc

@@ -11,10 +11,18 @@
 
 namespace {
 
+syncer::ClientTagHash GetClientTagHashFromStorageKey(
+    const std::string& storage_key) {
+  return syncer::ClientTagHash::FromUnhashed(syncer::SHARING_MESSAGE,
+                                             storage_key);
+}
+
 std::unique_ptr<syncer::EntityData> MoveToEntityData(
     std::unique_ptr<sync_pb::SharingMessageSpecifics> specifics) {
   auto entity_data = std::make_unique<syncer::EntityData>();
   entity_data->name = specifics->message_id();
+  entity_data->client_tag_hash =
+      GetClientTagHashFromStorageKey(specifics->message_id());
   entity_data->specifics.set_allocated_sharing_message(specifics.release());
   return entity_data;
 }
@@ -33,7 +41,8 @@ SharingMessageBridgeImpl::SharingMessageBridgeImpl(
 SharingMessageBridgeImpl::~SharingMessageBridgeImpl() = default;
 
 void SharingMessageBridgeImpl::SendSharingMessage(
-    std::unique_ptr<sync_pb::SharingMessageSpecifics> specifics) {
+    std::unique_ptr<sync_pb::SharingMessageSpecifics> specifics,
+    CommitFinishedCallback on_commit_callback) {
   std::unique_ptr<syncer::MetadataChangeList> metadata_change_list =
       CreateMetadataChangeList();
   // Fill in the internal message id with unique generated identifier.
@@ -41,6 +50,10 @@ void SharingMessageBridgeImpl::SendSharingMessage(
   specifics->set_message_id(message_id);
   std::unique_ptr<syncer::EntityData> entity_data =
       MoveToEntityData(std::move(specifics));
+  const auto result =
+      commit_callbacks_.emplace(GetClientTagHashFromStorageKey(message_id),
+                                std::move(on_commit_callback));
+  DCHECK(result.second);
   change_processor()->Put(message_id, std::move(entity_data),
                           metadata_change_list.get());
 }
@@ -71,9 +84,17 @@ base::Optional<syncer::ModelError> SharingMessageBridgeImpl::MergeSyncData(
 
 base::Optional<syncer::ModelError> SharingMessageBridgeImpl::ApplySyncChanges(
     std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
-    syncer::EntityChangeList entity_data) {
-  // This data type is commit only and does not store any data in persistent
-  // storage. We can ignore any data coming from the server.
+    syncer::EntityChangeList entity_changes) {
+  sync_pb::SharingMessageCommitError no_error_message;
+  no_error_message.set_error_code(sync_pb::SharingMessageCommitError::NONE);
+  for (const std::unique_ptr<syncer::EntityChange>& change : entity_changes) {
+    // For commit-only data type we expect only |ACTION_DELETE| changes.
+    DCHECK_EQ(syncer::EntityChange::ACTION_DELETE, change->type());
+
+    const syncer::ClientTagHash client_tag_hash =
+        GetClientTagHashFromStorageKey(change->storage_key());
+    ProcessCommitResponse(client_tag_hash, no_error_message);
+  }
   return {};
 }
 
@@ -98,3 +119,27 @@ std::string SharingMessageBridgeImpl::GetStorageKey(
   DCHECK(entity_data.specifics.has_sharing_message());
   return entity_data.specifics.sharing_message().message_id();
 }
+
+void SharingMessageBridgeImpl::OnCommitAttemptErrors(
+    const syncer::FailedCommitResponseDataList& error_response_list) {
+  for (const syncer::FailedCommitResponseData& response : error_response_list) {
+    // TODO(rushans): untrack entity in change processor on error. We cannot
+    // untrack it by only client tag hash and there is no storage key in
+    // response data.
+    ProcessCommitResponse(response.client_tag_hash,
+                          response.sharing_message_error);
+  }
+}
+
+void SharingMessageBridgeImpl::ProcessCommitResponse(
+    const syncer::ClientTagHash& client_tag_hash,
+    const sync_pb::SharingMessageCommitError& commit_error_message) {
+  const auto iter = commit_callbacks_.find(client_tag_hash);
+  if (iter == commit_callbacks_.end()) {
+    // TODO(crbug.com/1034930): mark as NOTREACHABLE() when the entity will be
+    // untracked on commit errors.
+    return;
+  }
+  std::move(iter->second).Run(commit_error_message);
+  commit_callbacks_.erase(iter);
+}

chrome/browser/sharing/sharing_message_bridge_impl.h

@@ -25,7 +25,8 @@ class SharingMessageBridgeImpl : public SharingMessageBridge,
 
   // SharingMessageBridge implementation.
   void SendSharingMessage(
-      std::unique_ptr<sync_pb::SharingMessageSpecifics> specifics) override;
+      std::unique_ptr<sync_pb::SharingMessageSpecifics> specifics,
+      CommitFinishedCallback on_commit_callback) override;
   base::WeakPtr<syncer::ModelTypeControllerDelegate> GetControllerDelegate()
       override;
 
@@ -42,6 +43,21 @@ 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;
+  void OnCommitAttemptErrors(
+      const syncer::FailedCommitResponseDataList& error_response_list) override;
+
+  size_t GetCallbacksCountForTesting() const {
+    return commit_callbacks_.size();
+  }
+
+ private:
+  // Sends commit outcome via callback for |client_tag_hash| and removes it from
+  // callbacks mapping.
+  void ProcessCommitResponse(
+      const syncer::ClientTagHash& client_tag_hash,
+      const sync_pb::SharingMessageCommitError& commit_error);
+
+  std::map<syncer::ClientTagHash, CommitFinishedCallback> commit_callbacks_;
 };
 
 #endif  // CHROME_BROWSER_SHARING_SHARING_MESSAGE_BRIDGE_IMPL_H_

chrome/browser/sharing/sharing_message_bridge_impl_unittest.cc

@@ -4,9 +4,12 @@
 
 #include "chrome/browser/sharing/sharing_message_bridge_impl.h"
 
+#include "base/bind_helpers.h"
 #include "base/run_loop.h"
+#include "base/test/mock_callback.h"
 #include "base/test/task_environment.h"
 #include "components/sync/model/metadata_batch.h"
+#include "components/sync/model/metadata_change_list.h"
 #include "components/sync/model/mock_model_type_change_processor.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -56,13 +59,15 @@ TEST_F(SharingMessageBridgeTest, ShouldWriteMessagesToProcessor) {
   syncer::EntityData entity_data;
   EXPECT_CALL(*processor(), Put(_, _, _))
       .WillRepeatedly(SaveArgPointeeMove<1>(&entity_data));
-  bridge()->SendSharingMessage(CreateSpecifics("test_payload"));
+  bridge()->SendSharingMessage(CreateSpecifics("test_payload"),
+                               base::DoNothing());
 
   EXPECT_TRUE(entity_data.specifics.has_sharing_message());
   EXPECT_EQ(entity_data.specifics.sharing_message().payload(), "test_payload");
 
   entity_data.specifics.Clear();
-  bridge()->SendSharingMessage(CreateSpecifics("another_payload"));
+  bridge()->SendSharingMessage(CreateSpecifics("another_payload"),
+                               base::DoNothing());
 
   EXPECT_TRUE(entity_data.specifics.has_sharing_message());
   EXPECT_EQ(entity_data.specifics.sharing_message().payload(),
@@ -76,12 +81,69 @@ TEST_F(SharingMessageBridgeTest, ShouldGenerateUniqueStorageKey) {
   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"));
+  bridge()->SendSharingMessage(CreateSpecifics("payload"), base::DoNothing());
+  bridge()->SendSharingMessage(CreateSpecifics("another_payload"),
+                               base::DoNothing());
 
   EXPECT_FALSE(first_storage_key.empty());
   EXPECT_FALSE(second_storage_key.empty());
   EXPECT_NE(first_storage_key, second_storage_key);
 }
 
+TEST_F(SharingMessageBridgeTest, ShouldInvokeCallbackOnSuccess) {
+  std::string storage_key;
+  EXPECT_CALL(*processor(), Put(_, _, _)).WillOnce(SaveArg<0>(&storage_key));
+
+  base::MockCallback<SharingMessageBridge::CommitFinishedCallback> callback;
+  sync_pb::SharingMessageCommitError commit_error;
+  EXPECT_CALL(callback, Run).WillOnce(SaveArg<0>(&commit_error));
+
+  bridge()->SendSharingMessage(CreateSpecifics("payload"), callback.Get());
+
+  // The callback should be called only after committing data.
+  EXPECT_FALSE(commit_error.has_error_code());
+
+  // Mark data as committed.
+  syncer::EntityChangeList change_list;
+  change_list.push_back(syncer::EntityChange::CreateDelete(storage_key));
+  bridge()->ApplySyncChanges(nullptr, std::move(change_list));
+
+  EXPECT_TRUE(commit_error.has_error_code());
+  EXPECT_EQ(commit_error.error_code(),
+            sync_pb::SharingMessageCommitError::NONE);
+  EXPECT_EQ(bridge()->GetCallbacksCountForTesting(), 0u);
+}
+
+TEST_F(SharingMessageBridgeTest, ShouldInvokeCallbackOnFailure) {
+  syncer::EntityData entity_data;
+  EXPECT_CALL(*processor(), Put(_, _, _))
+      .WillRepeatedly(SaveArgPointeeMove<1>(&entity_data));
+
+  base::MockCallback<SharingMessageBridge::CommitFinishedCallback> callback;
+  sync_pb::SharingMessageCommitError commit_error;
+  EXPECT_CALL(callback, Run).WillOnce(SaveArg<0>(&commit_error));
+
+  bridge()->SendSharingMessage(CreateSpecifics("payload"), callback.Get());
+
+  EXPECT_FALSE(entity_data.client_tag_hash.value().empty());
+
+  // The callback should be called only after committing data.
+  EXPECT_FALSE(commit_error.has_error_code());
+
+  syncer::FailedCommitResponseDataList response_list;
+  {
+    syncer::FailedCommitResponseData response;
+    response.client_tag_hash = entity_data.client_tag_hash;
+    response.sharing_message_error.set_error_code(
+        sync_pb::SharingMessageCommitError::PERMISSION_DENIED);
+    response_list.push_back(std::move(response));
+  }
+  bridge()->OnCommitAttemptErrors(response_list);
+
+  EXPECT_TRUE(commit_error.has_error_code());
+  EXPECT_EQ(commit_error.error_code(),
+            sync_pb::SharingMessageCommitError::PERMISSION_DENIED);
+  EXPECT_EQ(bridge()->GetCallbacksCountForTesting(), 0u);
+}
+
 }  // namespace