Make PlatformNotificationContext display all notifications.
This hands off the display call to PlatformNotificationContext which
gets rid of some code duplication as we always want to show a
notification after writing it into the NotificationDatabase.
Bug: 891339
Change-Id: If0787fcd9668cb0e419edcc16a56c91649983dcc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1511416
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#643370}diff --git a/chrome/browser/push_messaging/push_messaging_notification_manager.cc b/chrome/browser/push_messaging/push_messaging_notification_manager.cc
index d99fb36..75096c2 100644
--- a/chrome/browser/push_messaging/push_messaging_notification_manager.cc
+++ b/chrome/browser/push_messaging/push_messaging_notification_manager.cc
@@ -270,30 +270,16 @@
database_data,
base::BindOnce(
&PushMessagingNotificationManager::DidWriteNotificationData,
- weak_factory_.GetWeakPtr(), origin, database_data.notification_data,
- std::move(message_handled_closure)));
+ weak_factory_.GetWeakPtr(), std::move(message_handled_closure)));
}
void PushMessagingNotificationManager::DidWriteNotificationData(
- const GURL& origin,
- const blink::PlatformNotificationData& notification_data,
base::OnceClosure message_handled_closure,
bool success,
const std::string& notification_id) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
- if (!success) {
+ if (!success)
DLOG(ERROR) << "Writing forced notification to database should not fail";
- std::move(message_handled_closure).Run();
- return;
- }
-
- // Do not pass service worker scope. The origin will be used instead of the
- // service worker scope to determine whether a notification should be
- // attributed to a WebAPK on Android. This is OK because this code path is hit
- // rarely.
- PlatformNotificationServiceImpl::GetInstance()->DisplayPersistentNotification(
- profile_, notification_id, GURL() /* service_worker_scope */, origin,
- notification_data, blink::NotificationResources());
std::move(message_handled_closure).Run();
}
diff --git a/chrome/browser/push_messaging/push_messaging_notification_manager.h b/chrome/browser/push_messaging/push_messaging_notification_manager.h
index 626d82a..fe054ef0 100644
--- a/chrome/browser/push_messaging/push_messaging_notification_manager.h
+++ b/chrome/browser/push_messaging/push_messaging_notification_manager.h
@@ -25,11 +25,7 @@
namespace content {
struct NotificationDatabaseData;
class WebContents;
-}
-
-namespace blink {
-struct PlatformNotificationData;
-}
+} // namespace content
// Developers may be required to display a Web Notification in response to an
// incoming push message in order to clarify to the user that something has
@@ -82,12 +78,9 @@
base::OnceClosure message_handled_closure,
bool silent_push_allowed);
- void DidWriteNotificationData(
- const GURL& origin,
- const blink::PlatformNotificationData& notification_data,
- base::OnceClosure message_handled_closure,
- bool success,
- const std::string& notification_id);
+ void DidWriteNotificationData(base::OnceClosure message_handled_closure,
+ bool success,
+ const std::string& notification_id);
#if defined(OS_CHROMEOS)
bool ShouldSkipUserVisibleOnlyRequirements(const GURL& origin);
diff --git a/content/browser/notifications/blink_notification_service_impl.cc b/content/browser/notifications/blink_notification_service_impl.cc
index f251139..5f000b2 100644
--- a/content/browser/notifications/blink_notification_service_impl.cc
+++ b/content/browser/notifications/blink_notification_service_impl.cc
@@ -207,93 +207,26 @@
database_data.origin = origin_.GetURL();
database_data.service_worker_registration_id = service_worker_registration_id;
database_data.notification_data = platform_notification_data;
+ database_data.notification_resources = notification_resources;
// TODO(https://crbug.com/870258): Validate resources are not too big (either
// here or in the mojo struct traits).
- if (platform_notification_data.show_trigger_timestamp &&
- base::FeatureList::IsEnabled(features::kNotificationTriggers)) {
- // TODO(knollr): Let PlatformNotificationContext display all notifications,
- // even non scheduled ones and always set resources here.
- database_data.notification_resources = notification_resources;
- }
-
notification_context_->WriteNotificationData(
next_persistent_id, service_worker_registration_id, origin_.GetURL(),
database_data,
- base::BindOnce(
- &BlinkNotificationServiceImpl::DisplayPersistentNotificationWithId,
- weak_factory_for_ui_.GetWeakPtr(), service_worker_registration_id,
- platform_notification_data, notification_resources,
- std::move(callback)));
+ base::BindOnce(&BlinkNotificationServiceImpl::DidWriteNotificationData,
+ weak_factory_for_ui_.GetWeakPtr(), std::move(callback)));
}
-void BlinkNotificationServiceImpl::DisplayPersistentNotificationWithId(
- int64_t service_worker_registration_id,
- const blink::PlatformNotificationData& platform_notification_data,
- const blink::NotificationResources& notification_resources,
+void BlinkNotificationServiceImpl::DidWriteNotificationData(
DisplayPersistentNotificationCallback callback,
bool success,
const std::string& notification_id) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
- if (!success) {
- std::move(callback).Run(PersistentNotificationError::INTERNAL_ERROR);
- return;
- }
-
- if (platform_notification_data.show_trigger_timestamp &&
- base::FeatureList::IsEnabled(features::kNotificationTriggers)) {
- // This notification will be handled by the |notification_context_| because
- // it has to be scheduled rather than displayed immediately.
- // TODO(knollr): Let PlatformNotificationContext display all notifications,
- // even non scheduled ones to make this code path go away.
- std::move(callback).Run(PersistentNotificationError::NONE);
- return;
- }
-
- base::PostTaskWithTraits(
- FROM_HERE, {BrowserThread::IO},
- base::BindOnce(
- &ServiceWorkerContextWrapper::FindReadyRegistrationForId,
- service_worker_context_, service_worker_registration_id,
- origin_.GetURL(),
- base::BindOnce(
- &BlinkNotificationServiceImpl::
- DisplayPersistentNotificationWithServiceWorkerOnIOThread,
- weak_factory_for_io_.GetWeakPtr(), notification_id,
- platform_notification_data, notification_resources,
- std::move(callback))));
-}
-
-void BlinkNotificationServiceImpl::
- DisplayPersistentNotificationWithServiceWorkerOnIOThread(
- const std::string& notification_id,
- const blink::PlatformNotificationData& platform_notification_data,
- const blink::NotificationResources& notification_resources,
- DisplayPersistentNotificationCallback callback,
- blink::ServiceWorkerStatusCode service_worker_status,
- scoped_refptr<ServiceWorkerRegistration> registration) {
- DCHECK_CURRENTLY_ON(BrowserThread::IO);
- PersistentNotificationError error =
- PersistentNotificationError::INTERNAL_ERROR;
-
- // Display the notification if the Service Worker's origin matches the origin
- // of the notification's sender.
- if (service_worker_status == blink::ServiceWorkerStatusCode::kOk &&
- registration->scope().GetOrigin() == origin_.GetURL()) {
- base::PostTaskWithTraits(
- FROM_HERE, {BrowserThread::UI},
- base::BindOnce(
- &PlatformNotificationService::DisplayPersistentNotification,
- base::Unretained(GetNotificationService()), browser_context_,
- notification_id, registration->scope(), origin_.GetURL(),
- platform_notification_data, notification_resources));
-
- error = PersistentNotificationError::NONE;
- }
-
- base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI},
- base::BindOnce(std::move(callback), error));
+ std::move(callback).Run(success
+ ? PersistentNotificationError::NONE
+ : PersistentNotificationError::INTERNAL_ERROR);
}
void BlinkNotificationServiceImpl::ClosePersistentNotification(
diff --git a/content/browser/notifications/blink_notification_service_impl.h b/content/browser/notifications/blink_notification_service_impl.h
index f816022..574b038 100644
--- a/content/browser/notifications/blink_notification_service_impl.h
+++ b/content/browser/notifications/blink_notification_service_impl.h
@@ -73,21 +73,9 @@
bool ValidateNotificationResources(
const blink::NotificationResources& notification_resources);
- void DisplayPersistentNotificationWithId(
- int64_t service_worker_registration_id,
- const blink::PlatformNotificationData& platform_notification_data,
- const blink::NotificationResources& notification_resources,
- DisplayPersistentNotificationCallback callback,
- bool success,
- const std::string& notification_id);
-
- void DisplayPersistentNotificationWithServiceWorkerOnIOThread(
- const std::string& notification_id,
- const blink::PlatformNotificationData& platform_notification_data,
- const blink::NotificationResources& notification_resources,
- DisplayPersistentNotificationCallback callback,
- blink::ServiceWorkerStatusCode service_worker_status,
- scoped_refptr<ServiceWorkerRegistration> registration);
+ void DidWriteNotificationData(DisplayPersistentNotificationCallback callback,
+ bool success,
+ const std::string& notification_id);
void DidGetNotifications(
const std::string& filter_tag,
diff --git a/content/browser/notifications/platform_notification_context_impl.cc b/content/browser/notifications/platform_notification_context_impl.cc
index 739a98f..5fd9ea9 100644
--- a/content/browser/notifications/platform_notification_context_impl.cc
+++ b/content/browser/notifications/platform_notification_context_impl.cc
@@ -495,7 +495,7 @@
bool initialized) {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
DCHECK(database_data.notification_id.empty());
- if (!initialized) {
+ if (!initialized || !service_proxy_) {
base::PostTaskWithTraits(
FROM_HERE, {BrowserThread::UI, base::TaskPriority::USER_VISIBLE},
base::BindOnce(std::move(callback), /* success= */ false,
@@ -552,6 +552,10 @@
return;
}
+ // Only store resources for notifications that will be scheduled.
+ if (!CanTrigger(write_database_data))
+ write_database_data.notification_resources = base::nullopt;
+
NotificationDatabase::Status status =
database_->WriteNotificationData(origin, write_database_data);
@@ -559,19 +563,27 @@
NotificationDatabase::STATUS_COUNT);
if (status == NotificationDatabase::STATUS_OK) {
- if (CanTrigger(write_database_data) && service_proxy_) {
+ if (CanTrigger(write_database_data)) {
if (replaces_existing)
service_proxy_->CloseNotification(notification_id);
+
// Schedule notification to be shown.
service_proxy_->ScheduleTrigger(
write_database_data.notification_data.show_trigger_timestamp.value());
+
+ // Respond with success as this notification got scheduled successfully.
+ base::PostTaskWithTraits(
+ FROM_HERE, {BrowserThread::UI, base::TaskPriority::USER_VISIBLE},
+ base::BindOnce(std::move(callback), /* success= */ true,
+ write_database_data.notification_id));
+ return;
}
- base::PostTaskWithTraits(
- FROM_HERE, {BrowserThread::UI, base::TaskPriority::USER_VISIBLE},
- base::BindOnce(std::move(callback), /* success= */ true,
- write_database_data.notification_id));
-
+ // Display the notification immediately.
+ write_database_data.notification_resources =
+ database_data.notification_resources;
+ service_proxy_->DisplayNotification(std::move(write_database_data),
+ std::move(callback));
return;
}
diff --git a/content/browser/notifications/platform_notification_context_unittest.cc b/content/browser/notifications/platform_notification_context_unittest.cc
index 95ac6c16..f3c43b4 100644
--- a/content/browser/notifications/platform_notification_context_unittest.cc
+++ b/content/browser/notifications/platform_notification_context_unittest.cc
@@ -9,6 +9,7 @@
#include "base/files/scoped_temp_dir.h"
#include "base/run_loop.h"
#include "base/strings/utf_string_conversions.h"
+#include "base/test/bind_test_util.h"
#include "base/threading/thread_task_runner_handle.h"
#include "content/browser/notifications/platform_notification_context_impl.h"
#include "content/browser/service_worker/embedded_worker_test_helper.h"
@@ -115,6 +116,22 @@
context->SetTaskRunnerForTesting(base::ThreadTaskRunnerHandle::Get());
}
+ // Gets the currently displayed notifications from |service| synchronously.
+ std::set<std::string> GetDisplayedNotificationsSync(
+ PlatformNotificationService* service) {
+ std::set<std::string> displayed_notification_ids;
+ base::RunLoop run_loop;
+ service->GetDisplayedNotifications(
+ browser_context(),
+ base::BindLambdaForTesting(
+ [&](std::set<std::string> notification_ids, bool supports_sync) {
+ displayed_notification_ids = std::move(notification_ids);
+ run_loop.QuitClosure().Run();
+ }));
+ run_loop.Run();
+ return displayed_notification_ids;
+ }
+
// Returns the testing browsing context that can be used for this test.
BrowserContext* browser_context() { return &browser_context_; }
@@ -585,4 +602,35 @@
EXPECT_FALSE(success());
}
+TEST_F(PlatformNotificationContextTest, WriteDisplaysNotification) {
+ NotificationBrowserClient notification_browser_client;
+ SetBrowserClientForTesting(¬ification_browser_client);
+
+ scoped_refptr<PlatformNotificationContextImpl> context =
+ CreatePlatformNotificationContext();
+
+ GURL origin("https://example.com");
+ NotificationDatabaseData notification_database_data;
+
+ context->WriteNotificationData(
+ next_persistent_notification_id(), kFakeServiceWorkerRegistrationId,
+ origin, notification_database_data,
+ base::BindOnce(&PlatformNotificationContextTest::DidWriteNotificationData,
+ base::Unretained(this)));
+
+ base::RunLoop().RunUntilIdle();
+
+ // The write operation should have succeeded with a notification id.
+ ASSERT_TRUE(success());
+ EXPECT_FALSE(notification_id().empty());
+
+ // The written notification should be shown now.
+ std::set<std::string> displayed_notification_ids =
+ GetDisplayedNotificationsSync(
+ notification_browser_client.GetPlatformNotificationService());
+
+ ASSERT_EQ(1u, displayed_notification_ids.size());
+ EXPECT_EQ(notification_id(), *displayed_notification_ids.begin());
+}
+
} // namespace content
diff --git a/content/public/browser/platform_notification_context.h b/content/public/browser/platform_notification_context.h
index be8cfd6..dd3d703 100644
--- a/content/public/browser/platform_notification_context.h
+++ b/content/public/browser/platform_notification_context.h
@@ -90,10 +90,12 @@
int64_t service_worker_registration_id,
ReadAllResultCallback callback) = 0;
- // Writes the data associated with a notification to a database. When this
- // action completed, |callback| will be invoked with the success status and
- // the notification id when written successfully. The notification ID field
- // for |database_data| will be generated, and thus must be empty.
+ // Writes the data associated with a notification to a database and displays
+ // it either immediately or at the desired time if the notification has a show
+ // trigger defined. When this action is completed, |callback| will be invoked
+ // with the success status and the notification id when written successfully.
+ // The notification ID field for |database_data| will be generated, and thus
+ // must be empty.
virtual void WriteNotificationData(
int64_t persistent_notification_id,
int64_t service_worker_registration_id,
