The platform notification context should observe the Service Worker Context.

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/threading/sequenced_worker_pool.h"
 #include "content/browser/notifications/notification_database.h"
+#include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_database_data.h"
 
 namespace content {
+namespace {
+
+// Used as a failure callback there is no further action to be made.
+void EmptyFailureCallback() {}

[johnme, 2015/03/20 14:56:35]

Use DoNothing from base/bind_helpers.h instead.

[Peter Beverloo, 2015/03/20 18:39:06]

Done.

+
+}  // 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");
 
 PlatformNotificationContextImpl::PlatformNotificationContextImpl(
-    const base::FilePath& path)
-    : path_(path) {
+    const base::FilePath& path,
+    const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context)
+    : path_(path),
+      service_worker_context_(service_worker_context) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PlatformNotificationContextImpl::InitializeOnIO, this));
 }
 
 PlatformNotificationContextImpl::~PlatformNotificationContextImpl() {
   // 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_);
     task_runner_->DeleteSoon(FROM_HERE, database_.release());
   }
 }
 
+void PlatformNotificationContextImpl::InitializeOnIO() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // |service_worker_context_| may be NULL in tests.
+  if (service_worker_context_)
+    service_worker_context_->AddObserver(this);
+}
+
+void PlatformNotificationContextImpl::Shutdown() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  BrowserThread::PostTask(

[johnme, 2015/03/20 14:56:35]

My understanding is that there's now a race condition allowing the context to be
deleted on either the UI or IO threads. It seems this probably doesn't do any
harm (since you ensure NotificationDatabase is destroyed on the right thread),
but please add a comment* to the destructor documenting this, and please also
add BrowserThread::DeleteOnUIThread as the traits of
ServiceWorkerContextWrapper's RefCoountedThreadSafe superclass** (since
ServiceWorkerContextWrapper's ServiceWorkerProcessManager seems to assume it'll
always be deleted on the UI thread).

OR, more simply, add BrowserThread::DeleteOnUIThread as the traits of
PlatformNotificationContext's RefCountedThreadSafe superclass.

*: and/or a DCHECK:

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
BrowserThread::CurrentlyOn(BrowserThread::UI));

**: or (though this seems worse) reset service_worker_context_ in Shutdown().

[Peter Beverloo, 2015/03/20 18:39:05]

Good point.

I have moved the RefCountedThreadSafe<> to PlatformNotificationContextImpl and
removed it from the public API, and added the BrowserThread::DeleteOnUIThread to
the definition.

Additionally, a new DCHECK has appeared in the destructor. This means that we
won't release a reference to the ServiceWorkerContextWrapper on any other thread
than the UI thread anymore, matching the StoragePartitionImpl semantics. Their
destruction semantics are their concern.

+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PlatformNotificationContextImpl::ShutdownOnIO, this));
+}
+
+void PlatformNotificationContextImpl::ShutdownOnIO() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // |service_worker_context_| may be NULL in tests.
+  if (service_worker_context_)
+    service_worker_context_->RemoveObserver(this);
+}
+
 void PlatformNotificationContextImpl::ReadNotificationData(
     int64_t 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()));
 }
@@ skipping 94 matching lines @@
   const bool success = status == NotificationDatabase::STATUS_OK;
 
   // TODO(peter): Record UMA on |status| for reading from the database.
   // TODO(peter): Do the DeleteAndStartOver dance for STATUS_ERROR_CORRUPTED.
 
   BrowserThread::PostTask(BrowserThread::IO,
                           FROM_HERE,
                           base::Bind(callback, success));
 }
 
+void PlatformNotificationContextImpl::OnRegistrationDeleted(
+    int64_t registration_id,
+    const GURL& pattern) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  LazyInitialize(
+      base::Bind(&PlatformNotificationContextImpl::
+                     DoDeleteNotificationsForServiceWorkerRegistration,
+                 this, pattern.GetOrigin(), registration_id),
+      base::Bind(&EmptyFailureCallback));
+}
+
+void PlatformNotificationContextImpl::
+    DoDeleteNotificationsForServiceWorkerRegistration(
+        const GURL& origin,
+        int64_t service_worker_registration_id) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  std::set<int64_t> deleted_notifications_set;
+  database_->DeleteAllNotificationDataForServiceWorkerRegistration(
+        origin, service_worker_registration_id, &deleted_notifications_set);
+
+  // TODO(peter): Record UMA on status for deleting from the database.

[johnme, 2015/03/20 14:56:35]

Nit: add a matching "Do the DeleteAndStartOver dance for
STATUS_ERROR_CORRUPTED." TODO?

[Peter Beverloo, 2015/03/20 18:39:05]

Done.

+  // TODO(peter): Close the notifications in |deleted_notifications_set|.
+}
+
 void PlatformNotificationContextImpl::LazyInitialize(
     const base::Closure& success_closure,
     const base::Closure& failure_closure) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (!task_runner_) {
     base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
     base::SequencedWorkerPool::SequenceToken token = pool->GetSequenceToken();
 
     task_runner_ = pool->GetSequencedTaskRunner(token);
@@ skipping 40 matching lines @@
 
   return path_.Append(kPlatformNotificationsDirectory);
 }
 
 void PlatformNotificationContextImpl::SetTaskRunnerForTesting(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
   task_runner_ = task_runner;
 }
 
 }  // namespace content