Plumb invalidations from Tango to the extensions code for the Push Messaging API.

chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.cc

+// Copyright (c) 2012 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.
+
+#include "chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler.h"
+
+#include <algorithm>
+
+#include "base/string_number_conversions.h"
+#include "base/stringprintf.h"
+#include "chrome/browser/extensions/api/push_messaging/push_messaging_invalidation_handler_observer.h"
+#include "chrome/browser/sync/invalidation_service.h"
+#include "google/cacheinvalidation/types.pb.h"
+
+namespace extensions {
+
+namespace {
+
+const int kNumberOfSubchannels = 4;
+

[Pete Williamson, 2012/08/13 18:24:09]

Nit - the "kXxxIndex" constants below might be a bit clearer as kXxxStartIndex,
to imply that this is the starting index of a portion of a string instead of an
index into an array

[dcheng, 2012/08/13 20:44:14]

It's also the index into a string. I named it "Index" instead of "StartIndex"
since there's no complementary "EndIndex".

+const int kFormatTypeIndex = 0;
+const int kFormatTypeLength = 1;
+const int kProjectIdIndex = 1;
+const int kProjectIdLength = 2;
+const int kExtensionIdIndex = 3;
+const int kExtensionIdLength = 32;
+const int kSubchannelIndex = 35;
+const int kSubchannelLength = 1;
+
+// Chrome Components object IDs currently have the following format:
+// <1 byte format type>
+//   Currently the only format type defined is 'U' but we'll need to create a
+//   new one.
+// <8 byte GAIA ID>
+//   Handled entirely server-side. The client never see this.
+// <2 byte project ID>
+//   We use "PM" for Push Messaging.
+// <4-48 byte project object ID>
+//   We use the extension ID concatenated with the subchannel.
+syncer::ObjectIdSet ExtensionIdToObjectIds(const std::string& extension_id) {
+  syncer::ObjectIdSet object_ids;
+  for (int i = 0; i < kNumberOfSubchannels; ++i) {
+    std::string name = base::StringPrintf("UPM%s%d", extension_id.c_str(), i);
+    object_ids.insert(invalidation::ObjectId(
+        ipc::invalidation::ObjectSource::CHROME_COMPONENTS,
+        name));
+  }
+  return object_ids;
+}
+
+// Returns true iff the conversion was successful.
+bool ObjectIdToExtensionAndSubchannel(const invalidation::ObjectId& object_id,
+                                      std::string* extension_id,
+                                      int* subchannel) {
+  if (object_id.source() !=
+      ipc::invalidation::ObjectSource::CHROME_COMPONENTS) {
+    DLOG(WARNING) << "Invalid source: " << object_id.source();
+    return false;
+  }
+
+  const std::string& name = object_id.name();
+  const size_t kValidObjectIdLength = kFormatTypeLength + kProjectIdLength +
+      kExtensionIdLength + kSubchannelLength;
+  if (name.size() != kValidObjectIdLength) {
+    DLOG(WARNING) << "Invalid length for object name " << name;
+    return false;
+  }

[Pete Williamson, 2012/08/13 18:24:09]

Might be good to add a comment here:
// TODO: Modify the Server side code to take a format type of "V" then come back
and change this "U" to "V"

[dcheng, 2012/08/13 20:44:14]

I avoided making that comment since that would require changing this code again
if my current server side change is accepted as is.

+  if (name.compare(kFormatTypeIndex, kFormatTypeLength, "U")) {
+    DLOG(WARNING) << "Invalid format type for object name " << name;
+    return false;
+  }

[Pete Williamson, 2012/08/13 18:24:09]

Does this "PM" need to be registered somewhere so that nobody else tries to use
it?  If so, let's start a thread to make that happen, and add a TODO: so we
don't forget.

[dcheng, 2012/08/13 20:44:14]

This is an ad-hoc registration. We rely on good citizenship =)

+  if (name.compare(kProjectIdIndex, kProjectIdLength, "PM")) {
+    DLOG(WARNING) << "Invalid project ID for object name " << name;
+    return false;
+  }
+  *extension_id = name.substr(kExtensionIdIndex, kExtensionIdLength);
+  if (!base::StringToInt(name.substr(kSubchannelIndex, kSubchannelLength),
+                         subchannel)) {
+    DLOG(WARNING) << "Subchannel not a number for object name " << name;
+    return false;
+  }
+
+  // We don't need to check for negative since the length check makes it
+  // impossible.
+  return *subchannel <= 3;
+}
+
+}  // namespace
+

