Process incoming updates and deletes properly.

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 109 matching lines @@
           // If local state changed, notify Notification UI Manager.
         }
       // For any other conflict besides read state, treat it as an update.
       } else {
         // If different, just replace the local with the remote.
         // TODO(petewil): Someday we may allow changes from the client to
         // flow upwards, when we do, we will need better merge resolution.
         found->Update(sync_data);
 
         // Tell the notification manager to update the notification.
-        Display(found);
+        UpdateInMessageCenter(found);
       }
     }
   }
 
   // Send up the changes that were made locally.
   if (new_changes.size() > 0) {
     merge_result.set_error(sync_processor_->ProcessSyncChanges(
         FROM_HERE, new_changes));
   }
 
@@ skipping 34 matching lines @@
     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;
     }
 
+    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;
     }
   }
 
   return error;
 }
 
 // Support functions for data type conversion.
 
 // Static method.  Get to the sync data in our internal format.
@@ skipping 74 matching lines @@
       it != notification_data_.end();
       ++it) {
     SyncedNotification* notification = *it;
     if (notification_id == notification->GetKey())
       return *it;
   }
 
   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
   // sample app.
 
   // TODO(petewil): Really obtain the list of synced notification sending
   // services from the server and create the list of ids here.  Until then, we
   // are hardcoding the service names.  Once that is done, remove this
   // hardcoding.
   // crbug.com/248337
@@ skipping 47 matching lines @@
 
   // If the user is not interested in this type of notification, ignore it.
   std::vector<std::string>::iterator iter =
       find(enabled_sending_services_.begin(),
            enabled_sending_services_.end(),
            notification_copy->GetSendingServiceId());
   if (iter == enabled_sending_services_.end()) {
     return;
   }
 
-  Display(notification_copy);
+  UpdateInMessageCenter(notification_copy);
 }
 
 void ChromeNotifierService::AddForTest(
     scoped_ptr<notifier::SyncedNotification> notification) {
   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;
 
   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,
                                      profile_);
 
   // Our tests cannot use the network for reliability reasons.
   if (avoid_bitmap_fetching_for_test_) {
     return;
   }
 
@@ skipping 17 matching lines @@
   // Remove the notifier_id if it is disabled and present.
   } else if (iter != enabled_sending_services_.end() && !enabled) {
     enabled_sending_services_.erase(iter);
   }
 
   // Otherwise, nothing to do, we can exit.
   return;
 }
 
 }  // namespace notifier