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

components/gcm_driver/gcm_client.h

Index: components/gcm_driver/gcm_client.h
diff --git a/components/gcm_driver/gcm_client.h b/components/gcm_driver/gcm_client.h
index c4dd12adc914375e3c8cf118e114589f292a7df5..92e177b8caa75ea58301e6e3e254130c8840e726 100644
--- a/components/gcm_driver/gcm_client.h
+++ b/components/gcm_driver/gcm_client.h
@@ -16,6 +16,7 @@
 #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;
@@ -139,6 +140,8 @@ class GCMClient {
     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 {
@@ -173,8 +176,10 @@ class GCMClient {
     // 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.
@@ -276,6 +281,13 @@ class GCMClient {
                     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,