From 95c6bd1be7480704bb3eb16083bdc3d5f2959173 Mon Sep 17 00:00:00 2001 From: cschuet Date: Thu, 8 Jan 2015 03:32:17 -0800 Subject: [PATCH] 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} --- ash/system/date/date_default_view.cc | 4 +++- .../tray/default_system_tray_delegate.cc | 3 --- .../tray/default_system_tray_delegate.h | 1 - ash/system/tray/system_tray_delegate.h | 3 --- ash/test/test_system_tray_delegate.cc | 4 ---- ash/test/test_system_tray_delegate.h | 1 - .../settings/shutdown_policy_handler.cc} | 22 +++++++++---------- .../settings/shutdown_policy_handler.h} | 18 +++++++-------- .../shutdown_policy_handler_unittest.cc} | 22 +++++++++---------- .../ui/ash/system_tray_delegate_chromeos.cc | 5 ----- .../ui/ash/system_tray_delegate_chromeos.h | 1 - .../ui/ash/system_tray_delegate_common.cc | 3 --- .../ui/ash/system_tray_delegate_common.h | 1 - chrome/chrome_browser_chromeos.gypi | 2 ++ chrome/chrome_browser_ui.gypi | 2 -- chrome/chrome_tests_unit.gypi | 2 +- 16 files changed, 37 insertions(+), 57 deletions(-) rename chrome/browser/{ui/webui/chromeos/login/shutdown_policy_observer.cc => chromeos/settings/shutdown_policy_handler.cc} (64%) rename chrome/browser/{ui/webui/chromeos/login/shutdown_policy_observer.h => chromeos/settings/shutdown_policy_handler.h} (74%) rename chrome/browser/{ui/webui/chromeos/login/shutdown_policy_observer_unittest.cc => chromeos/settings/shutdown_policy_handler_unittest.cc} (83%) diff --git a/ash/system/date/date_default_view.cc b/ash/system/date/date_default_view.cc index 6b07e25e06177d..44cb9978d5b595 100644 --- 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 index e56f6424792f39..66f5d12e7e85fa 100644 --- 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 index 7a7a4ff20d371a..fede740d001a88 100644 --- 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 index b840e48fc80ec9..01b0de3557c36c 100644 --- 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 index 60fb4b31e1fc4f..19daea2b14f7ef 100644 --- 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 index e261588db9e93a..a8384ca51eaaf9 100644 --- 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/chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.cc b/chrome/browser/chromeos/settings/shutdown_policy_handler.cc similarity index 64% rename from chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.cc rename to chrome/browser/chromeos/settings/shutdown_policy_handler.cc index d63631c74a358f..cc1a322390857c 100644 --- a/chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.cc +++ b/chrome/browser/chromeos/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/chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.h b/chrome/browser/chromeos/settings/shutdown_policy_handler.h similarity index 74% rename from chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.h rename to chrome/browser/chromeos/settings/shutdown_policy_handler.h index c7a0881dfd49f2..2104b9aa1f605c 100644 --- a/chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer.h +++ b/chrome/browser/chromeos/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 shutdown_policy_subscription_; - base::WeakPtrFactory weak_factory_; + base::WeakPtrFactory 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/chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer_unittest.cc b/chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc similarity index 83% rename from chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer_unittest.cc rename to chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc index 4136f994f68276..56c7698b415725 100644 --- a/chrome/browser/ui/webui/chromeos/login/shutdown_policy_observer_unittest.cc +++ b/chrome/browser/chromeos/settings/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,8 +20,8 @@ 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; @@ -29,7 +29,7 @@ class ShutdownPolicyObserverTest : public testing::Test, } 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 index 11bddcfec7d982..a350eb38add81d 100644 --- 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 index 7a2ee9c3453637..469e59dfedc535 100644 --- 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 index dab00c65b47bf7..3b434e49b7008e 100644 --- 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 index 403d17ffd7829e..6888ee35b77c13 100644 --- 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 index 966a7451e77cba..73063a835db741 100644 --- 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 index 72d225bb10c4c6..3c72349ea8b937 100644 --- 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 index 80dcdb3d9e0174..c94be3171f2375 100644 --- 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',