Distinguishes "nothing new" and "should be empty" in display notifications.

Sometimes OnDisplayConfigurationChanged() can be called twice or
even more, and in that case the notification will be cleared. This
is a side effect of closing stale notifications. We should distinguish
"nothing new happened actually" from "the notification should be
cleared", both has empty |message|.

BUG=260977
R=oshima@chromium.org
TEST=the new test case covers

Review URL: https://chromiumcodereview.appspot.com/19502002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@212468 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 55 insertions, 17 deletions

diff --git a/ash/system/chromeos/tray_display.cc b/ash/system/chromeos/tray_display.cc
index a49c412..32bbfdc 100644
--- a/ash/system/chromeos/tray_display.cc
+++ b/ash/system/chromeos/tray_display.cc
@@ -356,7 +356,7 @@ TrayDisplay::~TrayDisplay() {
   Shell::GetInstance()->display_controller()->RemoveObserver(this);
 }
 
-base::string16 TrayDisplay::GetDisplayMessageForNotification() {
+bool TrayDisplay::GetDisplayMessageForNotification(base::string16* message) {
   DisplayManager* display_manager = GetDisplayManager();
   DisplayInfoMap old_info;
   old_info.swap(display_info_);
@@ -367,32 +367,38 @@ base::string16 TrayDisplay::GetDisplayMessageForNotification() {
 
   // Display is added or removed. Use the same message as the one in
   // the system tray.
-  if (display_info_.size() != old_info.size())
-    return GetTrayDisplayMessage();
+  if (display_info_.size() != old_info.size()) {
+    *message = GetTrayDisplayMessage();
+    return true;
+  }
 
   for (DisplayInfoMap::const_iterator iter = display_info_.begin();
        iter != display_info_.end(); ++iter) {
     DisplayInfoMap::const_iterator old_iter = old_info.find(iter->first);
-    // A display is removed and added at the same time. It won't happen
-    // in the actual environment, but falls back to the system tray's
-    // message just in case.
-    if (old_iter == old_info.end())
-      return GetTrayDisplayMessage();
+    // The display's number is same but different displays. This happens
+    // for the transition between docked mode and mirrored display. Falls back
+    // to GetTrayDisplayMessage().
+    if (old_iter == old_info.end()) {
+      *message = GetTrayDisplayMessage();
+      return true;
+    }
 
     if (iter->second.ui_scale() != old_iter->second.ui_scale()) {
-      return l10n_util::GetStringFUTF16(
+      *message = l10n_util::GetStringFUTF16(
           IDS_ASH_STATUS_TRAY_DISPLAY_RESOLUTION_CHANGED,
           GetDisplayName(iter->first),
           GetDisplaySize(iter->first));
+      return true;
     }
     if (iter->second.rotation() != old_iter->second.rotation()) {
-      return l10n_util::GetStringFUTF16(
+      *message = l10n_util::GetStringFUTF16(
           IDS_ASH_STATUS_TRAY_DISPLAY_ROTATED, GetDisplayName(iter->first));
+      return true;
     }
   }
 
   // Found nothing special
-  return base::string16();
+  return false;
 }
 
 views::View* TrayDisplay::CreateDefaultView(user::LoginStatus status) {
@@ -411,7 +417,9 @@ void TrayDisplay::OnDisplayConfigurationChanged() {
     return;
   }
 
-  UpdateDisplayNotification(GetDisplayMessageForNotification());
+  base::string16 message;
+  if (GetDisplayMessageForNotification(&message))
+    UpdateDisplayNotification(message);
 }
 
 base::string16 TrayDisplay::GetDefaultViewMessage() {
diff --git a/ash/system/chromeos/tray_display.h b/ash/system/chromeos/tray_display.h
index 7ede247..7adbd31 100644
--- a/ash/system/chromeos/tray_display.h
+++ b/ash/system/chromeos/tray_display.h
@@ -29,22 +29,24 @@ class ASH_EXPORT TrayDisplay : public SystemTrayItem,
   explicit TrayDisplay(SystemTray* system_tray);
   virtual ~TrayDisplay();
 
+  // Overridden from DisplayControllerObserver:
+  virtual void OnDisplayConfigurationChanged() OVERRIDE;
+
  private:
   friend class TrayDisplayTest;
 
   typedef std::map<int64, DisplayInfo> DisplayInfoMap;
 
   // Checks the current display settings and determine what message should be
-  // shown for notification.
-  base::string16 GetDisplayMessageForNotification();
+  // shown for notification. Returns true if there's a meaningful change. Note
+  // that it's possible to return true and set |message| to empty, which means
+  // the notification should be removed.
+  bool GetDisplayMessageForNotification(base::string16* message);
 
   // Overridden from SystemTrayItem.
   virtual views::View* CreateDefaultView(user::LoginStatus status) OVERRIDE;
   virtual void DestroyDefaultView() OVERRIDE;
 
-  // Overridden from DisplayControllerObserver:
-  virtual void OnDisplayConfigurationChanged() OVERRIDE;
-
   // Test accessors.
   base::string16 GetDefaultViewMessage();
   base::string16 GetNotificationMessage();
diff --git a/ash/system/chromeos/tray_display_unittest.cc b/ash/system/chromeos/tray_display_unittest.cc
index 540a2ff..a36b180 100644
--- a/ash/system/chromeos/tray_display_unittest.cc
+++ b/ash/system/chromeos/tray_display_unittest.cc
@@ -66,6 +66,7 @@ class TrayDisplayTest : public ash::test::AshTestBase {
 
  protected:
   SystemTray* tray() { return tray_; }
+  TrayDisplay* tray_display() { return tray_display_; }
 
   void CloseNotification();
   bool IsDisplayVisibleInTray();
@@ -396,5 +397,32 @@ TEST_F(TrayDisplayTest, DisplayNotifications) {
       GetDisplayNotificationText());
 }
 
+TEST_F(TrayDisplayTest, DisplayConfigurationChangedTwice) {
+  test::TestSystemTrayDelegate* tray_delegate =
+      static_cast<test::TestSystemTrayDelegate*>(
+          Shell::GetInstance()->system_tray_delegate());
+  tray_delegate->set_should_show_display_notification(true);
+
+  UpdateDisplay("400x400,200x200");
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(
+          IDS_ASH_STATUS_TRAY_DISPLAY_EXTENDED_NO_INTERNAL),
+      GetDisplayNotificationText());
+
+  // OnDisplayConfigurationChanged() may be called more than once for a single
+  // update display in case of primary is swapped or recovered from dock mode.
+  // Should not remove the notification in such case.
+  tray_display()->OnDisplayConfigurationChanged();
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(
+          IDS_ASH_STATUS_TRAY_DISPLAY_EXTENDED_NO_INTERNAL),
+      GetDisplayNotificationText());
+
+  // Back to the single display. It SHOULD remove the notification since the
+  // information is stale.
+  UpdateDisplay("400x400");
+  EXPECT_TRUE(GetDisplayNotificationText().empty());
+}
+
 }  // namespace internal
 }  // namespace ash