Use notification display service to collect persistent notifications.

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"
@@ skipping 42 matching lines @@
 }
 
 void PlatformNotificationContextImpl::Initialize() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   PlatformNotificationService* service =
       GetContentClient()->browser()->GetPlatformNotificationService();
   if (service) {
     std::set<std::string> displayed_notifications;
 
     bool notification_synchronization_supported =
-        service->GetDisplayedPersistentNotifications(browser_context_,
-                                                     &displayed_notifications);
+        service->GetDisplayedNotifications(browser_context_,
+                                           &displayed_notifications);
 
     // Synchronize the notifications stored in the database with the set of
     // displaying notifications in |displayed_notifications|. This is necessary
     // because flakiness may cause a platform to inform Chrome of a notification
     // that has since been closed, or because the platform does not support
     // notifications that exceed the lifetime of the browser process.
 
     // TODO(peter): Synchronizing the actual notifications will be done when the
     // persistent notification ids are stable. For M44 we need to support the
     // case where there may be no notifications after a Chrome restart.
@@ skipping 120 matching lines @@
                  std::vector<NotificationDatabaseData>()));
 }
 
 void PlatformNotificationContextImpl::
     DoReadAllNotificationDataForServiceWorkerRegistration(
         const GURL& origin,
         int64_t service_worker_registration_id,
         const ReadAllResultCallback& callback) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
+  PlatformNotificationService* service =
+      GetContentClient()->browser()->GetPlatformNotificationService();
+
+  // Ask the underlying implementation what notifications are still being
+  // displayed.
+  std::set<std::string> displayed_notifications;
+  bool notification_synchronization_supported =
+      service &&
+      service->GetDisplayedNotifications(browser_context_,
+                                         &displayed_notifications);

[Peter Beverloo, 2016/12/06 13:04:02]

We're not only calling GetDisplayedNotifications() on the UI thread anymore,
we're now calling it from any arbitrary thread - this method will be executed on
a sequenced task runner created through a blocking thread pool.

That violates a whole series of assumptions - ContentBrowserClient that's
assumed to live on the UI thread, access to the message center (through the
NotificationUIManager), access to the native notification centers. That could be
a potential source of bugs - who knows what operating systems do for concurrent
calls to the system APIs?

I'm not sure what to suggest. If at all possible, I'd like to limit the method
to the UI thread. That would imply that we get a jump like this:

PNCI::ReadAllNotificationDataForServiceWorkerRegistration() (IO thread)
--> Moves the LazyInitialize() call on line 190 to a base::Callback passed to...

PNCI::GetDisplayedNotificationsOnUiThread() (UI thread)
--> Gets the notifications (async if needed - something you'll likely need
anyway)
--> Posts the passed base::Callback to the IO thread.

PNCI::LazyInitialize() and beyond (IO thread) -- the existing execution path

You'd need to do something silly like allocating the std::set<> on the heap,
passing ownership to the base::Callback to
PNCI::DoReadAllNotificationDataForServiceWorkerRegistration() and passing it
weakly to the PNCI::GetDisplayedNotificationsOnUiThread() function, so that both
can access the same memory...

Let's chat? :)

+
   std::vector<NotificationDatabaseData> notification_datas;
 
   NotificationDatabase::Status status =
       database_->ReadAllNotificationDataForServiceWorkerRegistration(
           origin, service_worker_registration_id, &notification_datas);
 
   UMA_HISTOGRAM_ENUMERATION("Notifications.Database.ReadForServiceWorkerResult",
                             status, NotificationDatabase::STATUS_COUNT);
 
   if (status == NotificationDatabase::STATUS_OK) {
+    if (notification_synchronization_supported) {
+      // Filter out notifications that are not actually on display anymore.
+      // TODO(miguelg) synchronize the database if there are inconsistencies.
+      for (auto it = notification_datas.begin();
+           it != notification_datas.end();) {
+        if (NotificationIdGenerator::IsPersistentNotification(
+                it->notification_id) &&
+            displayed_notifications.count(it->notification_id)) {
+          ++it;
+        } else {
+          it = notification_datas.erase(it);
+        }
+      }
+    }
+
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(callback, true /* success */, notification_datas));
     return;
   }
 
   // Blow away the database if reading data failed due to corruption.
   if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED)
     DestroyDatabase();
 
@@ skipping 252 matching lines @@
 
   return path_.Append(kPlatformNotificationsDirectory);
 }
 
 void PlatformNotificationContextImpl::SetTaskRunnerForTesting(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
   task_runner_ = task_runner;
 }
 
 }  // namespace content