Make sure WindowObservers are removed from window before destruction

chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc

Index: chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
diff --git a/chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc b/chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
index 58000f8dfa9216fcf85b0a3f6d7137cd0cfb77c3..86c4fada65a910e8cc245fa3d354780a928e9836 100644
--- a/chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
+++ b/chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc
@@ -44,6 +44,12 @@ class MultiUserWindowManagerChromeOSTest : public AshTestBase {
     return window_[index];
   }
 
+  // Delete the window at the given index.

[Mr4D (OOO till 08-26), 2013/12/04 19:45:46]

What about
"Removes the window at the given index, shifting all following indices up."

[Because you don't do this you would need to also handle the NULL case in other
testing framework functions]

[oshima, 2013/12/05 01:26:44]

IMHO it's better this way than shifting index. At least you get segfault with
0x0 (meaning you're trying to access deleted window) while if you use wrong
index, you get segfault with random address. see my comment below for deletion.

+  void delete_at(size_t index) {

[Mr4D (OOO till 08-26), 2013/12/04 19:45:46]

Can you rename this to delete_window_at?

(this is a test and its not clear what gets deleted otherwise)

[oshima, 2013/12/05 01:26:44]

Done.

+    delete window_[index];
+    window_[index] = NULL;

[Mr4D (OOO till 08-26), 2013/12/04 19:45:46]

Looking into line 114 I would think that you should do an window_.erase(..)
here?

[oshima, 2013/12/05 01:26:44]

delete NULL is valid (and no op) in C++.

+  }
+
   // The accessor to the MultiWindowManager.
   chrome::MultiUserWindowManagerChromeOS* multi_user_window_manager() {
     return multi_user_window_manager_;
@@ -216,13 +222,26 @@ TEST_F(MultiUserWindowManagerChromeOSTest, CloseWindowTests) {
   EXPECT_EQ("S[B,A], S[]", GetStatus());
   EXPECT_TRUE(multi_user_window_manager()->AreWindowsSharedAmongUsers());
 
-  // Simulate a close of the shared window.
-  multi_user_window_manager()->OnWindowDestroyed(window(0));
+  aura::Window* to_be_deleted = window(0);
+
+  EXPECT_EQ(std::string("A"),
+            multi_user_window_manager()->GetUserPresentingWindow(
+                to_be_deleted));
+  EXPECT_EQ(std::string("B"),
+            multi_user_window_manager()->GetWindowOwner(
+                to_be_deleted));
+
+  // Close the window.
+  delete_at(0);
 
   // There should be no owner anymore for that window and the shared windows
   // should be gone as well.
-  EXPECT_EQ("S[], S[]", GetStatus());
-  EXPECT_FALSE(multi_user_window_manager()->AreWindowsSharedAmongUsers());
+  EXPECT_EQ(std::string(),
+            multi_user_window_manager()->GetUserPresentingWindow(
+                to_be_deleted));
+  EXPECT_EQ(std::string(),
+            multi_user_window_manager()->GetWindowOwner(
+                to_be_deleted));
 }
 
 TEST_F(MultiUserWindowManagerChromeOSTest, SharedWindowTests) {
@@ -286,8 +305,8 @@ TEST_F(MultiUserWindowManagerChromeOSTest, DoubleSharedWindowTests) {
   multi_user_window_manager()->ShowWindowForUser(window(0), "A");
   EXPECT_TRUE(multi_user_window_manager()->AreWindowsSharedAmongUsers());
 
-  // Simulate a close of the shared window.
-  multi_user_window_manager()->OnWindowDestroyed(window(0));
+  // Close the window.
+  delete_at(0);
 
   // There should be no shares anymore open.
   EXPECT_FALSE(multi_user_window_manager()->AreWindowsSharedAmongUsers());