Store the next notification id 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 756d01dc084f6bbf0c66318cf588e335f10a7a9e..d87ec97d348dd9fd4115de949120ef83fe15c997 100644
--- a/content/browser/notifications/notification_database.cc
+++ b/content/browser/notifications/notification_database.cc
@@ -4,15 +4,32 @@
 
 #include "content/browser/notifications/notification_database.h"
 
+#include <string>
+
 #include "base/files/file_util.h"
+#include "base/strings/string_number_conversions.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"
+
+// Notification LevelDB database schema (in alphabetized order)
+// =======================
+//
+// key: "NEXT_NOTIFICATION_ID"
+// value: <int64 'next_notification_id'>

[Bernhard Bauer, 2015/03/11 22:02:38]

Is "int64" correct here if the value is really stored as a string?

[Peter Beverloo, 2015/03/11 22:33:33]

Everything in LevelDB is stored as a string. My thinking is that it makes more
sense to document the underlying type.

[Bernhard Bauer, 2015/03/12 09:29:33]

Yes, but mentioning int64_t here doesn't say how it is encoded -- I would
actually expect a sequence of eight bytes that represent the value in memory.
Maybe it would make more sense to write it out in prose: "The next notification
id encoded as a decimal string. The value will fit into an int64_t."?

[Peter Beverloo, 2015/03/12 14:28:59]

That's a really good point, thanks. I settled on "Decimal string with fits into
an int64_t, as the name of the key should be clear on the next notification id
part.

+//
 
 namespace content {
 namespace {
 
+// Keys of the fields defined in the database.
+const char kNextNotificationIdKey[] = "NEXT_NOTIFICATION_ID";
+
+// 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) {
@@ -30,6 +47,7 @@ NotificationDatabase::Status LevelDBStatusToStatus(
 
 NotificationDatabase::NotificationDatabase(const base::FilePath& path)
     : path_(path),
+      next_available_notification_id_(0),
       state_(STATE_UNINITIALIZED) {
   sequence_checker_.DetachFromSequence();
 }
@@ -71,6 +89,33 @@ NotificationDatabase::Status NotificationDatabase::Open(
   return STATUS_OK;
 }
 
+NotificationDatabase::Status NotificationDatabase::GetNextNotificationId(
+    int64_t* notification_id) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK_EQ(state_, STATE_INITIALIZED);
+  DCHECK(notification_id);
+
+  std::string value;
+  Status status = LevelDBStatusToStatus(
+      db_->Get(leveldb::ReadOptions(), kNextNotificationIdKey, &value));

[Bernhard Bauer, 2015/03/11 22:02:38]

Do we actually need to read this from the DB every time, or could we only do
that if the stored value is 0?

[Peter Beverloo, 2015/03/11 22:33:33]

In production, the NotificationDatabase will be owned by the
PlatformNotificationContextImpl, which will cache and increment this value for
it in order to minimize file I/O. It'll be read immediately after it initialized
the NotificationDatabase.

[Bernhard Bauer, 2015/03/12 09:29:33]

Hm, if the owner is going to cache the value anyway, do we need this cache? All
it seems to do is save a read operation when incrementing the value, but not
when reading it, and it adds the invariant that Get needs to be called at least
once before Increment, or we are going to write a wrong value back.

[Peter Beverloo, 2015/03/12 14:28:59]

You are right. I've made GetNextNotificationId() const, renamed
IncrementNextNotificationId to WriteNextNotificationId (given an exact number)
and made that const as well.

+
+  if (status == STATUS_ERROR_NOT_FOUND) {
+    *notification_id = kFirstNotificationId;
+    return STATUS_OK;
+  }
+
+  if (status != STATUS_OK)
+    return status;
+
+  if (!base::StringToInt64(value, &next_available_notification_id_) ||
+      next_available_notification_id_ <= 0) {
+    return STATUS_ERROR_CORRUPTED;
+  }
+
+  *notification_id = next_available_notification_id_;
+  return STATUS_OK;
+}
+
 NotificationDatabase::Status NotificationDatabase::Destroy() {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
 
@@ -89,4 +134,15 @@ NotificationDatabase::Status NotificationDatabase::Destroy() {
       leveldb::DestroyDB(path_.AsUTF8Unsafe(), options));
 }
 
+void NotificationDatabase::IncrementNextNotificationId(
+    leveldb::WriteBatch* batch,
+    int64_t current_notification_id) {

[Bernhard Bauer, 2015/03/11 22:02:38]

When would this be called with a notification ID that is greater than
|next_available_notification_id_|?

[Peter Beverloo, 2015/03/11 22:33:33]

Good question - I don't have an answer. Simplified! :-)

+  DCHECK_GE(current_notification_id, next_available_notification_id_);
+  DCHECK(batch);
+
+  next_available_notification_id_ = current_notification_id + 1;
+  batch->Put(kNextNotificationIdKey,
+             base::Int64ToString(next_available_notification_id_));
+}
+
 }  // namespace content