The notification database should be able to read or delete multiple items.

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 1f98f3763ae896bb7edad0520ab43ebc0b6049da..68dbd4a2d4df24d5160a9980b99a7dc3dac315a1 100644
--- a/content/browser/notifications/notification_database.cc
+++ b/content/browser/notifications/notification_database.cc
@@ -8,9 +8,11 @@
 
 #include "base/files/file_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "content/browser/notifications/notification_database_data_conversions.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_database_data.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"
@@ -21,7 +23,7 @@
 // Notification LevelDB database schema (in alphabetized order)
 // =======================
 //
-// key: "DATA:" <origin identifier> '\x00' <notification_id>
+// key: "DATA:" <origin identifier> ':' <notification_id>
 // value: String containing the NotificationDatabaseDataProto protocol buffer
 //        in serialized form.
 //
@@ -37,11 +39,14 @@ const char kNextNotificationIdKey[] = "NEXT_NOTIFICATION_ID";
 const char kDataKeyPrefix[] = "DATA:";
 
 // Separates the components of compound keys.
-const char kKeySeparator = '\x00';
+const char kKeySeparator = ':';
 
 // The first notification id which to be handed out by the database.
 const int64_t kFirstNotificationId = 1;
 
+// The id used internally to represent an invalid Service Worker registration.
+const int64_t kInvalidServiceWorkerRegistrationId = -1;
+
 // Converts the LevelDB |status| to one of the notification database's values.
 NotificationDatabase::Status LevelDBStatusToStatus(
     const leveldb::Status& status) {
@@ -55,15 +60,55 @@ 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",
+// Creates a prefix for the data entries based on |origin|.
+std::string CreateDataPrefix(const GURL& origin) {
+  if (!origin.is_valid())
+    return kDataKeyPrefix;
+
+  return base::StringPrintf("%s%s%c",
                             kDataKeyPrefix,
                             storage::GetIdentifierFromOrigin(origin).c_str(),
-                            kKeySeparator,
+                            kKeySeparator);
+}
+
+// Creates the compound data key in which notification data is stored.
+std::string CreateDataKey(const GURL& origin, int64_t notification_id) {
+  DCHECK(origin.is_valid());
+
+  return base::StringPrintf("%s%s",

[cmumford, 2015/03/18 22:41:21]

Can simplify to:

return CreateDataPrefix(origin) + base::Int64ToString(notification_id);

[Peter Beverloo, 2015/03/19 14:35:51]

Done.

+                            CreateDataPrefix(origin).c_str(),
                             base::Int64ToString(notification_id).c_str());
 }
 
+// Removes the |prefix| from the |key| and writes the remainder to |output|.
+// Returns whether the |key| started with |prefix|.
+bool RemovePrefix(const std::string& key,

[cmumford, 2015/03/18 22:41:20]

Not sure this function really adds much. It does the StartsWithASCII check - but
that's really something you want to do to terminate the database iteration, and
finally the prefix stripping. I think that if you switch to
leveldb::Slice::starts_with() (see another comment), then you can use
leveldb::Slice::remove_prefix() and do away with this function without any added
code complexity.

[Peter Beverloo, 2015/03/19 14:35:51]

All done, that's awesome and much cleaner! This also allows me to keep using
\x00 as the prefix, reducing the chance of something breaking because part of a
compound key uses a colon.

+                  const std::string& prefix,
+                  std::string* output) {
+  DCHECK(output);
+
+  if (!StartsWithASCII(key, prefix, true))
+    return false;
+
+  *output = key.substr(prefix.size());
+  return true;
+}
+
+// Deserializes data in |serialized_data| to |notification_database_data|.
+// Will return if the deserialization was successful.
+NotificationDatabase::Status DeserializedNotificationData(
+    const std::string& serialized_data,
+    NotificationDatabaseData* notification_database_data) {
+  DCHECK(notification_database_data);
+  if (DeserializeNotificationDatabaseData(serialized_data,
+                                          notification_database_data)) {
+    return NotificationDatabase::STATUS_OK;
+  }
+
+  DLOG(ERROR) << "Unable to deserialize a notification's data.";
+  return NotificationDatabase::STATUS_ERROR_CORRUPTED;
+}
+
 }  // namespace
 
 NotificationDatabase::NotificationDatabase(const base::FilePath& path)
