Introduce the NotificationDatabase class.

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
new file mode 100644
index 0000000000000000000000000000000000000000..cc2eacda1630964fca5957b4f335c6f03528dcb5
--- /dev/null
+++ b/content/browser/notifications/notification_database.cc
@@ -0,0 +1,88 @@
+// 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 "base/files/file_util.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"
+
+namespace content {
+namespace {
+
+// 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;

[cmumford, 2015/03/11 17:13:24]

You could call status.IsNotFound() and return STATUS_ERROR_NOT_FOUND too.

[Peter Beverloo, 2015/03/11 18:01:25]

Ah yes, good point, thanks!

I'm ramping up the error codes as I'm able to write tests for them. I'll include
some tests for NOT_FOUND when we actually start reading data.

+
+  return NotificationDatabase::STATUS_ERROR_FAILED;
+}
+
+}  // namespace
+
+NotificationDatabase::NotificationDatabase(const base::FilePath& path)
+    : path_(path),
+      state_(STATE_UNINITIALIZED) {
+  sequence_checker_.DetachFromSequence();
+}
+
+NotificationDatabase::~NotificationDatabase() {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  db_.reset();

[Bernhard Bauer, 2015/03/11 17:15:22]

You don't need to explicitly reset the database here.

[Peter Beverloo, 2015/03/11 18:01:26]

I added this to clarify that db_ needs to be reset before env_.

C++ guarantees that class members will be destroyed in reverse declaration
order, so we're safe, but it's a somewhat hidden dependency on not commonly
known language details. Being able to cause crashes by re-arranging class
members feels really fragile to me, so unless you mind I would prefer to be
explicit about it.

[Bernhard Bauer, 2015/03/11 18:20:01]

I'm all for being explicit about it, but I'd rather do that via a comment in the
header, where the members are declared. As it is, that particular subtlety
wouldn't have been obvious to me.

[Peter Beverloo, 2015/03/11 19:32:25]

Ok. Done.

+}
+
+NotificationDatabase::Status NotificationDatabase::Open(
+    bool create_if_missing) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK_EQ(state_, STATE_UNINITIALIZED);

[Bernhard Bauer, 2015/03/11 17:15:22]

Put the expected value first for nicer error messages.

[Peter Beverloo, 2015/03/11 18:01:25]

Done.

+
+  if (!create_if_missing) {
+    if (IsInMemoryDatabase() ||
+        !base::PathExists(path_) ||
+        base::IsDirectoryEmpty(path_)) {
+      return NotificationDatabase::STATUS_ERROR_NOT_FOUND;
+    }
+  }
+
+  leveldb::Options options;
+  options.create_if_missing = create_if_missing;

[cmumford, 2015/03/11 17:13:24]

I recommend paranoid_checks as they are cheap and will catch some errors.

[Peter Beverloo, 2015/03/11 18:01:26]

Done, thanks. It won't be a high-frequency database of any kind, so it makes a
lot of sense to push for additional correctness.

+  if (IsInMemoryDatabase()) {
+    env_.reset(leveldb::NewMemEnv(leveldb::Env::Default()));
+    options.env = env_.get();
+  }
+
+  leveldb::DB* db;

[Bernhard Bauer, 2015/03/11 17:15:22]

Initialize with nullptr?

[Peter Beverloo, 2015/03/11 18:01:26]

Done.

+  Status status = LevelDBStatusToStatus(
+      leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db));

[cmumford, 2015/03/11 17:13:24]

Will UMA stats arrive in a future CL?

[Peter Beverloo, 2015/03/11 18:01:25]

Yes. I've clarified this in the header and added it to my task sheet.

+  if (status != STATUS_OK)
+    return status;

[cmumford, 2015/03/11 17:13:24]

FYI: leveldb supports the concept of a database repair. You may be able to get
away with calling leveldb::RepairDB if leveldb::DB::Open returns an error if you
are OK with a portion of your notifications being lost.

[Peter Beverloo, 2015/03/11 18:01:26]

That's a really good question actually - are we OK with a portion of the
notifications being lost?

My current inclination is to say no, because some platforms will depend on it
for keeping sync on which notifications are actually being displayed as well
(Android). Since we won't be able to know which notifications we lost track of,
they'll end up being no-ops on user interaction, which is a bad UX.

[cmumford, 2015/03/11 18:24:33]

If leveldb::DB::Open() returns an status.IsCorruption() then your only fallback
is to call DeleteDB() - losing everything which will cause no-ops (I think). If
you can tolerate some data going away w/o destroying db integrity it might be a
good idea. You know best - just wanted to point that out.

[Peter Beverloo, 2015/03/11 19:32:25]

Right, thanks for the explanation!

This is logic that code one layer higher will be dealing with
(PlatformNotificationContextImpl), so I'll be sure to include it there (+
tests).

+
+  state_ = STATE_INITIALIZED;
+  db_.reset(db);
+
+  return STATUS_OK;
+}
+
+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(

[cmumford, 2015/03/11 17:13:24]

If you destroy an in-memory db then won't you still get here and use path_? Is
that what you want?

[Peter Beverloo, 2015/03/11 18:01:25]

The |path_| is a reference, so AsUTF8Unsafe() will just return an empty string.
I see that Service Workers uses the database in the same way, and reading how it
gets used in the memory environment this seems to be safe?

+      leveldb::DestroyDB(path_.AsUTF8Unsafe(), options));

[cmumford, 2015/03/11 17:13:24]

FYI: DestroyDB does leave behind non leveldb files (and the dir) if they somehow
leak into that directory.

[Peter Beverloo, 2015/03/11 18:01:25]

How does that happen? Reading the code, it looks like we'll end up calling
ChromiumEnv::DeleteDir() on path_.

[cmumford, 2015/03/11 18:24:33]

ChromiumEnv::DeleteDir calls base::DeleteFile(..., /*recursive=*/false)
so if there are any files in the directory they (and the directory) will be left
behind.

Maybe not a big deal, but you have code that calls PathExists and assumes the
notification db is present if true.

[Peter Beverloo, 2015/03/11 19:32:25]

I see. I think this is OK, since non-database files shouldn't appear there in
the first place, and production will pretty much always use the
|create_if_missing=true| configuration, where LevelDB will create a new CURRENTS
file rather than throw an invalid argument error.

+}
+
+}  // namespace content