[Pete Williamson, 2012/08/13 18:24:09]

Coding standards question:  I personally think it is a good idea for each
function to have a one line header comment describing the intent of the
function.  Is that done here (either in the header or the cpp file?)  Also, I
think we should mention that this class just makes a copy of the service and
observer, and relies upon the caller to ensure they are valid for the lifetime
of this class.

[dcheng, 2012/08/13 20:44:14]

I've updated the header with comments about the lifetime of pointers that are
passed in.

WRT to commenting functions, I'll comment non-obvious functions or non-obvious
behaviors. For the most part, since this is a relatively straightforward class,
the interface is pretty simple and self-documenting.

+PushMessagingInvalidationHandler::PushMessagingInvalidationHandler(
+    InvalidationService* service,
+    PushMessagingInvalidationHandlerObserver* observer,
+    const std::set<std::string>& extension_ids)
+    : service_(service),
+      observer_(observer) {
+  DCHECK(service_);
+
+  for (std::set<std::string>::const_iterator it = extension_ids.begin();
+       it != extension_ids.end(); ++it) {
+    const syncer::ObjectIdSet& object_ids = ExtensionIdToObjectIds(*it);
+    registered_ids_.insert(object_ids.begin(), object_ids.end());
+  }
+  service_->UpdateRegisteredInvalidationIds(this, registered_ids_);
+}
+

[Pete Williamson, 2012/08/13 18:24:09]

Is this removing the registration when the DTOR fires, or completing it?  I'm
not quite sure what the intent is here (perhaps a comment would help the reader)

[dcheng, 2012/08/13 20:44:14]

I've rebased to akalin's latest set of changes. It should be clearer now, since
it uses the common pattern of Registering yourself as an Observer in the ctor
and Unregistering yourself in the dtor. A lot of the documentation for this is
already in profile_sync_service.h as well, so I wanted to avoid duplicating that
information here.

+PushMessagingInvalidationHandler::~PushMessagingInvalidationHandler() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  service_->UpdateRegisteredInvalidationIds(this, syncer::ObjectIdSet());
+}
+
+void PushMessagingInvalidationHandler::RegisterExtension(
+    const std::string& extension_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(32U, extension_id.size());
+  const syncer::ObjectIdSet& object_ids = ExtensionIdToObjectIds(extension_id);
+  registered_ids_.insert(object_ids.begin(), object_ids.end());
+  service_->UpdateRegisteredInvalidationIds(this, registered_ids_);
+}
+
+void PushMessagingInvalidationHandler::UnregisterExtension(
+    const std::string& extension_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(32U, extension_id.size());
+  const syncer::ObjectIdSet& object_ids = ExtensionIdToObjectIds(extension_id);
+  syncer::ObjectIdSet updated_registered_ids;
+  std::set_difference(registered_ids_.begin(), registered_ids_.end(),
+                      object_ids.begin(), object_ids.end(),
+                      std::inserter(updated_registered_ids,
+                                    updated_registered_ids.begin()),
+                      syncer::ObjectIdLessThan());
+  registered_ids_.swap(updated_registered_ids);
+  service_->UpdateRegisteredInvalidationIds(this, registered_ids_);
+}
+
+void PushMessagingInvalidationHandler::OnNotificationsEnabled() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Nothing to do.
+}
+
+void PushMessagingInvalidationHandler::OnNotificationsDisabled(
+    syncer::NotificationsDisabledReason reason) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Nothing to do.

[Pete Williamson, 2012/08/13 18:24:09]

Should we DCHECK(false); here since this shouldn't happen, and disables our
system?

[dcheng, 2012/08/13 20:44:14]

That just means that sync has been disabled. In that case, there's not too much
we can do, but DCHECK/CHECKing probably isn't what we want to do.

+}
+
+void PushMessagingInvalidationHandler::OnIncomingNotification(
+    const syncer::ObjectIdPayloadMap& id_payloads,
+    syncer::IncomingNotificationSource source) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  for (syncer::ObjectIdPayloadMap::const_iterator it = id_payloads.begin();
+       it != id_payloads.end(); ++it) {
+    std::string extension_id;
+    int subchannel;
+    if (ObjectIdToExtensionAndSubchannel(it->first,
+                                         &extension_id,
+                                         &subchannel)) {
+      observer_->OnMessage(extension_id, subchannel, it->second);
+    }
+  }
+}
+
+}  // namespace extensions