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 cc0721360d244f0a8c7b30b45a72de3c524103bd..ef90d33499065977d8a6e074b76dd33904bd4ec8 100644
--- a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
+++ b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
@@ -122,7 +122,7 @@ syncer::SyncMergeResult ChromeNotifierService::MergeDataAndStartSyncing(
         found->Update(sync_data);
 
         // Tell the notification manager to update the notification.
-        Display(found);
+        UpdateInMessageCenter(found);
       }
     }
   }
@@ -177,15 +177,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) {

[dewittj, 2013/08/08 22:15:32]

Unless I understand the protocol wrong, this could cause redundant
notifications.  Say an ACTION_UPDATE happens at the same time as a user clicking
(x), then they will get the same (updated) notification again as a toast. 
Perhaps there should be a cache of changes made on the client that can override
changes coming down from the server?

[Pete Williamson, 2013/08/09 01:27:22]

This seems like a fairly rare race condition, it seems abnormal for
notifications to be updated while onscreen since then normally only live for 7
seconds as toast.  Also, if the message changed in a visible way (say a new
message line) the user would understand why it is being shown again.

I propose we leave the code as is here.

+          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;
+
+      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();
         break;
     }
   }
@@ -224,13 +254,16 @@ scoped_ptr<SyncedNotification>
              << 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_read_notification =
+      (static_cast<SyncedNotification::ReadState>(
+          specifics.coalesced_notification().read_state()) ==
+       SyncedNotification::kRead &&
+       specifics.coalesced_notification().has_render_info());
   bool is_well_formed_dismissed_notification =
       (static_cast<SyncedNotification::ReadState>(
           specifics.coalesced_notification().read_state()) ==
@@ -238,12 +271,15 @@ scoped_ptr<SyncedNotification>
 
   // If the notification is poorly formed, return a null pointer.
   if (!is_well_formed_unread_notification &&
+      !is_well_formed_read_notification &&
       !is_well_formed_dismissed_notification) {
     DVLOG(1) << "Synced Notification is not well formed."
              << " unread well formed? "
              << is_well_formed_unread_notification
              << " dismissed well formed? "
-             << is_well_formed_dismissed_notification;
+             << is_well_formed_dismissed_notification
+             << " read well formed? "
+             << is_well_formed_read_notification;
     return scoped_ptr<SyncedNotification>();
   }
 
@@ -273,6 +309,20 @@ SyncedNotification* ChromeNotifierService::FindNotificationById(
   return NULL;
 }
 
+void ChromeNotifierService::FreeNotificationById(
+    const std::string& notification_id) {
+  for (std::vector<SyncedNotification*>::iterator it =
+          notification_data_.begin();
+      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
@@ -340,7 +390,7 @@ void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
     return;
   }
 
-  Display(notification_copy);
+  UpdateInMessageCenter(notification_copy);
 }
 
 void ChromeNotifierService::AddForTest(
@@ -348,12 +398,24 @@ 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()))
     return;
 
+  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,