Reland "Fix missing sync error notification pop-up"

- Only MarkSinglePopupAsShown of already shown pup-ups;
- Make LoginStateNotificationBlocker to defer showing notifications
  until the active user PrefService is initialized. This allows
  Shelf to apply user prefs first so that WebNotificationTray does
  not HidePopupBubble when user PrefService is initialized sometime
  later after session state changes to active;

Bug: 721717
Change-Id: I86e9978be7b56e92ce847f0086a5fa2e4f6b72db
Reviewed-on: https://chromium-review.googlesource.com/782359
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518406}

ash/session/test_session_controller_client.cc

@@ -134,11 +134,18 @@ void TestSessionControllerClient::AddUserSession(
 
   if (provide_pref_service &&
       !controller_->GetUserPrefServiceForUser(account_id)) {
+    ProvidePrefServiceForUser(account_id);
+  }
+}
+
+void TestSessionControllerClient::ProvidePrefServiceForUser(
+    const AccountId& account_id) {
+  DCHECK(!controller_->GetUserPrefServiceForUser(account_id));
+
   auto pref_service = std::make_unique<TestingPrefServiceSimple>();
   Shell::RegisterProfilePrefs(pref_service->registry(), true /* for_test */);
   controller_->ProvideUserPrefServiceForTest(account_id,
                                              std::move(pref_service));
-  }
 }
 
 void TestSessionControllerClient::UnlockScreen() {

ash/session/test_session_controller_client.h

@@ -63,6 +63,9 @@ class TestSessionControllerClient : public ash::mojom::SessionControllerClient {
       bool provide_pref_service = true,
       bool is_new_profile = false);
 
+  // Creates a test PrefService and associates it with the user.
+  void ProvidePrefServiceForUser(const AccountId& account_id);
+
   // Simulates screen unlocking. It is virtual so that test cases can override
   // it. The default implementation sets the session state of SessionController
   // to be ACTIVE.

ash/system/web_notification/login_state_notification_blocker.cc

@@ -13,9 +13,34 @@ using session_manager::SessionState;
 
 namespace ash {
 
+namespace {
+
+// Returns true when screen is not locked.
+bool CalculateShouldShowNotification() {
+  return !Shell::Get()->session_controller()->IsScreenLocked();
+}
+
+// Returns true if session state is active, there is an active user and the
+// PrefService of the active user is initialized.
+bool CalculateShouldShowPopup() {
+  SessionController* const session_controller =
+      Shell::Get()->session_controller();
+  if (session_controller->GetSessionState() != SessionState::ACTIVE)
+    return false;
+
+  const mojom::UserSession* active_user_session =
+      session_controller->GetUserSession(0);
+  return active_user_session && session_controller->GetUserPrefServiceForUser(
+                                    active_user_session->user_info->account_id);
+}
+
+}  // namespace
+
 LoginStateNotificationBlocker::LoginStateNotificationBlocker(
     message_center::MessageCenter* message_center)
-    : NotificationBlocker(message_center) {
+    : NotificationBlocker(message_center),
+      should_show_notification_(CalculateShouldShowNotification()),
+      should_show_popup_(CalculateShouldShowPopup()) {
   Shell::Get()->session_controller()->AddObserver(this);
 }
 
@@ -25,7 +50,7 @@ LoginStateNotificationBlocker::~LoginStateNotificationBlocker() {
 
 bool LoginStateNotificationBlocker::ShouldShowNotification(
     const message_center::Notification& notification) const {
-  return !Shell::Get()->session_controller()->IsScreenLocked();
+  return should_show_notification_;
 }
 
 bool LoginStateNotificationBlocker::ShouldShowNotificationAsPopup(
@@ -33,11 +58,28 @@ bool LoginStateNotificationBlocker::ShouldShowNotificationAsPopup(
   if (ash::system_notifier::ShouldAlwaysShowPopups(notification.notifier_id()))
     return true;
 
-  return Shell::Get()->session_controller()->GetSessionState() ==
-         SessionState::ACTIVE;
+  return should_show_popup_;
 }
 
 void LoginStateNotificationBlocker::OnSessionStateChanged(SessionState state) {
+  CheckStateAndNotifyIfChanged();
+}
+
+void LoginStateNotificationBlocker::OnActiveUserPrefServiceChanged(
+    PrefService* pref_service) {
+  CheckStateAndNotifyIfChanged();
+}
+
+void LoginStateNotificationBlocker::CheckStateAndNotifyIfChanged() {
+  const bool new_should_show_notification = CalculateShouldShowNotification();
+  const bool new_should_show_popup = CalculateShouldShowPopup();
+  if (new_should_show_notification == should_show_notification_ &&
+      new_should_show_popup == should_show_popup_) {
+    return;
+  }
+
+  should_show_notification_ = new_should_show_notification;
+  should_show_popup_ = new_should_show_popup;
   NotifyBlockingStateChanged();
 }
 

ash/system/web_notification/login_state_notification_blocker.h

@@ -13,8 +13,9 @@
 namespace ash {
 
 // A notification blocker which suppresses notifications popups based on the
-// session state reported by the SessionController. Only active (logged in,
-// unlocked) sessions will show notifications.
+// session state and active user PrefService readiness reported by the
+// SessionController. Only active (logged in, unlocked) sessions with
+// initialized PrefService will show notifications.
 class ASH_EXPORT LoginStateNotificationBlocker
     : public message_center::NotificationBlocker,
       public SessionObserver {
@@ -32,6 +33,12 @@ class ASH_EXPORT LoginStateNotificationBlocker
 
   // SessionObserver overrides:
   void OnSessionStateChanged(session_manager::SessionState state) override;
+  void OnActiveUserPrefServiceChanged(PrefService* pref_service) override;
+
+  void CheckStateAndNotifyIfChanged();
+
+  bool should_show_notification_ = false;
+  bool should_show_popup_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(LoginStateNotificationBlocker);
 };

ash/system/web_notification/login_state_notification_blocker_unittest.cc

@@ -134,5 +134,44 @@ TEST_F(LoginStateNotificationBlockerTest, AlwaysAllowedNotifier) {
   EXPECT_TRUE(ShouldShowNotificationAsPopup(notifier_id));
 }
 
+TEST_F(LoginStateNotificationBlockerTest, BlockOnPrefService) {
+  // Default status: OOBE.
+  message_center::NotifierId notifier_id(
+      message_center::NotifierId::APPLICATION, "test-notifier");
+  EXPECT_FALSE(ShouldShowNotificationAsPopup(notifier_id));
+
+  // Login screen.
+  GetSessionControllerClient()->SetSessionState(SessionState::LOGIN_PRIMARY);
+  EXPECT_EQ(0, GetStateChangedCountAndReset());
+  EXPECT_FALSE(ShouldShowNotificationAsPopup(notifier_id));
+
+  // Simulates login event sequence in production code:
+  // - Add a user session;
+  // - User session is set as active session;
+  // - Session state changes to active;
+  // - User PrefService is initialized sometime later.
+  const AccountId kUserAccountId = AccountId::FromUserEmail("user@test.com");
+  TestSessionControllerClient* const session_controller_client =
+      GetSessionControllerClient();
+  session_controller_client->AddUserSession(kUserAccountId.GetUserEmail(),
+                                            user_manager::USER_TYPE_REGULAR,
+                                            true, /* enable_settings */
+                                            false /* provide_pref_service */);
+  EXPECT_EQ(0, GetStateChangedCountAndReset());
+  EXPECT_FALSE(ShouldShowNotificationAsPopup(notifier_id));
+
+  session_controller_client->SwitchActiveUser(kUserAccountId);
+  EXPECT_EQ(0, GetStateChangedCountAndReset());
+  EXPECT_FALSE(ShouldShowNotificationAsPopup(notifier_id));
+
+  session_controller_client->SetSessionState(SessionState::ACTIVE);
+  EXPECT_EQ(0, GetStateChangedCountAndReset());
+  EXPECT_FALSE(ShouldShowNotificationAsPopup(notifier_id));
+
+  session_controller_client->ProvidePrefServiceForUser(kUserAccountId);
+  EXPECT_EQ(1, GetStateChangedCountAndReset());
+  EXPECT_TRUE(ShouldShowNotificationAsPopup(notifier_id));
+}
+
 }  // namespace
 }  // namespace ash

chrome/browser/notifications/login_state_notification_blocker_chromeos_browsertest.cc

@@ -81,7 +81,10 @@ class LoginStateNotificationBlockerChromeOSBrowserTest
   // message_center::MessageCenterObserver:
   void OnBlockingStateChanged(
       message_center::NotificationBlocker* blocker) override {
-    state_changed_count_++;
+    ++state_changed_count_;
+
+    if (wait_loop_ && state_changed_count_ == expected_state_change_)
+      wait_loop_->Quit();
   }
 
   int GetStateChangedCountAndReset() {
@@ -90,6 +93,21 @@ class LoginStateNotificationBlockerChromeOSBrowserTest
     return result;
   }
 
+  void WaitForStateChangeAndReset(int expected_state_change) {
+    expected_state_change_ = expected_state_change;
+
+    if (state_changed_count_ != expected_state_change_) {
+      wait_loop_ = std::make_unique<base::RunLoop>();
+      wait_loop_->Run();
+      wait_loop_.reset();
+    }
+
+    EXPECT_EQ(expected_state_change_, state_changed_count_);
+
+    expected_state_change_ = 0;
+    state_changed_count_ = 0;
+  }
+
   // Compares the number of notifications before and after adding a notification
   // to verify whether the new one is shown.
   bool ShouldShowNotificationAsPopup(
@@ -112,6 +130,9 @@ class LoginStateNotificationBlockerChromeOSBrowserTest
  private:
   int state_changed_count_ = 0;
 
+  std::unique_ptr<base::RunLoop> wait_loop_;
+  int expected_state_change_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(LoginStateNotificationBlockerChromeOSBrowserTest);
 };
 
@@ -130,23 +151,23 @@ IN_PROC_BROWSER_TEST_F(LoginStateNotificationBlockerChromeOSBrowserTest,
 
   // Logged in as a normal user.
   LoginUser(kTestUsers[0]);
-  // Two session state changes for login:
-  //   LOGIN_PRIMARY -> LOGGED_IN_NOT_ACTIVE -> ACTIVE.
-  // Plus one state change for the InactiveUserNotificationBlocker.
-  EXPECT_EQ(3, GetStateChangedCountAndReset());
+
+  // One state change from LoginStateNotificationBloker plus one state change
+  // for the InactiveUserNotificationBlocker.
+  WaitForStateChangeAndReset(2);
   EXPECT_TRUE(ShouldShowNotificationAsPopup(notifier_id));
 
   // Multi-login user switch.
   UserAddingFinishObserver observer;
   chromeos::UserAddingScreen::Get()->Start();
-  content::RunAllPendingInMessageLoop();
+  content::RunAllTasksUntilIdle();
   EXPECT_EQ(1, GetStateChangedCountAndReset());
   EXPECT_FALSE(ShouldShowNotificationAsPopup(notifier_id));
 
   // Multi-login user switch off.
   chromeos::UserAddingScreen::Get()->Cancel();
   observer.WaitUntilUserAddingFinishedOrCancelled();
-  content::RunAllPendingInMessageLoop();
+  content::RunAllTasksUntilIdle();
   EXPECT_EQ(1, GetStateChangedCountAndReset());
   EXPECT_TRUE(ShouldShowNotificationAsPopup(notifier_id));
 }
@@ -168,23 +189,23 @@ IN_PROC_BROWSER_TEST_F(LoginStateNotificationBlockerChromeOSBrowserTest,
 
   // Logged in as a normal user.
   LoginUser(kTestUsers[0]);
-  // Two session state changes for login:
-  //   LOGIN_PRIMARY -> LOGGED_IN_NOT_ACTIVE -> ACTIVE.
-  // Plus one state change for the InactiveUserNotificationBlocker.
-  EXPECT_EQ(3, GetStateChangedCountAndReset());
+
+  // One state change from LoginStateNotificationBloker plus one state change
+  // for the InactiveUserNotificationBlocker.
+  WaitForStateChangeAndReset(2);
   EXPECT_TRUE(ShouldShowNotificationAsPopup(notifier_id));
 
   // Multi-login user switch.
   UserAddingFinishObserver observer;
   chromeos::UserAddingScreen::Get()->Start();
-  content::RunAllPendingInMessageLoop();
+  content::RunAllTasksUntilIdle();
   EXPECT_EQ(1, GetStateChangedCountAndReset());
   EXPECT_TRUE(ShouldShowNotificationAsPopup(notifier_id));
 
   // Multi-login user switch off.
   chromeos::UserAddingScreen::Get()->Cancel();
   observer.WaitUntilUserAddingFinishedOrCancelled();
-  content::RunAllPendingInMessageLoop();
+  content::RunAllTasksUntilIdle();
   EXPECT_EQ(1, GetStateChangedCountAndReset());
   EXPECT_TRUE(ShouldShowNotificationAsPopup(notifier_id));
 }

ui/message_center/message_center_impl.cc

@@ -108,24 +108,26 @@ void MessageCenterImpl::RemoveNotificationBlocker(
 void MessageCenterImpl::OnBlockingStateChanged(NotificationBlocker* blocker) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(!iterating_);
-  std::list<std::string> blocked_ids;
+  std::list<const Notification*> blocked;
   NotificationList::PopupNotifications popups =
-      notification_list_->GetPopupNotifications(blockers_, &blocked_ids);
+      notification_list_->GetPopupNotifications(blockers_, &blocked);
 
-  for (const auto& id : blocked_ids) {
+  // Close already displayed pop-ups that are blocked now.
+  for (const auto* notification : blocked) {
     // Do not call MessageCenterImpl::MarkSinglePopupAsShown() directly here
     // just for performance reason. MessageCenterImpl::MarkSinglePopupAsShown()
     // calls NotificationList::MarkSinglePopupAsShown(), but the whole cache
     // will be recreated below.
-    notification_list_->MarkSinglePopupAsShown(id, true);
+    if (notification->IsRead())
+      notification_list_->MarkSinglePopupAsShown(notification->id(), true);
   }
   visible_notifications_ =
       notification_list_->GetVisibleNotifications(blockers_);
 
-  for (const auto& id : blocked_ids) {
+  for (const auto* notification : blocked) {
     internal::ScopedNotificationsIterationLock lock(this);
     for (auto& observer : observer_list_)
-      observer.OnNotificationUpdated(id);
+      observer.OnNotificationUpdated(notification->id());
   }
   for (auto& observer : observer_list_)
     observer.OnBlockingStateChanged(blocker);

ui/message_center/message_center_impl_unittest.cc

@@ -402,6 +402,10 @@ TEST_F(MessageCenterImplTest, NotificationBlocker) {
   EXPECT_EQ(2u, message_center()->GetPopupNotifications().size());
   EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 
+  // "id1" is displayed as a pop-up so that it will be closed when blocked.
+  message_center()->DisplayedNotification("id1",
+                                          message_center::DISPLAY_SOURCE_POPUP);
+
   // Block all notifications. All popups are gone and message center should be
   // hidden.
   blocker1.SetNotificationsEnabled(false);
@@ -424,10 +428,14 @@ TEST_F(MessageCenterImplTest, NotificationBlocker) {
   EXPECT_TRUE(message_center()->GetPopupNotifications().empty());
   EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 
-  // Unblock both blockers, which recovers the global state, but the popups
-  // aren't shown.
+  // Unblock both blockers, which recovers the global state, the displayed
+  // pop-ups before blocking aren't shown but the never-displayed ones will
+  // be shown.
   blocker2.SetNotificationsEnabled(true);
-  EXPECT_TRUE(message_center()->GetPopupNotifications().empty());
+  NotificationList::PopupNotifications popups =
+      message_center()->GetPopupNotifications();
+  EXPECT_EQ(1u, popups.size());
+  EXPECT_TRUE(PopupNotificationsContain(popups, "id2"));
   EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 }
 
@@ -443,6 +451,10 @@ TEST_F(MessageCenterImplTest, NotificationsDuringBlocked) {
   EXPECT_EQ(1u, message_center()->GetPopupNotifications().size());
   EXPECT_EQ(1u, message_center()->GetVisibleNotifications().size());
 
+  // "id1" is displayed as a pop-up so that it will be closed when blocked.
+  message_center()->DisplayedNotification("id1",
+                                          message_center::DISPLAY_SOURCE_POPUP);
+
   // Create a notification during blocked. Still no popups.
   blocker.SetNotificationsEnabled(false);
   message_center()->AddNotification(std::unique_ptr<Notification>(
@@ -480,6 +492,10 @@ TEST_F(MessageCenterImplTest, NotificationBlockerAllowsPopups) {
                        base::string16() /* display_source */, GURL(),
                        notifier_id2, RichNotificationData(), NULL)));
 
+  // "id1" is displayed as a pop-up so that it will be closed when blocked.
+  message_center()->DisplayedNotification("id1",
+                                          message_center::DISPLAY_SOURCE_POPUP);
+
   // "id1" is closed but "id2" is still visible as a popup.
   blocker.SetNotificationsEnabled(false);
   NotificationList::PopupNotifications popups =

ui/message_center/notification_list.cc

@@ -178,7 +178,7 @@ bool NotificationList::HasPopupNotifications(
 
 NotificationList::PopupNotifications NotificationList::GetPopupNotifications(
     const NotificationBlockers& blockers,
-    std::list<std::string>* blocked_ids) {
+    std::list<const Notification*>* blocked) {
   PopupNotifications result;
   size_t default_priority_popup_count = 0;
 
@@ -194,8 +194,8 @@ NotificationList::PopupNotifications NotificationList::GetPopupNotifications(
       continue;
 
     if (!ShouldShowNotificationAsPopup(*notification, blockers)) {
-      if (blocked_ids)
-        blocked_ids->push_back(notification->id());
+      if (blocked)
+        blocked->push_back(notification);
       continue;
     }
 

ui/message_center/notification_list.h

@@ -110,12 +110,12 @@ class MESSAGE_CENTER_EXPORT NotificationList {
   // Returns the recent notifications of the priority higher then LOW,
   // that have not been shown as a popup. kMaxVisiblePopupNotifications are
   // used to limit the number of notifications for the DEFAULT priority.
-  // It also stores the list of notification ids which is blocked by |blockers|
-  // to |blocked_ids|. |blocked_ids| can be NULL if the caller doesn't care
-  // which notifications are blocked.
+  // It also stores the list of notifications which are blocked by |blockers|
+  // to |blocked|. |blocked| can be NULL if the caller doesn't care which
+  // notifications are blocked.
   PopupNotifications GetPopupNotifications(
       const NotificationBlockers& blockers,
-      std::list<std::string>* blocked_ids);
+      std::list<const Notification*>* blocked_ids);
 
   // Marks a specific popup item as shown. Set |mark_notification_as_read| to
   // true in case marking the notification as read too.