From 2e68fac2b8868e0e192bdac781578a59332aa24b Mon Sep 17 00:00:00 2001 From: miguelg Date: Fri, 18 Sep 2015 14:12:07 -0700 Subject: [PATCH] Add a Notification Settings Button to all web notifications behind the web platform experimental features flag. Upon click it opens chrome://settings/contentExceptions#notifications. TBR dewittj since Peter's LGTM covers everything but the typo fix in the message center directory. Will be more than happy to follow up if you have more suggestions Justin. BUG=524474 TBR=dewittj Review URL: https://codereview.chromium.org/1324013002 Cr-Commit-Position: refs/heads/master@{#349769} --- chrome/app/generated_resources.grd | 3 ++ .../notifications/notification_object_proxy.cc | 13 +++++- .../notifications/notification_object_proxy.h | 6 +++ .../persistent_notification_delegate.cc | 13 +++++- .../persistent_notification_delegate.h | 13 ++++-- .../platform_notification_service_browsertest.cc | 37 ++++++++++++++- .../platform_notification_service_impl.cc | 54 +++++++++++++++++++++- .../platform_notification_service_impl.h | 8 ++++ .../platform_notification_service_unittest.cc | 34 ++++++++++++-- chrome/common/chrome_switches.cc | 6 +++ chrome/common/chrome_switches.h | 3 +- .../views/message_center_controller.h | 2 +- 12 files changed, 176 insertions(+), 16 deletions(-) diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 79a3ac9fadd2..6f21ca5509e3 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -12632,6 +12632,9 @@ Some features may be unavailable. Please check that the profile exists and you Google Now for Chrome! + + Notification settings + $1Chrome Notifications diff --git a/chrome/browser/notifications/notification_object_proxy.cc b/chrome/browser/notifications/notification_object_proxy.cc index e98a82cde6d2..91063b448492 100644 --- a/chrome/browser/notifications/notification_object_proxy.cc +++ b/chrome/browser/notifications/notification_object_proxy.cc @@ -5,11 +5,15 @@ #include "chrome/browser/notifications/notification_object_proxy.h" #include "base/guid.h" +#include "base/logging.h" +#include "chrome/browser/notifications/platform_notification_service_impl.h" #include "content/public/browser/desktop_notification_delegate.h" NotificationObjectProxy::NotificationObjectProxy( + content::BrowserContext* browser_context, scoped_ptr delegate) - : delegate_(delegate.Pass()), + : browser_context_(browser_context), + delegate_(delegate.Pass()), displayed_(false), id_(base::GenerateGUID()) {} @@ -33,6 +37,13 @@ void NotificationObjectProxy::Click() { delegate_->NotificationClick(); } +void NotificationObjectProxy::ButtonClick(int button_index) { + // Notification buttons not are supported for non persistent notifications. + DCHECK_EQ(button_index, 0); + PlatformNotificationServiceImpl::GetInstance()->OpenNotificationSettings( + browser_context_); +} + std::string NotificationObjectProxy::id() const { return id_; } diff --git a/chrome/browser/notifications/notification_object_proxy.h b/chrome/browser/notifications/notification_object_proxy.h index eb25729e4e59..f8bd8e3a6136 100644 --- a/chrome/browser/notifications/notification_object_proxy.h +++ b/chrome/browser/notifications/notification_object_proxy.h @@ -11,6 +11,7 @@ #include "chrome/browser/notifications/notification_delegate.h" namespace content { +class BrowserContext; class DesktopNotificationDelegate; } @@ -23,21 +24,26 @@ class NotificationObjectProxy : public NotificationDelegate { // Creates a Proxy object with the necessary callback information. The Proxy // will take ownership of |delegate|. NotificationObjectProxy( + content::BrowserContext* browser_context, scoped_ptr delegate); // NotificationDelegate implementation. void Display() override; void Close(bool by_user) override; void Click() override; + void ButtonClick(int button_index) override; std::string id() const override; protected: ~NotificationObjectProxy() override; private: + content::BrowserContext* browser_context_; scoped_ptr delegate_; bool displayed_; std::string id_; + + DISALLOW_COPY_AND_ASSIGN(NotificationObjectProxy); }; #endif // CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_OBJECT_PROXY_H_ diff --git a/chrome/browser/notifications/persistent_notification_delegate.cc b/chrome/browser/notifications/persistent_notification_delegate.cc index 212a6d84cb2a..4acd06a47de0 100644 --- a/chrome/browser/notifications/persistent_notification_delegate.cc +++ b/chrome/browser/notifications/persistent_notification_delegate.cc @@ -12,11 +12,13 @@ PersistentNotificationDelegate::PersistentNotificationDelegate( content::BrowserContext* browser_context, int64_t persistent_notification_id, - const GURL& origin) + const GURL& origin, + int notification_settings_index) : browser_context_(browser_context), persistent_notification_id_(persistent_notification_id), origin_(origin), - id_(base::GenerateGUID()) {} + id_(base::GenerateGUID()), + notification_settings_index_(notification_settings_index) {} PersistentNotificationDelegate::~PersistentNotificationDelegate() {} @@ -38,6 +40,13 @@ void PersistentNotificationDelegate::Click() { } void PersistentNotificationDelegate::ButtonClick(int button_index) { + DCHECK_GE(button_index, 0); + if (button_index == notification_settings_index_) { + PlatformNotificationServiceImpl::GetInstance()->OpenNotificationSettings( + browser_context_); + return; + } + PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClick( browser_context_, persistent_notification_id_, diff --git a/chrome/browser/notifications/persistent_notification_delegate.h b/chrome/browser/notifications/persistent_notification_delegate.h index d7f88470aa9d..d32b0cc613d2 100644 --- a/chrome/browser/notifications/persistent_notification_delegate.h +++ b/chrome/browser/notifications/persistent_notification_delegate.h @@ -8,6 +8,7 @@ #include #include +#include "base/macros.h" #include "chrome/browser/notifications/notification_delegate.h" #include "url/gurl.h" @@ -20,10 +21,10 @@ class BrowserContext; // JavaScript events can be fired on the associated Service Worker. class PersistentNotificationDelegate : public NotificationDelegate { public: - PersistentNotificationDelegate( - content::BrowserContext* browser_context, - int64_t persistent_notification_id, - const GURL& origin); + PersistentNotificationDelegate(content::BrowserContext* browser_context, + int64_t persistent_notification_id, + const GURL& origin, + int notification_settings_index); // Persistent id of this notification in the notification database. To be // used when retrieving all information associated with it. @@ -45,8 +46,10 @@ class PersistentNotificationDelegate : public NotificationDelegate { content::BrowserContext* browser_context_; int64_t persistent_notification_id_; GURL origin_; - std::string id_; + int notification_settings_index_; + + DISALLOW_COPY_AND_ASSIGN(PersistentNotificationDelegate); }; #endif // CHROME_BROWSER_NOTIFICATIONS_PERSISTENT_NOTIFICATION_DELEGATE_H_ diff --git a/chrome/browser/notifications/platform_notification_service_browsertest.cc b/chrome/browser/notifications/platform_notification_service_browsertest.cc index c7b6c898917e..8ddd7b2b3697 100644 --- a/chrome/browser/notifications/platform_notification_service_browsertest.cc +++ b/chrome/browser/notifications/platform_notification_service_browsertest.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include +#include #include "base/command_line.h" #include "base/files/file_path.h" @@ -17,12 +18,15 @@ #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/website_settings/permission_bubble_manager.h" +#include "chrome/common/chrome_switches.h" +#include "chrome/grit/generated_resources.h" #include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/ui_test_utils.h" #include "content/public/test/browser_test_utils.h" #include "net/base/filename_util.h" #include "net/test/spawned_test_server/spawned_test_server.h" #include "testing/gmock/include/gmock/gmock.h" +#include "ui/base/l10n/l10n_util.h" // ----------------------------------------------------------------------------- @@ -39,6 +43,7 @@ class PlatformNotificationServiceBrowserTest : public InProcessBrowserTest { // InProcessBrowserTest overrides. void SetUp() override; + void SetUpCommandLine(base::CommandLine* command_line) override; void SetUpOnMainThread() override; void TearDown() override; @@ -97,6 +102,14 @@ PlatformNotificationServiceBrowserTest::PlatformNotificationServiceBrowserTest() test_page_url_(std::string("files/") + kTestFileName) { } +void PlatformNotificationServiceBrowserTest::SetUpCommandLine( + base::CommandLine* command_line) { + const testing::TestInfo* const test_info = + testing::UnitTest::GetInstance()->current_test_info(); + if (strcmp(test_info->name(), "WebNotificationSiteSettingsButton") == 0) + command_line->AppendSwitch(switches::kNotificationSettingsButton); +} + void PlatformNotificationServiceBrowserTest::SetUp() { ui_manager_.reset(new StubNotificationUIManager); https_server_.reset(new net::SpawnedTestServer( @@ -258,6 +271,28 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest, } IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest, + WebNotificationSiteSettingsButton) { + ASSERT_NO_FATAL_FAILURE(GrantNotificationPermissionForTest()); + + std::string script_result; + ASSERT_TRUE( + RunScript("DisplayPersistentAllOptionsNotification()", &script_result)); + EXPECT_EQ("ok", script_result); + + ASSERT_EQ(1u, ui_manager()->GetNotificationCount()); + + const Notification& notification = ui_manager()->GetNotificationAt(0); + const std::vector& buttons = + notification.buttons(); + EXPECT_EQ(1u, buttons.size()); + EXPECT_EQ(l10n_util::GetStringUTF16(IDS_NOTIFICATION_SETTINGS), + buttons[0].title); + + notification.delegate()->ButtonClick(0); + ASSERT_EQ(1u, ui_manager()->GetNotificationCount()); +} + +IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest, WebNotificationOptionsVibrationPattern) { ASSERT_NO_FATAL_FAILURE(GrantNotificationPermissionForTest()); @@ -282,7 +317,7 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest, std::string script_result; ASSERT_TRUE(RunScript("DisplayPersistentNotification('action_close')", - &script_result)); + &script_result)); EXPECT_EQ("ok", script_result); ASSERT_EQ(1u, ui_manager()->GetNotificationCount()); diff --git a/chrome/browser/notifications/platform_notification_service_impl.cc b/chrome/browser/notifications/platform_notification_service_impl.cc index 80322b63f1c7..5db2c74a1a00 100644 --- a/chrome/browser/notifications/platform_notification_service_impl.cc +++ b/chrome/browser/notifications/platform_notification_service_impl.cc @@ -4,6 +4,7 @@ #include "chrome/browser/notifications/platform_notification_service_impl.h" +#include "base/command_line.h" #include "base/metrics/histogram_macros.h" #include "base/prefs/pref_service.h" #include "base/strings/utf_string_conversions.h" @@ -15,9 +16,15 @@ #include "chrome/browser/notifications/persistent_notification_delegate.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_io_data.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/chrome_pages.h" +#include "chrome/browser/ui/scoped_tabbed_browser_displayer.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/pref_names.h" +#include "chrome/grit/generated_resources.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/content_settings/core/common/content_settings.h" +#include "components/content_settings/core/common/content_settings_types.h" #include "components/url_formatter/url_formatter.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/desktop_notification_delegate.h" @@ -25,7 +32,10 @@ #include "content/public/browser/platform_notification_context.h" #include "content/public/browser/storage_partition.h" #include "content/public/common/platform_notification_data.h" +#include "ui/base/l10n/l10n_util.h" +#include "ui/base/resource/resource_bundle.h" #include "ui/message_center/notifier_settings.h" +#include "ui/resources/grit/ui_resources.h" #include "url/url_constants.h" #if defined(ENABLE_EXTENSIONS) @@ -225,8 +235,10 @@ void PlatformNotificationServiceImpl::DisplayNotification( Profile* profile = Profile::FromBrowserContext(browser_context); DCHECK(profile); + DCHECK_EQ(0u, notification_data.actions.size()); - NotificationObjectProxy* proxy = new NotificationObjectProxy(delegate.Pass()); + NotificationObjectProxy* proxy = + new NotificationObjectProxy(browser_context, delegate.Pass()); Notification notification = CreateNotificationFromData( profile, origin, icon, notification_data, proxy); @@ -252,8 +264,12 @@ void PlatformNotificationServiceImpl::DisplayPersistentNotification( Profile* profile = Profile::FromBrowserContext(browser_context); DCHECK(profile); + // The notification settings button will be appended after the developer- + // supplied buttons, available in |notification_data.actions|. + int settings_button_index = notification_data.actions.size(); PersistentNotificationDelegate* delegate = new PersistentNotificationDelegate( - browser_context, persistent_notification_id, origin); + browser_context, persistent_notification_id, origin, + settings_button_index); Notification notification = CreateNotificationFromData( profile, origin, icon, notification_data, delegate); @@ -339,9 +355,26 @@ Notification PlatformNotificationServiceImpl::CreateNotificationFromData( notification.set_silent(notification_data.silent); std::vector buttons; + + // Developer supplied buttons. for (const auto& action : notification_data.actions) buttons.push_back(message_center::ButtonInfo(action.title)); +// Android always includes the settings button in all notifications, whereas for +// desktop only web (not extensions) notifications do. +#if !defined(OS_ANDROID) + // The notification Settings button always comes at the end. + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kNotificationSettingsButton)) { + message_center::ButtonInfo settings_button = message_center::ButtonInfo( + l10n_util::GetStringUTF16(IDS_NOTIFICATION_SETTINGS)); + settings_button.icon = + ui::ResourceBundle::GetSharedInstance().GetImageNamed( + IDR_NOTIFICATION_SETTINGS); + buttons.push_back(settings_button); + } +#endif // !defined(OS_ANDROID) + notification.set_buttons(buttons); // On desktop, notifications with require_interaction==true stay on-screen @@ -361,6 +394,23 @@ PlatformNotificationServiceImpl::GetNotificationUIManager() const { return g_browser_process->notification_ui_manager(); } +bool PlatformNotificationServiceImpl::OpenNotificationSettings( + BrowserContext* browser_context) { +#if !defined(OS_ANDROID) + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kNotificationSettingsButton)) { + Profile* profile = Profile::FromBrowserContext(browser_context); + DCHECK(profile); + chrome::ScopedTabbedBrowserDisplayer browser_displayer( + profile, chrome::GetActiveDesktop()); + chrome::ShowContentSettingsExceptions(browser_displayer.browser(), + CONTENT_SETTINGS_TYPE_NOTIFICATIONS); + return true; + } +#endif // !defined(OS_ANDROID) + return false; +} + void PlatformNotificationServiceImpl::SetNotificationUIManagerForTesting( NotificationUIManager* manager) { notification_ui_manager_for_tests_ = manager; diff --git a/chrome/browser/notifications/platform_notification_service_impl.h b/chrome/browser/notifications/platform_notification_service_impl.h index 06ee5638f3e6..e6f995190e53 100644 --- a/chrome/browser/notifications/platform_notification_service_impl.h +++ b/chrome/browser/notifications/platform_notification_service_impl.h @@ -21,6 +21,10 @@ class NotificationDelegate; class NotificationUIManager; class Profile; +namespace content { +class BrowserContext; +} + namespace gcm { class PushMessagingBrowserTest; } @@ -56,6 +60,10 @@ class PlatformNotificationServiceImpl // displayed to the user. Can be overridden for testing. NotificationUIManager* GetNotificationUIManager() const; + // Open the Notification settings screen when clicking the right button. + // Returns |true| if the settings screen could be successfully opened. + bool OpenNotificationSettings(content::BrowserContext* browser_context); + // content::PlatformNotificationService implementation. blink::WebNotificationPermission CheckPermissionOnUIThread( content::BrowserContext* browser_context, diff --git a/chrome/browser/notifications/platform_notification_service_unittest.cc b/chrome/browser/notifications/platform_notification_service_unittest.cc index 1a80cb432523..1429d6de61fb 100644 --- a/chrome/browser/notifications/platform_notification_service_unittest.cc +++ b/chrome/browser/notifications/platform_notification_service_unittest.cc @@ -189,9 +189,6 @@ TEST_F(PlatformNotificationServiceTest, DisplayPageNotificationMatches) { notification_data.body = base::ASCIIToUTF16("Hello, world!"); notification_data.vibration_pattern = vibration_pattern; notification_data.silent = true; - notification_data.actions.resize(2); - notification_data.actions[0].title = base::ASCIIToUTF16("Button 1"); - notification_data.actions[1].title = base::ASCIIToUTF16("Button 2"); MockDesktopNotificationDelegate* delegate = new MockDesktopNotificationDelegate(); @@ -215,6 +212,37 @@ TEST_F(PlatformNotificationServiceTest, DisplayPageNotificationMatches) { testing::ElementsAreArray(kNotificationVibrationPattern)); EXPECT_TRUE(notification.silent()); +} + +TEST_F(PlatformNotificationServiceTest, DisplayPersistentNotificationMatches) { + std::vector vibration_pattern( + kNotificationVibrationPattern, + kNotificationVibrationPattern + arraysize(kNotificationVibrationPattern)); + + content::PlatformNotificationData notification_data; + notification_data.title = base::ASCIIToUTF16("My notification's title"); + notification_data.body = base::ASCIIToUTF16("Hello, world!"); + notification_data.vibration_pattern = vibration_pattern; + notification_data.silent = true; + notification_data.actions.resize(2); + notification_data.actions[0].title = base::ASCIIToUTF16("Button 1"); + notification_data.actions[1].title = base::ASCIIToUTF16("Button 2"); + + service()->DisplayPersistentNotification( + profile(), 0u /* persistent notification */, GURL("https://chrome.com/"), + SkBitmap(), notification_data); + + ASSERT_EQ(1u, ui_manager()->GetNotificationCount()); + + const Notification& notification = ui_manager()->GetNotificationAt(0); + EXPECT_EQ("https://chrome.com/", notification.origin_url().spec()); + EXPECT_EQ("My notification's title", base::UTF16ToUTF8(notification.title())); + EXPECT_EQ("Hello, world!", base::UTF16ToUTF8(notification.message())); + + EXPECT_THAT(notification.vibration_pattern(), + testing::ElementsAreArray(kNotificationVibrationPattern)); + + EXPECT_TRUE(notification.silent()); const auto& buttons = notification.buttons(); ASSERT_EQ(2u, buttons.size()); diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc index e021defc7514..51a5257336c4 100644 --- a/chrome/common/chrome_switches.cc +++ b/chrome/common/chrome_switches.cc @@ -798,6 +798,12 @@ const char kNoStartupWindow[] = "no-startup-window"; const char kNoSupervisedUserAcknowledgmentCheck[] = "no-managed-user-acknowledgment-check"; +// Enables a button in all Web Notifications which upon click +// opens the notification settings screen. +// TODO(miguelg) remove once the button ships. +const char kNotificationSettingsButton[] = + "enable-notification-settings-button"; + // Specifies the maximum number of threads to use for running the Proxy // Autoconfig (PAC) script. const char kNumPacThreads[] = "num-pac-threads"; diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h index b477d1c34b1c..09e80747d562 100644 --- a/chrome/common/chrome_switches.h +++ b/chrome/common/chrome_switches.h @@ -96,6 +96,7 @@ extern const char kDisablePopupBlocking[]; extern const char kDisablePreconnect[]; extern const char kDisablePrintPreview[]; extern const char kDisablePromptOnRepost[]; +extern const char kDisablePushApiBackgroundMode[]; extern const char kDisableQuic[]; extern const char kDisableQuicPortSelection[]; extern const char kDisableSdchPersistence[]; @@ -148,7 +149,6 @@ extern const char kEnablePowerOverlay[]; extern const char kEnablePrintPreviewRegisterPromos[]; extern const char kEnableProfiling[]; extern const char kEnablePushApiBackgroundMode[]; -extern const char kDisablePushApiBackgroundMode[]; extern const char kEnableQuic[]; extern const char kEnableQuicPortSelection[]; extern const char kEnableAlternativeServices[]; @@ -223,6 +223,7 @@ extern const char kNoPings[]; extern const char kNoServiceAutorun[]; extern const char kNoStartupWindow[]; extern const char kNoSupervisedUserAcknowledgmentCheck[]; +extern const char kNotificationSettingsButton[]; extern const char kNumPacThreads[]; extern const char kOpenInNewWindow[]; extern const char kOriginToForceQuicOn[]; diff --git a/ui/message_center/views/message_center_controller.h b/ui/message_center/views/message_center_controller.h index df413555b704..9b6a10bcad49 100644 --- a/ui/message_center/views/message_center_controller.h +++ b/ui/message_center/views/message_center_controller.h @@ -16,7 +16,7 @@ namespace message_center { // Interface used by views to report clicks and other user actions. The views // by themselves do not know how to perform those operations, they ask -// MessageCenterController to do them. Implemented by MessageCeneterView and +// MessageCenterController to do them. Implemented by MessageCenterView and // MessagePopupCollection. class MessageCenterController { public: -- 2.11.4.GIT