Persist notification creation time.

Ensure notification displayed is latest
Limit number of notifications that can be received by the client. (Client side Garbage collection)

Review URL: http://codereview.chromium.org/8567018

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

chrome/browser/extensions/app_notification.cc

@@ -4,12 +4,14 @@
 
 #include "chrome/browser/extensions/app_notification.h"
 
+#include "base/string_number_conversions.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/common/guid.h"
 
 namespace {
 
 const char* kIsLocalKey = "is_local";
+const char* kCreationTime= "creation_time";
 const char* kGuidKey = "guid";
 const char* kExtensionIdKey = "extension_id";
 const char* kTitleKey = "title";
@@ -20,11 +22,13 @@ const char* kLinkTextKey = "link_text";
 }  // namespace
 
 AppNotification::AppNotification(bool is_local,
+                                 const base::Time& creation_time,
                                  const std::string& guid,
                                  const std::string& extension_id,
                                  const std::string& title,
                                  const std::string& body)
     : is_local_(is_local),
+      creation_time_(creation_time),
       extension_id_(extension_id),
       title_(title),
       body_(body) {
@@ -35,7 +39,8 @@ AppNotification::~AppNotification() {}
 
 AppNotification* AppNotification::Copy() {
   AppNotification* copy = new AppNotification(
-      this->is_local(), this->guid(), this->extension_id(),
+      this->is_local(), this->creation_time(),
+      this->guid(), this->extension_id(),
       this->title(), this->body());
   copy->set_link_url(this->link_url());
   copy->set_link_text(this->link_text());
@@ -45,6 +50,9 @@ AppNotification* AppNotification::Copy() {
 void AppNotification::ToDictionaryValue(DictionaryValue* result) {
   CHECK(result);
   result->SetBoolean(kIsLocalKey, is_local_);
+  if (!creation_time_.is_null())
+      result->SetString(kCreationTime,
+                        base::Int64ToString(creation_time_.ToInternalValue()));
   if (!guid_.empty())
     result->SetString(kGuidKey, guid_);
   if (!extension_id_.empty())
@@ -62,12 +70,31 @@ void AppNotification::ToDictionaryValue(DictionaryValue* result) {
 // static
 AppNotification* AppNotification::FromDictionaryValue(
     const DictionaryValue& value) {
-  scoped_ptr<AppNotification> result(new AppNotification(true, "", "", "", ""));
+  scoped_ptr<AppNotification> result(
+      new AppNotification(true,
+                          base::Time::FromInternalValue(0), "", "", "", ""));
 
   if (value.HasKey(kIsLocalKey) && !value.GetBoolean(
       kIsLocalKey, &result->is_local_)) {
     return NULL;
   }
+  if (value.HasKey(kCreationTime)) {
+      std::string time_string;
+      if (!value.GetString(kCreationTime, &time_string))
+        return NULL;
+      int64 time_internal;
+      if (!base::StringToInt64(time_string, &time_internal)) {
+        return NULL;
+      }
+      base::Time time = base::Time::FromInternalValue(time_internal);
+      if (time.is_null()) {
+        return NULL;
+      }
+      result->set_creation_time(time);
+  } else {
+    return NULL;
+  }
+
   if (value.HasKey(kGuidKey) && !value.GetString(kGuidKey, &result->guid_))
     return NULL;
   if (value.HasKey(kExtensionIdKey) &&
@@ -96,6 +123,7 @@ AppNotification* AppNotification::FromDictionaryValue(
 
 bool AppNotification::Equals(const AppNotification& other) const {
   return (is_local_ == other.is_local_ &&
+          creation_time_ == other.creation_time_ &&
           guid_ == other.guid_ &&
           extension_id_ == other.extension_id_ &&
           title_ == other.title_ &&

chrome/browser/extensions/app_notification.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/time.h"
 #include "base/memory/linked_ptr.h"
 #include "base/values.h"
 #include "googleurl/src/gurl.h"
@@ -21,6 +22,7 @@ class AppNotification {
   // If |is_local| is true, notification is not synced.
   // If |guid| is empty, a new guid is automatically created.
   AppNotification(bool is_local,
+                  const base::Time& creation_time,
                   const std::string& guid,
                   const std::string& extension_id,
                   const std::string& title,
@@ -34,9 +36,13 @@ class AppNotification {
   // Setters for optional properties.
   void set_link_url(const GURL& url) { link_url_ = url; }
   void set_link_text(const std::string& text) { link_text_ = text; }
+  void set_creation_time(const base::Time& creation_time) {
+    creation_time_ = creation_time;
+  }
 
   // Accessors.
   bool is_local() const { return is_local_; }
+  const base::Time creation_time() const { return creation_time_; }
   const std::string& guid() const { return guid_; }
   const std::string& extension_id() const { return extension_id_; }
   const std::string& title() const { return title_; }
@@ -59,6 +65,7 @@ class AppNotification {
   // Whether notification is local only, which means it is not synced
   // across machines.
   bool is_local_;
+  base::Time creation_time_;
   std::string guid_;
   std::string extension_id_;
   std::string title_;

chrome/browser/extensions/app_notification_manager.cc

@@ -63,9 +63,10 @@ void PopulateGuidToSyncDataMap(const SyncDataList& sync_data,
         sync_pb::app_notification).guid()] = *iter;
   }
 }
-
 }  // namespace
 
+const unsigned int AppNotificationManager::kMaxNotificationPerApp = 5;
+
 AppNotificationManager::AppNotificationManager(Profile* profile)
     : profile_(profile),
       sync_processor_(NULL),
@@ -93,6 +94,11 @@ void AppNotificationManager::Init() {
           this, storage_path));
 }
 
+bool AppNotificationSortPredicate(const linked_ptr<AppNotification> a1,
+                                  const linked_ptr<AppNotification> a2) {
+  return a1.get()->creation_time() < a2.get()->creation_time();
+}
+
 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 +111,14 @@ bool AppNotificationManager::Add(AppNotification* item) {
 
   SyncAddChange(*linked_item);
 
+  sort(list.begin(), list.end(), AppNotificationSortPredicate);
+
+  if (list.size() > AppNotificationManager::kMaxNotificationPerApp) {
+    AppNotification* removed = list.begin()->get();
+    SyncRemoveChange(*removed);
+    list.erase(list.begin());
+  }
+
   if (storage_.get()) {
     BrowserThread::PostTask(
         BrowserThread::FILE,
@@ -412,6 +426,20 @@ void AppNotificationManager::SyncAddChange(const AppNotification& notif) {
   sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
+void AppNotificationManager::SyncRemoveChange(const AppNotification& notif) {
+  // Skip if either:
+  // - Sync is not enabled by user.
+  // - Change is generated from within the manager.
+  if (notif.is_local() || !models_associated_) {
+    return;
+  }
+
+  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 +497,13 @@ SyncData AppNotificationManager::CreateSyncDataFromNotification(
   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_time().ToInternalValue());
   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 +516,14 @@ AppNotification* AppNotificationManager::CreateNotificationFromSyncData(
 
   // 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, base::Time::FromInternalValue(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());

chrome/browser/extensions/app_notification_manager.h

@@ -29,6 +29,7 @@ class AppNotificationManager
       public content::NotificationObserver,
       public SyncableService {
  public:
+  static const unsigned int kMaxNotificationPerApp;
   explicit AppNotificationManager(Profile* profile);
   virtual ~AppNotificationManager();
 
@@ -130,6 +131,9 @@ class AppNotificationManager
   // Sends a change to syncer to add the given notification.
   void SyncAddChange(const AppNotification& notif);
 
+  // Sends a change to syncer to remove the given notification.
+  void SyncRemoveChange(const AppNotification& notif);
+
   // Sends changes to syncer to remove all notifications in the given list.
   void SyncClearAllChange(const AppNotificationList& list);
 

chrome/browser/extensions/app_notification_manager_sync_unittest.cc

@@ -112,14 +112,14 @@ class AppNotificationManagerSyncTest : public testing::Test {
   static AppNotification* CreateNotification(bool is_local, int suffix) {
     std::string s = base::IntToString(suffix);
     return CreateNotification(
-        is_local, "guid" + s, "ext" + s, "text" + s, "body" + s,
+        is_local, suffix, "guid" + s, "ext" + s, "text" + s, "body" + s,
         "http://www.url" + s + ".com", "link text " + s);
   }
   static AppNotification* CreateNotification(
       bool is_local, int suffix, const std::string& extension_id) {
     std::string s = base::IntToString(suffix);
     return CreateNotification(
-        is_local, "guid" + s, extension_id, "text" + s, "body" + s,
+        is_local, suffix, "guid" + s, extension_id, "text" + s, "body" + s,
         "http://www.url" + s + ".com", "link text " + s);
   }
 
@@ -131,11 +131,13 @@ class AppNotificationManagerSyncTest : public testing::Test {
   static AppNotification* CreateNotificationNoLink(bool is_local, int suffix) {
     std::string s = base::IntToString(suffix);
     return CreateNotification(
-        is_local, "guid" + s, "ext" + s, "text" + s, "body" + s, "", "");
+        is_local, suffix,
+        "guid" + s, "ext" + s, "text" + s, "body" + s, "", "");
   }
 
   // link_url and link_text are only set if the passed in values are not empty.
   static AppNotification* CreateNotification(bool is_local,
+                                             int64 time,
                                              const std::string& guid,
                                              const std::string& extension_id,
                                              const std::string& title,
@@ -143,7 +145,8 @@ class AppNotificationManagerSyncTest : public testing::Test {
                                              const std::string& link_url,
                                              const std::string& link_text) {
     AppNotification* notif = new AppNotification(
-        is_local, guid, extension_id, title, body);
+        is_local, base::Time::FromInternalValue(time),
+        guid, extension_id, title, body);
     if (!link_url.empty())
       notif->set_link_url(GURL(link_url));
     if (!link_text.empty())
@@ -412,7 +415,8 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothNonEmptyTitleMismatch) {
   SyncDataList initial_data;
   initial_data.push_back(CreateSyncData(1));
   scoped_ptr<AppNotification> n1_a(CreateNotification(
-      n1->is_local(), n1->guid(), n1->extension_id(),
+      n1->is_local(), n1->creation_time().ToInternalValue(),
+      n1->guid(), n1->extension_id(),
       n1->title() + "_changed", // different title
       n1->body(), n1->link_url().spec(), n1->link_text()));
   initial_data.push_back(
@@ -500,6 +504,34 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesNonEmptyModel) {
   EXPECT_EQ(2, processor()->change_list_size());
 }
 
+// Process over 15 changes changes when model is not empty.
+TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesEmptyModelWithMax) {
+  const std::string& ext_id = "e1";
+  model()->MergeDataAndStartSyncing(
+      syncable::APP_NOTIFICATIONS,
+      SyncDataList(),
+      processor());
+  for (unsigned int i = 0;
+       i < AppNotificationManager::kMaxNotificationPerApp * 2; i++) {
+    SyncChangeList changes;
+    changes.push_back(CreateSyncChange(
+      SyncChange::ACTION_ADD, CreateNotification(false, i, ext_id)));
+    model()->ProcessSyncChanges(FROM_HERE, changes);
+    if (i < AppNotificationManager::kMaxNotificationPerApp) {
+      EXPECT_EQ(i + 1,
+                model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size());
+    } else {
+      EXPECT_EQ(AppNotificationManager::kMaxNotificationPerApp,
+        model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size());
+      for (unsigned int j = i; j > i - 5; j--) {
+        EXPECT_EQ(
+            AppNotificationManager::kMaxNotificationPerApp,
+            model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size());
+      }
+    }
+  }
+}
+
 // Process sync changes should return error if model association is not done.
 TEST_F(AppNotificationManagerSyncTest,
        ProcessSyncChangesErrorModelAssocNotDone) {

chrome/browser/extensions/app_notification_test_util.cc

@@ -29,7 +29,7 @@ void AddNotifications(AppNotificationList* list,
     std::string title = prefix + "_title_" + IntToString(i);
     std::string body = prefix + "_body_" + IntToString(i);
     AppNotification* item = new AppNotification(
-        true, guid, extension_id, title, body);
+        true, base::Time::Now(), guid, extension_id, title, body);
     if (i % 2 == 0) {
       item->set_link_url(GURL("http://www.example.com/" + prefix));
       item->set_link_text(prefix + "_link_" + IntToString(i));

chrome/browser/extensions/extension_app_api.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/extensions/extension_app_api.h"
 
 #include "base/values.h"
+#include "base/time.h"
 #include "chrome/browser/extensions/app_notification_manager.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -47,7 +48,7 @@ bool AppNotifyFunction::RunImpl() {
     EXTENSION_FUNCTION_VALIDATE(details->GetString(kBodyTextKey, &body));
 
   scoped_ptr<AppNotification> item(new AppNotification(
-      true, "", id, title, body));
+      true, base::Time::Now(), "", id, title, body));
 
   if (details->HasKey(kLinkUrlKey)) {
     std::string link_url;