Clean-up of the system shutdown code paths

As suggested by +stevenjb in issue 811033002, this CL cleans up the shutdown code paths by (1) removing the ShutDown method from all system tray delegates (2) and letting the UI code call directly into ash::Shell Additionally the ShutdownPolicyObserver is moved to chrome/browser/settings and renamed to ShutdownPolicyHandler Review URL: https://codereview.chromium.org/811283003 Cr-Commit-Position: refs/heads/master@{#310477}
dednal · Jan 8, 2015 · 95c6bd1 · 95c6bd1
1 parent f00919f
commit 95c6bd1
Show file tree

Hide file tree

Showing 16 changed files with 37 additions and 57 deletions.
diff --git a/ash/system/date/date_default_view.cc b/ash/system/date/date_default_view.cc
@@ -12,6 +12,7 @@
 #include "ash/system/tray/system_tray_delegate.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_popup_header_button.h"
+#include "ash/wm/lock_state_controller.h"
 #include "grit/ash_resources.h"
 #include "grit/ash_strings.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -111,7 +112,8 @@ void DateDefaultView::ButtonPressed(views::Button* sender,
     tray_delegate->ShowHelp();
   } else if (sender == shutdown_) {
     shell->metrics()->RecordUserMetricsAction(ash::UMA_TRAY_SHUT_DOWN);
-    tray_delegate->ShutDown();
+    ash::Shell::GetInstance()->lock_state_controller()->RequestShutdown(
+        ash::LockStateController::POWER_OFF);
   } else if (sender == lock_) {
     shell->metrics()->RecordUserMetricsAction(ash::UMA_TRAY_LOCK_SCREEN);
     tray_delegate->RequestLockScreen();

diff --git a/ash/system/tray/default_system_tray_delegate.cc b/ash/system/tray/default_system_tray_delegate.cc
@@ -155,9 +155,6 @@ void DefaultSystemTrayDelegate::ShowSupervisedUserInfo() {
 void DefaultSystemTrayDelegate::ShowUserLogin() {
 }
 
-void DefaultSystemTrayDelegate::ShutDown() {
-}
-
 void DefaultSystemTrayDelegate::SignOut() {
 }
 

diff --git a/ash/system/tray/default_system_tray_delegate.h b/ash/system/tray/default_system_tray_delegate.h
@@ -49,7 +49,6 @@ class ASH_EXPORT DefaultSystemTrayDelegate : public SystemTrayDelegate {
   void ShowEnterpriseInfo() override;
   void ShowSupervisedUserInfo() override;
   void ShowUserLogin() override;
-  void ShutDown() override;
   void SignOut() override;
   void RequestLockScreen() override;
   void RequestRestartForUpdate() override;

diff --git a/ash/system/tray/system_tray_delegate.h b/ash/system/tray/system_tray_delegate.h
@@ -205,9 +205,6 @@ class ASH_EXPORT SystemTrayDelegate {
   // Shows login UI to add other users to this session.
   virtual void ShowUserLogin() = 0;
 
-  // Attempts to shut down the system.
-  virtual void ShutDown() = 0;
-
   // Attempts to sign out the user.
   virtual void SignOut() = 0;
 

diff --git a/ash/test/test_system_tray_delegate.cc b/ash/test/test_system_tray_delegate.cc
@@ -94,10 +94,6 @@ bool TestSystemTrayDelegate::GetSessionLengthLimit(
   return session_length_limit_set_;
 }
 
-void TestSystemTrayDelegate::ShutDown() {
-  base::MessageLoop::current()->Quit();
-}
-
 void TestSystemTrayDelegate::SignOut() {
   base::MessageLoop::current()->Quit();
 }

diff --git a/ash/test/test_system_tray_delegate.h b/ash/test/test_system_tray_delegate.h
@@ -47,7 +47,6 @@ class TestSystemTrayDelegate : public DefaultSystemTrayDelegate {
   bool ShouldShowDisplayNotification() override;
   bool GetSessionStartTime(base::TimeTicks* session_start_time) override;
   bool GetSessionLengthLimit(base::TimeDelta* session_length_limit) override;
-  void ShutDown() override;
   void SignOut() override;
 
  private:

diff --git a/...hromeos/login/shutdown_policy_observer.cc → ...omeos/settings/shutdown_policy_handler.cc b/...hromeos/login/shutdown_policy_observer.cc → ...omeos/settings/shutdown_policy_handler.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.h"
+#include "chrome/browser/chromeos/settings/shutdown_policy_handler.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ -11,40 +11,40 @@
 
 namespace chromeos {
 
-ShutdownPolicyObserver::ShutdownPolicyObserver(CrosSettings* cros_settings,
-                                               Delegate* delegate)
+ShutdownPolicyHandler::ShutdownPolicyHandler(CrosSettings* cros_settings,
+                                             Delegate* delegate)
     : cros_settings_(cros_settings), delegate_(delegate), weak_factory_(this) {
   if (delegate_) {
     shutdown_policy_subscription_ = cros_settings_->AddSettingsObserver(
         kRebootOnShutdown,
-        base::Bind(&ShutdownPolicyObserver::OnShutdownPolicyChanged,
+        base::Bind(&ShutdownPolicyHandler::OnShutdownPolicyChanged,
                    weak_factory_.GetWeakPtr()));
   }
 }
 
-ShutdownPolicyObserver::~ShutdownPolicyObserver() {
+ShutdownPolicyHandler::~ShutdownPolicyHandler() {
 }
 
-void ShutdownPolicyObserver::Shutdown() {
+void ShutdownPolicyHandler::Shutdown() {
   shutdown_policy_subscription_.reset();
   delegate_ = nullptr;
 }
 
-void ShutdownPolicyObserver::CallDelegate(bool reboot_on_shutdown) {
+void ShutdownPolicyHandler::CallDelegate(bool reboot_on_shutdown) {
   if (delegate_)
     delegate_->OnShutdownPolicyChanged(reboot_on_shutdown);
 }
 
-void ShutdownPolicyObserver::OnShutdownPolicyChanged() {
-  CheckIfRebootOnShutdown(base::Bind(&ShutdownPolicyObserver::CallDelegate,
+void ShutdownPolicyHandler::OnShutdownPolicyChanged() {
+  CheckIfRebootOnShutdown(base::Bind(&ShutdownPolicyHandler::CallDelegate,
                           weak_factory_.GetWeakPtr()));
 }
 
-void ShutdownPolicyObserver::CheckIfRebootOnShutdown(
+void ShutdownPolicyHandler::CheckIfRebootOnShutdown(
     const RebootOnShutdownCallback& callback) {
   CrosSettingsProvider::TrustedStatus status =
       cros_settings_->PrepareTrustedValues(
-          base::Bind(&ShutdownPolicyObserver::CheckIfRebootOnShutdown,
+          base::Bind(&ShutdownPolicyHandler::CheckIfRebootOnShutdown,
                      weak_factory_.GetWeakPtr(), callback));
   if (status != CrosSettingsProvider::TRUSTED)
     return;

diff --git a/...chromeos/login/shutdown_policy_observer.h → ...romeos/settings/shutdown_policy_handler.h b/...chromeos/login/shutdown_policy_observer.h → ...romeos/settings/shutdown_policy_handler.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_UI_WEBUI_CHROMEOS_LOGIN_SHUTDOWN_POLICY_OBSERVER_H_
-#define CHROME_BROWSER_UI_WEBUI_CHROMEOS_LOGIN_SHUTDOWN_POLICY_OBSERVER_H_
+#ifndef CHROME_BROWSER_CHROMEOS_SETTINGS_SHUTDOWN_POLICY_HANDLER_H_
+#define CHROME_BROWSER_CHROMEOS_SETTINGS_SHUTDOWN_POLICY_HANDLER_H_
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
@@ -14,9 +14,9 @@
 namespace chromeos {
 
 // This class observes the device setting |DeviceRebootOnShutdown|. Changes to
-// this policy are communicated to the ShutdownPolicyObserver::Delegate by
+// this policy are communicated to the ShutdownPolicyHandler::Delegate by
 // calling its OnShutdownPolicyChanged method with the new state of the policy.
-class ShutdownPolicyObserver {
+class ShutdownPolicyHandler {
  public:
   // This callback is passed to CheckIfRebootOnShutdown, which invokes it with
   // the current state of the |DeviceRebootOnShutdown| policy once a trusted of
@@ -32,8 +32,8 @@ class ShutdownPolicyObserver {
     virtual ~Delegate() {}
   };
 
-  ShutdownPolicyObserver(CrosSettings* cros_settings, Delegate* delegate);
-  ~ShutdownPolicyObserver();
+  ShutdownPolicyHandler(CrosSettings* cros_settings, Delegate* delegate);
+  ~ShutdownPolicyHandler();
 
   // Once a trusted set of policies is established, this function calls
   // |callback| with the trusted state of the |DeviceRebootOnShutdown| policy.
@@ -53,11 +53,11 @@ class ShutdownPolicyObserver {
 
   scoped_ptr<CrosSettings::ObserverSubscription> shutdown_policy_subscription_;
 
-  base::WeakPtrFactory<ShutdownPolicyObserver> weak_factory_;
+  base::WeakPtrFactory<ShutdownPolicyHandler> weak_factory_;
 
-  DISALLOW_COPY_AND_ASSIGN(ShutdownPolicyObserver);
+  DISALLOW_COPY_AND_ASSIGN(ShutdownPolicyHandler);
 };
 
 }  // namespace chromeos
 
-#endif  // CHROME_BROWSER_UI_WEBUI_CHROMEOS_LOGIN_SHUTDOWN_POLICY_OBSERVER_H_
+#endif  // CHROME_BROWSER_CHROMEOS_SETTINGS_SHUTDOWN_POLICY_HANDLER_H_
diff --git a/...ogin/shutdown_policy_observer_unittest.cc → ...tings/shutdown_policy_handler_unittest.cc b/...ogin/shutdown_policy_observer_unittest.cc → ...tings/shutdown_policy_handler_unittest.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.h"
+#include "chrome/browser/chromeos/settings/shutdown_policy_handler.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -20,16 +20,16 @@
 
 namespace chromeos {
 
-class ShutdownPolicyObserverTest : public testing::Test,
-                                   public ShutdownPolicyObserver::Delegate {
+class ShutdownPolicyHandlerTest : public testing::Test,
+                                  public ShutdownPolicyHandler::Delegate {
  public:
   void ResultCallback(bool reboot_on_shutdown) {
     reboot_on_shutdown_ = reboot_on_shutdown;
     callback_called_ = true;
   }
 
  protected:
-  ShutdownPolicyObserverTest()
+  ShutdownPolicyHandlerTest()
       : cros_settings_(nullptr),
         callback_called_(false),
         reboot_on_shutdown_(false),
@@ -58,7 +58,7 @@ class ShutdownPolicyObserverTest : public testing::Test,
     base::RunLoop().RunUntilIdle();
   }
 
-  // ShutdownPolicyObserver::Delegate:
+  // ShutdownPolicyHandler::Delegate:
   void OnShutdownPolicyChanged(bool reboot_on_shutdown) override {
     reboot_on_shutdown_ = reboot_on_shutdown;
     delegate_invocations_count_++;
@@ -78,8 +78,8 @@ class ShutdownPolicyObserverTest : public testing::Test,
   int delegate_invocations_count_;
 };
 
-TEST_F(ShutdownPolicyObserverTest, RetrieveTrustedDevicePolicies) {
-  ShutdownPolicyObserver shutdown_policy_observer(cros_settings_, this);
+TEST_F(ShutdownPolicyHandlerTest, RetrieveTrustedDevicePolicies) {
+  ShutdownPolicyHandler shutdown_policy_observer(cros_settings_, this);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(0, delegate_invocations_count_);
 
@@ -101,15 +101,15 @@ TEST_F(ShutdownPolicyObserverTest, RetrieveTrustedDevicePolicies) {
   EXPECT_FALSE(reboot_on_shutdown_);
 }
 
-TEST_F(ShutdownPolicyObserverTest, CheckIfRebootOnShutdown) {
-  ShutdownPolicyObserver shutdown_policy_observer(cros_settings_, this);
+TEST_F(ShutdownPolicyHandlerTest, CheckIfRebootOnShutdown) {
+  ShutdownPolicyHandler shutdown_policy_observer(cros_settings_, this);
   base::RunLoop().RunUntilIdle();
 
   // Allow shutdown.
   SetRebootOnShutdown(true);
   callback_called_ = false;
   shutdown_policy_observer.CheckIfRebootOnShutdown(
-      base::Bind(&ShutdownPolicyObserverTest::ResultCallback,
+      base::Bind(&ShutdownPolicyHandlerTest::ResultCallback,
                  base::Unretained(this)));
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(callback_called_);
@@ -118,7 +118,7 @@ TEST_F(ShutdownPolicyObserverTest, CheckIfRebootOnShutdown) {
   SetRebootOnShutdown(false);
   callback_called_ = false;
   shutdown_policy_observer.CheckIfRebootOnShutdown(
-      base::Bind(&ShutdownPolicyObserverTest::ResultCallback,
+      base::Bind(&ShutdownPolicyHandlerTest::ResultCallback,
                  base::Unretained(this)));
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(callback_called_);

diff --git a/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc b/chrome/browser/ui/ash/system_tray_delegate_chromeos.cc
@@ -596,11 +596,6 @@ void SystemTrayDelegateChromeOS::ShowUserLogin() {
   }
 }
 
-void SystemTrayDelegateChromeOS::ShutDown() {
-  ash::Shell::GetInstance()->lock_state_controller()->RequestShutdown(
-      ash::LockStateController::POWER_OFF);
-}
-
 void SystemTrayDelegateChromeOS::SignOut() {
   chrome::AttemptUserExit();
 }

diff --git a/chrome/browser/ui/ash/system_tray_delegate_chromeos.h b/chrome/browser/ui/ash/system_tray_delegate_chromeos.h
@@ -98,7 +98,6 @@ class SystemTrayDelegateChromeOS
   void ShowSupervisedUserInfo() override;
   void ShowEnterpriseInfo() override;
   void ShowUserLogin() override;
-  void ShutDown() override;
   void SignOut() override;
   void RequestLockScreen() override;
   void RequestRestartForUpdate() override;

diff --git a/chrome/browser/ui/ash/system_tray_delegate_common.cc b/chrome/browser/ui/ash/system_tray_delegate_common.cc
@@ -156,9 +156,6 @@ void SystemTrayDelegateCommon::ShowEnterpriseInfo() {
 void SystemTrayDelegateCommon::ShowUserLogin() {
 }
 
-void SystemTrayDelegateCommon::ShutDown() {
-}
-
 void SystemTrayDelegateCommon::SignOut() {
 }
 

diff --git a/chrome/browser/ui/ash/system_tray_delegate_common.h b/chrome/browser/ui/ash/system_tray_delegate_common.h
@@ -59,7 +59,6 @@ class SystemTrayDelegateCommon : public ash::SystemTrayDelegate,
   void ShowSupervisedUserInfo() override;
   void ShowEnterpriseInfo() override;
   void ShowUserLogin() override;
-  void ShutDown() override;
   void SignOut() override;
   void RequestLockScreen() override;
   void RequestRestartForUpdate() override;

diff --git a/chrome/chrome_browser_chromeos.gypi b/chrome/chrome_browser_chromeos.gypi
@@ -873,6 +873,8 @@
         'browser/chromeos/settings/session_manager_operation.h',
         'browser/chromeos/settings/stub_cros_settings_provider.cc',
         'browser/chromeos/settings/stub_cros_settings_provider.h',
+        'browser/chromeos/settings/shutdown_policy_handler.cc',
+        'browser/chromeos/settings/shutdown_policy_handler.h',
         'browser/chromeos/settings/system_settings_provider.cc',
         'browser/chromeos/settings/system_settings_provider.h',
         'browser/chromeos/settings/token_encryptor.cc',

diff --git a/chrome/chrome_browser_ui.gypi b/chrome/chrome_browser_ui.gypi
@@ -963,8 +963,6 @@
       'browser/ui/webui/chromeos/login/screenlock_icon_provider.h',
       'browser/ui/webui/chromeos/login/screenlock_icon_source.cc',
       'browser/ui/webui/chromeos/login/screenlock_icon_source.h',
-      'browser/ui/webui/chromeos/login/shutdown_policy_observer.cc',
-      'browser/ui/webui/chromeos/login/shutdown_policy_observer.h',
       'browser/ui/webui/chromeos/login/signin_screen_handler.cc',
       'browser/ui/webui/chromeos/login/signin_screen_handler.h',
       'browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc',

diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
@@ -244,6 +244,7 @@
       'browser/chromeos/settings/device_settings_provider_unittest.cc',
       'browser/chromeos/settings/device_settings_service_unittest.cc',
       'browser/chromeos/settings/session_manager_operation_unittest.cc',
+      'browser/chromeos/settings/shutdown_policy_handler_unittest.cc',
       'browser/chromeos/settings/stub_cros_settings_provider_unittest.cc',
       'browser/chromeos/system/automatic_reboot_manager_unittest.cc',
       'browser/chromeos/system/device_disabling_manager_unittest.cc',
@@ -1233,7 +1234,6 @@
       'browser/ui/webui/chromeos/login/l10n_util_test_util.cc',
       'browser/ui/webui/chromeos/login/l10n_util_test_util.h',
       'browser/ui/webui/chromeos/login/l10n_util_unittest.cc',
-      'browser/ui/webui/chromeos/login/shutdown_policy_observer_unittest.cc',
       'browser/ui/webui/chromeos/login/signin_userlist_unittest.cc',
       'browser/ui/webui/fileicon_source_unittest.cc',
       'browser/ui/webui/help/version_updater_chromeos_unittest.cc',
-Original file line number
+Diff line change
@@ Expand Up / @@ -155,9 +155,6 @@ void DefaultSystemTrayDelegate::ShowSupervisedUserInfo() { @@
     void DefaultSystemTrayDelegate::ShowUserLogin() {
     }
-    void DefaultSystemTrayDelegate::ShutDown() {
-    }
     void DefaultSystemTrayDelegate::SignOut() {
     }
@@ Expand Down @@