Make //content responsible for generating notification Ids

content/browser/notifications/platform_notification_context_impl.cc

Index: content/browser/notifications/platform_notification_context_impl.cc
diff --git a/content/browser/notifications/platform_notification_context_impl.cc b/content/browser/notifications/platform_notification_context_impl.cc
index 6a777cc7949e9b693f55d1ca9dea9babf8332a27..de1d34ba3a787934c7579e66b0eb670e9d3e684e 100644
--- a/content/browser/notifications/platform_notification_context_impl.cc
+++ b/content/browser/notifications/platform_notification_context_impl.cc
@@ -21,19 +21,23 @@
 using base::DoNothing;
 
 namespace content {
+namespace {
 
 // Name of the directory in the user's profile directory where the notification
 // database files should be stored.
 const base::FilePath::CharType kPlatformNotificationsDirectory[] =
     FILE_PATH_LITERAL("Platform Notifications");
 
+}  // namespace
+
 PlatformNotificationContextImpl::PlatformNotificationContextImpl(
     const base::FilePath& path,
     BrowserContext* browser_context,
     const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context)
     : path_(path),
       browser_context_(browser_context),
-      service_worker_context_(service_worker_context) {
+      service_worker_context_(service_worker_context),
+      notification_id_generator_(browser_context) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 }
 
@@ -136,11 +140,20 @@ void PlatformNotificationContextImpl::RemoveService(
   services_.erase(services_to_remove, services_.end());
 }
 
+std::string PlatformNotificationContextImpl::GenerateNotificationId(
+    const GURL& origin,
+    const std::string& tag,
+    int64_t persistent_notification_id) {
+  return notification_id_generator_.GenerateForPersistentNotification(
+      origin, tag, persistent_notification_id);
+}
+
 void PlatformNotificationContextImpl::ReadNotificationData(
-    int64_t notification_id,
+    const std::string& notification_id,
     const GURL& origin,
     const ReadResultCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   LazyInitialize(
       base::Bind(&PlatformNotificationContextImpl::DoReadNotificationData, this,
                  notification_id, origin, callback),
@@ -148,14 +161,21 @@ void PlatformNotificationContextImpl::ReadNotificationData(
 }
 
 void PlatformNotificationContextImpl::DoReadNotificationData(
-    int64_t notification_id,
+    const std::string& notification_id,
     const GURL& origin,
     const ReadResultCallback& callback) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   NotificationDatabaseData database_data;
-  NotificationDatabase::Status status =
-      database_->ReadNotificationData(notification_id, origin, &database_data);
+  int64_t persistent_notification_id;
+
+  NotificationDatabase::Status status = database_->ReadIdAssociation(

[johnme, 2016/09/02 15:07:44]

I guess this is the hacked in part you were talking about?

[Peter Beverloo, 2016/09/05 15:11:01]

No? This is kinda fine...

+      notification_id, &persistent_notification_id);
+
+  if (status == NotificationDatabase::STATUS_OK) {
+    status = database_->ReadNotificationData(persistent_notification_id, origin,
+                                             &database_data);
+  }
 
   UMA_HISTOGRAM_ENUMERATION("Notifications.Database.ReadResult", status,
                             NotificationDatabase::STATUS_COUNT);
@@ -230,7 +250,7 @@ void PlatformNotificationContextImpl::WriteNotificationData(
   LazyInitialize(
       base::Bind(&PlatformNotificationContextImpl::DoWriteNotificationData,
                  this, origin, database_data, callback),
-      base::Bind(callback, false /* success */, 0 /* notification_id */));
+      base::Bind(callback, false /* success */, "" /* notification_id */));
 }
 
 void PlatformNotificationContextImpl::DoWriteNotificationData(
@@ -259,23 +279,35 @@ void PlatformNotificationContextImpl::DoWriteNotificationData(
 
       BrowserThread::PostTask(
           BrowserThread::IO, FROM_HERE,
-          base::Bind(callback, false /* success */, 0 /* notification_id */));
+          base::Bind(callback, false /* success */, "" /* notification_id */));
       return;
     }
   }
 
-  int64_t notification_id = 0;
-  NotificationDatabase::Status status =
-      database_->WriteNotificationData(origin, database_data, &notification_id);
+  int64_t persistent_notification_id = 0;
+  std::string notification_id;
+
+  NotificationDatabase::Status status = database_->WriteNotificationData(
+      origin, database_data, &persistent_notification_id);
+  if (status == NotificationDatabase::STATUS_OK) {
+    DCHECK_GT(persistent_notification_id, 0);
+    notification_id =
+        notification_id_generator_.GenerateForPersistentNotification(
+            origin, database_data.notification_data.tag,
+            persistent_notification_id);
+
+    status = database_->StoreIdAssociation(notification_id,
+                                           persistent_notification_id);
+  }
 
   UMA_HISTOGRAM_ENUMERATION("Notifications.Database.WriteResult", status,
                             NotificationDatabase::STATUS_COUNT);
 
   if (status == NotificationDatabase::STATUS_OK) {
-    DCHECK_GT(notification_id, 0);
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(callback, true /* success */, notification_id));
+
     return;
   }
 
@@ -285,14 +317,15 @@ void PlatformNotificationContextImpl::DoWriteNotificationData(
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::Bind(callback, false /* success */, 0 /* notification_id */));
+      base::Bind(callback, false /* success */, "" /* notification_id */));
 }
 
 void PlatformNotificationContextImpl::DeleteNotificationData(
-    int64_t notification_id,
+    const std::string& notification_id,
     const GURL& origin,
     const DeleteResultCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   LazyInitialize(
       base::Bind(&PlatformNotificationContextImpl::DoDeleteNotificationData,
                  this, notification_id, origin, callback),
@@ -300,18 +333,26 @@ void PlatformNotificationContextImpl::DeleteNotificationData(
 }
 
 void PlatformNotificationContextImpl::DoDeleteNotificationData(
-    int64_t notification_id,
+    const std::string& notification_id,
     const GURL& origin,
     const DeleteResultCallback& callback) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
-  NotificationDatabase::Status status =
-      database_->DeleteNotificationData(notification_id, origin);
+  int64_t persistent_notification_id;
+
+  NotificationDatabase::Status status = database_->ReadIdAssociation(
+      notification_id, &persistent_notification_id);
+
+  if (status == NotificationDatabase::STATUS_OK) {
+    status =
+        database_->DeleteNotificationData(persistent_notification_id, origin);
+  }
 
   UMA_HISTOGRAM_ENUMERATION("Notifications.Database.DeleteResult", status,
                             NotificationDatabase::STATUS_COUNT);
 
-  bool success = status == NotificationDatabase::STATUS_OK;
+  bool success = status == NotificationDatabase::STATUS_OK ||
+                 status == NotificationDatabase::STATUS_ERROR_NOT_FOUND;

[johnme, 2016/09/02 15:07:44]

Seems reasonable. Does changing this have any side-effects?

[Peter Beverloo, 2016/09/05 15:11:01]

No. LevelDB already returns STATUS_OK instead of ERROR_NOT_FOUND when deleting a
key that doesn't exist.

 
   // Blow away the database if deleting data failed due to corruption. Following
   // the contract of the delete methods, consider this to be a success as the