Widen Data Pipes and newer protobufs

chrome/browser/notifications/sync_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/notifications/sync_notifier/chrome_notifier_service.h"
 
@@ skipping 43 matching lines @@
        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->notification_id();
-    DCHECK_GT(id.length(), 0U);
-    SyncedNotification* found = FindNotificationById(id);
+    const std::string& key = incoming->key();
+    DCHECK_GT(key.length(), 0U);
+    SyncedNotification* found = FindNotificationByKey(key);
 
     if (NULL == found) {
       // If there are no conflicts, copy in the data from remote.
       Add(incoming.Pass());
     } else {
       // If the incoming (remote) and stored (local) notifications match
       // in all fields, we don't need to do anything here.
       if (incoming->EqualsIgnoringReadState(*found)) {
 
         if (incoming->read_state() == found->read_state()) {
@@ skipping 91 matching lines @@
   return error;
 }
 
 // Support functions for data type conversion.
 
 // Static method.  Get to the sync data in our internal format.
 syncer::SyncData ChromeNotifierService::CreateSyncDataFromNotification(
     const SyncedNotification& notification) {
   // Construct the sync_data using the specifics from the notification.
   return syncer::SyncData::CreateLocalData(
-      notification.notification_id(), notification.notification_id(),
+      notification.key(), notification.key(),
       notification.GetEntitySpecifics());
 }
 
 // Static Method.  Convert from SyncData to our internal format.
 scoped_ptr<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_key() ||
-      !specifics.coalesced_notification().has_read_state()) {
+      !specifics.coalesced_notification().has_read_state())
     return scoped_ptr<SyncedNotification>();
-  }
 
   // TODO(petewil): Is this the right set?  Should I add more?
   bool is_well_formed_unread_notification =
       (static_cast<SyncedNotification::ReadState>(
           specifics.coalesced_notification().read_state()) ==
        SyncedNotification::kUnread &&
        specifics.coalesced_notification().has_render_info());
   bool is_well_formed_dismissed_notification =
       (static_cast<SyncedNotification::ReadState>(
           specifics.coalesced_notification().read_state()) ==
        SyncedNotification::kDismissed);
 
   // if the notification is poorly formed, return a null pointer
   if (!is_well_formed_unread_notification &&
       !is_well_formed_dismissed_notification)
     return scoped_ptr<SyncedNotification>();
 
   // Create a new notification object based on the supplied sync_data.
   scoped_ptr<SyncedNotification> notification(
       new SyncedNotification(sync_data));
 
   return notification.Pass();
 }
 
 // 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) {
+SyncedNotification* ChromeNotifierService::FindNotificationByKey(
+    const std::string& key) {
   // 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 big 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->notification_id())
+    if (key == notification->key())
       return *it;
   }
 
   return NULL;
 }
 
