Make //content responsible for generating notification Ids

content/browser/notifications/platform_notification_context_impl.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/platform_notification_context_impl.h"
 
 #include "base/bind_helpers.h"
 #include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "content/browser/notifications/blink_notification_service_impl.h"
 #include "content/browser/notifications/notification_database.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_database_data.h"
 #include "content/public/browser/platform_notification_service.h"
 
 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);
 }
 
 PlatformNotificationContextImpl::~PlatformNotificationContextImpl() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // If the database has been initialized, it must be deleted on the task runner
   // thread as closing it may cause file I/O.
   if (database_) {
     DCHECK(task_runner_);
@@ skipping 82 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   auto services_to_remove = std::remove_if(
       services_.begin(), services_.end(),
       [service](const std::unique_ptr<BlinkNotificationServiceImpl>& ptr) {
         return ptr.get() == service;
       });
 
   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),
       base::Bind(callback, false /* success */, NotificationDatabaseData()));
 }
 
 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);
 
   if (status == NotificationDatabase::STATUS_OK) {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(callback, true /* success */, database_data));
     return;
   }
@@ skipping 54 matching lines @@
 }
 
 void PlatformNotificationContextImpl::WriteNotificationData(
     const GURL& origin,
     const NotificationDatabaseData& database_data,
     const WriteResultCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   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(
     const GURL& origin,
     const NotificationDatabaseData& database_data,
     const WriteResultCallback& callback) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   // Eagerly delete data for replaced notifications from the database.
   if (!database_data.notification_data.tag.empty()) {
     std::set<int64_t> deleted_notification_set;
     NotificationDatabase::Status delete_status =
         database_->DeleteAllNotificationDataForOrigin(
             origin, database_data.notification_data.tag,
             &deleted_notification_set);
 
     UMA_HISTOGRAM_ENUMERATION("Notifications.Database.DeleteBeforeWriteResult",
                               delete_status,
                               NotificationDatabase::STATUS_COUNT);
 
     // Unless the database was corrupted following this change, there is no
     // reason to bail out here in event of failure because the notification
     // display logic will handle notification replacement for the user.
     if (delete_status == NotificationDatabase::STATUS_ERROR_CORRUPTED) {
       DestroyDatabase();
 
       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;
   }
 
   // Blow away the database if writing data failed due to corruption.
   if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED)
     DestroyDatabase();
 
   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),
       base::Bind(callback, false /* success */));
 }
 
 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
   // caller's goal has been achieved: the data is gone.
   if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED) {
     DestroyDatabase();
     success = true;
   }
 
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
@@ skipping 139 matching lines @@
 
   return path_.Append(kPlatformNotificationsDirectory);
 }
 
 void PlatformNotificationContextImpl::SetTaskRunnerForTesting(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
   task_runner_ = task_runner;
 }
 
 }  // namespace content