Synced Notification dismissals and updates need to be go to message ctr.

If an update for a Synced notification or a dismissal arrives, we should
update the message center by letting it know.  This changelist includes the
changes to do that, and some unit tests for the feature changes.

BUG=248330

Review URL: https://chromiumcodereview.appspot.com/18083028

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@210956 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 115 insertions, 42 deletions

diff --git a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
index 8cb839f..c97d37d 100644
--- a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
+++ b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
@@ -94,7 +94,8 @@ syncer::SyncMergeResult ChromeNotifierService::MergeDataAndStartSyncing(
           if (incoming->GetReadState() == SyncedNotification::kDismissed) {
             // If it is marked as read on the server, but not the client.
             found->NotificationHasBeenDismissed();
-            // TODO(petewil): Tell the Notification UI Manager to mark it read.
+            // Tell the Notification UI Manager to mark it read.
+            notification_manager_->CancelById(found->GetKey());
           } else {
             // If it is marked as read on the client, but not the server.
             syncer::SyncData sync_data = CreateSyncDataFromNotification(*found);
@@ -111,6 +112,9 @@ syncer::SyncMergeResult ChromeNotifierService::MergeDataAndStartSyncing(
         // 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);
       }
     }
   }
@@ -149,7 +153,7 @@ syncer::SyncDataList ChromeNotifierService::GetAllSyncData(
 syncer::SyncError ChromeNotifierService::ProcessSyncChanges(
       const tracked_objects::Location& from_here,
       const syncer::SyncChangeList& change_list) {
-  // TODO(petewil): add a check that we are called on the thread we expect.
+  // TODO(petewil): Add a check that we are called on the thread we expect.
   syncer::SyncError error;
 
   for (syncer::SyncChangeList::const_iterator it = change_list.begin();
@@ -224,7 +228,7 @@ scoped_ptr<SyncedNotification>
           specifics.coalesced_notification().read_state()) ==
        SyncedNotification::kDismissed);
 
-  // if the notification is poorly formed, return a null pointer
+  // If the notification is poorly formed, return a null pointer.
   if (!is_well_formed_unread_notification &&
       !is_well_formed_dismissed_notification) {
     DVLOG(1) << "Synced Notification is not well formed."
@@ -263,8 +267,8 @@ SyncedNotification* ChromeNotifierService::FindNotificationByKey(
 
 void ChromeNotifierService::GetSyncedNotificationServices(
     std::vector<message_center::Notifier*>* notifiers) {
-  // TODO(mukai|petewil): should check the profile's eligibility before adding
-  // the sample app.
+  // TODO(mukai|petewil): Check the profile's eligibility before adding the
+  // sample app.
 
   // Currently we just use kSampleSyncedNotificationServiceId as a place holder.
   // TODO(petewil): Really obtain the list of apps from the server and create
@@ -279,7 +283,7 @@ void ChromeNotifierService::GetSyncedNotificationServices(
       l10n_util::GetStringUTF16(
           IDS_MESSAGE_CENTER_SAMPLE_SYNCED_NOTIFICATION_SERVICE_NAME),
       desktop_notification_service->IsNotifierEnabled(notifier_id)));
-  // TODO(mukai): should add icon for the sample app.
+  // TODO(mukai): Add icon for the sample app.
 }
 
 void ChromeNotifierService::MarkNotificationAsDismissed(
@@ -307,8 +311,17 @@ void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
   // Take ownership of the object and put it into our local storage.
   notification_data_.push_back(notification.release());
 
+  Display(notification_copy);
+}
+
+void ChromeNotifierService::AddForTest(
+    scoped_ptr<notifier::SyncedNotification> notification) {
+    notification_data_.push_back(notification.release());
+  }
+
+void ChromeNotifierService::Display(SyncedNotification* notification) {
   // Set up to fetch the bitmaps.
-  notification_copy->QueueBitmapFetchJobs(notification_manager_,
+  notification->QueueBitmapFetchJobs(notification_manager_,
                                           this,
                                           profile_);
 
@@ -319,7 +332,7 @@ void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
 
   // Start the bitmap fetching, Show() will be called when the last bitmap
   // either arrives or times out.
-  notification_copy->StartBitmapFetch();
+  notification->StartBitmapFetch();
 }
 
 void ChromeNotifierService::OnSyncedNotificationServiceEnabled(
diff --git a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
index ba3666d..52fd6bf 100644
--- a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
+++ b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
@@ -73,10 +73,8 @@ class ChromeNotifierService : public syncer::SyncableService,
       const std::string& notifier_id,
       bool enabled);
 
-  // functions for test
-  void AddForTest(scoped_ptr<notifier::SyncedNotification> notification) {
-    Add(notification.Pass());
-  }
+  // Functions for test.
+  void AddForTest(scoped_ptr<notifier::SyncedNotification> notification);
 
   // If we allow the tests to do bitmap fetching, they will attempt to fetch
   // a URL from the web, which will fail.  We can already test the majority
@@ -90,8 +88,8 @@ class ChromeNotifierService : public syncer::SyncableService,
   // Add a notification to our list.  This takes ownership of the pointer.
   void Add(scoped_ptr<notifier::SyncedNotification> notification);
 
-  // Display a notification in the notification center.
-  void Show(notifier::SyncedNotification* notification);
+  // Display a notification in the notification center (eventually).
+  void Display(notifier::SyncedNotification* notification);
 
   // Back pointer to the owning profile.
   Profile* const profile_;
@@ -99,7 +97,7 @@ class ChromeNotifierService : public syncer::SyncableService,
   scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
   static bool avoid_bitmap_fetching_for_test_;
 
-  // TODO(petewil): consider whether a map would better suit our data.
+  // TODO(petewil): Consider whether a map would better suit our data.
   // If there are many entries, lookup time may trump locality of reference.
   ScopedVector<notifier::SyncedNotification> notification_data_;
 
diff --git a/chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc b/chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
index 5ad3b95..5f30cac 100644
--- a/chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
+++ b/chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
@@ -140,7 +140,8 @@ class StubNotificationUIManager : public NotificationUIManager {
   // Removes any notifications matching the supplied ID, either currently
   // displayed or in the queue.  Returns true if anything was removed.
   virtual bool CancelById(const std::string& notification_id) OVERRIDE {
-    return false;
+    dismissed_id_ = notification_id;
+    return true;
   }
 
   // Adds the notification_id for each outstanding notification to the set
@@ -173,10 +174,14 @@ class StubNotificationUIManager : public NotificationUIManager {
   // Test hook to get the notification so we can check it
   const Notification& notification() const { return notification_; }
 
+  // Test hook to check the ID of the last notification cancelled.
+  std::string& dismissed_id() { return dismissed_id_; }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(StubNotificationUIManager);
   Notification notification_;
   Profile* profile_;
+  std::string dismissed_id_;
 };
 
 // Dummy SyncChangeProcessor used to help review what SyncChanges are pushed
@@ -700,6 +705,10 @@ TEST_F(ChromeNotifierServiceTest, ModelAssocBothNonEmptyReadMismatch1) {
   EXPECT_TRUE(notifier.FindNotificationByKey(kKey2));
   EXPECT_FALSE(notifier.FindNotificationByKey(kKey3));
 
+  // Make sure that the notification manager was told to dismiss the
+  // notification.
+  EXPECT_EQ(std::string(kKey1), notification_manager.dismissed_id());
+
   // Ensure no new data will be sent to the remote store for notification1.
   EXPECT_EQ(0U, processor()->change_list_size());
   EXPECT_FALSE(processor()->ContainsId(kKey1));
diff --git a/chrome/browser/notifications/sync_notifier/synced_notification.cc b/chrome/browser/notifications/sync_notifier/synced_notification.cc
index 69c19d6..43eafd9 100644
--- a/chrome/browser/notifications/sync_notifier/synced_notification.cc
+++ b/chrome/browser/notifications/sync_notifier/synced_notification.cc
@@ -54,7 +54,7 @@ SyncedNotification::SyncedNotification(const syncer::SyncData& sync_data)
 SyncedNotification::~SyncedNotification() {}
 
 void SyncedNotification::Update(const syncer::SyncData& sync_data) {
-  // TODO(petewil): Let's add checking that the notification looks valid.
+  // TODO(petewil): Add checking that the notification looks valid.
   specifics_.CopyFrom(sync_data.GetSpecifics().synced_notification());
 }
 
@@ -72,7 +72,7 @@ void SyncedNotification::OnFetchComplete(const GURL url,
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   // Match the incoming bitmaps to URLs.  In case this is a dup, make sure to
-  // try all potentially matching urls.
+  // Try all potentially matching urls.
   if (GetAppIconUrl() == url && bitmap != NULL) {
     app_icon_bitmap_ = gfx::Image::CreateFrom1xBitmap(*bitmap);
   }
@@ -99,7 +99,6 @@ void SyncedNotification::QueueBitmapFetchJobs(
     NotificationUIManager* notification_manager,
     ChromeNotifierService* notifier_service,
     Profile* profile) {
-
   // If we are not using the MessageCenter, call show now, and the existing
   // code will handle the bitmap fetch for us.
   if (!UseRichNotifications()) {
@@ -114,7 +113,7 @@ void SyncedNotification::QueueBitmapFetchJobs(
   DCHECK_EQ(active_fetcher_count_, 0);
 
   // Get the URLs that we might need to fetch from Synced Notification.
-  // TODO(petewil): clean up the fact that icon and image return a GURL, and
+  // TODO(petewil): Clean up the fact that icon and image return a GURL, and
   // button urls return a string.
   // TODO(petewil): Eventually refactor this to accept an arbitrary number of
   // button URLs.
@@ -156,6 +155,16 @@ void SyncedNotification::AddBitmapToFetchQueue(const GURL& url) {
 void SyncedNotification::Show(NotificationUIManager* notification_manager,
                               ChromeNotifierService* notifier_service,
                               Profile* profile) {
+
+  // Let NotificationUIManager know that the notification has been dismissed.
+  if (SyncedNotification::kRead == GetReadState() ||
+      SyncedNotification::kDismissed == GetReadState() ) {
+    notification_manager->CancelById(GetKey());
+    DVLOG(2) << "Dismissed notification arrived"
+             << GetHeading() << " " << GetText();
+    return;
+  }
+
   // Set up the fields we need to send and create a Notification object.
   GURL image_url = GetImageUrl();
   string16 text = UTF8ToUTF16(GetText());
@@ -169,15 +178,6 @@ void SyncedNotification::Show(NotificationUIManager* notification_manager,
   scoped_refptr<NotificationDelegate> delegate =
       new ChromeNotifierDelegate(GetKey(), notifier_service);
 
-  // TODO(petewil): For now, just punt on dismissed notifications until
-  // I change the interface to let NotificationUIManager know the right way.
-  if (SyncedNotification::kRead == GetReadState() ||
-      SyncedNotification::kDismissed == GetReadState() ) {
-    DVLOG(2) << "Dismissed notification arrived"
-             << GetHeading() << " " << GetText();
-    return;
-  }
-
   // Some inputs and fields are only used if there is a notification center.
   if (UseRichNotifications()) {
     base::Time creation_time =
@@ -265,23 +265,58 @@ void SyncedNotification::Show(NotificationUIManager* notification_manager,
   return;
 }
 
-// TODO(petewil): Decide what we need for equals - is this enough, or should
-// we exhaustively compare every field in case the server refreshed the notif?
+// This should detect even small changes in case the server updated the
+// notification.
+// TODO(petewil): Should I also ignore the timestamp if other fields match?
 bool SyncedNotification::EqualsIgnoringReadState(
     const SyncedNotification& other) const {
-  return (GetTitle() == other.GetTitle() &&
-          GetAppId() == other.GetAppId() &&
-          GetKey() == other.GetKey() &&
-          GetText() == other.GetText() &&
-          GetOriginUrl() == other.GetOriginUrl() &&
-          GetAppIconUrl() == other.GetAppIconUrl() &&
-          GetImageUrl() == other.GetImageUrl() );
+  if (GetTitle() == other.GetTitle() &&
+      GetHeading() == other.GetHeading() &&
+      GetDescription() == other.GetDescription() &&
+      GetAppId() == other.GetAppId() &&
+      GetKey() == other.GetKey() &&
+      GetOriginUrl() == other.GetOriginUrl() &&
+      GetAppIconUrl() == other.GetAppIconUrl() &&
+      GetImageUrl() == other.GetImageUrl() &&
+      GetText() == other.GetText() &&
+      // We intentionally skip read state
+      GetCreationTime() == other.GetCreationTime() &&
+      GetPriority() == other.GetPriority() &&
+      GetDefaultDestinationTitle() == other.GetDefaultDestinationTitle() &&
+      GetDefaultDestinationIconUrl() == other.GetDefaultDestinationIconUrl() &&
+      GetButtonOneTitle() == other.GetButtonOneTitle() &&
+      GetButtonOneIconUrl() == other.GetButtonOneIconUrl() &&
+      GetButtonTwoTitle() == other.GetButtonTwoTitle() &&
+      GetButtonTwoIconUrl() == other.GetButtonTwoIconUrl() &&
+      GetNotificationCount() == other.GetNotificationCount() &&
+      GetButtonCount() == other.GetButtonCount()) {
+    // If all the surface data matched, check, to see if contained data also
+    // matches.
+    size_t count = GetNotificationCount();
+    for (size_t ii = 0; ii < count; ++ii) {
+      // Check the contained titles match
+      if (GetContainedNotificationTitle(ii) !=
+          other.GetContainedNotificationTitle(ii))
+        return false;
+      // Check the contained messages match
+      if (GetContainedNotificationMessage(ii) !=
+          other.GetContainedNotificationMessage(ii))
+        return false;
+    }
+
+    // TODO(petewil): When I make buttons into a vector, check them here too.
+
+    // If buttons and notifications matched, they are equivalent.
+    return true;
+  }
+
+  return false;
 }
 
 // Set the read state on the notification, returns true for success.
 void SyncedNotification::SetReadState(const ReadState& read_state) {
 
-  // convert the read state to the protobuf type for read state
+  // Convert the read state to the protobuf type for read state.
   if (kDismissed == read_state)
     specifics_.mutable_coalesced_notification()->set_read_state(
         sync_pb::CoalescedSyncedNotification_ReadState_DISMISSED);
@@ -344,8 +379,8 @@ GURL SyncedNotification::GetOriginUrl() const {
   return GURL(origin_url);
 }
 
-// TODO(petewil): This only returns the first icon. We should make all the
-// icons available.
+// TODO(petewil): This only returns the first icon. Make all the icons
+// available.
 GURL SyncedNotification::GetAppIconUrl() const {
   if (specifics_.coalesced_notification().render_info().expanded_info().
       collapsed_info_size() == 0)
diff --git a/chrome/browser/notifications/sync_notifier/synced_notification_unittest.cc b/chrome/browser/notifications/sync_notifier/synced_notification_unittest.cc
index f1f94838..8ee17a5 100644
--- a/chrome/browser/notifications/sync_notifier/synced_notification_unittest.cc
+++ b/chrome/browser/notifications/sync_notifier/synced_notification_unittest.cc
@@ -110,7 +110,8 @@ class StubNotificationUIManager : public NotificationUIManager {
   // Removes any notifications matching the supplied ID, either currently
   // displayed or in the queue.  Returns true if anything was removed.
   virtual bool CancelById(const std::string& notification_id) OVERRIDE {
-    return false;
+    dismissed_id_ = notification_id;
+    return true;
   }
 
   virtual std::set<std::string> GetAllIdsByProfileAndSourceOrigin(
@@ -141,10 +142,14 @@ class StubNotificationUIManager : public NotificationUIManager {
   // Test hook to get the notification so we can check it
   const Notification& notification() const { return notification_; }
 
+  // Test hook to check the ID of the last notification cancelled.
+  std::string& dismissed_id() { return dismissed_id_; }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(StubNotificationUIManager);
   Notification notification_;
   Profile* profile_;
+  std::string dismissed_id_;
 };
 
 class SyncedNotificationTest : public testing::Test {
@@ -618,6 +623,19 @@ TEST_F(SyncedNotificationTest, ShowTest) {
   EXPECT_EQ(UTF8ToUTF16(kContainedMessage3), notification.items()[2].message);
 }
 
+TEST_F(SyncedNotificationTest, DismissTest) {
+
+  if (!UseRichNotifications())
+    return;
+
+  StubNotificationUIManager notification_manager;
+
+  // Call the method under test using a dismissed notification.
+  notification4_->Show(&notification_manager, NULL, NULL);
+
+  EXPECT_EQ(std::string(kKey1), notification_manager.dismissed_id());
+}
+
 TEST_F(SyncedNotificationTest, AddBitmapToFetchQueueTest) {
   scoped_ptr<SyncedNotification> notification6;
   notification6.reset(new SyncedNotification(sync_data1_));