Provide a mechanism for the GCM driver to send message receipts to GCM.

components/gcm_driver/gcm_client.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_GCM_DRIVER_GCM_CLIENT_H_
 #define COMPONENTS_GCM_DRIVER_GCM_CLIENT_H_
 
 #include <stdint.h>
 
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/memory/linked_ptr.h"
 #include "components/gcm_driver/common/gcm_messages.h"
 #include "components/gcm_driver/crypto/gcm_encryption_provider.h"
 #include "components/gcm_driver/gcm_activity.h"
+#include "components/gcm_driver/gcm_message_status.h"
 #include "components/gcm_driver/registration_info.h"
 
 template <class T> class scoped_refptr;
 
 namespace base {
 class FilePath;
 class SequencedTaskRunner;
 class Timer;
 }
 
@@ skipping 103 matching lines @@
     RecordedActivities recorded_activities;
   };
 
   // Information about account.
   struct AccountTokenInfo {
     std::string account_id;
     std::string email;
     std::string access_token;
   };
 
+  typedef base::Callback<void(GCMMessageStatus)> MessageReceiptCallback;

[Peter Beverloo, 2017/01/13 01:46:11]

Architecturally, why did you choose to keep this in the GCMClient rather than
defining it in gcm_message_status.h?

My thinking is that users of receipt callbacks will need to include
gcm_message_status.h anyway. That way we can avoid having to include
gcm_client.h, which really is more of an implementation detail. (Given that it
doesn't exist on Android, for example.)

[Peter Beverloo, 2017/01/13 01:46:11]

nit: C++11ize

using MessageReceiptCallback = base::Callback<void(GCMMessageStatus)>;

[harkness, 2017/01/19 13:20:41]

Done.

[harkness, 2017/01/19 13:20:41]

The main architectural reason was to have it in the same file that defined
SendMessageReceipt, so if someone was reading that code and wanted to look up
the definition, it would be right there.

I did a bit of looking, and I think all the files which need the definition
already had included gcm_client.h because of other reasons  (often
GCMClient::Result), and actually there are a lot of files that just passively
pass the callback through without needing to know the message status enum.

I do think it makes more sense in the gcm_client.h, but I don't feel super
strongly about it so if you want I can move it. Your Android point is a good
one.

+
   // A delegate interface that allows the GCMClient instance to interact with
   // its caller, i.e. notifying asynchronous event.
   class Delegate {
    public:
     // Called when the registration completed successfully or an error occurs.
     // |registration_info|: the specific information required for the
     //                      registration.
     // |registration_id|: non-empty if the registration completed successfully.
     // |result|: the type of the error if an error occured, success otherwise.
     virtual void OnRegisterFinished(
@@ skipping 14 matching lines @@
     // |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 OnSendFinished(const std::string& app_id,
                                 const std::string& message_id,
                                 Result result) = 0;
 
     // Called when a message has been received.
     // |app_id|: application ID.
     // |message|: message received.
+    // |callback|: callback to invoke when processing the message is complete.
     virtual void OnMessageReceived(const std::string& app_id,
-                                   const IncomingMessage& message) = 0;
+                                   const IncomingMessage& message,
+                                   const MessageReceiptCallback& callback) = 0;

[Peter Beverloo, 2017/01/13 01:46:11]

(This will largely apply to app handlers, but it's important to document it
here, too.)

App handlers are certainly not required to invoke |callback|. Delivery receipts
should only be used by feature teams who would benefit from having additional
information available. That should be reflected in both the documentation
(:179), as well as the parameter name (:182).

I'd suggest using |receipt_callback| for the parameter name, although
|optional_receipt_callback| would work too if you're not afraid of long names.

In addition, please also document the threading guarantees.

[harkness, 2017/01/19 13:20:41]

Done.

 
     // Called when some messages have been deleted from the server.
     // |app_id|: application ID.
     virtual void OnMessagesDeleted(const std::string& app_id) = 0;
 
     // Called when a message failed to send to the server.
     // |app_id|: application ID.
     // |send_error_detials|: Details of the send error event, like mesasge ID.
     virtual void OnMessageSendError(
         const std::string& app_id,
@@ skipping 81 matching lines @@
 
   // Sends a message to a given receiver. Delegate::OnSendFinished will be
   // called asynchronously upon completion.
   // |app_id|: application ID.
   // |receiver_id|: registration ID of the receiver party.
   // |message|: message to be sent.
   virtual void Send(const std::string& app_id,
                     const std::string& receiver_id,
                     const OutgoingMessage& message) = 0;
 
+  // Send a message to GCM with information about the final status of a
+  // previously received message. This can be run as a result of a callback
+  // being executed from an AppHandler.
+  virtual void SendMessageReceipt(const std::string& message_id,
+                                  const std::string& app_id,
+                                  GCMMessageStatus status) = 0;

[Peter Beverloo, 2017/01/13 01:46:11]

This doesn't have to live in the gcm_client.h interface, and can instead live as
a private method on GCMClientImpl.

[harkness, 2017/01/19 13:20:41]

Good point! I've moved it.

+
   // Records a decryption failure due to |result| for the |app_id|.
   virtual void RecordDecryptionFailure(
       const std::string& app_id,
       GCMEncryptionProvider::DecryptionResult result) = 0;
 
   // Enables or disables internal activity recording.
   virtual void SetRecording(bool recording) = 0;
 
   // Clear all recorded GCM activity logs.
   virtual void ClearActivityLogs() = 0;
@@ skipping 39 matching lines @@
   // to be set, and allows that component to later revoke the setting. It should
   // be unique.
   virtual void AddHeartbeatInterval(const std::string& scope,
                                     int interval_ms) = 0;
   virtual void RemoveHeartbeatInterval(const std::string& scope) = 0;
 };
 
 }  // namespace gcm
 
 #endif  // COMPONENTS_GCM_DRIVER_GCM_CLIENT_H_