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 104 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) {

[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;
     }
   }
 
   return error;
 }
 
 // Support functions for data type conversion.
 
 // Static method.  Get to the sync data in our internal format.
@@ skipping 18 matching lines @@
       !specifics.coalesced_notification().has_key() ||
       !specifics.coalesced_notification().has_read_state()) {
     DVLOG(1) << "Synced Notification missing mandatory fields "
              << "has coalesced notification? "
              << specifics.has_coalesced_notification()
              << " has key? " << specifics.coalesced_notification().has_key()
              << " 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_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()) ==
        SyncedNotification::kDismissed);
 
   // 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>();
   }
 
   // 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.
 SyncedNotification* ChromeNotifierService::FindNotificationById(
     const std::string& notification_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 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 (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();
+      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;
 
+  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