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..64eba36ee6bb314e063fbab4a7504455855b1caa 100644
--- a/content/browser/notifications/notification_database.cc
+++ b/content/browser/notifications/notification_database.cc
@@ -8,11 +8,14 @@
 
 #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 "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)
 // =======================
@@ -26,6 +29,10 @@ 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';

[cmumford, 2015/03/13 17:13:15]

Not seriously suggesting this, but I've never seen ASCII record/group separator
char values used (ASCII values 0x1C - 0x1F).

[Peter Beverloo, 2015/03/13 20:32:32]

Heh, I didn't even know these existed. I guess \x1D could be appropriate.
LevelDB handles both \x00 and \x1D well, so implementation wise it doesn't
really matter.

 
 // The first notification id which to be handed out by the database.
 const int64_t kFirstNotificationId = 1;
@@ -43,6 +50,15 @@ 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,
+                            origin.spec().c_str(),

[cmumford, 2015/03/13 17:13:15]

Instead of using the origin directly (which may be totally OK), you may want to
consider using storage::GetIdentifierFromOrigin() as it has some extra logic
used by other systems (IDB, etc.) which have needs similar to this one.

[Peter Beverloo, 2015/03/13 20:32:32]

I'm all for re-using logic :). Done

+                            kKeySeparator,
+                            base::Int64ToString(notification_id).c_str());
+}
+
 }  // namespace
 
 NotificationDatabase::NotificationDatabase(const base::FilePath& path)
@@ -116,6 +132,78 @@ NotificationDatabase::Status NotificationDatabase::GetNextNotificationId(
   return STATUS_OK;
 }
 
+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);

[Bernhard Bauer, 2015/03/13 17:25:35]

Put the expected value first :)

[Peter Beverloo, 2015/03/13 20:32:32]

Done.

+  DCHECK_GE(notification_id, kFirstNotificationId);
+  DCHECK(origin.is_valid());
+  DCHECK(notification_database_data);
+
+  std::string key = CreateDataKey(notification_id, origin);
+  std::string serialized_data;
+
+  Status status = LevelDBStatusToStatus(
+      db_->Get(leveldb::ReadOptions(), key, &serialized_data));
+  if (status != STATUS_OK)
+    return status;
+
+  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(
+    int64_t notification_id,
+    const GURL& origin,
+    const NotificationDatabaseData& notification_database_data) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK_EQ(state_, STATE_INITIALIZED);
+  DCHECK_GE(notification_id, kFirstNotificationId);
+  DCHECK(origin.is_valid());
+
+  std::string serialized_data;
+  if (!notification_database_data.SerializeToString(&serialized_data)) {
+    DLOG(ERROR) << "Unable to serialize data for notification "
+                << notification_id << " belonging to " << origin << ".";
+    return STATUS_ERROR_FAILED;
+  }
+
+  leveldb::WriteBatch batch;
+  batch.Put(kNextNotificationIdKey, base::Int64ToString(notification_id + 1));

[cmumford, 2015/03/13 17:13:15]

I'm feeling a little less comfortable with this class not really being under
control of the next notification ID. The contract here (as I understand it) is
the caller must first call GetNextNotificationId(), and then pass that into this
function. If the caller messes this up then we could wind up lowering the next
notification ID, and then start overwriting notification entries. Unless I'm
overlooking something in the implementation I'd rather make
GetNextNotificationId private, and just have an
AddNotificationData/UpdateNotificationData set of functions.

Please let me know if I'm missing something obvious.

[Peter Beverloo, 2015/03/13 20:32:32]

Yes, this makes sense. I've changed the API.

+  batch.Put(CreateDataKey(notification_id, origin), serialized_data);
+
+  return LevelDBStatusToStatus(db_->Write(leveldb::WriteOptions(), &batch));
+}
+
+NotificationDatabase::Status NotificationDatabase::DeleteNotificationData(
+    int64_t notification_id,
+    const GURL& origin) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK_EQ(state_, STATE_INITIALIZED);
+  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
+  // 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 +222,4 @@ 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);
-
-  batch->Put(kNextNotificationIdKey, base::Int64ToString(next_notification_id));
-}
-
 }  // namespace content