-void ChromeNotifierService::MarkNotificationAsDismissed(const std::string& id) {
-  SyncedNotification* notification = FindNotificationById(id);
+void ChromeNotifierService::MarkNotificationAsDismissed(
+    const std::string& key) {
+  SyncedNotification* notification = FindNotificationByKey(key);
   CHECK(notification != NULL);
 
   notification->NotificationHasBeenDismissed();
   syncer::SyncChangeList new_changes;
 
   syncer::SyncData sync_data = CreateSyncDataFromNotification(*notification);
   new_changes.push_back(
       syncer::SyncChange(FROM_HERE,
                          syncer::SyncChange::ACTION_UPDATE,
                          sync_data));
 
   // Send up the changes that were made locally.
   sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes);
 }
 
 // Add a new notification to our data structure.  This takes ownership
 // of the passed in pointer.
 void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
   SyncedNotification* notification_copy = notification.get();
   // Take ownership of the object and put it into our local storage.
   notification_data_.push_back(notification.release());
 
   // Show it to the user.
   Show(notification_copy);
 }
 
 // Send the notification to the NotificationUIManager to show to the user.
 void ChromeNotifierService::Show(SyncedNotification* notification) {

[dcheng, 2013/03/26 17:49:29]

I wonder if we should be unit testing this. This is performing some amount of
logic in mapping SyncedNotification* to a Notification for the message center,
and it'd be nice to be able to validate that something isn't broken accidentally
by an update.

[Pete Williamson, 2013/03/27 17:07:53]

Unit test added, and test framework enhanced to accommodate the new test.

   // Set up the fields we need to send and create a Notification object.
   GURL origin_url(notification->origin_url());
   GURL app_icon_url(notification->app_icon_url());
+  GURL image_url(notification->image_url());
   string16 title = UTF8ToUTF16(notification->title());
   string16 text = UTF8ToUTF16(notification->text());
   string16 heading = UTF8ToUTF16(notification->heading());
   string16 description = UTF8ToUTF16(notification->description());
-
+  double creation_time = static_cast<double>(notification->creation_time());
   // TODO(petewil): What goes in the display source, is empty OK?
   string16 display_source;
-  string16 replace_id = UTF8ToUTF16(notification->notification_id());
+  string16 replace_key = UTF8ToUTF16(notification->key());
+  int priority = notification->priority();
+  int notification_count = notification->notification_count();
+  int button_count = notification->button_count();
+  string16 button_one_title = UTF8ToUTF16(notification->button_one_title());

[dcheng, 2013/03/26 17:49:29]

Comment about UTF8 to UTF16 conversion still applies here and several other
places as well. Unfortunately, doing this just means the data goes from UTF8 to
UTF16 to UTF8.

[Pete Williamson, 2013/03/27 17:07:53]

Done.

+  std::string button_one_icon_url = notification->button_one_icon_url();
+  string16 button_two_title = UTF8ToUTF16(notification->button_two_title());
+  std::string button_two_icon_url = notification->button_two_icon_url();
+
+  // Deduce which notification template to use from the data.
+  message_center::NotificationType notification_type =
+      message_center::NOTIFICATION_TYPE_SIMPLE;
+  if (!image_url.is_empty()) {
+    notification_type = message_center::NOTIFICATION_TYPE_IMAGE;
+  } else if (notification_count > 1) {
+    notification_type = message_center::NOTIFICATION_TYPE_MULTIPLE;
+  } else if (button_count > 0) {
+    notification_type = message_center::NOTIFICATION_TYPE_BASE_FORMAT;
+  }
+
+  // Fill the optional fields with the information we need to make a
+  // notification.
+  scoped_ptr<DictionaryValue> optional_fields(new DictionaryValue());

[dcheng, 2013/03/26 17:49:29]

Consider just saying:
DictionaryValue optional_fields;
if we don't need to transfer ownership.

[Pete Williamson, 2013/03/27 17:07:53]

Done.

+  optional_fields->SetDouble(message_center::kTimestampKey, creation_time);
+  if (priority != 0)

[dcheng, 2013/03/26 17:49:29]

What correlation does priority have to timestamp? Why is it OK to treat a
non-zero priority as a timestamp? Also, isn't this originally an enum?

[Pete Williamson, 2013/03/27 17:07:53]

None, this is a bug, fixed.

+    optional_fields->SetInteger(message_center::kTimestampKey, priority);
+  if (!button_one_title.empty())
+    optional_fields->SetString(message_center::kButtonOneTitleKey,
+                               button_one_title);
+  if (!button_one_icon_url.empty())
+    optional_fields->SetString(message_center::kButtonOneIconUrlKey,
+                               button_one_icon_url);
+  if (!button_two_title.empty())
+    optional_fields->SetString(message_center::kButtonTwoTitleKey,
+                               button_two_title);
+  if (!button_two_icon_url.empty())
+    optional_fields->SetString(message_center::kButtonTwoIconUrlKey,
+                               button_two_icon_url);
+
+  // Fill the individual notification fields for a mutiple notification.
+  if (notification_count > 1) {
+    base::ListValue* items = new base::ListValue();
+
+    for (int ii = 0; ii < notification_count; ++ii) {
+      DictionaryValue* item = new DictionaryValue();
+      item->SetString(message_center::kItemTitleKey,
+                      UTF8ToUTF16(notification->contained_notification_title(
+                          ii)));
+      item->SetString(message_center::kItemMessageKey,
+                      UTF8ToUTF16(notification->contained_notification_message(
+                          ii)));
+      items->Append(item);
+    }
+
+    optional_fields->Set(message_center::kItemsKey, items);
+  }
 
   // TODO(petewil): For now, just punt on dismissed notifications until
   // I change the interface to let NotificationUIManager know the right way.
   if (SyncedNotification::kRead == notification->read_state() ||
       SyncedNotification::kDismissed == notification->read_state() ) {
-    DVLOG(2) << "Not showing dismissed notification"
-              << notification->title() << " " << notification->text();
+    DVLOG(2) << "Dismissed notification arrived"
+             << notification->title() << " " << notification->text();
     return;
   }
-
   // The delegate will eventually catch calls that the notification
   // was read or deleted, and send the changes back to the server.
   scoped_refptr<NotificationDelegate> delegate =
-      new ChromeNotifierDelegate(notification->notification_id(), this);
+      new ChromeNotifierDelegate(notification->key(), this);
 
-  Notification ui_notification(origin_url, app_icon_url, heading, description,
+  Notification ui_notification(notification_type,
+                               origin_url,
+                               app_icon_url,
+                               heading,
+                               text,
                                WebKit::WebTextDirectionDefault,
-                               display_source, replace_id, delegate);
+                               display_source,
+                               replace_key,
+                               optional_fields.get(),
+                               delegate);
 
   notification_manager_->Add(ui_notification, profile_);
 
   DVLOG(1) << "Synced Notification arrived! " << title << " " << text
-           << " " << app_icon_url << " " << replace_id << " "
+           << " " << app_icon_url << " " << replace_key << " "
            << notification->read_state();
 
   return;
 }
 
 }  // namespace notifier