Synced Notification Sync Change Processor

chrome/browser/notifier/chrome_notifier_service.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.
+
+// The ChromeNotifierService works together with sync to maintain the state of
+// user notifications, which can then be presented in the notification center,
+// via the Notification UI Manager.
+
+#include "chrome/browser/notifier/chrome_notifier_service.h"
+
+#include "sync/api/sync_change.h"
+#include "sync/api/sync_change_processor.h"
+#include "sync/protocol/sync.pb.h"
+#include "sync/protocol/synced_notification_specifics.pb.h"
+
+using namespace sync_pb;

[dcheng, 2013/01/07 20:42:10]

It's much more common to just wrap it in namespace sync_pb {} rather than
pulling in the namespace. Also, I believe Nicolas commented on this as well, but
the namespace shouldn't be sync_pb (as this is not generated protobuf code)

[Pete Williamson, 2013/01/18 18:58:39]

This now uses a different type of using statement,
using sync_pb::SyncedNotification;
Is that better?

+
+ChromeNotifierService::ChromeNotifierService() {}
+ChromeNotifierService::~ChromeNotifierService() {}
+
+// Methods from ProfileKeyedService.
+void ChromeNotifierService::Shutdown() {
+}
+
+// syncer::SyncableService implementation.
+
+// This is called at startup to sync with the server.
+// I expect this to always get called on the same thread, so no need
+// to worry about re-entrancy.

[Nicolas Zea, 2013/01/05 01:18:48]

Re-entrancy doesn't really relate to thread safety.

That said, Sync does guarantee these methods will only be called on the
datatype's thread. Rather than adding a comment, if you want to enforce the lack
of thread safety, base::NonThreadSafe might be useful.

[Pete Williamson, 2013/01/18 18:58:39]

Done.

+syncer::SyncMergeResult ChromeNotifierService::MergeDataAndStartSyncing(
+      syncer::ModelType type,
+      const syncer::SyncDataList& initial_sync_data,
+      scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
+      scoped_ptr<syncer::SyncErrorFactory> error_handler) {
+  // TODO(petewil):After I add the infrastructure for the test, add a check
+  // that we are currently on the UI thread here.
+  DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, type);
+  syncer::SyncMergeResult merge_result(syncer::SYNCED_NOTIFICATIONS);
+
+  // Mark all data we have now as local.  New incoming data will clear
+  // the local flag, and merged data will clear the local flag.  We use
+  // this to know what has to go back up to the server.
+  for (std::vector<SyncedNotification*>::iterator iter = our_data_.begin();

[dcheng, 2013/01/07 20:42:10]

Nit: "it" is much more common than "iter".

[Pete Williamson, 2013/01/18 18:58:39]

Done.

+      iter != our_data_.end();
+      ++iter) {
+    (*iter)->set_has_local_changes(true);
+  }
+
+  for (syncer::SyncDataList::const_iterator iter = initial_sync_data.begin();
+       iter != initial_sync_data.end(); ++iter) {
+    const syncer::SyncData& sync_data = *iter;
+    DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, sync_data.GetDataType());
+
+    // Build a local notification object from the sync data.
+    scoped_ptr<SyncedNotification> incoming(CreateNotificationFromSyncData(
+        sync_data));
+    CHECK(incoming.get());

[dcheng, 2013/01/07 20:42:10]

Do we really want to CHECK() here? Is it possible we sometimes receive erroneous
data from the server?

It seems much more robust to continue gracefully to the next entry on an error
like this and log an error or warning message. Same comment applies everywhere
else CHECK() appears here.

[Pete Williamson, 2013/01/18 18:58:39]

After I looked at the other code I originally copied the template from, I
changed all these to DCHECK()

+
+    std::string external_id;
+    external_id = incoming->get_first_external_id();

[dcheng, 2013/01/07 20:42:10]

const std::string& external_id =

[Pete Williamson, 2013/01/18 18:58:39]

Done.

+    CHECK(external_id.length() > 0);
+
+    // Process each incoming remote notification.
+    // TODO(petewil): this should really get a unique id, not message text.
+    std::string title = incoming->title();

[dcheng, 2013/01/07 20:42:10]

const std::string& title

[Pete Williamson, 2013/01/18 18:58:39]

Done.

