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 "base/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/notifications/notification.h"
+#include "chrome/browser/notifications/notification_ui_manager.h"
+#include "chrome/browser/notifier/chrome_notifier_delegate.h"
+#include "chrome/browser/profiles/profile.h"
+#include "googleurl/src/gurl.h"
+#include "sync/api/sync_change.h"
+#include "sync/api/sync_change_processor.h"
+#include "sync/api/sync_error_factory.h"
+#include "sync/protocol/sync.pb.h"
+#include "sync/protocol/synced_notification_specifics.pb.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebTextDirection.h"
+
+using sync_pb::SyncedNotification;
+
+
+ChromeNotifierService::ChromeNotifierService(Profile* profile,
+                                             NotificationUIManager* manager) :

[dcheng, 2013/01/18 21:56:13]

Incorrect formatting. : should be on the next line.

[Pete Williamson, 2013/01/23 01:45:55]

Done.

+    profile_(profile), notification_manager_(manager) {}
+ChromeNotifierService::~ChromeNotifierService() {}
+
+// Methods from ProfileKeyedService.
+void ChromeNotifierService::Shutdown() {
+}
+
+// syncer::SyncableService implementation.
+
+// This is called at startup to sync with the server.
+// This code is not thread safe.
+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 it =
+           notification_data_.begin();
+      it != notification_data_.end();
+      ++it) {
+    (*it)->set_has_local_changes(true);
+  }
+
+  for (syncer::SyncDataList::const_iterator it = initial_sync_data.begin();
+       it != initial_sync_data.end(); ++it) {
+    const syncer::SyncData& sync_data = *it;
+    DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, sync_data.GetDataType());
+
+    // Build a local notification object from the sync data.
+    scoped_ptr<SyncedNotification> incoming(CreateNotificationFromSyncData(
+        sync_data));
+    DCHECK(incoming.get());
+
+    // Process each incoming remote notification.
+    const std::string& id = incoming->get_notification_id();
+    DCHECK(id.length() > 0);
+    SyncedNotification* found = FindNotificationById(id);
+
+    if (NULL == found) {
+      // If there are no conflicts, copy in the data from remote.
+      Add(incoming.Pass());
+    } 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.
+  // See chrome/browser/history/history.cc for an example to follow.
+
+  // Make a list of local changes to send up to the sync server.
+  syncer::SyncChangeList new_changes;
+  for (std::vector<SyncedNotification*>::iterator it =
+           notification_data_.begin();
+      it != notification_data_.end();
+      ++it) {
+    if ((*it)->has_local_changes()) {
+      syncer::SyncData sync_data = CreateSyncDataFromNotification(**it);
+      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.get())->ProcessSyncChanges(FROM_HERE, new_changes));
+  }
+
+  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 local_sync_data;

[dcheng, 2013/01/18 21:56:13]

Just call this sync_data?

[Pete Williamson, 2013/01/23 01:45:55]

Done.

+
+  // Copy our native format data into a SyncDataList format.
+  for (std::vector<SyncedNotification*>::const_iterator it =
+          notification_data_.begin();
+      it != notification_data_.end();
+      ++it) {
+    local_sync_data.push_back(CreateSyncDataFromNotification(**it));
+  }
+
+  return local_sync_data;
+  }

[dcheng, 2013/01/18 21:56:13]

Indent.

[Pete Williamson, 2013/01/23 01:45:55]

Done.

+
+// 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 it = change_list.begin();
+       it != change_list.end(); ++it) {
+    syncer::SyncData sync_data = it->sync_data();
+    DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, sync_data.GetDataType());
+    syncer::SyncChange::SyncChangeType change_type = it->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): Update the notification if it already exists
+        // as opposed to adding it.
+          Add(new_notification.Pass());

[dcheng, 2013/01/18 21:56:13]

Indent.

[Pete Williamson, 2013/01/23 01:45:55]

Done.

