Store the actual notification data in the notification database.

content/browser/notifications/notification_database.cc

Index: content/browser/notifications/notification_database.cc
diff --git a/content/browser/notifications/notification_database.cc b/content/browser/notifications/notification_database.cc
index 4d8b4f9ca9a94a00a49455b33b75c8c0d4298872..c8a5eb1a4516c85c1fbbe7ef7ff775664ab19e74 100644
--- a/content/browser/notifications/notification_database.cc
+++ b/content/browser/notifications/notification_database.cc
@@ -8,24 +8,37 @@
 
 #include "base/files/file_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "content/browser/notifications/notification_database_data.h"
 #include "content/public/browser/browser_thread.h"
+#include "storage/common/database/database_identifier.h"
 #include "third_party/leveldatabase/src/helpers/memenv/memenv.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
 #include "third_party/leveldatabase/src/include/leveldb/env.h"
 #include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
+#include "url/gurl.h"
 
 // Notification LevelDB database schema (in alphabetized order)
 // =======================
 //
+// key: "DATA:" <origin identifier> '\x00' <notification_id>
+// value: String containing the NotificationDatabaseDataProto protocol buffer
+//        in serialized form.
+//
 // key: "NEXT_NOTIFICATION_ID"
 // value: Decimal string which fits into an int64_t.
 //
+//
 
 namespace content {
 namespace {
 
 // Keys of the fields defined in the database.
 const char kNextNotificationIdKey[] = "NEXT_NOTIFICATION_ID";
+const char kDataKeyPrefix[] = "DATA:";
+
+// Separates the components of compound keys.
+const char kKeySeparator = '\x00';
 
 // The first notification id which to be handed out by the database.
 const int64_t kFirstNotificationId = 1;
@@ -43,10 +56,20 @@ NotificationDatabase::Status LevelDBStatusToStatus(
   return NotificationDatabase::STATUS_ERROR_FAILED;
 }
 
+// Creates the compound data key in which notification data is stored.
+std::string CreateDataKey(int64_t notification_id, const GURL& origin) {
+  return base::StringPrintf("%s%s%c%s",
+                            kDataKeyPrefix,
+                            storage::GetIdentifierFromOrigin(origin).c_str(),
+                            kKeySeparator,
+                            base::Int64ToString(notification_id).c_str());
+}
+
 }  // namespace
 
 NotificationDatabase::NotificationDatabase(const base::FilePath& path)
     : path_(path),
+      next_notification_id_(0),

[cmumford, 2015/03/16 17:14:07]

Could initialize this (and state_) in the header if you want. Not required I
believe.

[Peter Beverloo, 2015/03/16 17:48:23]

Cool, I didn't know we allowed that :). Changed!

       state_(STATE_UNINITIALIZED) {
   sequence_checker_.DetachFromSequence();
 }
@@ -85,37 +108,90 @@ NotificationDatabase::Status NotificationDatabase::Open(
   state_ = STATE_INITIALIZED;
   db_.reset(db);
 
-  return STATUS_OK;
+  return GetNextNotificationId(&next_notification_id_);

[cmumford, 2015/03/16 17:14:07]

Now that GetNextNotificationId is private and is always called to set
next_notification_id_ you could simplify it to accept no params, and set
next_notification_id_ within.

[Peter Beverloo, 2015/03/16 17:48:23]

Done.

 }
 
-NotificationDatabase::Status NotificationDatabase::GetNextNotificationId(
-    int64_t* notification_id) const {
+NotificationDatabase::Status NotificationDatabase::ReadNotificationData(
+    int64_t notification_id,
+    const GURL& origin,
+    NotificationDatabaseData* notification_database_data) const {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  DCHECK_EQ(state_, STATE_INITIALIZED);
-  DCHECK(notification_id);
+  DCHECK_EQ(STATE_INITIALIZED, state_);
+  DCHECK_GE(notification_id, kFirstNotificationId);

[cmumford, 2015/03/16 17:14:07]

Could you also add DCHECK_LT(notification_id, next_notification_id_), or do you
want to allow attempts to read non-existent notifications?

[Peter Beverloo, 2015/03/16 17:48:23]

It would just return STATUS_NOT_FOUND, which in itself is fine.

The reason I didn't add that DCHECK is that keeping the database synchronized
with what's being shown on Android is going to be tricky (FYI - the Android
framework doesn't tell us which of Chrome's notifications are still being
shown). There will be cases which we initially miss, and crashing the entire
browser for that is harsh, even if it's in debug builds only.

+  DCHECK(origin.is_valid());
+  DCHECK(notification_database_data);
+
+  std::string key = CreateDataKey(notification_id, origin);
+  std::string serialized_data;
 
-  std::string value;
   Status status = LevelDBStatusToStatus(
-      db_->Get(leveldb::ReadOptions(), kNextNotificationIdKey, &value));
+      db_->Get(leveldb::ReadOptions(), key, &serialized_data));
+  if (status != STATUS_OK)
+    return status;
 
-  if (status == STATUS_ERROR_NOT_FOUND) {
-    *notification_id = kFirstNotificationId;
+  if (notification_database_data->ParseFromString(serialized_data))
     return STATUS_OK;
+
+  DLOG(ERROR) << "Unable to deserialize data for notification "
+              << notification_id << " belonging to " << origin << ".";
+  return STATUS_ERROR_CORRUPTED;
+}
+
+NotificationDatabase::Status NotificationDatabase::WriteNotificationData(
+    const GURL& origin,
+    const NotificationDatabaseData& notification_database_data,
+    int64_t* notification_id) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK_EQ(STATE_INITIALIZED, state_);
+  DCHECK(notification_id);
+  DCHECK(origin.is_valid());
+
+  std::string serialized_data;
+  if (!notification_database_data.SerializeToString(&serialized_data)) {
+    DLOG(ERROR) << "Unable to serialize data for a notification belonging "
+                << "to: " << origin;
+    return STATUS_ERROR_FAILED;
   }
 
+  DCHECK_GE(next_notification_id_, kFirstNotificationId);
+
+  leveldb::WriteBatch batch;
+  batch.Put(CreateDataKey(next_notification_id_, origin), serialized_data);
+  batch.Put(kNextNotificationIdKey,
+            base::Int64ToString(next_notification_id_ + 1));
+
+  Status status = LevelDBStatusToStatus(
+      db_->Write(leveldb::WriteOptions(), &batch));
   if (status != STATUS_OK)
     return status;
 
-  int64_t next_notification_id;
-  if (!base::StringToInt64(value, &next_notification_id) ||
-      next_notification_id < kFirstNotificationId) {
-    return STATUS_ERROR_CORRUPTED;
-  }
-
-  *notification_id = next_notification_id;
+  *notification_id = next_notification_id_++;
   return STATUS_OK;
 }
 
