Limit number of notifications that can be received by the client.

chrome/browser/extensions/app_notification_manager.cc

Index: chrome/browser/extensions/app_notification_manager.cc
===================================================================
--- chrome/browser/extensions/app_notification_manager.cc	(revision 109957)
+++ chrome/browser/extensions/app_notification_manager.cc	(working copy)
@@ -93,6 +93,11 @@
           this, storage_path));
 }
 
+bool AppNotificationSortPredicate(const linked_ptr<AppNotification> a1,
+                                  const linked_ptr<AppNotification> a2) {
+  return a1.get()->creation_timestamp_ms() < a2.get()->creation_timestamp_ms();
+}
+
 bool AppNotificationManager::Add(AppNotification* item) {
   // Do this first since we own the incoming item and hence want to delete
   // it in error paths.
@@ -105,6 +110,14 @@
 
   SyncAddChange(*linked_item);
 
+  if (list.size() > notification_manager_constants::kMaxNotificationPerApp) {
+    AppNotification* removed =
+        std::min_element(list.begin(), list.end(),
+                         AppNotificationSortPredicate)->get();
+    SyncRemoveChange(*removed, true);
+    list.erase(list.begin());
+  }

[Munjal (Google), 2011/11/17 19:45:32]

Actually, I am wondering if we should add in a sorted manner into the list. That
way we can always return the head in GetLast and delete the last one here (or
vice versa depending on whether sort is asc or desc).

[asargent_no_longer_on_chrome, 2011/11/17 22:36:24]

+1 - keeping them sorted is a good idea

+
   if (storage_.get()) {
     BrowserThread::PostTask(
         BrowserThread::FILE,
@@ -151,7 +164,8 @@
   const AppNotificationList& list = found->second;
   if (list.empty())
     return NULL;
-  return list.rbegin()->get();
+  return std::max_element(list.begin(), list.end(),
+                          AppNotificationSortPredicate)->get();
 }
 
 void AppNotificationManager::ClearAll(const std::string& extension_id) {
@@ -412,6 +426,24 @@
   sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
+void AppNotificationManager::SyncRemoveChange(const AppNotification& notif,
+                                              bool shouldProcessSyncerChanges) {

[Munjal (Google), 2011/11/17 19:45:32]

Actually seems like you call this with true for second parameter. Remove second
parameter then?

[elvin, 2011/11/17 22:28:17]

Done.

+  // Skip if either:
+  // - Sync is not enabled by user.
+  // - Change is generated from within the manager.
+  if (notif.is_local() ||
+       ((!models_associated_ || processing_syncer_changes_)
+         && !shouldProcessSyncerChanges))

[Munjal (Google), 2011/11/17 19:45:32]

Don't we want this to happen even when we are processing syncer changes?

Would the following suffice?
if (notif.is_local() || !modles_associated_)
  reutnr;

[elvin, 2011/11/17 22:28:17]

Done.

+    return;

[Munjal (Google), 2011/11/17 19:45:32]

If your if is multiple lines you need braces even for single line in the body.

[elvin, 2011/11/17 22:28:17]

Done.

+
+  // TODO(munjal): crbug.com/10059. Work with Lingesh/Antony to resolve.

[Munjal (Google), 2011/11/17 19:45:32]

This TODO is not relevant here I think.

[elvin, 2011/11/17 22:28:17]

Done.

+
+  SyncChangeList changes;
+  SyncData sync_data = CreateSyncDataFromNotification(notif);
+  changes.push_back(SyncChange(SyncChange::ACTION_DELETE, sync_data));
+  sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
+}
+
 void AppNotificationManager::SyncClearAllChange(
   const AppNotificationList& list) {
   // Skip if either:
@@ -469,12 +501,13 @@
   sync_pb::AppNotificationSpecifics* notif_specifics =
       specifics.MutableExtension(sync_pb::app_notification);
   notif_specifics->set_app_id(notification.extension_id());
+  notif_specifics->set_creation_timestamp_ms(
+      notification.creation_timestamp_ms());
   notif_specifics->set_body_text(notification.body());
   notif_specifics->set_guid(notification.guid());
   notif_specifics->set_link_text(notification.link_text());
   notif_specifics->set_link_url(notification.link_url().spec());
   notif_specifics->set_title(notification.title());
-
   return SyncData::CreateLocalData(
       notif_specifics->guid(), notif_specifics->app_id(), specifics);
 }
@@ -487,12 +520,14 @@
 
   // Check for mandatory fields.
   if (!specifics.has_app_id() || !specifics.has_guid() ||
-      !specifics.has_title() || !specifics.has_body_text()) {
+      !specifics.has_title() || !specifics.has_body_text() ||
+      !specifics.has_creation_timestamp_ms()) {
     return NULL;
   }
 
   AppNotification* notification = new AppNotification(
-      false, specifics.guid(), specifics.app_id(),
+      false, specifics.creation_timestamp_ms(),
+      specifics.guid(), specifics.app_id(),
       specifics.title(), specifics.body_text());
   if (specifics.has_link_text())
     notification->set_link_text(specifics.link_text());