+        break;
+      // TODO(petewil): Implement code to add delete and update actions.
+
+      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.mutable_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.
+  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.
+// Returns NULL if no match is found.
+// This uses the <app_id/coalescing_key> pair as a key.
+SyncedNotification* ChromeNotifierService::FindNotificationById(
+    const std::string& id) {
+
+  // TODO(petewil): We can make a performance trade off here.
+  // While the vector has good locality of reference, a map has faster lookup.
+  // Based on how bit we expect this to get, maybe change this to a map.
+  for (std::vector<SyncedNotification*>::const_iterator it =
+          notification_data_.begin();
+      it != notification_data_.end();
+      ++it) {
+    SyncedNotification* notification = *it;
+    if (id == notification->get_notification_id())
+      return *it;
+  }
+
+  return NULL;
+}
+
+// Add a new notification to our data structure.  This takes ownership
+// of the passed in pointer.
+void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
+
+  // Logically take ownership of the object.  Since we still need the pointer
+  // below for show, we don't do the release here, but further below.
+  notification_data_.push_back(notification.get());
+
+  // Show it to the user (and complete taking ownership of the pointer).
+  Show(notification.release());
+}
+
+// Send the notification to the NotificationUIManager to show to the user.
+void ChromeNotifierService::Show(SyncedNotification* notification) {
+

[dcheng, 2013/01/18 21:56:13]

Empty newlines here and elsewhere at the beginning of methods are not needed.

[Pete Williamson, 2013/01/23 01:45:55]

Done.

+  // Set up the fields we need to send and create a Notification object.
+  GURL origin_url(notification->get_origin_url());
+  GURL icon_url(notification->get_icon_url());
+  std::string title = notification->get_title();
+  string16 title16 = UTF8ToWide(title);

[dcheng, 2013/01/18 21:56:13]

UTF8ToUTF16.

Despite the fact that wstring happens to be string16 on some platforms, wstring
is deprecated and should not be used. This also won't compile on, say, Linux.

[dcheng, 2013/01/18 21:56:13]

Why not string16 title = UTF8ToUTF16(notification->get_title()). It can
potentially save a copy and it doesn't seem like the std::string variables are
really used for anything else.

[Pete Williamson, 2013/01/23 01:45:55]

Done.

[Pete Williamson, 2013/01/23 01:45:55]

I expect the compiler to be smart and not allocate an extra variable.  Also,
IMO, it makes the code easier to read, and helps with debugging.  I do reuse the
utf8 variables in the DVLOG statement.

[dcheng, 2013/01/23 18:39:43]

I personally find it harder to read a block of code when there are variables foo
and foo16 and would prefer to see it avoided where not necessary. In this case,
debugging seems to be limited to DVLOG()ing and I'm pretty sure string16
overrides operator<< so that it Just Works with LOG() statements.

[Pete Williamson, 2013/01/24 01:48:11]

Done.

+  std::string body = notification->get_body();
+  string16 body16 = UTF8ToWide(body);
+  // TODO(petewil):what goes in the display source, is empty OK?
+  string16 display_source16;
+  std::string notification_id = notification->get_notification_id();
+  string16 replace_id16 = UTF8ToWide(notification_id);
+  // The delegate will eventually catch calls that the notification
+  // was read or deleted, and send the changes back to the server.
+  // TODO(petewil): who should free this?
+  NotificationDelegate* delegate = new ChromeNotifierDelegate(notification_id);

[dcheng, 2013/01/18 21:56:13]

Use scoped_refptr. Not sure this comment is needed either.

[Pete Williamson, 2013/01/23 01:45:55]

Done.
I removed the TODO, but kept the rest of the comment.

+
+  Notification ui_notification(origin_url, icon_url, title16, body16,
+                               WebKit::WebTextDirectionDefault,
+                               display_source16, replace_id16, delegate);
+
+  notification_manager_->Add(ui_notification, profile_);
+
+  DVLOG(1) << "Synced Notification arrived! " << title << " " << body
+            << " " << icon_url << " " << notification_id;

[dcheng, 2013/01/18 21:56:13]

Formatting.

[Pete Williamson, 2013/01/23 01:45:55]

Done.

+
+  return;
+}