+    CHECK(title.length() > 0);
+    SyncedNotification* found = GetNotification(title);
+
+    if (NULL == found) {
+      // If there are no conflicts, copy in the data from remote.
+      Add(incoming.release());
+    } else {
+      // If the remote and local data conflict (the read or deleted flag),
+      // resolve the conflict.
+      // TODO(petewil): Implement.
+    }
+  }
+
+  // TODO(petewil): Implement the code that does the actual merging.
+
+  // Make a list of local changes to send up to the sync server.
+  syncer::SyncChangeList new_changes;
+  for (std::vector<SyncedNotification*>::iterator iter = our_data_.begin();
+      iter != our_data_.end();
+      ++iter) {
+    if ((*iter)->has_local_changes()) {
+      DCHECK(NULL != *iter);

[dcheng, 2013/01/07 20:42:10]

Isn't it a little late to DCHECK() that *iter is non-NULL here? Remove please.

[Pete Williamson, 2013/01/18 18:58:39]

Done.

+      syncer::SyncData sync_data = CreateSyncDataFromNotification(**iter);
+      new_changes.push_back(
+          syncer::SyncChange(FROM_HERE,
+                             syncer::SyncChange::ACTION_ADD,
+                             sync_data));
+    }
+  }
+
+  // Send up the changes that were made locally.
+  if (new_changes.size() > 0) {
+    merge_result.set_error(
+        (sync_processor.Pass())->ProcessSyncChanges(FROM_HERE, new_changes));

[dcheng, 2013/01/07 20:42:10]

I don't understand why you call Pass() here. Usually this would indicate
ownership is being transferred... but that's doesn't seem like what's happening
here.

[Pete Williamson, 2013/01/18 18:58:39]

Oops, good catch, converted to get()

+  }
+
+  return merge_result;
+}
+
+void ChromeNotifierService::StopSyncing(syncer::ModelType type) {
+  DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, type);
+}
+
+syncer::SyncDataList ChromeNotifierService::GetAllSyncData(
+      syncer::ModelType type) const {
+  DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, type);
+  syncer::SyncDataList our_sync_data;
+
+  // Copy our native format data into a SyncDataList format.
+  for (std::vector<SyncedNotification*>::const_iterator iter =
+          our_data_.begin();
+      iter != our_data_.end();
+      ++iter) {
+    our_sync_data.push_back(CreateSyncDataFromNotification(**iter));
+  }
+
+  return our_sync_data;
+  }
+
+// This method is called when there is an incoming sync change from the server.
+syncer::SyncError ChromeNotifierService::ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const syncer::SyncChangeList& change_list) {
+  // TODO(petewil): add a check that we are called on the thread we expect.
+  syncer::SyncError error;
+
+  for (syncer::SyncChangeList::const_iterator iter = change_list.begin();
+       iter != change_list.end(); ++iter) {
+    syncer::SyncData sync_data = iter->sync_data();
+    DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, sync_data.GetDataType());
+    syncer::SyncChange::SyncChangeType change_type = iter->change_type();
+
+    scoped_ptr<SyncedNotification> new_notification(
+        CreateNotificationFromSyncData(sync_data));
+    if (!new_notification.get()) {
+      NOTREACHED() << "Failed to read notification.";
+      continue;
+    }
+
+    switch (change_type) {
+      case syncer::SyncChange::ACTION_ADD:
+        // TODO(petewil): Don't add the notification if it already exists.
+          Add(new_notification.release());
+        break;
+      // TODO(petewil): Implement code to add delete and update actions.
+      // case syncer::SyncChange::ACTION_DELETE:
+
+      default:
+        break;
+    }
+  }
+
+  return error;
+}
+
+// Support functions for data type conversion.
+
+// Static method.  Get to the sync data in our internal format.
+syncer::SyncData ChromeNotifierService::CreateSyncDataFromNotification(
+    SyncedNotification& notification) {
+  return *notification.sync_data();
+}
+
+// Static Method.  Convert from SyncData to our internal format.
+SyncedNotification* ChromeNotifierService::CreateNotificationFromSyncData(
+    const syncer::SyncData& sync_data) {
+
+  // Get a pointer to our data within the sync_data object.
+  sync_pb::SyncedNotificationSpecifics specifics =
+      sync_data.GetSpecifics().synced_notification();
+
+  // Check for mandatory fields in the sync_data object
+  // TODO(petewil): Consider adding check for a title in one of the
+  // title and subtext or title and image objects.
+  if (!specifics.has_coalesced_notification() ||
+      !specifics.coalesced_notification().has_id() ||
+      !specifics.coalesced_notification().id().has_app_id() ||
+      specifics.coalesced_notification().id().app_id().empty() ||
+      !specifics.coalesced_notification().has_render_info() ||
+      !specifics.coalesced_notification().has_creation_time_msec()) {
+    return NULL;
+  }
+
+  // Create a new notification object based on the supplied sync_data.
+  SyncedNotification* notification = new SyncedNotification(sync_data);
+
+  return notification;
+}
+
+// This returns a pointer into a vector that we own.  Caller must not free it.
+// This returns NULL if no match is found.
+// TODO(petewil): Use the  <app_id/coalescing_key> pair instead of title to
+// check for uniqueness.
+SyncedNotification* ChromeNotifierService::GetNotification(

[dcheng, 2013/01/07 20:42:10]

Perhaps FindNotificationWithTitle() would be a more descriptive name.

[Pete Williamson, 2013/01/18 18:58:39]

Renamed to FindNotifictionById()

+    const std::string& value) {

[dcheng, 2013/01/07 20:42:10]

"value" is not a meaningful name. I would call rename this "title"

[Pete Williamson, 2013/01/18 18:58:39]

Renamed to "id" since we now find by ID instead of title

+
+  // TODO(petewil): Convert this to a map if the expected number
+  // of notifications on the device is high enough to justify the worsened
+  // locality of reference.
+  for (std::vector<SyncedNotification*>::const_iterator iter =
+          our_data_.begin();
+      iter != our_data_.end();
+      ++iter) {
+    SyncedNotification* notification = *iter;
+    if (value == notification->title())
+      return *iter;
+  }
+
+  return NULL;
+}
+
+// Add a new notification to our data structure.  This takes ownership
+// of the passed in pointer.
+bool ChromeNotifierService::Add(SyncedNotification* notification) {

[dcheng, 2013/01/07 20:42:10]

scoped_ptr<SyncedNotification>

[Pete Williamson, 2013/01/18 18:58:39]

Done.

+
+  our_data_.push_back(notification);
+
+  // TODO(petewil): Implement code to send the notification.
+
+  return true;
+}