@@ -117,7 +162,7 @@ NotificationDatabase::Status NotificationDatabase::ReadNotificationData(
   DCHECK(origin.is_valid());
   DCHECK(notification_database_data);
 
-  std::string key = CreateDataKey(notification_id, origin);
+  std::string key = CreateDataKey(origin, notification_id);
   std::string serialized_data;
 
   Status status = LevelDBStatusToStatus(
@@ -125,14 +170,35 @@ NotificationDatabase::Status NotificationDatabase::ReadNotificationData(
   if (status != STATUS_OK)
     return status;
 
-  if (DeserializeNotificationDatabaseData(serialized_data,
-                                          notification_database_data)) {
-    return STATUS_OK;
-  }
+  return DeserializedNotificationData(serialized_data,
+                                      notification_database_data);
+}
+
+NotificationDatabase::Status
+NotificationDatabase::ReadAllNotificationData(
+    std::vector<NotificationDatabaseData>* notification_data_vector) const {
+  return ReadAllNotificationDataInternal(GURL() /* origin */,
+                                         kInvalidServiceWorkerRegistrationId,
+                                         notification_data_vector);
+}
+
+NotificationDatabase::Status
+NotificationDatabase::ReadAllNotificationDataForOrigin(
+    const GURL& origin,
+    std::vector<NotificationDatabaseData>* notification_data_vector) const {
+  return ReadAllNotificationDataInternal(origin,
+                                         kInvalidServiceWorkerRegistrationId,
+                                         notification_data_vector);
+}
 
-  DLOG(ERROR) << "Unable to deserialize data for notification "
-              << notification_id << " belonging to " << origin << ".";
-  return STATUS_ERROR_CORRUPTED;
+NotificationDatabase::Status
+NotificationDatabase::ReadAllNotificationDataForServiceWorkerRegistration(
+    const GURL& origin,
+    int64_t service_worker_registration_id,
+    std::vector<NotificationDatabaseData>* notification_data_vector) const {
+  return ReadAllNotificationDataInternal(origin,
+                                         service_worker_registration_id,
+                                         notification_data_vector);
 }
 
 NotificationDatabase::Status NotificationDatabase::WriteNotificationData(
@@ -155,7 +221,7 @@ NotificationDatabase::Status NotificationDatabase::WriteNotificationData(
   DCHECK_GE(next_notification_id_, kFirstNotificationId);
 
   leveldb::WriteBatch batch;
-  batch.Put(CreateDataKey(next_notification_id_, origin), serialized_data);
+  batch.Put(CreateDataKey(origin, next_notification_id_), serialized_data);
   batch.Put(kNextNotificationIdKey,
             base::Int64ToString(next_notification_id_ + 1));
 
@@ -176,10 +242,29 @@ NotificationDatabase::Status NotificationDatabase::DeleteNotificationData(
   DCHECK_GE(notification_id, kFirstNotificationId);
   DCHECK(origin.is_valid());
 
-  std::string key = CreateDataKey(notification_id, origin);
+  std::string key = CreateDataKey(origin, notification_id);
   return LevelDBStatusToStatus(db_->Delete(leveldb::WriteOptions(), key));
 }
 
+NotificationDatabase::Status
+NotificationDatabase::DeleteAllNotificationDataForOrigin(
+    const GURL& origin,
+    std::set<int64_t>* deleted_notification_ids) {
+  return DeleteAllNotificationDataInternal(origin,
+                                           kInvalidServiceWorkerRegistrationId,
+                                           deleted_notification_ids);
+}
+
+NotificationDatabase::Status
+NotificationDatabase::DeleteAllNotificationDataForServiceWorkerRegistration(
+    const GURL& origin,
+    int64_t service_worker_registration_id,
+    std::set<int64_t>* deleted_notification_ids) {
+  return DeleteAllNotificationDataInternal(origin,
+                                           service_worker_registration_id,
+                                           deleted_notification_ids);
+}
+
 NotificationDatabase::Status NotificationDatabase::Destroy() {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
 
@@ -219,4 +304,87 @@ NotificationDatabase::Status NotificationDatabase::ReadNextNotificationId() {
   return STATUS_OK;
 }
 
+NotificationDatabase::Status
+NotificationDatabase::ReadAllNotificationDataInternal(
+    const GURL& origin,
+    int64_t service_worker_registration_id,
+    std::vector<NotificationDatabaseData>* notification_data_vector) const {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK(notification_data_vector);
+
+  std::string prefix = CreateDataPrefix(origin);
+
+  NotificationDatabaseData notification_database_data;
+  scoped_ptr<leveldb::Iterator> iter(db_->NewIterator(leveldb::ReadOptions()));
+  for (iter->Seek(prefix); iter->Valid(); iter->Next()) {
+    Status status = LevelDBStatusToStatus(iter->status());

[cmumford, 2015/03/18 22:41:21]

I don't think you'll ever get here (status will always be OK) if iter->Valid()
is in the if block. See my comment below.

[Peter Beverloo, 2015/03/19 14:35:51]

Done.

+    if (status != STATUS_OK)
+      return status;
+
+    if (!StartsWithASCII(iter->key().ToString(), prefix, true))

[cmumford, 2015/03/18 22:41:20]

This works, but I recommend creating a prefix_slice, and then using
leveldb::Slice::starts_with for better performance.

[Peter Beverloo, 2015/03/19 14:35:51]

Done.

+      break;
+
+    status = DeserializedNotificationData(iter->value().ToString(),
+                                          &notification_database_data);
+    if (status != STATUS_OK)
+      return status;
+
+    if (service_worker_registration_id != kInvalidServiceWorkerRegistrationId &&
+        notification_database_data.service_worker_registration_id !=
+            service_worker_registration_id) {
+      continue;
+    }
+
+    notification_data_vector->push_back(notification_database_data);
+  }
+
+  return STATUS_OK;

[cmumford, 2015/03/18 22:41:21]

I think that moving LevelDBStatusToStatus(iter->status()) down here will
properly catch iter Seek/Next failures.

[Peter Beverloo, 2015/03/19 14:35:51]

Done.

+}
+
+NotificationDatabase::Status
+NotificationDatabase::DeleteAllNotificationDataInternal(
+    const GURL& origin,
+    int64_t service_worker_registration_id,
+    std::set<int64_t>* deleted_notification_ids) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK(deleted_notification_ids);
+  DCHECK(origin.is_valid());
+
+  std::string prefix = CreateDataPrefix(origin);
+  leveldb::WriteBatch batch;
+
+  NotificationDatabaseData notification_database_data;
+  scoped_ptr<leveldb::Iterator> iter(db_->NewIterator(leveldb::ReadOptions()));
+  for (iter->Seek(prefix); iter->Valid(); iter->Next()) {
+    Status status = LevelDBStatusToStatus(iter->status());

[cmumford, 2015/03/18 22:41:20]

Same comment on status check as above.

[Peter Beverloo, 2015/03/19 14:35:51]

Done.

+    if (status != STATUS_OK)
+      return status;
+
+    std::string value;
+    if (!RemovePrefix(iter->key().ToString(), prefix, &value))

[cmumford, 2015/03/18 22:41:21]

leveldb::Slice::remove_prefix()

[Peter Beverloo, 2015/03/19 14:35:51]

Done.

+      break;
+
+    int64_t notification_id = 0;
+    if (!base::StringToInt64(value, &notification_id))

[cmumford, 2015/03/18 22:41:21]

Nit: notification_id isn't used until line 383 below. You could move this closer
to where it is used.

[Peter Beverloo, 2015/03/19 14:35:51]

Done.

+      return STATUS_ERROR_CORRUPTED;
+
+    if (service_worker_registration_id != kInvalidServiceWorkerRegistrationId) {
+      status = DeserializedNotificationData(iter->value().ToString(),
+                                            &notification_database_data);
+      if (status != STATUS_OK)
+        return status;
+
+      if (notification_database_data.service_worker_registration_id !=
+              service_worker_registration_id) {
+        continue;
+      }
+    }
+
+    deleted_notification_ids->insert(notification_id);
+    batch.Delete(iter->key());
+  }
+
+  return LevelDBStatusToStatus(db_->Write(leveldb::WriteOptions(), &batch));

[cmumford, 2015/03/18 22:41:21]

Nit: Not sure if would get called much with no matches, but if so you could
check against deleted_notification_set.empty(), and not call db_->Write(...).

[Peter Beverloo, 2015/03/19 14:35:51]

Done.

+}
+
 }  // namespace content