Process incoming updates and deletes properly.

chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc

Index: chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
diff --git a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
index 84b0d6a3a758ed2fce774685383346b7c01c8d8a..deccdacf5adfa46da8ce0a345251e32caa5a8ef9 100644
--- a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
+++ b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
@@ -127,7 +127,7 @@ syncer::SyncMergeResult ChromeNotifierService::MergeDataAndStartSyncing(
         found->Update(sync_data);
 
         // Tell the notification manager to update the notification.
-        Display(found);
+        UpdateInMessageCenter(found);
       }
     }
   }
@@ -182,15 +182,45 @@ syncer::SyncError ChromeNotifierService::ProcessSyncChanges(
       continue;
     }
 
+    const std::string& key = new_notification->GetKey();
+    DCHECK_GT(key.length(), 0U);
+    SyncedNotification* found = FindNotificationById(key);
+
     switch (change_type) {
       case syncer::SyncChange::ACTION_ADD:
-        // TODO(petewil): Update the notification if it already exists
-        // as opposed to adding it.
+        if (found != NULL) {
+          found->Update(sync_data);
+          UpdateInMessageCenter(found);
+          break;
+        }
         Add(new_notification.Pass());
         break;
-      // TODO(petewil): Implement code to add delete and update actions.
+
+      case syncer::SyncChange::ACTION_UPDATE:
+        if (found == NULL) {
+          Add(new_notification.Pass());
+          break;
+        }
+        // Update it in our store.
+        found->Update(sync_data);
+        // Tell the notification manager to update the notification.
+        UpdateInMessageCenter(found);
+        break;

[Nicolas Zea, 2013/08/09 20:04:13]

it seems to me this is the same as the code above, perhaps merge the two?

[Pete Williamson, 2013/08/09 21:00:18]

Done.  I've heard before that it is worse to fall through than to duplicate
code, which is why I Originally didn"t do a fall through here.  See if you like
this better.

+
+      case syncer::SyncChange::ACTION_DELETE:
+        if (found == NULL) {
+          break;
+        }
+        // Remove it from our store.
+        FreeNotificationById(key);
+        // Remove it from the message center.
+        UpdateInMessageCenter(new_notification.get());
+        // TODO(petewil): Do I need to remember that it was deleted in case the
+        // add arrives after the delete?  If so, how long do I need to remember?
+        break;
 
       default:
+        NOTREACHED();

[dewittj, 2013/08/09 19:40:28]

You don't handle ACTION_INVALID, yet you have a NOTREACHED here.  I suspect
you'd want to handle that rather than crash the browser in the event of an
invalid action.

[Nicolas Zea, 2013/08/09 20:04:13]

The syncer will never return ACTION_INVALID in a valid scenario, and if it does
something is very broken. NOTREACHED would help us identify and fix the issue
quicker (and keep in mind only CHECK's in debug builds).

[Pete Williamson, 2013/08/09 21:00:18]

Left as is.

[Pete Williamson, 2013/08/09 21:00:18]

Left as is per Zea@'s suggestion (below)

         break;
     }
   }
@@ -285,6 +315,20 @@ SyncedNotification* ChromeNotifierService::FindNotificationById(
   return NULL;
 }
 
+void ChromeNotifierService::FreeNotificationById(
+    const std::string& notification_id) {
+  for (std::vector<SyncedNotification*>::iterator it =
+          notification_data_.begin();

[dewittj, 2013/08/09 19:40:28]

This indentation is really weird here.  Can I recommend:

ScopedVector<SyncedNotification>::iterator it = notification_data_.begin();
for (; it != notification_data_.end(); ++it) { ... }

In fact, I think (unless my c++ fu is failing me here) that you should use
ScopedVector<SyncedNotification>::iterator everywhere in this file instead of
std::vector<..*>::iterator since that's an implementation detail.

[Pete Williamson, 2013/08/09 21:00:18]

Done.

+      it != notification_data_.end();
+      ++it) {
+    SyncedNotification* notification = *it;
+    if (notification_id == notification->GetKey()) {
+      notification_data_.erase(it);
+      return;
+    }
+  }
+}
+
 void ChromeNotifierService::GetSyncedNotificationServices(
     std::vector<message_center::Notifier*>* notifiers) {
   // TODO(mukai|petewil): Check the profile's eligibility before adding the
@@ -352,7 +396,7 @@ void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
     return;
   }
 
-  Display(notification_copy);
+  UpdateInMessageCenter(notification_copy);
 }
 
 void ChromeNotifierService::AddForTest(
@@ -360,7 +404,8 @@ void ChromeNotifierService::AddForTest(
   notification_data_.push_back(notification.release());
 }
 
-void ChromeNotifierService::Display(SyncedNotification* notification) {
+void ChromeNotifierService::UpdateInMessageCenter(
+    SyncedNotification* notification) {
   // If the feature is disabled, exit now.
   if (!notifier::ChromeNotifierServiceFactory::UseSyncedNotifications(
       CommandLine::ForCurrentProcess()))
@@ -368,6 +413,17 @@ void ChromeNotifierService::Display(SyncedNotification* notification) {
 
   notification->LogNotification();
 
+  if (notification->GetReadState() == SyncedNotification::kUnread) {
+    // If the message is unread, update it.
+    Display(notification);
+  } else {
+    // If the message is read or deleted, dismiss it from the center.
+    // We intentionally ignore errors if it is not in the center.
+    notification_manager_->CancelById(notification->GetKey());
+  }
+}
+
+void ChromeNotifierService::Display(SyncedNotification* notification) {
   // Set up to fetch the bitmaps.
   notification->QueueBitmapFetchJobs(notification_manager_,
                                      this,