Chrome OS: remove non-native notifications codepaths.

Only support native notifications, i.e. those passed over mojo.

Some tests are permanently removed on Chrome OS. Some tests are
migrated to message_center_unittests.

Bug: 578868
Change-Id: Ibd4edcd1968c94af2d24b15ed3c1329ea7bf49b1
Reviewed-on: https://chromium-review.googlesource.com/1083937Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564928}

chrome/browser/BUILD.gn

@@ -2609,11 +2609,8 @@ jumbo_split_static_library("browser") {
       "metrics/tab_stats_tracker_win.cc",
       "metrics/tab_usage_recorder.cc",
       "metrics/tab_usage_recorder.h",
-      "notifications/message_center_notification_manager.cc",
-      "notifications/message_center_notification_manager.h",
       "notifications/notification_system_observer.cc",
       "notifications/notification_system_observer.h",
-      "notifications/notification_ui_manager_desktop.cc",
       "notifications/profile_notification.cc",
       "notifications/profile_notification.h",
       "obsolete_system/obsolete_system.h",
@@ -3251,6 +3248,9 @@ jumbo_split_static_library("browser") {
       "net/disk_cache_dir_policy_handler.h",
       "notifications/fullscreen_notification_blocker.cc",
       "notifications/fullscreen_notification_blocker.h",
+      "notifications/message_center_notification_manager.cc",
+      "notifications/message_center_notification_manager.h",
+      "notifications/notification_ui_manager_desktop.cc",
       "notifications/screen_lock_notification_blocker.cc",
       "notifications/screen_lock_notification_blocker.h",
       "platform_util.cc",

chrome/browser/about_flags.cc

@@ -1833,7 +1833,7 @@ const FeatureEntry kFeatureEntries[] = {
      flag_descriptions::kTranslateRankerEnforcementName,
      flag_descriptions::kTranslateRankerEnforcementDescription, kOsAll,
      FEATURE_VALUE_TYPE(translate::kTranslateRankerEnforcement)},
-#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS) && !defined(OS_CHROMEOS)
     {"enable-native-notifications",
      flag_descriptions::kNotificationsNativeFlagName,
      flag_descriptions::kNotificationsNativeFlagDescription,

chrome/browser/browser_process_impl.cc

@@ -139,7 +139,6 @@
 #include "ui/base/idle/idle.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/ui_base_features.h"
-#include "ui/message_center/message_center.h"
 
 #if defined(OS_WIN)
 #include "base/win/windows_version.h"
@@ -149,6 +148,8 @@
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/ui/ash/ash_util.h"
+#else
+#include "ui/message_center/message_center.h"
 #endif
 
 #if !defined(OS_ANDROID)
@@ -264,16 +265,9 @@ void BrowserProcessImpl::Init() {
   extensions::ExtensionsBrowserClient::Set(extensions_browser_client_.get());
 #endif
 
-  bool initialize_message_center = true;
-#if defined(OS_CHROMEOS)
-  // On Chrome OS, the message center is initialized and shut down by Ash and
-  // should not be directly accessible to Chrome. However, ARC++ still relies
-  // on the existence of a MessageCenter object, so in Mash, initialize one
-  // here.
-  initialize_message_center = ash_util::IsRunningInMash();
+#if !defined(OS_CHROMEOS)
+  message_center::MessageCenter::Initialize();
 #endif
-  if (initialize_message_center)
-    message_center::MessageCenter::Initialize();
 
   update_client::UpdateQueryParams::SetDelegate(
       ChromeUpdateQueryParamsDelegate::GetInstance());
@@ -403,8 +397,10 @@ void BrowserProcessImpl::StartTearDown() {
   storage_monitor::StorageMonitor::Destroy();
 #endif
 
+#if !defined(OS_CHROMEOS)
   if (message_center::MessageCenter::Get())
     message_center::MessageCenter::Shutdown();
+#endif
 
   // The policy providers managed by |browser_policy_connector_| need to shut
   // down while the IO and FILE threads are still alive. The monitoring
@@ -686,19 +682,13 @@ NotificationUIManager* BrowserProcessImpl::notification_ui_manager() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 // TODO(miguelg) return NULL for MAC as well once native notifications
 // are enabled by default.
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_CHROMEOS)
   return nullptr;
-#else  // !defined(OS_ANDROID)
-#if defined(OS_CHROMEOS)
-  if (base::FeatureList::IsEnabled(features::kNativeNotifications) ||
-      base::FeatureList::IsEnabled(features::kMash)) {
-    return nullptr;
-  }
-#endif  // defined(OS_CHROMEOS)
+#else
   if (!created_notification_ui_manager_)
     CreateNotificationUIManager();
   return notification_ui_manager_.get();
-#endif  // !defined(OS_ANDROID)
+#endif
 }
 
 NotificationPlatformBridge* BrowserProcessImpl::notification_platform_bridge() {

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

@@ -98,7 +98,6 @@
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/net/chrome_network_delegate.h"
 #include "chrome/browser/net/system_network_context_manager.h"
-#include "chrome/browser/notifications/notification_platform_bridge.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/task_manager/task_manager_interface.h"
@@ -676,11 +675,6 @@ void ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() {
   chromeos::ResourceReporter::GetInstance()->StartMonitoring(
       task_manager::TaskManagerInterface::GetTaskManager());
 
-  if (!base::FeatureList::IsEnabled(features::kNativeNotifications) &&
-      !base::FeatureList::IsEnabled(features::kMash)) {
-    notification_client_.reset(NotificationPlatformBridge::Create());
-  }
-
   ChromeBrowserMainPartsLinux::PreMainMessageLoopRun();
 }
 
@@ -1116,9 +1110,6 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() {
       ->browser_policy_connector_chromeos()
       ->PreShutdown();
 
-  // Close the notification client before destroying the profile manager.
-  notification_client_.reset();
-
   // NOTE: Closes ash and destroys ash::Shell.
   ChromeBrowserMainPartsLinux::PostMainMessageLoopRun();
 

chrome/browser/chromeos/chrome_browser_main_chromeos.h

@@ -14,7 +14,6 @@
 #include "chrome/browser/memory/memory_kills_monitor.h"
 #include "chromeos/system/version_loader.h"
 
-class NotificationPlatformBridge;
 class SpokenFeedbackEventRewriterDelegate;
 
 namespace lock_screen_apps {
@@ -121,11 +120,6 @@ class ChromeBrowserMainPartsChromeos : public ChromeBrowserMainPartsLinux {
   std::unique_ptr<lock_screen_apps::StateController>
       lock_screen_apps_state_controller_;
 
-  // TODO(estade): Remove this when Chrome OS uses native notifications by
-  // default (as it will be instantiated elsewhere). For now it's necessary to
-  // send notifier settings information to Ash.
-  std::unique_ptr<NotificationPlatformBridge> notification_client_;
-
   std::unique_ptr<power::ml::AdaptiveScreenBrightnessManager>
       adaptive_screen_brightness_manager_;
 

chrome/browser/chromeos/login/easy_unlock/easy_unlock_notification_controller_chromeos_unittest.cc

@@ -48,8 +48,6 @@ class EasyUnlockNotificationControllerTest : public BrowserWithTestWindowTest {
             profile());
   }
 
-  // const content::TestBrowserThreadBundle thread_bundle_;
-  // TestMessageCenter message_center_;
   std::unique_ptr<testing::StrictMock<TestableNotificationController>>
       notification_controller_;
   std::unique_ptr<NotificationDisplayServiceTester> display_service_;

chrome/browser/extensions/api/autotest_private/DEPS

-include_rules = [
-  "+ui/message_center",
-]

chrome/browser/extensions/api/autotest_private/autotest_private_api.cc

@@ -46,7 +46,6 @@
 #include "net/base/filename_util.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "ui/base/ui_base_features.h"
-#include "ui/message_center/message_center.h"
 #include "ui/message_center/public/cpp/notification.h"
 #endif
 
@@ -419,27 +418,17 @@ AutotestPrivateGetVisibleNotificationsFunction::
 ExtensionFunction::ResponseAction
 AutotestPrivateGetVisibleNotificationsFunction::Run() {
   DVLOG(1) << "AutotestPrivateGetVisibleNotificationsFunction";
-  std::unique_ptr<base::ListValue> values(new base::ListValue);
 #if defined(OS_CHROMEOS)
-  if (base::FeatureList::IsEnabled(features::kNativeNotifications) ||
-      base::FeatureList::IsEnabled(features::kMash)) {
-    auto* connection = content::ServiceManagerConnection::GetForProcess();
-    connection->GetConnector()->BindInterface(ash::mojom::kServiceName,
-                                              &controller_);
-    controller_->GetActiveNotifications(base::BindOnce(
-        &AutotestPrivateGetVisibleNotificationsFunction::OnGotNotifications,
-        this));
-    return RespondLater();
-  }
-
-  CHECK(message_center::MessageCenter::Get());
-  for (auto* notification :
-       message_center::MessageCenter::Get()->GetVisibleNotifications()) {
-    values->Append(MakeDictionaryFromNotification(*notification));
-  }
-
+  auto* connection = content::ServiceManagerConnection::GetForProcess();
+  connection->GetConnector()->BindInterface(ash::mojom::kServiceName,
+                                            &controller_);
+  controller_->GetActiveNotifications(base::BindOnce(
+      &AutotestPrivateGetVisibleNotificationsFunction::OnGotNotifications,
+      this));
+  return RespondLater();
+#else
+  return RespondNow(OneArgument(std::make_unique<base::ListValue>()));
 #endif
-  return RespondNow(OneArgument(std::move(values)));
 }
 
 #if defined(OS_CHROMEOS)

chrome/browser/notifications/chrome_ash_message_center_client_browsertest.cc

@@ -5,31 +5,15 @@
 #include "chrome/browser/notifications/chrome_ash_message_center_client.h"
 
 #include "base/strings/utf_string_conversions.h"
-#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/notifications/notification_display_service.h"
 #include "chrome/browser/notifications/notification_test_util.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "ui/message_center/public/cpp/notification_delegate.h"
 
 namespace {
 
-class ChromeAshMessageCenterClientBrowserTest : public InProcessBrowserTest {
- public:
-  ChromeAshMessageCenterClientBrowserTest() = default;
-  ~ChromeAshMessageCenterClientBrowserTest() override = default;
-
-  // InProcessBrowserTest overrides.
-  void SetUpInProcessBrowserTestFixture() override {
-    scoped_feature_list_.InitWithFeatures({features::kNativeNotifications}, {});
-  }
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-
-  DISALLOW_COPY_AND_ASSIGN(ChromeAshMessageCenterClientBrowserTest);
-};
+using ChromeAshMessageCenterClientBrowserTest = InProcessBrowserTest;
 
 class TestNotificationDelegate : public message_center::NotificationDelegate {
  public:

chrome/browser/notifications/message_center_notification_manager.h

@@ -31,13 +31,12 @@ class NotificationBlocker;
 FORWARD_DECLARE_TEST(NotificationTrayTest, ManuallyCloseMessageCenter);
 }
 
-#if !defined(OS_CHROMEOS)
 // Implementations are platform specific.
 message_center::UiDelegate* CreateUiDelegate();
-#endif
 
 // This class extends NotificationUIManagerImpl and delegates actual display
-// of notifications to MessageCenter, doing necessary conversions.
+// of notifications to MessageCenter, doing necessary conversions. This is only
+// used on platforms that support non-native notifications.
 class MessageCenterNotificationManager
     : public NotificationUIManager,
       public message_center::MessageCenterObserver {

chrome/browser/notifications/message_center_notifications_browsertest.cc

@@ -31,37 +31,6 @@
 
 using message_center::Notification;
 
-class TestAddObserver : public message_center::MessageCenterObserver {
- public:
-  explicit TestAddObserver(message_center::MessageCenter* message_center)
-      : message_center_(message_center) {
-    message_center_->AddObserver(this);
-  }
-
-  ~TestAddObserver() override { message_center_->RemoveObserver(this); }
-
-  void OnNotificationAdded(const std::string& id) override {
-    std::string log = logs_[id];
-    if (!log.empty())
-      log += "_";
-    logs_[id] = log + "add-" + id;
-  }
-
-  void OnNotificationUpdated(const std::string& id) override {
-    std::string log = logs_[id];
-    if (!log.empty())
-      log += "_";
-    logs_[id] = log + "update-" + id;
-  }
-
-  const std::string log(const std::string& id) { return logs_[id]; }
-  void reset_logs() { logs_.clear(); }
-
- private:
-  std::map<std::string, std::string> logs_;
-  message_center::MessageCenter* message_center_;
-};
-
 class MessageCenterNotificationsTest : public InProcessBrowserTest {
  public:
   MessageCenterNotificationsTest() {
@@ -199,125 +168,6 @@ IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest,
   delegate2->Release();
 }
 
-// Notification center is only used on ChromeOS.
-#if defined(OS_CHROMEOS)
-
-IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest, QueueWhenCenterVisible) {
-  TestAddObserver observer(message_center());
-
-  TestDelegate* delegate;
-  TestDelegate* delegate2;
-
-  manager()->Add(CreateTestNotification("n", &delegate), profile());
-  const std::string id_n =
-      manager()->GetMessageCenterNotificationIdForTest("n", profile());
-  message_center()->SetVisibility(message_center::VISIBILITY_MESSAGE_CENTER);
-  manager()->Add(CreateTestNotification("n2", &delegate2), profile());
-  const std::string id_n2 =
-      manager()->GetMessageCenterNotificationIdForTest("n2", profile());
-
-  // 'update-n' should happen since SetVisibility updates is_read status of n.
-  // TODO(mukai): fix event handling to happen update-n just once.
-  EXPECT_EQ(base::StringPrintf("add-%s_update-%s_update-%s",
-                               id_n.c_str(),
-                               id_n.c_str(),
-                               id_n.c_str()),
-            observer.log(id_n));
-
-  message_center()->SetVisibility(message_center::VISIBILITY_TRANSIENT);
-
-  EXPECT_EQ(base::StringPrintf("add-%s", id_n2.c_str()), observer.log(id_n2));
-
-  delegate->Release();
-  delegate2->Release();
-}
-
-IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest,
-                       UpdateProgressNotificationWhenCenterVisible) {
-  TestAddObserver observer(message_center());
-
-  TestDelegate* delegate;
-
-  // Add a progress notification and update it while the message center
-  // is visible.
-  Notification notification = CreateTestNotification("n", &delegate);
-  notification.set_type(message_center::NOTIFICATION_TYPE_PROGRESS);
-  manager()->Add(notification, profile());
-  const std::string notification_id =
-      manager()->GetMessageCenterNotificationIdForTest("n", profile());
-  message_center()->ClickOnNotification(notification_id);
-  message_center()->SetVisibility(message_center::VISIBILITY_MESSAGE_CENTER);
-  observer.reset_logs();
-  notification.set_progress(50);
-  manager()->Update(notification, profile());
-
-  // Expect that the progress notification update is performed.
-  EXPECT_EQ(base::StringPrintf("update-%s", notification_id.c_str()),
-            observer.log(notification_id));
-
-  delegate->Release();
-}
-
-IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest,
-                       UpdateNonProgressNotificationWhenCenterVisible) {
-  TestAddObserver observer(message_center());
-
-  TestDelegate* delegate;
-
-  // Add a non-progress notification and update it while the message center
-  // is visible.
-  Notification notification = CreateTestNotification("n", &delegate);
-  manager()->Add(notification, profile());
-  const std::string notification_id =
-      manager()->GetMessageCenterNotificationIdForTest("n", profile());
-  message_center()->ClickOnNotification(notification_id);
-  message_center()->SetVisibility(message_center::VISIBILITY_MESSAGE_CENTER);
-  observer.reset_logs();
-  notification.set_title(base::ASCIIToUTF16("title2"));
-  manager()->Update(notification, profile());
-
-  // Expect that the notification update is done.
-  EXPECT_NE("", observer.log(notification_id));
-
-  message_center()->SetVisibility(message_center::VISIBILITY_TRANSIENT);
-  EXPECT_EQ(base::StringPrintf("update-%s", notification_id.c_str()),
-            observer.log(notification_id));
-
-  delegate->Release();
-}
-
-IN_PROC_BROWSER_TEST_F(
-    MessageCenterNotificationsTest,
-    UpdateNonProgressToProgressNotificationWhenCenterVisible) {
-  TestAddObserver observer(message_center());
-
-  TestDelegate* delegate;
-
-  // Add a non-progress notification and change the type to progress while the
-  // message center is visible.
-  Notification notification = CreateTestNotification("n", &delegate);
-  manager()->Add(notification, profile());
-  const std::string notification_id =
-      manager()->GetMessageCenterNotificationIdForTest("n", profile());
-  message_center()->ClickOnNotification(notification_id);
-  message_center()->SetVisibility(message_center::VISIBILITY_MESSAGE_CENTER);
-  observer.reset_logs();
-  notification.set_type(message_center::NOTIFICATION_TYPE_PROGRESS);
-  manager()->Update(notification, profile());
-
-  // Expect that the notification update is done.
-  EXPECT_NE("", observer.log(notification_id));
-
-  message_center()->SetVisibility(message_center::VISIBILITY_TRANSIENT);
-  EXPECT_EQ(base::StringPrintf("update-%s", notification_id.c_str()),
-            observer.log(notification_id));
-
-  delegate->Release();
-}
-
-#else  // !defined(OS_CHROMEOS)
-
-// ScopedKeepAlives are not used on Chrome OS notifications.
 IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest, VerifyKeepAlives) {
   EXPECT_FALSE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
       KeepAliveOrigin::NOTIFICATION));
@@ -347,5 +197,3 @@ IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest, VerifyKeepAlives) {
   delegate->Release();
   delegate2->Release();
 }
-
-#endif  // !defined(OS_CHROMEOS)

chrome/browser/notifications/message_center_notifications_unittest.cc

@@ -33,10 +33,7 @@ class MessageCenterNotificationManagerTest : public BrowserWithTestWindowTest {
 
  protected:
   void SetUp() override {
-#if !defined(OS_CHROMEOS)
-    // Ash shell initializes the message center.
     MessageCenter::Initialize();
-#endif
 
     BrowserWithTestWindowTest::SetUp();
     message_center_ = MessageCenter::Get();
@@ -46,10 +43,7 @@ class MessageCenterNotificationManagerTest : public BrowserWithTestWindowTest {
 
   void TearDown() override {
     BrowserWithTestWindowTest::TearDown();
-#if !defined(OS_CHROMEOS)
-    // Shutdown the message center if we initialized it manually.
     MessageCenter::Shutdown();
-#endif
   }
 
   MessageCenterNotificationManager* notification_manager() {

chrome/browser/notifications/notification_display_service_impl.cc

@@ -71,10 +71,7 @@ NotificationPlatformBridge* GetNativeNotificationPlatformBridge() {
   if (NotificationPlatformBridgeWin::NativeNotificationEnabled())
     return g_browser_process->notification_platform_bridge();
 #elif defined(OS_CHROMEOS)
-  if (base::FeatureList::IsEnabled(features::kNativeNotifications) ||
-      base::FeatureList::IsEnabled(features::kMash)) {
-    return g_browser_process->notification_platform_bridge();
-  }
+  return g_browser_process->notification_platform_bridge();
 #else
   if (base::FeatureList::IsEnabled(features::kNativeNotifications) &&
       g_browser_process->notification_platform_bridge()) {
@@ -287,7 +284,7 @@ void NotificationDisplayServiceImpl::ProfileLoadedCallback(
 void NotificationDisplayServiceImpl::OnNotificationPlatformBridgeReady(
     bool success) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS) && !defined(OS_CHROMEOS)
   if (base::FeatureList::IsEnabled(features::kNativeNotifications)) {
     UMA_HISTOGRAM_BOOLEAN("Notifications.UsingNativeNotificationCenter",
                           success);

chrome/browser/notifications/notification_platform_bridge_chromeos.h

@@ -46,8 +46,6 @@ class NotificationPlatformBridgeDelegate {
 };
 
 // A platform bridge that uses Ash's message center to display notifications.
-// Currently under development and controlled by feature:
-//   --enable-features=NativeNotifications
 class NotificationPlatformBridgeChromeOs
     : public NotificationPlatformBridge,
       public NotificationPlatformBridgeDelegate {

chrome/browser/notifications/notification_ui_manager.h

@@ -21,16 +21,18 @@ namespace message_center {
 class Notification;
 }
 
-// This interface is used to manage the UI surfaces for desktop notifications.
-// There is just one instance for all profiles. This represents the middle layer
-// of notification and it's aware of profile. It identifies a notification by
-// the id string and a profile, hence two notifications from two different
-// profiles, even though they may have identical ids, will not be considered the
-// same notification. This interface will generate a new id behind the scene
-// based on the id string and the profile's characteristics for each
-// notification and use this new id to call lower layer MessageCenter interface
-// which is profile agnostic. Therefore the ids passed into this interface are
-// not the same as those passed into the MessageCenter interface.
+// This interface is used to manage the UI surfaces for non-native desktop
+// notifications (hence, this is not used by Chrome OS and is not used for
+// Windows Action Center notifications). There is just one instance for all
+// profiles. This represents the middle layer of notification and it's aware of
+// profile. It identifies a notification by the id string and a profile, hence
+// two notifications from two different profiles, even though they may have
+// identical ids, will not be considered the same notification. This interface
+// will generate a new id behind the scene based on the id string and the
+// profile's characteristics for each notification and use this new id to call
+// lower layer MessageCenter interface which is profile agnostic. Therefore the
+// ids passed into this interface are not the same as those passed into the
+// MessageCenter interface.
 class NotificationUIManager {
  public:
   // Convert a profile pointer into an opaque profile id, which can be safely

chrome/common/chrome_features.cc

@@ -398,8 +398,9 @@ const base::Feature kMultidevice{"Multidevice",
 #endif
 
 // Enables the use of native notification centers instead of using the Message
-// Center for displaying the toasts. Note that OS_LINUX includes Chrome OS.
-#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+// Center for displaying the toasts. The feature is hardcoded to enabled for
+// Chrome OS.
+#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS) && !defined(OS_CHROMEOS)
 #if defined(OS_MACOSX) || defined(OS_ANDROID) || defined(OS_LINUX)
 const base::Feature kNativeNotifications{"NativeNotifications",
                                          base::FEATURE_ENABLED_BY_DEFAULT};

chrome/test/BUILD.gn

@@ -3250,6 +3250,7 @@ test("unit_tests") {
 
   if (is_chromeos) {
     sources -= [
+      "../browser/notifications/message_center_notifications_unittest.cc",
       "../browser/signin/chrome_signin_status_metrics_provider_delegate_unittest.cc",
       "../browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc",
 
@@ -4646,7 +4647,6 @@ if (!is_android) {
       "../browser/extensions/updater/extension_cache_fake.h",
       "../browser/extensions/window_open_interactive_apitest.cc",
       "../browser/mouse_events_interactive_uitest.cc",
-      "../browser/notifications/message_center_notifications_browsertest.cc",
       "../browser/notifications/platform_notification_service_interactive_uitest.cc",
       "../browser/password_manager/password_generation_interactive_uitest.cc",
       "../browser/password_manager/password_manager_interactive_test_base.cc",
@@ -4966,9 +4966,9 @@ if (!is_android) {
         }
       }
     } else {  # ! is_chromeos
-      # Non-ChromeOS notifications tests (ChromeOS does not use cross-platform
-      # panels).
+      # Non-ChromeOS notifications tests.
       sources += [
+        "../browser/notifications/message_center_notifications_browsertest.cc",
         "../browser/notifications/notification_interactive_uitest.cc",
         "../browser/notifications/notification_interactive_uitest_mac.mm",
         "../browser/notifications/notification_interactive_uitest_support.cc",

chrome/test/base/testing_browser_process.cc

@@ -289,7 +289,7 @@ TestingBrowserProcess::extension_event_router_forwarder() {
 }
 
 NotificationUIManager* TestingBrowserProcess::notification_ui_manager() {
-#if !defined(OS_ANDROID)
+#if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
   if (!notification_ui_manager_.get())
     notification_ui_manager_.reset(NotificationUIManager::Create());
   return notification_ui_manager_.get();

ui/message_center/message_center_impl_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/message_loop/message_loop_current.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -72,6 +73,62 @@ class RemoveObserver : public MessageCenterObserver {
   DISALLOW_COPY_AND_ASSIGN(RemoveObserver);
 };
 
+class TestAddObserver : public MessageCenterObserver {
+ public:
+  explicit TestAddObserver(MessageCenter* message_center)
+      : message_center_(message_center) {
+    message_center_->AddObserver(this);
+  }
+
+  ~TestAddObserver() override { message_center_->RemoveObserver(this); }
+
+  void OnNotificationAdded(const std::string& id) override {
+    std::string log = logs_[id];
+    if (!log.empty())
+      log += "_";
+    logs_[id] = log + "add-" + id;
+  }
+
+  void OnNotificationUpdated(const std::string& id) override {
+    std::string log = logs_[id];
+    if (!log.empty())
+      log += "_";
+    logs_[id] = log + "update-" + id;
+  }
+
+  const std::string log(const std::string& id) { return logs_[id]; }
+  void reset_logs() { logs_.clear(); }
+
+ private:
+  std::map<std::string, std::string> logs_;
+  MessageCenter* message_center_;
+};
+
+class TestDelegate : public NotificationDelegate {
+ public:
+  TestDelegate() = default;
+  void Close(bool by_user) override {
+    log_ += "Close_";
+    log_ += (by_user ? "by_user_" : "programmatically_");
+  }
+  void Click(const base::Optional<int>& button_index,
+             const base::Optional<base::string16>& reply) override {
+    if (button_index) {
+      log_ += "ButtonClick_";
+      log_ += base::IntToString(*button_index) + "_";
+    } else {
+      log_ += "Click_";
+    }
+  }
+  const std::string& log() { return log_; }
+
+ private:
+  ~TestDelegate() override {}
+  std::string log_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestDelegate);
+};
+
 // The default app id used to create simple notifications.
 const std::string kDefaultAppId = "app1";
 
@@ -105,35 +162,38 @@ class MessageCenterImplTest : public testing::Test {
   base::Closure closure() const { return closure_; }
 
  protected:
-  Notification* CreateSimpleNotification(const std::string& id) {
+  std::unique_ptr<Notification> CreateSimpleNotification(
+      const std::string& id) {
     return CreateNotificationWithNotifierId(id, kDefaultAppId,
                                             NOTIFICATION_TYPE_SIMPLE);
   }
 
-  Notification* CreateSimpleNotificationWithNotifierId(
-    const std::string& id, const std::string& notifier_id) {
+  std::unique_ptr<Notification> CreateSimpleNotificationWithNotifierId(
+      const std::string& id,
+      const std::string& notifier_id) {
     return CreateNotificationWithNotifierId(
         id,
         notifier_id,
         NOTIFICATION_TYPE_SIMPLE);
   }
 
-  Notification* CreateNotification(const std::string& id,
-                                   NotificationType type) {
+  std::unique_ptr<Notification> CreateNotification(const std::string& id,
+                                                   NotificationType type) {
     return CreateNotificationWithNotifierId(id, kDefaultAppId, type);
   }
 
-  Notification* CreateNotificationWithNotifierId(const std::string& id,
-                                                 const std::string& notifier_id,
-                                                 NotificationType type) {
+  std::unique_ptr<Notification> CreateNotificationWithNotifierId(
+      const std::string& id,
+      const std::string& notifier_id,
+      NotificationType type) {
     RichNotificationData optional_fields;
     optional_fields.buttons.push_back(ButtonInfo(UTF8ToUTF16("foo")));
     optional_fields.buttons.push_back(ButtonInfo(UTF8ToUTF16("foo")));
-    return new Notification(type, id, UTF8ToUTF16("title"), UTF8ToUTF16(id),
-                            gfx::Image() /* icon */,
-                            base::string16() /* display_source */, GURL(),
-                            NotifierId(NotifierId::APPLICATION, notifier_id),
-                            optional_fields, NULL);
+    return std::make_unique<Notification>(
+        type, id, UTF8ToUTF16("title"), UTF8ToUTF16(id),
+        gfx::Image() /* icon */, base::string16() /* display_source */, GURL(),
+        NotifierId(NotifierId::APPLICATION, notifier_id), optional_fields,
+        base::MakeRefCounted<TestDelegate>());
   }
 
  private:
@@ -698,22 +758,22 @@ TEST_F(MessageCenterImplTest, RemoveAllNotificationsWithPinned) {
 
 TEST_F(MessageCenterImplTest, NotifierEnabledChanged) {
   ASSERT_EQ(0u, message_center()->NotificationCount());
-  message_center()->AddNotification(std::unique_ptr<Notification>(
-      CreateSimpleNotificationWithNotifierId("id1-1", "app1")));
-  message_center()->AddNotification(std::unique_ptr<Notification>(
-      CreateSimpleNotificationWithNotifierId("id1-2", "app1")));
-  message_center()->AddNotification(std::unique_ptr<Notification>(
-      CreateSimpleNotificationWithNotifierId("id1-3", "app1")));
-  message_center()->AddNotification(std::unique_ptr<Notification>(
-      CreateSimpleNotificationWithNotifierId("id2-1", "app2")));
-  message_center()->AddNotification(std::unique_ptr<Notification>(
-      CreateSimpleNotificationWithNotifierId("id2-2", "app2")));
-  message_center()->AddNotification(std::unique_ptr<Notification>(
-      CreateSimpleNotificationWithNotifierId("id2-3", "app2")));
-  message_center()->AddNotification(std::unique_ptr<Notification>(
-      CreateSimpleNotificationWithNotifierId("id2-4", "app2")));
-  message_center()->AddNotification(std::unique_ptr<Notification>(
-      CreateSimpleNotificationWithNotifierId("id2-5", "app2")));
+  message_center()->AddNotification(
+      CreateSimpleNotificationWithNotifierId("id1-1", "app1"));
+  message_center()->AddNotification(
+      CreateSimpleNotificationWithNotifierId("id1-2", "app1"));
+  message_center()->AddNotification(
+      CreateSimpleNotificationWithNotifierId("id1-3", "app1"));
+  message_center()->AddNotification(
+      CreateSimpleNotificationWithNotifierId("id2-1", "app2"));
+  message_center()->AddNotification(
+      CreateSimpleNotificationWithNotifierId("id2-2", "app2"));
+  message_center()->AddNotification(
+      CreateSimpleNotificationWithNotifierId("id2-3", "app2"));
+  message_center()->AddNotification(
+      CreateSimpleNotificationWithNotifierId("id2-4", "app2"));
+  message_center()->AddNotification(
+      CreateSimpleNotificationWithNotifierId("id2-5", "app2"));
   ASSERT_EQ(8u, message_center()->NotificationCount());
 
   // Removing all of app2's notifications should only leave app1's.
@@ -739,9 +799,9 @@ TEST_F(MessageCenterImplTest, UpdateWhileMessageCenterVisible) {
 
   // First, add and update a notification to ensure updates happen
   // normally.
-  std::unique_ptr<Notification> notification(CreateSimpleNotification(id1));
+  std::unique_ptr<Notification> notification = CreateSimpleNotification(id1);
   message_center()->AddNotification(std::move(notification));
-  notification.reset(CreateSimpleNotification(id2));
+  notification = CreateSimpleNotification(id2);
   message_center()->UpdateNotification(id1, std::move(notification));
   EXPECT_TRUE(message_center()->FindVisibleNotificationById(id2));
   EXPECT_FALSE(message_center()->FindVisibleNotificationById(id1));
@@ -750,7 +810,7 @@ TEST_F(MessageCenterImplTest, UpdateWhileMessageCenterVisible) {
   message_center()->SetVisibility(VISIBILITY_MESSAGE_CENTER);
 
   // Then update a notification; the update should have propagated.
-  notification.reset(CreateSimpleNotification(id1));
+  notification = CreateSimpleNotification(id1);
   message_center()->UpdateNotification(id2, std::move(notification));
   EXPECT_FALSE(message_center()->FindVisibleNotificationById(id2));
   EXPECT_TRUE(message_center()->FindVisibleNotificationById(id1));
@@ -763,7 +823,7 @@ TEST_F(MessageCenterImplTest, AddWhileMessageCenterVisible) {
   message_center()->SetVisibility(VISIBILITY_MESSAGE_CENTER);
 
   // Add a notification and confirm the adding should have propagated.
-  std::unique_ptr<Notification> notification(CreateSimpleNotification(id));
+  std::unique_ptr<Notification> notification = CreateSimpleNotification(id);
   message_center()->AddNotification(std::move(notification));
   EXPECT_TRUE(message_center()->FindVisibleNotificationById(id));
 }
@@ -772,7 +832,7 @@ TEST_F(MessageCenterImplTest, RemoveWhileMessageCenterVisible) {
   std::string id("id1");
 
   // First, add a notification to ensure updates happen normally.
-  std::unique_ptr<Notification> notification(CreateSimpleNotification(id));
+  std::unique_ptr<Notification> notification = CreateSimpleNotification(id);
   message_center()->AddNotification(std::move(notification));
   EXPECT_TRUE(message_center()->FindVisibleNotificationById(id));
 
@@ -789,8 +849,8 @@ TEST_F(MessageCenterImplTest, FindNotificationsByAppId) {
   const std::string id1("id1");
 
   // Add a notification for |app_id1|.
-  std::unique_ptr<Notification> notification(
-      CreateNotificationWithNotifierId(id1, app_id1, NOTIFICATION_TYPE_SIMPLE));
+  std::unique_ptr<Notification> notification =
+      CreateNotificationWithNotifierId(id1, app_id1, NOTIFICATION_TYPE_SIMPLE);
   message_center()->AddNotification(std::move(notification));
   EXPECT_EQ(1u, message_center()->FindNotificationsByAppId(app_id1).size());
 
@@ -802,8 +862,8 @@ TEST_F(MessageCenterImplTest, FindNotificationsByAppId) {
   // Marking a popup as shown in !OS_CHROMEOS removes the notification.
   EXPECT_EQ(0u, message_center()->FindNotificationsByAppId(app_id1).size());
   // Re-add the notification for the next test scenario.
-  notification.reset(
-      CreateNotificationWithNotifierId(id1, app_id1, NOTIFICATION_TYPE_SIMPLE));
+  notification =
+      CreateNotificationWithNotifierId(id1, app_id1, NOTIFICATION_TYPE_SIMPLE);
   message_center()->AddNotification(std::move(notification));
   EXPECT_EQ(1u, message_center()->FindNotificationsByAppId(app_id1).size());
 #endif
@@ -816,8 +876,8 @@ TEST_F(MessageCenterImplTest, FindNotificationsByAppId) {
   // Marking a popup as shown in !OS_CHROMEOS removes the notification.
   EXPECT_EQ(0u, message_center()->FindNotificationsByAppId(app_id1).size());
   // Re-add the notification for the next test scenario.
-  notification.reset(
-      CreateNotificationWithNotifierId(id1, app_id1, NOTIFICATION_TYPE_SIMPLE));
+  notification =
+      CreateNotificationWithNotifierId(id1, app_id1, NOTIFICATION_TYPE_SIMPLE);
   message_center()->AddNotification(std::move(notification));
   EXPECT_EQ(1u, message_center()->FindNotificationsByAppId(app_id1).size());
 #endif
@@ -827,13 +887,13 @@ TEST_F(MessageCenterImplTest, FindNotificationsByAppId) {
   EXPECT_EQ(0u, message_center()->FindNotificationsByAppId(app_id1).size());
 
   // Add two notifications for |app_id1|.
-  notification.reset(
-      CreateNotificationWithNotifierId(id1, app_id1, NOTIFICATION_TYPE_SIMPLE));
+  notification =
+      CreateNotificationWithNotifierId(id1, app_id1, NOTIFICATION_TYPE_SIMPLE);
   message_center()->AddNotification(std::move(notification));
 
   const std::string id2("id2");
-  notification.reset(
-      CreateNotificationWithNotifierId(id2, app_id1, NOTIFICATION_TYPE_SIMPLE));
+  notification =
+      CreateNotificationWithNotifierId(id2, app_id1, NOTIFICATION_TYPE_SIMPLE);
   message_center()->AddNotification(std::move(notification));
   EXPECT_EQ(2u, message_center()->FindNotificationsByAppId(app_id1).size());
 
@@ -844,8 +904,8 @@ TEST_F(MessageCenterImplTest, FindNotificationsByAppId) {
   // Add a notification for |app_id2|.
   const std::string app_id2("app_id2");
   const std::string id3("id3");
-  notification.reset(
-      CreateNotificationWithNotifierId(id3, app_id2, NOTIFICATION_TYPE_SIMPLE));
+  notification =
+      CreateNotificationWithNotifierId(id3, app_id2, NOTIFICATION_TYPE_SIMPLE);
   message_center()->AddNotification(std::move(notification));
   EXPECT_EQ(1u, message_center()->FindNotificationsByAppId(app_id1).size());
   EXPECT_EQ(1u, message_center()->FindNotificationsByAppId(app_id2).size());
@@ -865,5 +925,86 @@ TEST_F(MessageCenterImplTest, FindNotificationsByAppId) {
   EXPECT_EQ(0u, message_center()->FindNotificationsByAppId(app_id2).size());
 }
 
+TEST_F(MessageCenterImplTest, QueueWhenCenterVisible) {
+  TestAddObserver observer(message_center());
+
+  message_center()->AddNotification(CreateSimpleNotification("n"));
+  message_center()->SetVisibility(VISIBILITY_MESSAGE_CENTER);
+  message_center()->AddNotification(CreateSimpleNotification("n2"));
+
+  // 'update-n' should happen since SetVisibility updates is_read status of n.
+  EXPECT_EQ("add-n_update-n", observer.log("n"));
+
+  message_center()->SetVisibility(VISIBILITY_TRANSIENT);
+
+  EXPECT_EQ("add-n2", observer.log("n2"));
+}
+
+TEST_F(MessageCenterImplTest, UpdateProgressNotificationWhenCenterVisible) {
+  TestAddObserver observer(message_center());
+
+  // Add a progress notification and update it while the message center
+  // is visible.
+  std::unique_ptr<Notification> notification = CreateSimpleNotification("n");
+  notification->set_type(NOTIFICATION_TYPE_PROGRESS);
+  Notification notification_copy = *notification;
+  message_center()->AddNotification(std::move(notification));
+  message_center()->ClickOnNotification("n");
+  message_center()->SetVisibility(VISIBILITY_MESSAGE_CENTER);
+  observer.reset_logs();
+  notification_copy.set_progress(50);
+  message_center()->UpdateNotification(
+      "n", std::make_unique<Notification>(notification_copy));
+
+  // Expect that the progress notification update is performed.
+  EXPECT_EQ("update-n", observer.log("n"));
+}
+
+TEST_F(MessageCenterImplTest, UpdateNonProgressNotificationWhenCenterVisible) {
+  TestAddObserver observer(message_center());
+
+  // Add a non-progress notification and update it while the message center
+  // is visible.
+  std::unique_ptr<Notification> notification = CreateSimpleNotification("n");
+  Notification notification_copy = *notification;
+  message_center()->AddNotification(std::move(notification));
+  message_center()->ClickOnNotification("n");
+  message_center()->SetVisibility(VISIBILITY_MESSAGE_CENTER);
+  observer.reset_logs();
+  notification_copy.set_title(base::ASCIIToUTF16("title2"));
+  message_center()->UpdateNotification(
+      notification_copy.id(),
+      std::make_unique<Notification>(notification_copy));
+
+  // Expect that the notification update is done.
+  EXPECT_NE("", observer.log("n"));
+
+  message_center()->SetVisibility(VISIBILITY_TRANSIENT);
+  EXPECT_EQ("update-n", observer.log("n"));
+}
+
+TEST_F(MessageCenterImplTest,
+       UpdateNonProgressToProgressNotificationWhenCenterVisible) {
+  TestAddObserver observer(message_center());
+
+  // Add a non-progress notification and change the type to progress while the
+  // message center is visible.
+  std::unique_ptr<Notification> notification = CreateSimpleNotification("n");
+  Notification notification_copy = *notification;
+  message_center()->AddNotification(std::move(notification));
+  message_center()->ClickOnNotification("n");
+  message_center()->SetVisibility(VISIBILITY_MESSAGE_CENTER);
+  observer.reset_logs();
+  notification_copy.set_type(NOTIFICATION_TYPE_PROGRESS);
+  message_center()->UpdateNotification(
+      "n", std::make_unique<Notification>(notification_copy));
+
+  // Expect that the notification update is done.
+  EXPECT_NE("", observer.log("n"));
+
+  message_center()->SetVisibility(VISIBILITY_TRANSIENT);
+  EXPECT_EQ("update-n", observer.log("n"));
+}
+
 }  // namespace internal
 }  // namespace message_center