Store the actual notification data in the notification database.

content/browser/notifications/notification_database.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/notifications/notification_database.h"
 
 #include <string>
 
 #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)
 // =======================
 //
 // key: "NEXT_NOTIFICATION_ID"
 // value: Decimal string which fits into an int64_t.

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

Add the new key to the schema?

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

Done.

 //
 
 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';

[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;
 
 // Converts the LevelDB |status| to one of the notification database's values.
 NotificationDatabase::Status LevelDBStatusToStatus(
     const leveldb::Status& status) {
   if (status.ok())
     return NotificationDatabase::STATUS_OK;
   else if (status.IsNotFound())
     return NotificationDatabase::STATUS_ERROR_NOT_FOUND;
   else if (status.IsCorruption())
     return NotificationDatabase::STATUS_ERROR_CORRUPTED;
 
   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)
     : path_(path),
       state_(STATE_UNINITIALIZED) {
   sequence_checker_.DetachFromSequence();
 }
 
 NotificationDatabase::~NotificationDatabase() {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
@@ skipping 53 matching lines @@
   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;
   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());
 
   leveldb::Options options;
   if (IsInMemoryDatabase()) {
     if (!env_)
       return STATUS_OK;  // The database has not been initialized.
 
     options.env = env_.get();
   }
 
   state_ = STATE_DISABLED;
   db_.reset();
 
   return LevelDBStatusToStatus(
       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