Make sure WindowObservers are removed from window before destruction

ash/shell.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/shell.h"
 
 #include <algorithm>
 #include <string>
 
 #include "ash/accelerators/accelerator_controller.h"
@@ skipping 612 matching lines @@
 
   // Destroy SystemTrayDelegate before destroying the status area(s).
   system_tray_delegate_->Shutdown();
   system_tray_delegate_.reset();
 
   locale_notification_controller_.reset();
 
   // Drag-and-drop must be canceled prior to close all windows.
   drag_drop_controller_.reset();
 
+  // Controllers who has WindowObserver added must be deleted
+  // before |display_controller_| being deleted.

[sky, 2013/12/04 22:52:59]

being -> is

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

Done.

+#if defined(OS_CHROMEOS)
+  // Destroy VideoActivityNotifier before destroying VideoDetector.

[sky, 2013/12/04 22:52:59]

This comment documents the code, and therefor isn't helpful. If you're going to
add a comment, document why the order is important here.

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

I need these comments not to break the order when I refactor (like this)
because such dependency isn't clear from the code here. I updated the
comment to be more clear.

[sky, 2013/12/05 01:36:35]

I'm fine with comments, but they should describe why the code is the way it is,
not what the code does. For example:
// Destroy VideoActivityNotifier before VideoDetector as VideoActivityNotifier
references VideoDetector.

+  video_activity_notifier_.reset();
+#endif  // defined(OS_CHROMEOS)
+  video_detector_.reset();
+
+  shadow_controller_.reset();
+  resize_shadow_controller_.reset();
+
+  window_cycle_controller_.reset();
+  mru_window_tracker_.reset();
+
   // Destroy all child windows including widgets.
   display_controller_->CloseChildWindows();
   display_controller_->CloseNonDesktopDisplay();
 
+  // Launcher delete must be deleted before |focus_client_|.

[sky, 2013/12/04 22:52:59]

Same thing here.

+  launcher_delegate_.reset();
+  focus_client_.reset();
+
   // Destroy SystemTrayNotifier after destroying SystemTray as TrayItems
   // needs to remove observers from it.
   system_tray_notifier_.reset();
 
-#if defined(OS_CHROMEOS)
-  // Destroy VideoActivityNotifier before destroying VideoDetector.
-  video_activity_notifier_.reset();
-#endif  // defined(OS_CHROMEOS)
-
   // These need a valid Shell instance to clean up properly, so explicitly
   // delete them before invalidating the instance.
   // Alphabetical. TODO(oshima): sort.
   magnification_controller_.reset();
   partial_magnification_controller_.reset();
-  resize_shadow_controller_.reset();
-  shadow_controller_.reset();
   tooltip_controller_.reset();
   event_client_.reset();
-  window_cycle_controller_.reset();
   nested_dispatcher_controller_.reset();
   user_action_client_.reset();
   visibility_controller_.reset();
-  launcher_delegate_.reset();
   // |launcher_item_delegate_manager_| observes |shelf_model_|. It must be
   // destroyed before |shelf_model_| is destroyed.
   launcher_item_delegate_manager_.reset();
   shelf_model_.reset();
-  video_detector_.reset();
 
   power_button_controller_.reset();
   lock_state_controller_.reset();
-  mru_window_tracker_.reset();
 
   resolution_notification_controller_.reset();
   desktop_background_controller_.reset();
 
   // This also deletes all RootWindows. Note that we invoke Shutdown() on
   // DisplayController before resetting |display_controller_|, since destruction
   // of its owned RootWindowControllers relies on the value.
   display_controller_->Shutdown();
   display_controller_.reset();
   screen_position_controller_.reset();
@@ skipping 333 matching lines @@
 ////////////////////////////////////////////////////////////////////////////////
 // Shell, aura::client::ActivationChangeObserver implementation:
 
 void Shell::OnWindowActivated(aura::Window* gained_active,
                               aura::Window* lost_active) {
   if (gained_active)
     target_root_window_ = gained_active->GetRootWindow();
 }
 
 }  // namespace ash