+NotificationDatabase::Status NotificationDatabase::DeleteNotificationData(
+    int64_t notification_id,
+    const GURL& origin) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK_EQ(STATE_INITIALIZED, state_);
+  DCHECK_GE(notification_id, kFirstNotificationId);
+  DCHECK(origin.is_valid());
+
+  std::string key = CreateDataKey(notification_id, origin);
+
+  // TODO(peter): Instead of validating the existence of the data by reading the

[cmumford, 2015/03/16 17:14:07]

Regarding validation. I would suggest making this function idempotent and simply
not error out if the key doesn't exist. leveldb will still read data from the db
- even if the value doesn't exist (which you can improve by adding a Bloom
filter). See issue 409520 for more info on the cost of "empty get's".

Can you make this the responsibility of the caller?

[Peter Beverloo, 2015/03/16 17:48:23]

Issue 409520 provides good context. I have been doubting about whether this is
necessary or not - if someone calls DeleteNotificationData then their intended
goal will be for the data to be gone. If it didn't exist in the first place,
their goal is still satisfied.

Per your arguments I've changed this to return STATUS_OK for non-existing
notifications as well.

+  // notification data, the notification list for |origin| should be considered
+  // instead. That list hasn't been implemented yet, however.
+
+  std::string value;
+  Status status = LevelDBStatusToStatus(
+      db_->Get(leveldb::ReadOptions(), key, &value));
+  if (status != STATUS_OK)
+    return status;
+
+  return LevelDBStatusToStatus(db_->Delete(leveldb::WriteOptions(), key));
+}
+
 NotificationDatabase::Status NotificationDatabase::Destroy() {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
 
@@ -134,13 +210,30 @@ NotificationDatabase::Status NotificationDatabase::Destroy() {
       leveldb::DestroyDB(path_.AsUTF8Unsafe(), options));
 }
 
-void NotificationDatabase::WriteNextNotificationId(
-    leveldb::WriteBatch* batch,
-    int64_t next_notification_id) const {
-  DCHECK_GE(next_notification_id, kFirstNotificationId);
-  DCHECK(batch);
+NotificationDatabase::Status NotificationDatabase::GetNextNotificationId(
+    int64_t* notification_id) const {
+  DCHECK(notification_id);
+
+  std::string value;
+  Status status = LevelDBStatusToStatus(
+      db_->Get(leveldb::ReadOptions(), kNextNotificationIdKey, &value));
+
+  if (status == STATUS_ERROR_NOT_FOUND) {
+    *notification_id = kFirstNotificationId;
+    return STATUS_OK;
+  }
+
+  if (status != STATUS_OK)
+    return status;
+
+  int64_t next_notification_id;

[cmumford, 2015/03/16 17:14:07]

No need for local next_notification_id variable, but regarding other comment I
recommend using next_notification_id_ directly.

[Peter Beverloo, 2015/03/16 17:48:23]

Done.

+  if (!base::StringToInt64(value, &next_notification_id) ||
+      next_notification_id < kFirstNotificationId) {
+    return STATUS_ERROR_CORRUPTED;
+  }
 
-  batch->Put(kNextNotificationIdKey, base::Int64ToString(next_notification_id));
+ *notification_id = next_notification_id;
+  return STATUS_OK;
 }
 
 }  // namespace content