Create a new tab when the user activates orphaned non-persistent notifications
The ability to respond to activations of non-persistent notification
relies on the originating document to stay alive. Once the document (or
tab) closes, the associated JavaScript event handler can no longer be
invoked.
This is by design, because certain kinds of notifications are largely
informative and should not be missed because the tab goes away. Calendar
reminders are an example of this. However, it also means that clicking
on the notification doesn't do anything.
Starting with this CL, clicking on such a notification will create a new
tab for the originating origin. The notification that was activated will
be closed as the user explicitly acknowledged it, too.
Bug: 442141
Change-Id: I44ed2f943c747aae4c9f589b1fee2e488aeaa5e0
Reviewed-on: https://chromium-review.googlesource.com/1057631
Reviewed-by: Avi Drissman <[email protected]>
Reviewed-by: Anita Woodruff <[email protected]>
Reviewed-by: Tom Sepez <[email protected]>
Commit-Queue: Peter Beverloo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#562123}diff --git a/chrome/browser/notifications/non_persistent_notification_handler.cc b/chrome/browser/notifications/non_persistent_notification_handler.cc
index fc461b16..23f571b 100644
--- a/chrome/browser/notifications/non_persistent_notification_handler.cc
+++ b/chrome/browser/notifications/non_persistent_notification_handler.cc
@@ -4,12 +4,21 @@
#include "chrome/browser/notifications/non_persistent_notification_handler.h"
+#include "base/bind.h"
#include "base/callback.h"
#include "base/strings/nullable_string16.h"
+#include "build/build_config.h"
#include "chrome/browser/notifications/desktop_notification_profile_util.h"
-#include "chrome/browser/notifications/platform_notification_service_impl.h"
+#include "chrome/browser/notifications/notification_common.h"
#include "content/public/browser/notification_event_dispatcher.h"
+#if !defined(OS_ANDROID)
+#include "chrome/browser/notifications/platform_notification_service_impl.h"
+#include "chrome/browser/ui/browser_navigator.h"
+#include "chrome/browser/ui/browser_navigator_params.h"
+#include "ui/base/page_transition_types.h"
+#endif // !defined(OS_ANDROID)
+
NonPersistentNotificationHandler::NonPersistentNotificationHandler() = default;
NonPersistentNotificationHandler::~NonPersistentNotificationHandler() = default;
@@ -44,10 +53,39 @@
DCHECK(!reply.has_value());
content::NotificationEventDispatcher::GetInstance()
- ->DispatchNonPersistentClickEvent(notification_id);
+ ->DispatchNonPersistentClickEvent(
+ notification_id,
+ base::BindOnce(
+ &NonPersistentNotificationHandler::DidDispatchClickEvent,
+ weak_ptr_factory_.GetWeakPtr(), profile, origin, notification_id,
+ std::move(completed_closure)));
+}
- // TODO(crbug.com/787459): Implement event acknowledgements once
- // non-persistent notifications have updated to use Mojo instead of IPC.
+void NonPersistentNotificationHandler::DidDispatchClickEvent(
+ Profile* profile,
+ const GURL& origin,
+ const std::string& notification_id,
+ base::OnceClosure completed_closure,
+ bool success) {
+#if !defined(OS_ANDROID)
+ // Non-persistent notifications are able to outlive the document that created
+ // them. In such cases the JavaScript event handler might not be available
+ // when the notification is interacted with. Launch a new tab for the
+ // notification's |origin| instead, and close the activated notification. Not
+ // applicable to Android as non-persistent notifications are not available.
+ if (!success) {
+ NavigateParams params(profile, origin, ui::PAGE_TRANSITION_LINK);
+
+ params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+ params.window_action = NavigateParams::SHOW_WINDOW;
+ Navigate(¶ms);
+
+ // Close the |notification_id| as the user has explicitly acknowledged it.
+ PlatformNotificationServiceImpl::GetInstance()->CloseNotification(
+ profile, notification_id);
+ }
+#endif // !defined(OS_ANDROID)
+
std::move(completed_closure).Run();
}
diff --git a/chrome/browser/notifications/non_persistent_notification_handler.h b/chrome/browser/notifications/non_persistent_notification_handler.h
index 8e7025a..ba5b3a81 100644
--- a/chrome/browser/notifications/non_persistent_notification_handler.h
+++ b/chrome/browser/notifications/non_persistent_notification_handler.h
@@ -6,6 +6,7 @@
#define CHROME_BROWSER_NOTIFICATIONS_NON_PERSISTENT_NOTIFICATION_HANDLER_H_
#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
#include "chrome/browser/notifications/notification_handler.h"
// NotificationHandler implementation for non persistent notifications.
@@ -31,6 +32,18 @@
void OpenSettings(Profile* profile, const GURL& origin) override;
private:
+ // Called when the "click" event for non-persistent notification has been
+ // dispatched. The |success| boolean indicates whether the click could be
+ // delivered to the originating document as a JavaScript event.
+ void DidDispatchClickEvent(Profile* profile,
+ const GURL& origin,
+ const std::string& notification_id,
+ base::OnceClosure completed_closure,
+ bool success);
+
+ base::WeakPtrFactory<NonPersistentNotificationHandler> weak_ptr_factory_{
+ this};
+
DISALLOW_COPY_AND_ASSIGN(NonPersistentNotificationHandler);
};
diff --git a/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc b/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
index c63145a..05b3847 100644
--- a/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
+++ b/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
@@ -24,6 +24,7 @@
#include "chrome/browser/notifications/notification_common.h"
#include "chrome/browser/notifications/notification_display_service_impl.h"
#include "chrome/browser/notifications/notification_display_service_tester.h"
+#include "chrome/browser/notifications/notification_handler.h"
#include "chrome/browser/notifications/notification_test_util.h"
#include "chrome/browser/notifications/platform_notification_service_impl.h"
#include "chrome/browser/permissions/permission_manager.h"
@@ -62,6 +63,8 @@
constexpr double kNotificationTimestamp = 621046800000.;
const char kTestFileName[] = "notifications/platform_notification_service.html";
+const char kTestNotificationOrigin[] = "https://example.com/";
+const char kTestNotificationId[] = "random#notification-id";
} // namespace
@@ -832,6 +835,47 @@
ASSERT_EQ(notification_ids[0], first_id);
}
+IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
+ OrphanedNonPersistentNotificationCreatesForegroundTab) {
+ // Verifies that activating a non-persistent notification that no longer has
+ // any event listeners attached (e.g. because the tab closed) creates a new
+ // foreground tab.
+
+ Profile* profile = browser()->profile();
+
+ NotificationDisplayServiceImpl* display_service =
+ NotificationDisplayServiceImpl::GetForProfile(profile);
+ ASSERT_TRUE(display_service);
+
+ NotificationHandler* handler = display_service->GetNotificationHandler(
+ NotificationHandler::Type::WEB_NON_PERSISTENT);
+ ASSERT_TRUE(handler);
+
+ // There should be one open tab for the current |browser()|.
+ EXPECT_EQ(browser()->tab_strip_model()->count(), 1);
+ content::WebContents* original_web_contents =
+ browser()->tab_strip_model()->GetActiveWebContents();
+
+ // Signal an activation for a notification that's never been shown.
+ {
+ base::RunLoop run_loop;
+ handler->OnClick(profile, GURL(kTestNotificationOrigin),
+ kTestNotificationId, base::nullopt /* action_index */,
+ base::nullopt /* reply */, run_loop.QuitClosure());
+ run_loop.Run();
+ }
+
+ // A second tab should've been created and have been brought to the foreground
+ // for the notification's test origin.
+ ASSERT_EQ(browser()->tab_strip_model()->count(), 2);
+ content::WebContents* active_web_contents =
+ browser()->tab_strip_model()->GetActiveWebContents();
+
+ EXPECT_NE(active_web_contents, original_web_contents);
+ EXPECT_EQ(active_web_contents->GetVisibleURL(),
+ GURL(kTestNotificationOrigin));
+}
+
// Mac OS X exclusively uses native notifications, so the decision on whether to
// display notifications whilst fullscreen is deferred to the operating system.
#if !defined(OS_MACOSX)
diff --git a/chrome/browser/notifications/platform_notification_service_unittest.cc b/chrome/browser/notifications/platform_notification_service_unittest.cc
index f8ab86d..1283eaed 100644
--- a/chrome/browser/notifications/platform_notification_service_unittest.cc
+++ b/chrome/browser/notifications/platform_notification_service_unittest.cc
@@ -40,7 +40,7 @@
#include "extensions/common/permissions/api_permission.h"
#include "extensions/common/value_builder.h"
#include "third_party/blink/public/platform/modules/permissions/permission_status.mojom.h"
-#endif
+#endif // BUILDFLAG(ENABLE_EXTENSIONS)
using content::NotificationResources;
using content::PlatformNotificationData;
