[GCM] Propagating SendError details all the way to the event.

OnSendError allows to specify more message content than simply and error message
and message ID, but it currently is ignored. This patch fixes that problem by
introducing SendErrorDetails structure and updating the whole stack to handle it
properly.

BUG=350026

Review URL: https://codereview.chromium.org/191443002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@256187 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 98 insertions, 50 deletions

diff --git a/google_apis/gcm/gcm_client.cc b/google_apis/gcm/gcm_client.cc
index 78b4e08..3bdd699 100644
--- a/google_apis/gcm/gcm_client.cc
+++ b/google_apis/gcm/gcm_client.cc
@@ -19,6 +19,10 @@ GCMClient::IncomingMessage::IncomingMessage() {
 GCMClient::IncomingMessage::~IncomingMessage() {
 }
 
+GCMClient::SendErrorDetails::SendErrorDetails() : result(UNKNOWN_ERROR) {}
+
+GCMClient::SendErrorDetails::~SendErrorDetails() {}
+
 GCMClient::GCMStatistics::GCMStatistics()
     : gcm_client_created(false), connection_client_created(false) {
 }
diff --git a/google_apis/gcm/gcm_client.h b/google_apis/gcm/gcm_client.h
index 7bffa4c..0d68712 100644
--- a/google_apis/gcm/gcm_client.h
+++ b/google_apis/gcm/gcm_client.h
@@ -78,6 +78,16 @@ class GCM_EXPORT GCMClient {
     std::string sender_id;
   };
 
+  // Detailed information of the Send Error event.
+  struct GCM_EXPORT SendErrorDetails {
+    SendErrorDetails();
+    ~SendErrorDetails();
+
+    std::string message_id;
+    MessageData additional_data;
+    Result result;
+  };
+
   // Internal states and activity statistics of a GCM client.
   struct GCM_EXPORT GCMStatistics {
    public:
@@ -130,11 +140,10 @@ class GCM_EXPORT GCMClient {
 
     // Called when a message failed to send to the server.
     // |app_id|: application ID.
-    // |message_id|: ID of the message being sent.
-    // |result|: the type of the error if an error occured, success otherwise.
-    virtual void OnMessageSendError(const std::string& app_id,
-                                    const std::string& message_id,
-                                    Result result) = 0;
+    // |send_error_detials|: Details of the send error event, like mesasge ID.
+    virtual void OnMessageSendError(
+        const std::string& app_id,
+        const SendErrorDetails& send_error_details) = 0;
 
     // Called when the GCM becomes ready. To get to this state, GCMClient
     // finished loading from the GCM store and retrieved the device check-in
diff --git a/google_apis/gcm/gcm_client_impl.cc b/google_apis/gcm/gcm_client_impl.cc
index af20a5a..0c9d0fa 100644
--- a/google_apis/gcm/gcm_client_impl.cc
+++ b/google_apis/gcm/gcm_client_impl.cc
@@ -471,17 +471,21 @@ void GCMClientImpl::OnMessageSentToMCS(int64 user_serial_number,
 
   // TTL_EXCEEDED is singled out here, because it can happen long time after the
   // message was sent. That is why it comes as |OnMessageSendError| event rather
-  // than |OnSendFinished|. All other errors will be raised immediately, through
-  // asynchronous callback.
+  // than |OnSendFinished|. SendErrorDetails.additional_data is left empty.
+  // All other errors will be raised immediately, through asynchronous callback.
   // It is expected that TTL_EXCEEDED will be issued for a message that was
   // previously issued |OnSendFinished| with status SUCCESS.
   // For now, we do not report that the message has been sent and acked
   // successfully.
   // TODO(jianli): Consider adding UMA for this status.
-  if (status == MCSClient::TTL_EXCEEDED)
-    delegate_->OnMessageSendError(app_id, message_id, GCMClient::TTL_EXCEEDED);
-  else if (status != MCSClient::SENT)
+  if (status == MCSClient::TTL_EXCEEDED) {
+    SendErrorDetails send_error_details;
+    send_error_details.message_id = message_id;
+    send_error_details.result = GCMClient::TTL_EXCEEDED;
+    delegate_->OnMessageSendError(app_id, send_error_details);
+  } else if (status != MCSClient::SENT) {
     delegate_->OnSendFinished(app_id, message_id, ToGCMClientResult(status));
+  }
 }
 
 void GCMClientImpl::OnMCSError() {
@@ -497,31 +501,31 @@ void GCMClientImpl::HandleIncomingMessage(const gcm::MCSMessage& message) {
           message.GetProtobuf());
   DCHECK_EQ(data_message_stanza.device_user_id(), kDefaultUserSerialNumber);
 
-  IncomingMessage incoming_message;
-  MessageType message_type = DATA_MESSAGE;
+  // Copying all the data from the stanza to a MessageData object. When present,
+  // keys like kMessageTypeKey or kSendErrorMessageIdKey will be filtered out
+  // later.
+  MessageData message_data;
   for (int i = 0; i < data_message_stanza.app_data_size(); ++i) {
     std::string key = data_message_stanza.app_data(i).key();
-    if (key == kMessageTypeKey)
-      message_type = DecodeMessageType(data_message_stanza.app_data(i).value());
-    else
-      incoming_message.data[key] = data_message_stanza.app_data(i).value();
+    message_data[key] = data_message_stanza.app_data(i).value();
   }
 
+  MessageType message_type = UNKNOWN;
+  MessageData::iterator iter = message_data.find(kMessageTypeKey);
+  if (iter != message_data.end()) {
+    message_type = DecodeMessageType(iter->second);
+    message_data.erase(iter);
+  }
 
   switch (message_type) {
     case DATA_MESSAGE:
-      incoming_message.sender_id = data_message_stanza.from();
-      if (data_message_stanza.has_token())
-        incoming_message.collapse_key = data_message_stanza.token();
-      delegate_->OnMessageReceived(data_message_stanza.category(),
-                                   incoming_message);
+      HandleIncomingDataMessage(data_message_stanza, message_data);
       break;
     case DELETED_MESSAGES:
       delegate_->OnMessagesDeleted(data_message_stanza.category());
       break;
     case SEND_ERROR:
-      NotifyDelegateOnMessageSendError(
-          delegate_, data_message_stanza.category(), incoming_message);
+      HandleIncomingSendError(data_message_stanza, message_data);
       break;
     case UNKNOWN:
     default:  // Treat default the same as UNKNOWN.
@@ -531,16 +535,34 @@ void GCMClientImpl::HandleIncomingMessage(const gcm::MCSMessage& message) {
   }
 }
 
-void GCMClientImpl::NotifyDelegateOnMessageSendError(
-    GCMClient::Delegate* delegate,
-    const std::string& app_id,
-    const IncomingMessage& incoming_message) {
-  MessageData::const_iterator iter =
-      incoming_message.data.find(kSendErrorMessageIdKey);
-  std::string message_id;
-  if (iter != incoming_message.data.end())
-    message_id = iter->second;
-  delegate->OnMessageSendError(app_id, message_id, SERVER_ERROR);
+void GCMClientImpl::HandleIncomingDataMessage(
+    const mcs_proto::DataMessageStanza& data_message_stanza,
+    MessageData& message_data) {
+  IncomingMessage incoming_message;
+  incoming_message.sender_id = data_message_stanza.from();
+  if (data_message_stanza.has_token())
+    incoming_message.collapse_key = data_message_stanza.token();
+  incoming_message.data = message_data;
+  delegate_->OnMessageReceived(data_message_stanza.category(),
+                               incoming_message);
+}
+
+void GCMClientImpl::HandleIncomingSendError(
+    const mcs_proto::DataMessageStanza& data_message_stanza,
+    MessageData& message_data) {
+  SendErrorDetails send_error_details;
+  send_error_details.additional_data = message_data;
+  send_error_details.result = SERVER_ERROR;
+
+  MessageData::iterator iter =
+      send_error_details.additional_data.find(kSendErrorMessageIdKey);
+  if (iter != send_error_details.additional_data.end()) {
+    send_error_details.message_id = iter->second;
+    send_error_details.additional_data.erase(iter);
+  }
+
+  delegate_->OnMessageSendError(data_message_stanza.category(),
+                                send_error_details);
 }
 
 void GCMClientImpl::SetMCSClientForTesting(scoped_ptr<MCSClient> mcs_client) {
diff --git a/google_apis/gcm/gcm_client_impl.h b/google_apis/gcm/gcm_client_impl.h
index f7eafcb..d7f47d0 100644
--- a/google_apis/gcm/gcm_client_impl.h
+++ b/google_apis/gcm/gcm_client_impl.h
@@ -161,16 +161,20 @@ class GCM_EXPORT GCMClientImpl : public GCMClient {
   // Completes the GCM store destroy request.
   void OnGCMStoreDestroyed(bool success);
 
-  // Handles incoming data message and dispatches it the a relevant user
-  // delegate.
+  // Handles incoming data message and dispatches it the delegate of this class.
   void HandleIncomingMessage(const gcm::MCSMessage& message);
 
-  // Fires OnMessageSendError event on |delegate|, with specified |app_id| and
-  // message ID obtained from |incoming_message| if one is available.
-  void NotifyDelegateOnMessageSendError(
-      GCMClient::Delegate* delegate,
-      const std::string& app_id,
-      const IncomingMessage& incoming_message);
+  // Fires OnMessageReceived event on the delegate of this class, based on the
+  // details in |data_message_stanza| and |message_data|.
+  void HandleIncomingDataMessage(
+      const mcs_proto::DataMessageStanza& data_message_stanza,
+      MessageData& message_data);
+
+  // Fires OnMessageSendError event on the delegate of this calss, based on the
+  // details in |data_message_stanza| and |message_data|.
+  void HandleIncomingSendError(
+      const mcs_proto::DataMessageStanza& data_message_stanza,
+      MessageData& message_data);
 
   // For testing purpose only.
   // Sets an |mcs_client_| for testing. Takes the ownership of |mcs_client|.
diff --git a/google_apis/gcm/gcm_client_impl_unittest.cc b/google_apis/gcm/gcm_client_impl_unittest.cc
index 2e1d4c9..7320c00 100644
--- a/google_apis/gcm/gcm_client_impl_unittest.cc
+++ b/google_apis/gcm/gcm_client_impl_unittest.cc
@@ -140,9 +140,9 @@ class GCMClientImplTest : public testing::Test,
                                  const GCMClient::IncomingMessage& message)
       OVERRIDE;
   virtual void OnMessagesDeleted(const std::string& app_id) OVERRIDE;
-  virtual void OnMessageSendError(const std::string& app_id,
-                                  const std::string& message_id,
-                                  GCMClient::Result result) OVERRIDE;
+  virtual void OnMessageSendError(
+      const std::string& app_id,
+      const gcm::GCMClient::SendErrorDetails& send_error_details) OVERRIDE;
   virtual void OnGCMReady() OVERRIDE;
 
   GCMClientImpl* gcm_client() const { return gcm_client_.get(); }
@@ -160,6 +160,9 @@ class GCMClientImplTest : public testing::Test,
   const GCMClient::IncomingMessage& last_message() const {
     return last_message_;
   }
+  const GCMClient::SendErrorDetails& last_error_details() const {
+    return last_error_details_;
+  }
 
   int64 CurrentTime();
 
@@ -180,6 +183,7 @@ class GCMClientImplTest : public testing::Test,
   std::string last_message_id_;
   GCMClient::Result last_result_;
   GCMClient::IncomingMessage last_message_;
+  GCMClient::SendErrorDetails last_error_details_;
 
   scoped_ptr<GCMClientImpl> gcm_client_;
   scoped_ptr<FakeConnectionFactory> connection_factory_;
@@ -338,13 +342,12 @@ void GCMClientImplTest::OnMessagesDeleted(const std::string& app_id) {
   last_app_id_ = app_id;
 }
 
-void GCMClientImplTest::OnMessageSendError(const std::string& app_id,
-                                           const std::string& message_id,
-                                           GCMClient::Result result) {
+void GCMClientImplTest::OnMessageSendError(
+    const std::string& app_id,
+    const gcm::GCMClient::SendErrorDetails& send_error_details) {
   last_event_ = MESSAGE_SEND_ERROR;
   last_app_id_ = app_id;
-  last_message_id_ = message_id;
-  last_result_ = result;
+  last_error_details_ = send_error_details;
 }
 
 int64 GCMClientImplTest::CurrentTime() {
@@ -406,6 +409,7 @@ TEST_F(GCMClientImplTest, DispatchDownstreamMessageSendError) {
   std::map<std::string, std::string> expected_data;
   expected_data["message_type"] = "send_error";
   expected_data["google.message_id"] = "007";
+  expected_data["error_details"] = "some details";
   MCSMessage message(BuildDownstreamMessage(
       "project_id", "app_id", expected_data));
   EXPECT_TRUE(message.IsValid());
@@ -413,7 +417,12 @@ TEST_F(GCMClientImplTest, DispatchDownstreamMessageSendError) {
 
   EXPECT_EQ(MESSAGE_SEND_ERROR, last_event());
   EXPECT_EQ("app_id", last_app_id());
-  EXPECT_EQ("007", last_message_id());
+  EXPECT_EQ("007", last_error_details().message_id);
+  EXPECT_EQ(1UL, last_error_details().additional_data.size());
+  GCMClient::MessageData::const_iterator iter =
+      last_error_details().additional_data.find("error_details");
+  EXPECT_TRUE(iter != last_error_details().additional_data.end());
+  EXPECT_EQ("some details", iter->second);
 }
 
 TEST_F(GCMClientImplTest, DispatchDownstreamMessgaesDeleted) {