Skeleton GCMAppHandler for Push API

chrome/browser/services/gcm/push_messaging_service_impl.cc

Index: chrome/browser/services/gcm/push_messaging_service_impl.cc
diff --git a/chrome/browser/services/gcm/push_messaging_service_impl.cc b/chrome/browser/services/gcm/push_messaging_service_impl.cc
index ad78da137fbd693567b627578c2c1193b74af284..8a07e128a6289d2de4aa246db333dcd14cc063eb 100644
--- a/chrome/browser/services/gcm/push_messaging_service_impl.cc
+++ b/chrome/browser/services/gcm/push_messaging_service_impl.cc
@@ -15,9 +15,67 @@ namespace gcm {
 PushMessagingServiceImpl::PushMessagingServiceImpl(
     GCMProfileService* gcm_profile_service)
     : gcm_profile_service_(gcm_profile_service),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  // TODO(mvanouwerkerk): Automatically unregister Service Workers from GCM when
+  // they are uninstalled.
+}
+
+PushMessagingServiceImpl::~PushMessagingServiceImpl() {
+  // TODO(johnme): If it's possible for this to be destroyed before GCMDriver,
+  // then we should RemoveAppHandler all our app handlers.

[fgorski, 2014/06/10 17:09:27]

Perhaps you can do that in Shutdown app handler. Unless you worry about cases
when it would never get called.

[johnme, 2014/06/10 20:33:48]

Not sure, I based this on ExtensionGCMAppHandler. In practice, I don't think
it's possible for GCMDriver to outlive PushMessagingServiceImpl, except perhaps
when GCMProfileService is being torn down and they get destructed in reverse
order of declaration of the member variables, but GCMDriver is unlikely to call
any app handlers from its destructor.

+}
+
+base::WeakPtr<PushMessagingServiceImpl> PushMessagingServiceImpl::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void PushMessagingServiceImpl::LazyLoadAppHandlers() {
+  // TODO(mvanouwerkerk): For each Service Worker that is able to receive push
+  // events, run gcm_profile_service_->driver()->AddAppHandler(sw_id, this)
+}
+
+void PushMessagingGCMAppHandler::ShutdownHandler() {
+  // TODO(mvanouwerkerk): Do any necessary cleanup.
+}
 
-PushMessagingServiceImpl::~PushMessagingServiceImpl() {}
+void PushMessagingGCMAppHandler::OnMessage(
+    const std::string& app_id,
+    const GCMClient::IncomingMessage& message) {
+  // The Push API only exposes a single string of data in the push event fired
+  // on the Service Worker. When developers send messages using GCM to the Push
+  // API, they must pass a single key-value pair, where the key is "data" and
+  // the value is the string they want to be passed to their Service Worker.
+  // For example, they could send the following JSON using the HTTPS GCM API:
+  // {
+  //     "registration_ids": ["FOO", "BAR"],
+  //     "data": {
+  //         "data": "BAZ",

[fgorski, 2014/06/10 17:09:27]

this looks redundant... is that a work in progress still? Or is that already
part of the standard?

[johnme, 2014/06/10 20:33:48]

There are no standards around the GCM part. The Push API just says to provide a
unicode string. Unfortunately, GCM has this quirky legacy behaviour where it
only allows a map<string,string> (even though you pass it in as JSON!) because
that's all Android Bundles support. So this seems the simplest way of sending a
simple string using GCM, though the duplication of data is unfortunate.

+  //     },
+  //     "delay_while_idle": true,
+  // }
+  // TODO(johnme): Make sure this is clearly documented for developers.
+  const GCMClient::MessageData::iterator it = message.data.find("data");
+  if (it != message.data.end()) {
+    const std::string& data = it->second;
+    // TODO(mvanouwerkerk): Fire push event with data on the Service Worker
+    // corresponding to app_id.
+  } else {
+    // Drop the message, as it is invalid.
+    // TODO(mvanouwerkerk): Show a warning in the developer console of the
+    // Service Worker corresponding to app_id.

[fgorski, 2014/06/10 17:09:27]

Add a TODO for diagnostic observers (you likely want to add UMA and an internals
page to know that bad things happen)

[johnme, 2014/06/10 20:33:48]

Done.

+  }
+}
+
+void PushMessagingGCMAppHandler::OnMessagesDeleted(const std::string& app_id) {
+  // TODO(mvanouwerkerk): Fire push error event on the Service Worker

[fgorski, 2014/06/10 17:09:27]

Are you sure this deserves a semantic of an error?
onMessagesDeleted means that server sent so many messages when the device was
unreachable, that the queue size quota was exceeded.
Is that case mentioned in standard? Should it be an error?

[johnme, 2014/06/10 20:33:48]

Last time we discussed the spec, there seemed to be agreement to have a
PushError event for the two cases:
1. Messages were deleted
2. The registration was invalidated for some reason
with a type member to let script distinguish between the two causes.

But the spec is still pretty fluid (indeed this hasn't yet been incorporated
into the editor's draft), so I'm sure we could rename this (and/or split it up).

+  // corresponding to app_id.
+}
+
+void PushMessagingGCMAppHandler::OnSendError(
+    const std::string& app_id,
+    const GCMClient::SendErrorDetails& send_error_details) {
+  NOTREACHED() << "The Push API shouldn't have sent messages upstream";
+}
 
 void PushMessagingServiceImpl::Register(
     const std::string& app_id,
@@ -31,17 +89,23 @@ void PushMessagingServiceImpl::Register(
       app_id,
       sender_ids,
       base::Bind(&PushMessagingServiceImpl::DidRegister,
-                 weak_factory_.GetWeakPtr(),
+                 GetWeakPtr(),
+                 app_id,
                  callback));
 }
 
 void PushMessagingServiceImpl::DidRegister(
+    const std::string& app_id,
     const content::PushMessagingService::RegisterCallback& callback,
     const std::string& registration_id,
     GCMClient::Result result) {
   GURL endpoint = GURL("https://android.googleapis.com/gcm/send");
   bool error = (result != GCMClient::SUCCESS);
   callback.Run(endpoint, registration_id, error);
+  if (!error)
+    gcm_profile_service_->driver()->AddAppHandler(app_id, this);
 }
 
+// TODO(johnme): Unregister should call RemoveAppHandler before unregistering.
+
 }  // namespace gcm