shill: Use SuspendDone signal instead of PowerStateChanged. PowerStateChanged will be removed from powerd soon; clients should use SuspendImminent and SuspendDone instead. Also simplify the interface exposed by PowerManager for adding and removing suspend delays. BUG=chromium:359619 TEST=updated tests Change-Id: I7f96774570170591c2e5944245c368e165aaf972 Reviewed-on: https://chromium-review.googlesource.com/195690 Reviewed-by: Daniel Erat <derat@chromium.org> Tested-by: Daniel Erat <derat@chromium.org> Commit-Queue: Daniel Erat <derat@chromium.org>
diff --git a/dbus_bindings/power_manager.xml b/dbus_bindings/power_manager.xml index 18e0170..24d5a58 100644 --- a/dbus_bindings/power_manager.xml +++ b/dbus_bindings/power_manager.xml
@@ -17,8 +17,8 @@ <signal name="SuspendImminent"> <arg name="serialized_proto" type="ay"/> </signal> - <signal name="PowerStateChanged"> - <arg name="new_power_state" type="s"/> + <signal name="SuspendDone"> + <arg name="serialized_proto" type="ay"/> </signal> </interface> </node>
diff --git a/manager.cc b/manager.cc index 73ef057..92d94be 100644 --- a/manager.cc +++ b/manager.cc
@@ -121,7 +121,6 @@ use_startup_portal_list_(false), termination_actions_(dispatcher), suspend_delay_registered_(false), - suspend_delay_id_(0), default_service_callback_tag_(0), crypto_util_proxy_(new CryptoUtilProxy(dispatcher, glib)), health_checker_remote_ips_(new IPAddressStore()) { @@ -201,13 +200,6 @@ power_manager_.reset( new PowerManager(dispatcher_, ProxyFactory::GetInstance())); - power_manager_->AddStateChangeCallback( - kPowerManagerKey, - Bind(&Manager::OnPowerStateChanged, AsWeakPtr())); - // TODO(ers): weak ptr for metrics_? - PowerManager::PowerStateCallback cb = - Bind(&Metrics::NotifyPowerStateChange, Unretained(metrics_)); - power_manager_->AddStateChangeCallback(Metrics::kMetricPowerManagerKey, cb); CHECK(base::CreateDirectory(run_path_)) << run_path_.value(); resolver_->set_path(run_path_.Append("resolv.conf")); @@ -1092,16 +1084,15 @@ void Manager::AddTerminationAction(const string &name, const base::Closure &start) { - if (termination_actions_.IsEmpty() && power_manager_.get()) { - power_manager_->AddSuspendDelayCallback( + if (termination_actions_.IsEmpty() && power_manager_.get() && + !suspend_delay_registered_) { + suspend_delay_registered_ = power_manager_->AddSuspendDelay( kPowerManagerKey, - Bind(&Manager::OnSuspendImminent, AsWeakPtr())); - CHECK(!suspend_delay_registered_); - suspend_delay_registered_ = power_manager_->RegisterSuspendDelay( + kSuspendDelayDescription, base::TimeDelta::FromMilliseconds( kTerminationActionsTimeoutMilliseconds), - kSuspendDelayDescription, - &suspend_delay_id_); + Bind(&Manager::OnSuspendImminent, AsWeakPtr()), + Bind(&Manager::OnSuspendDone, AsWeakPtr())); } termination_actions_.Add(name, start); } @@ -1117,14 +1108,12 @@ return; } termination_actions_.Remove(name); - if (termination_actions_.IsEmpty() && power_manager_.get()) { - if (suspend_delay_registered_) { - SLOG(Manager, 2) << "Unregistering suspend delay."; - power_manager_->UnregisterSuspendDelay(suspend_delay_id_); + if (termination_actions_.IsEmpty() && power_manager_.get() && + suspend_delay_registered_) { + SLOG(Manager, 2) << "Unregistering suspend delay."; + if (power_manager_->RemoveSuspendDelay(kPowerManagerKey)) { suspend_delay_registered_ = false; - suspend_delay_id_ = 0; } - power_manager_->RemoveSuspendDelayCallback(kPowerManagerKey); } } @@ -1286,32 +1275,29 @@ } } -void Manager::OnPowerStateChanged( - PowerManagerProxyDelegate::SuspendState power_state) { - if (power_state == PowerManagerProxyDelegate::kOn) { - vector<ServiceRefPtr>::iterator sit; - for (sit = services_.begin(); sit != services_.end(); ++sit) { - (*sit)->OnAfterResume(); - } - SortServices(); - vector<DeviceRefPtr>::iterator it; - for (it = devices_.begin(); it != devices_.end(); ++it) { - (*it)->OnAfterResume(); - } - } else if (power_state == PowerManagerProxyDelegate::kMem) { - vector<DeviceRefPtr>::iterator it; - for (it = devices_.begin(); it != devices_.end(); ++it) { - (*it)->OnBeforeSuspend(); - } +void Manager::OnSuspendImminent(int suspend_id) { + vector<DeviceRefPtr>::iterator it; + for (it = devices_.begin(); it != devices_.end(); ++it) { + (*it)->OnBeforeSuspend(); + } + if (!RunTerminationActionsAndNotifyMetrics( + Bind(&Manager::OnSuspendActionsComplete, AsWeakPtr(), suspend_id), + Metrics::kTerminationActionReasonSuspend)) { + LOG(INFO) << "No asynchronous suspend actions were run."; + power_manager_->ReportSuspendReadiness(kPowerManagerKey, suspend_id); } } -void Manager::OnSuspendImminent(int suspend_id) { - if (!RunTerminationActionsAndNotifyMetrics( - Bind(&Manager::OnSuspendActionsComplete, AsWeakPtr(), suspend_id), - Metrics::kTerminationActionReasonSuspend)) { - LOG(INFO) << "No suspend actions were run."; - power_manager_->ReportSuspendReadiness(suspend_delay_id_, suspend_id); +void Manager::OnSuspendDone(int suspend_id) { + metrics_->NotifySuspendDone(); + vector<ServiceRefPtr>::iterator sit; + for (sit = services_.begin(); sit != services_.end(); ++sit) { + (*sit)->OnAfterResume(); + } + SortServices(); + vector<DeviceRefPtr>::iterator it; + for (it = devices_.begin(); it != devices_.end(); ++it) { + (*it)->OnAfterResume(); } } @@ -1319,7 +1305,7 @@ LOG(INFO) << "Finished suspend actions. Result: " << error; metrics_->NotifyTerminationActionsCompleted( Metrics::kTerminationActionReasonSuspend, error.IsSuccess()); - power_manager_->ReportSuspendReadiness(suspend_delay_id_, suspend_id); + power_manager_->ReportSuspendReadiness(kPowerManagerKey, suspend_id); } void Manager::FilterByTechnology(Technology::Identifier tech, @@ -1463,10 +1449,8 @@ LOG(INFO) << "Auto-connect suppressed -- not running."; return; } - if (power_manager_.get() && - power_manager_->power_state() != PowerManagerProxyDelegate::kOn && - power_manager_->power_state() != PowerManagerProxyDelegate::kUnknown) { - LOG(INFO) << "Auto-connect suppressed -- power state is not 'on'."; + if (power_manager_ && power_manager_->suspending()) { + LOG(INFO) << "Auto-connect suppressed -- system is suspending."; return; } if (services_.empty()) {
diff --git a/manager.h b/manager.h index 2f969b1..4e5ba64 100644 --- a/manager.h +++ b/manager.h
@@ -291,8 +291,6 @@ bool GetArpGateway() const { return props_.arp_gateway; } const std::string &GetHostName() const { return props_.host_name; } - int suspend_delay_id_for_testing() const { return suspend_delay_id_; } - virtual void UpdateEnabledTechnologies(); virtual void UpdateUninitializedTechnologies(); @@ -525,9 +523,14 @@ // Error::kSuccess. Otherwise, it is called with Error::kOperationTimeout. void RunTerminationActions(const base::Callback<void(const Error &)> &done); - void OnPowerStateChanged(PowerManagerProxyDelegate::SuspendState power_state); + // Called when the system is about to be suspended. Each call will be + // followed by a call to OnSuspendDone(). void OnSuspendImminent(int suspend_id); + // Called when the system has completed a suspend attempt (possibly without + // actually suspending, in the event of the user canceling the attempt). + void OnSuspendDone(int suspend_id); + void OnSuspendActionsComplete(int suspend_id, const Error &error); void VerifyToEncryptLink(std::string public_key, std::string data, ResultStringCallback cb, const Error &error, @@ -613,10 +616,6 @@ // Is a suspend delay currently registered with the power manager? bool suspend_delay_registered_; - // If |suspend_delay_registered_| is true, contains the unique ID - // corresponding to the suspend delay. - int suspend_delay_id_; - // Maps tags to callbacks for monitoring default service changes. std::map<int, ServiceCallback> default_service_callbacks_; int default_service_callback_tag_;
diff --git a/manager_unittest.cc b/manager_unittest.cc index fb44c26..335be36 100644 --- a/manager_unittest.cc +++ b/manager_unittest.cc
@@ -360,8 +360,8 @@ DISALLOW_COPY_AND_ASSIGN(DisableTechnologyReplyHandler); }; - void SetPowerState(PowerManagerProxyDelegate::SuspendState state) { - power_manager_->power_state_ = state; + void SetSuspending(bool suspending) { + power_manager_->suspending_ = suspending; } void SetPowerManager() { @@ -372,14 +372,14 @@ return &manager()->termination_actions_; } - void OnPowerStateChanged(PowerManagerProxyDelegate::SuspendState state) { - manager()->OnPowerStateChanged(state); - } - void OnSuspendImminent(int suspend_id) { manager()->OnSuspendImminent(suspend_id); } + void OnSuspendDone(int suspend_id) { + manager()->OnSuspendDone(suspend_id); + } + void OnSuspendActionsComplete(int suspend_id, const Error &error) { manager()->OnSuspendActionsComplete(suspend_id, error); } @@ -2970,36 +2970,18 @@ dispatcher()->DispatchPendingEvents(); } -TEST_F(ManagerTest, AutoConnectOnPowerStateSuspending) { +TEST_F(ManagerTest, AutoConnectOnSuspending) { MockServiceRefPtr service = MakeAutoConnectableService(); - SetPowerState(PowerManagerProxyDelegate::kSuspending); + SetSuspending(true); SetPowerManager(); EXPECT_CALL(*service, AutoConnect()).Times(0); manager()->RegisterService(service); dispatcher()->DispatchPendingEvents(); } -TEST_F(ManagerTest, AutoConnectOnPowerStateMem) { +TEST_F(ManagerTest, AutoConnectOnNotSuspending) { MockServiceRefPtr service = MakeAutoConnectableService(); - SetPowerState(PowerManagerProxyDelegate::kMem); - SetPowerManager(); - EXPECT_CALL(*service, AutoConnect()).Times(0); - manager()->RegisterService(service); - dispatcher()->DispatchPendingEvents(); -} - -TEST_F(ManagerTest, AutoConnectOnPowerStateOn) { - MockServiceRefPtr service = MakeAutoConnectableService(); - SetPowerState(PowerManagerProxyDelegate::kOn); - SetPowerManager(); - EXPECT_CALL(*service, AutoConnect()); - manager()->RegisterService(service); - dispatcher()->DispatchPendingEvents(); -} - -TEST_F(ManagerTest, AutoConnectOnPowerStateUnknown) { - MockServiceRefPtr service = MakeAutoConnectableService(); - SetPowerState(PowerManagerProxyDelegate::kUnknown); + SetSuspending(false); SetPowerManager(); EXPECT_CALL(*service, AutoConnect()); manager()->RegisterService(service); @@ -3014,31 +2996,30 @@ dispatcher()->DispatchPendingEvents(); } -TEST_F(ManagerTest, OnPowerStateChanged) { +TEST_F(ManagerTest, Suspend) { MockServiceRefPtr service = MakeAutoConnectableService(); - SetPowerState(PowerManagerProxyDelegate::kOn); SetPowerManager(); EXPECT_CALL(*service, AutoConnect()); manager()->RegisterService(service); manager()->RegisterDevice(mock_devices_[0]); dispatcher()->DispatchPendingEvents(); - EXPECT_CALL(*mock_devices_[0], OnAfterResume()); - OnPowerStateChanged(PowerManagerProxyDelegate::kOn); - EXPECT_CALL(*service, AutoConnect()); + const int kSuspendId = 1; + EXPECT_CALL(*mock_devices_[0], OnBeforeSuspend()); + OnSuspendImminent(kSuspendId); + EXPECT_CALL(*service, AutoConnect()).Times(0); dispatcher()->DispatchPendingEvents(); Mock::VerifyAndClearExpectations(mock_devices_[0]); - EXPECT_CALL(*mock_devices_[0], OnBeforeSuspend()); - OnPowerStateChanged(PowerManagerProxyDelegate::kMem); - EXPECT_CALL(*service, AutoConnect()).Times(0); + EXPECT_CALL(*mock_devices_[0], OnAfterResume()); + OnSuspendDone(kSuspendId); + EXPECT_CALL(*service, AutoConnect()); dispatcher()->DispatchPendingEvents(); Mock::VerifyAndClearExpectations(mock_devices_[0]); } TEST_F(ManagerTest, AddTerminationAction) { - EXPECT_CALL(*power_manager_, AddSuspendDelayCallback(_, _)); - EXPECT_CALL(*power_manager_, RegisterSuspendDelay(_, _, _)); + EXPECT_CALL(*power_manager_, AddSuspendDelay(_, _, _, _, _)); SetPowerManager(); EXPECT_TRUE(GetTerminationActions()->IsEmpty()); manager()->AddTerminationAction("action1", base::Closure()); @@ -3049,39 +3030,35 @@ TEST_F(ManagerTest, RemoveTerminationAction) { const char kKey1[] = "action1"; const char kKey2[] = "action2"; - const int kSuspendDelayId = 123; MockPowerManager &power_manager = *power_manager_; SetPowerManager(); // Removing an action when the hook table is empty should not result in any // calls to the power manager. - EXPECT_CALL(power_manager, UnregisterSuspendDelay(_)).Times(0); - EXPECT_CALL(power_manager, RemoveSuspendDelayCallback(_)).Times(0); + EXPECT_CALL(power_manager, RemoveSuspendDelay(_)).Times(0); EXPECT_TRUE(GetTerminationActions()->IsEmpty()); manager()->RemoveTerminationAction("unknown"); Mock::VerifyAndClearExpectations(&power_manager); - EXPECT_CALL(power_manager, RegisterSuspendDelay(_, _, _)) - .WillOnce(DoAll(SetArgumentPointee<2>(kSuspendDelayId), Return(true))); - EXPECT_CALL(power_manager, AddSuspendDelayCallback(_, _)).Times(1); + EXPECT_CALL(power_manager, AddSuspendDelay(_, _, _, _, _)) + .WillOnce(Return(true)); manager()->AddTerminationAction(kKey1, base::Closure()); EXPECT_FALSE(GetTerminationActions()->IsEmpty()); manager()->AddTerminationAction(kKey2, base::Closure()); + Mock::VerifyAndClearExpectations(&power_manager); // Removing an action that ends up with a non-empty hook table should not // result in any calls to the power manager. - EXPECT_CALL(power_manager, UnregisterSuspendDelay(_)).Times(0); - EXPECT_CALL(power_manager, RemoveSuspendDelayCallback(_)).Times(0); + EXPECT_CALL(power_manager, RemoveSuspendDelay(_)).Times(0); manager()->RemoveTerminationAction(kKey1); EXPECT_FALSE(GetTerminationActions()->IsEmpty()); Mock::VerifyAndClearExpectations(&power_manager); // Removing the last action should trigger unregistering from the power // manager. - EXPECT_CALL(power_manager, UnregisterSuspendDelay(kSuspendDelayId)) + EXPECT_CALL(power_manager, RemoveSuspendDelay(_)) .WillOnce(Return(true)); - EXPECT_CALL(power_manager, RemoveSuspendDelayCallback(_)); manager()->RemoveTerminationAction(kKey2); EXPECT_TRUE(GetTerminationActions()->IsEmpty()); } @@ -3106,9 +3083,7 @@ TEST_F(ManagerTest, OnSuspendImminent) { const int kSuspendId = 123; EXPECT_TRUE(GetTerminationActions()->IsEmpty()); - EXPECT_CALL(*power_manager_, - ReportSuspendReadiness( - manager()->suspend_delay_id_for_testing(), kSuspendId)); + EXPECT_CALL(*power_manager_, ReportSuspendReadiness(_, kSuspendId)); SetPowerManager(); OnSuspendImminent(kSuspendId); } @@ -3116,9 +3091,7 @@ TEST_F(ManagerTest, OnSuspendActionsComplete) { const int kSuspendId = 54321; Error error; - EXPECT_CALL(*power_manager_, - ReportSuspendReadiness( - manager()->suspend_delay_id_for_testing(), kSuspendId)); + EXPECT_CALL(*power_manager_, ReportSuspendReadiness(_, kSuspendId)); SetPowerManager(); OnSuspendActionsComplete(kSuspendId, error); }
diff --git a/metrics.cc b/metrics.cc index 78fec18..90c1e90 100644 --- a/metrics.cc +++ b/metrics.cc
@@ -647,12 +647,8 @@ kMetricSignalAtDisconnectNumBuckets); } -void Metrics::NotifyPowerStateChange(PowerManager::SuspendState new_state) { - if (new_state == PowerManagerProxyDelegate::kOn) { - time_resume_to_ready_timer_->Start(); - } else { - time_resume_to_ready_timer_->Reset(); - } +void Metrics::NotifySuspendDone() { + time_resume_to_ready_timer_->Start(); } void Metrics::NotifyTerminationActionsStarted(
diff --git a/metrics.h b/metrics.h index ba1d36c..f0f9459 100644 --- a/metrics.h +++ b/metrics.h
@@ -518,8 +518,8 @@ void NotifySignalAtDisconnect(const Service &service, int16_t signal_strength); - // Notifies this object of a power management state change. - void NotifyPowerStateChange(PowerManager::SuspendState new_state); + // Notifies this object of the end of a suspend attempt. + void NotifySuspendDone(); // Notifies this object that termination actions started executing. void NotifyTerminationActionsStarted(TerminationActionReason reason);
diff --git a/metrics_unittest.cc b/metrics_unittest.cc index 1ff9ed4..0b4f927 100644 --- a/metrics_unittest.cc +++ b/metrics_unittest.cc
@@ -209,8 +209,7 @@ Metrics::kTimerHistogramNumBuckets)); EXPECT_CALL(*mock_time_resume_to_ready_timer, GetElapsedTime(_)). WillOnce(DoAll(SetArgumentPointee<0>(non_zero_time_delta), Return(true))); - metrics_.NotifyPowerStateChange(PowerManagerProxyDelegate::kMem); - metrics_.NotifyPowerStateChange(PowerManagerProxyDelegate::kOn); + metrics_.NotifySuspendDone(); metrics_.NotifyServiceStateChanged(*wep_wifi_service_, Service::kStateConnected); Mock::VerifyAndClearExpectations(&library_);
diff --git a/mock_power_manager.h b/mock_power_manager.h index 34aa670..f2e272a 100644 --- a/mock_power_manager.h +++ b/mock_power_manager.h
@@ -19,20 +19,15 @@ MockPowerManager(EventDispatcher *dispatcher, ProxyFactory *proxy_factory); virtual ~MockPowerManager(); - MOCK_METHOD2(AddStateChangeCallback, - void(const std::string &key, - const PowerStateCallback &callback)); - MOCK_METHOD2(AddSuspendDelayCallback, - void(const std::string &key, - const SuspendDelayCallback &callback)); - MOCK_METHOD1(RemoveStateChangeCallback, void(const std::string &key)); - MOCK_METHOD1(RemoveSuspendDelayCallback, void(const std::string &key)); - MOCK_METHOD3(RegisterSuspendDelay, - bool(base::TimeDelta timeout, + MOCK_METHOD5(AddSuspendDelay, + bool(const std::string &key, const std::string &description, - int *delay_id_out)); - MOCK_METHOD1(UnregisterSuspendDelay, bool(int delay_id)); - MOCK_METHOD2(ReportSuspendReadiness, bool(int delay_id, int suspend_id)); + base::TimeDelta timeout, + const SuspendImminentCallback &immiment_callback, + const SuspendDoneCallback &done_callback)); + MOCK_METHOD1(RemoveSuspendDelay, bool(const std::string &key)); + MOCK_METHOD2(ReportSuspendReadiness, + bool(const std::string &key, int suspend_id)); private: DISALLOW_COPY_AND_ASSIGN(MockPowerManager);
diff --git a/power_manager.cc b/power_manager.cc index beb6343..cae3e41 100644 --- a/power_manager.cc +++ b/power_manager.cc
@@ -25,31 +25,63 @@ ProxyFactory *proxy_factory) : dispatcher_(dispatcher), power_manager_proxy_(proxy_factory->CreatePowerManagerProxy(this)), - power_state_(kUnknown) {} + suspending_(false) {} PowerManager::~PowerManager() { } -void PowerManager::AddStateChangeCallback(const string &key, - const PowerStateCallback &callback) { - SLOG(Power, 2) << __func__ << " key " << key; - AddCallback(key, callback, &state_change_callbacks_); +bool PowerManager::AddSuspendDelay( + const std::string &key, + const std::string &description, + base::TimeDelta timeout, + const SuspendImminentCallback &imminent_callback, + const SuspendDoneCallback &done_callback) { + CHECK(!imminent_callback.is_null()); + CHECK(!done_callback.is_null()); + + if (ContainsKey(suspend_delays_, key)) { + LOG(ERROR) << "Ignoring request to insert duplicate key " << key; + return false; + } + + int delay_id = 0; + if (!power_manager_proxy_->RegisterSuspendDelay( + timeout, description, &delay_id)) { + return false; + } + + SuspendDelay delay; + delay.imminent_callback = imminent_callback; + delay.done_callback = done_callback; + delay.delay_id = delay_id; + suspend_delays_[key] = delay; + return true; } -void PowerManager::AddSuspendDelayCallback( - const string &key, const SuspendDelayCallback &callback) { - SLOG(Power, 2) << __func__ << " key " << key; - AddCallback(key, callback, &suspend_delay_callbacks_); +bool PowerManager::RemoveSuspendDelay(const std::string &key) { + SuspendDelayMap::const_iterator it = suspend_delays_.find(key); + if (it == suspend_delays_.end()) { + LOG(ERROR) << "Ignoring unregistered key " << key; + return false; + } + + if (!power_manager_proxy_->UnregisterSuspendDelay(it->second.delay_id)) { + return false; + } + + suspend_delays_.erase(it); + return true; } -void PowerManager::RemoveStateChangeCallback(const string &key) { - SLOG(Power, 2) << __func__ << " key " << key; - RemoveCallback(key, &state_change_callbacks_); -} - -void PowerManager::RemoveSuspendDelayCallback(const string &key) { - SLOG(Power, 2) << __func__ << " key " << key; - RemoveCallback(key, &suspend_delay_callbacks_); +bool PowerManager::ReportSuspendReadiness(const std::string &key, + int suspend_id) { + SuspendDelayMap::const_iterator it = suspend_delays_.find(key); + if (it == suspend_delays_.end()) { + LOG(ERROR) << "Ignoring unregistered key " << key; + return false; + } + return power_manager_proxy_->ReportSuspendReadiness( + it->second.delay_id, suspend_id); } void PowerManager::OnSuspendImminent(int suspend_id) { @@ -58,78 +90,36 @@ // that the manager can suppress auto-connect, for example. Schedules a // suspend timeout in case the suspend attempt failed or got interrupted, and // there's no proper notification from the power manager. - power_state_ = kSuspending; + suspending_ = true; suspend_timeout_.Reset( - base::Bind(&PowerManager::OnSuspendTimeout, base::Unretained(this))); + base::Bind(&PowerManager::OnSuspendTimeout, base::Unretained(this), + suspend_id)); dispatcher_->PostDelayedTask(suspend_timeout_.callback(), kSuspendTimeoutMilliseconds); - OnEvent(suspend_id, &suspend_delay_callbacks_); + + for (SuspendDelayMap::const_iterator it = suspend_delays_.begin(); + it != suspend_delays_.end(); ++it) { + SuspendImminentCallback callback = it->second.imminent_callback; + CHECK(!callback.is_null()); + callback.Run(suspend_id); + } } -void PowerManager::OnPowerStateChanged(SuspendState new_power_state) { - LOG(INFO) << "Power state changed: " - << power_state_ << "->" << new_power_state; +void PowerManager::OnSuspendDone(int suspend_id) { + LOG(INFO) << __func__ << "(" << suspend_id << ")"; suspend_timeout_.Cancel(); - power_state_ = new_power_state; - OnEvent(new_power_state, &state_change_callbacks_); + suspending_ = false; + for (SuspendDelayMap::const_iterator it = suspend_delays_.begin(); + it != suspend_delays_.end(); ++it) { + SuspendDoneCallback callback = it->second.done_callback; + CHECK(!callback.is_null()); + callback.Run(suspend_id); + } } -bool PowerManager::RegisterSuspendDelay(base::TimeDelta timeout, - const string &description, - int *delay_id_out) { - return power_manager_proxy_->RegisterSuspendDelay(timeout, description, - delay_id_out); -} - -bool PowerManager::UnregisterSuspendDelay(int delay_id) { - return power_manager_proxy_->UnregisterSuspendDelay(delay_id); -} - -bool PowerManager::ReportSuspendReadiness(int delay_id, int suspend_id) { - return power_manager_proxy_->ReportSuspendReadiness(delay_id, suspend_id); -} - -void PowerManager::OnSuspendTimeout() { +void PowerManager::OnSuspendTimeout(int suspend_id) { LOG(ERROR) << "Suspend timed out -- assuming power-on state."; - OnPowerStateChanged(kOn); + OnSuspendDone(suspend_id); } -template<class Callback> -void PowerManager::AddCallback(const string &key, const Callback &callback, - map<const string, Callback> *callback_map) { - CHECK(callback_map != NULL); - CHECK(!callback.is_null()); - if (ContainsKey(*callback_map, key)) { - LOG(DFATAL) << "Inserting duplicate key " << key; - LOG(INFO) << "Removing previous callback for key " << key; - RemoveCallback(key, callback_map); - } - (*callback_map)[key] = callback; -} - -template<class Callback> -void PowerManager::RemoveCallback(const string &key, - map<const string, Callback> *callback_map) { - CHECK(callback_map != NULL); - DCHECK(ContainsKey(*callback_map, key)) << "Removing unknown key " << key; - typename map<const string, Callback>::iterator it = callback_map->find(key); - if (it != callback_map->end()) { - callback_map->erase(it); - } -} - -template<class Param, class Callback> -void PowerManager::OnEvent(const Param ¶m, - map<const string, Callback> *callback_map) const { - CHECK(callback_map != NULL); - for (typename map<const string, Callback>::const_iterator it = - callback_map->begin(); - it != callback_map->end(); ++it) { - CHECK(!it->second.is_null()); - it->second.Run(param); - } -} - - - } // namespace shill
diff --git a/power_manager.h b/power_manager.h index 42ca2ea..e28500c 100644 --- a/power_manager.h +++ b/power_manager.h
@@ -8,27 +8,6 @@ // This class instantiates a PowerManagerProxy and distributes power events to // registered users. It also provides a means for calling methods on the // PowerManagerProxy. -// -// Usage: -// -// Registering for power state changes is done as follows: -// -// class Foo { -// public: -// void HandleStateChange(PowerManager::SuspendState new_state); -// }; -// Foo foo; -// PowerManager power_manager(ProxyFactory::GetInstance()); -// PowerManager::PowerStateCallback cb = Bind(&Foo::HandleStateChange, &foo); -// power_manager.AddStateChangeCallback("foo_key", cb); -// -// Note that depending on the definition of Foo, "&foo" may need to appear -// inside an appropriate wrapper, such as base::Unretained. -// -// Whenever the power state changes, foo.HandleStateChange() is called with the -// new state passed in. To unregister: -// -// power_manager.RemoveStateChangeCallback("foo_key"); #include <map> #include <string> @@ -46,19 +25,16 @@ class PowerManager : public PowerManagerProxyDelegate { public: - typedef PowerManagerProxyDelegate::SuspendState SuspendState; - - // Callbacks registered with the power manager are of this type. They take - // one argument, the new power state. The callback function or method should - // look like this: - // - // void HandlePowerStateChange(PowerStateCallbacks::SuspendState); - typedef base::Callback<void(SuspendState)> PowerStateCallback; - - // This callback is called prior to a suspend event. When it is OK for the + // This callback is called prior to a suspend attempt. When it is OK for the // system to suspend, this callback should call ReportSuspendReadiness(), - // passing it the suspend ID passed to this callback. - typedef base::Callback<void(int)> SuspendDelayCallback; + // passing it the key passed to AddSuspendDelay() and the suspend ID passed to + // this callback. + typedef base::Callback<void(int)> SuspendImminentCallback; + + // This callback is called after the completion of a suspend attempt. The + // receiver should undo any pre-suspend work that was done by the + // SuspendImminentCallback. + typedef base::Callback<void(int)> SuspendDoneCallback; static const int kSuspendTimeoutMilliseconds; @@ -68,74 +44,61 @@ ProxyFactory *proxy_factory); virtual ~PowerManager(); - SuspendState power_state() const { return power_state_; } + bool suspending() const { return suspending_; } + + // Registers a suspend delay with the power manager under the unique name + // |key|. See PowerManagerProxyInterface::RegisterSuspendDelay() for + // information about |description| and |timeout|. |imminent_callback| will be + // invoked when a suspend attempt is commenced and |done_callback| will be + // invoked when the attempt is completed. Returns false on failure. + virtual bool AddSuspendDelay(const std::string &key, + const std::string &description, + base::TimeDelta timeout, + const SuspendImminentCallback &immiment_callback, + const SuspendDoneCallback &done_callback); + + // Removes a suspend delay previously created via AddSuspendDelay(). Returns + // false on failure. + virtual bool RemoveSuspendDelay(const std::string &key); + + // Reports readiness for suspend attempt |suspend_id| on behalf of the suspend + // delay described by |key|, the value originally passed to AddSuspendDelay(). + // Returns false on failure. + virtual bool ReportSuspendReadiness(const std::string &key, int suspend_id); // Methods inherited from PowerManagerProxyDelegate. virtual void OnSuspendImminent(int suspend_id); - virtual void OnPowerStateChanged(SuspendState new_power_state); - - // See corresponding methods in PowerManagerProxyInterface. - virtual bool RegisterSuspendDelay(base::TimeDelta timeout, - const std::string &description, - int *delay_id_out); - virtual bool UnregisterSuspendDelay(int delay_id); - virtual bool ReportSuspendReadiness(int delay_id, int suspend_id); - - // Registers a power state change callback with the power manager. When a - // power state change occurs, this callback will be called with the new power - // state as its argument. |key| must be unique. - virtual void AddStateChangeCallback(const std::string &key, - const PowerStateCallback &callback); - - // Registers a suspend delay callback with the power manager. This callback - // will be called prior to a suspend event by the amount specified in the most - // recent call to RegisterSuspendDelay(). |key| must be unique. - virtual void AddSuspendDelayCallback(const std::string &key, - const SuspendDelayCallback &callback); - - // Unregisters a power state change callback identified by |key|. The - // callback must have been previously registered and not yet removed. - virtual void RemoveStateChangeCallback(const std::string &key); - - // Unregisters a suspend delay callback identified by |key|. The callback - // must have been previously registered and not yet removed. - virtual void RemoveSuspendDelayCallback(const std::string &key); + virtual void OnSuspendDone(int suspend_id); private: friend class ManagerTest; friend class PowerManagerTest; friend class ServiceTest; - typedef std::map<const std::string, PowerStateCallback> - StateChangeCallbackMap; + // Information about a suspend delay added via AddSuspendDelay(). + struct SuspendDelay { + SuspendImminentCallback imminent_callback; + SuspendDoneCallback done_callback; + int delay_id; + }; - typedef std::map<const std::string, SuspendDelayCallback> - SuspendDelayCallbackMap; + // Keyed by the |key| argument to AddSuspendDelay(). + typedef std::map<const std::string, SuspendDelay> SuspendDelayMap; - void OnSuspendTimeout(); - - template<class Callback> - void AddCallback(const std::string &key, const Callback &callback, - std::map<const std::string, Callback> *callback_map); - - template<class Callback> - void RemoveCallback(const std::string &key, - std::map<const std::string, Callback> *callback_map); - - template<class Param, class Callback> - void OnEvent(const Param ¶m, - std::map<const std::string, Callback> *callback_map) const; + // Run by |suspend_timeout_| to manually invoke OnSuspendDone() if the signal + // never arrives. + void OnSuspendTimeout(int suspend_id); EventDispatcher *dispatcher_; // The power manager proxy created by this class. It dispatches the inherited // delegate methods of this object when changes in the power state occur. const scoped_ptr<PowerManagerProxyInterface> power_manager_proxy_; - StateChangeCallbackMap state_change_callbacks_; - SuspendDelayCallbackMap suspend_delay_callbacks_; + SuspendDelayMap suspend_delays_; base::CancelableClosure suspend_timeout_; - SuspendState power_state_; + // Set to true by OnSuspendImminent() and to false by OnSuspendDone(). + bool suspending_; DISALLOW_COPY_AND_ASSIGN(PowerManager); };
diff --git a/power_manager_proxy.cc b/power_manager_proxy.cc index 4823171..e5b95c7 100644 --- a/power_manager_proxy.cc +++ b/power_manager_proxy.cc
@@ -132,23 +132,15 @@ delegate_->OnSuspendImminent(proto.suspend_id()); } -void PowerManagerProxy::Proxy::PowerStateChanged( - const string &new_power_state) { - LOG(INFO) << __func__ << "(" << new_power_state << ")"; - - PowerManagerProxyDelegate::SuspendState suspend_state; - if (new_power_state == "on") { - suspend_state = PowerManagerProxyDelegate::kOn; - } else if (new_power_state == "standby") { - suspend_state = PowerManagerProxyDelegate::kStandby; - } else if (new_power_state == "mem") { - suspend_state = PowerManagerProxyDelegate::kMem; - } else if (new_power_state == "disk") { - suspend_state = PowerManagerProxyDelegate::kDisk; - } else { - suspend_state = PowerManagerProxyDelegate::kUnknown; +void PowerManagerProxy::Proxy::SuspendDone( + const vector<uint8_t> &serialized_proto) { + LOG(INFO) << __func__; + power_manager::SuspendDone proto; + if (!DeserializeProtocolBuffer(serialized_proto, &proto)) { + LOG(ERROR) << "Failed to parse SuspendDone signal."; + return; } - delegate_->OnPowerStateChanged(suspend_state); + delegate_->OnSuspendDone(proto.suspend_id()); } } // namespace shill
diff --git a/power_manager_proxy.h b/power_manager_proxy.h index 7e0c8fc..12f5e4b 100644 --- a/power_manager_proxy.h +++ b/power_manager_proxy.h
@@ -51,20 +51,13 @@ private: // Signal callbacks inherited from org::chromium::PowerManager_proxy. virtual void SuspendImminent(const std::vector<uint8_t> &serialized_proto); - virtual void PowerStateChanged(const std::string &new_power_state); + virtual void SuspendDone(const std::vector<uint8_t> &serialized_proto); PowerManagerProxyDelegate *const delegate_; DISALLOW_COPY_AND_ASSIGN(Proxy); }; - // The PowerStateChanged event occurs on this path. The root path ("/") is - // used because the powerd_suspend script sends signals on that path. When - // the powerd_suspend script is fixed to use the same path as the rest of the - // power manager, then this proxy can use the usual power manager path - // power_manager::kPowerManagerServicePath. - static const char kRootPath[]; - // Constructs a PowerManager DBus object proxy with signals dispatched to // |delegate|. PowerManagerProxy(PowerManagerProxyDelegate *delegate,
diff --git a/power_manager_proxy_interface.h b/power_manager_proxy_interface.h index 8b590a4..b8be440 100644 --- a/power_manager_proxy_interface.h +++ b/power_manager_proxy_interface.h
@@ -45,28 +45,16 @@ // PowerManager signal delegate to be associated with the proxy. class PowerManagerProxyDelegate { public: - // Possible states broadcast from the powerd_suspend script. - enum SuspendState { - kOn, - kStandby, - kMem, - kDisk, - kSuspending, // Internal to shill. - // Place new states above kUnknown. - kUnknown - }; - virtual ~PowerManagerProxyDelegate() {} - // Broadcasted by the power manager when it's about to suspend. RPC clients + // Broadcast by the power manager when it's about to suspend. RPC clients // that have registered through RegisterSuspendDelay() should tell the power // manager that they're ready to suspend by calling ReportSuspendReadiness() // with the delay ID returned by RegisterSuspendDelay() and |suspend_id|. virtual void OnSuspendImminent(int suspend_id) = 0; - // This method will be called when suspending or resuming. |new_power_state| - // will be "kMem" when suspending (to memory), or "kOn" when resuming. - virtual void OnPowerStateChanged(SuspendState new_power_state) = 0; + // Broadcast by the power manager when a suspend attempt has completed. + virtual void OnSuspendDone(int suspend_id) = 0; }; } // namespace shill
diff --git a/power_manager_unittest.cc b/power_manager_unittest.cc index 42b85c6..949d88e 100644 --- a/power_manager_unittest.cc +++ b/power_manager_unittest.cc
@@ -21,7 +21,10 @@ using std::map; using std::string; using testing::_; +using testing::DoAll; +using testing::Mock; using testing::Return; +using testing::SetArgumentPointee; using testing::Test; namespace shill { @@ -53,205 +56,240 @@ public: static const char kKey1[]; static const char kKey2[]; + static const char kDescription1[]; + static const char kDescription2[]; static const int kSuspendId1 = 123; static const int kSuspendId2 = 456; + static const int kDelayId1 = 4; + static const int kDelayId2 = 16; PowerManagerTest() - : power_manager_(&dispatcher_, &factory_), + : kTimeout(base::TimeDelta::FromSeconds(3)), + power_manager_(&dispatcher_, &factory_), + proxy_(factory_.proxy()), delegate_(factory_.delegate()) { - state_change_callback1_ = - Bind(&PowerManagerTest::StateChangeAction1, Unretained(this)); - state_change_callback2_ = - Bind(&PowerManagerTest::StateChangeAction2, Unretained(this)); - suspend_delay_callback1_ = - Bind(&PowerManagerTest::SuspendDelayAction1, Unretained(this)); - suspend_delay_callback2_ = - Bind(&PowerManagerTest::SuspendDelayAction2, Unretained(this)); + suspend_imminent_callback1_ = + Bind(&PowerManagerTest::SuspendImminentAction1, Unretained(this)); + suspend_imminent_callback2_ = + Bind(&PowerManagerTest::SuspendImminentAction2, Unretained(this)); + suspend_done_callback1_ = + Bind(&PowerManagerTest::SuspendDoneAction1, Unretained(this)); + suspend_done_callback2_ = + Bind(&PowerManagerTest::SuspendDoneAction2, Unretained(this)); } - MOCK_METHOD1(StateChangeAction1, void(PowerManager::SuspendState)); - MOCK_METHOD1(StateChangeAction2, void(PowerManager::SuspendState)); - MOCK_METHOD1(SuspendDelayAction1, void(int)); - MOCK_METHOD1(SuspendDelayAction2, void(int)); + MOCK_METHOD1(SuspendImminentAction1, void(int)); + MOCK_METHOD1(SuspendImminentAction2, void(int)); + MOCK_METHOD1(SuspendDoneAction1, void(int)); + MOCK_METHOD1(SuspendDoneAction2, void(int)); protected: + void AddProxyRegisterSuspendDelayExpectation( + base::TimeDelta timeout, + const std::string &description, + int delay_id, + bool return_value) { + EXPECT_CALL(*proxy_, RegisterSuspendDelay(timeout, description, _)) + .WillOnce(DoAll(SetArgumentPointee<2>(delay_id), + Return(return_value))); + } + + void AddProxyUnregisterSuspendDelayExpectation(int delay_id, + bool return_value) { + EXPECT_CALL(*proxy_, UnregisterSuspendDelay(delay_id)) + .WillOnce(Return(return_value)); + } + + void AddProxyReportSuspendReadinessExpectation(int delay_id, + int suspend_id, + bool return_value) { + EXPECT_CALL(*proxy_, ReportSuspendReadiness(delay_id, suspend_id)) + .WillOnce(Return(return_value)); + } + void OnSuspendImminent(int suspend_id) { EXPECT_CALL(dispatcher_, PostDelayedTask(_, PowerManager::kSuspendTimeoutMilliseconds)); factory_.delegate()->OnSuspendImminent(suspend_id); - EXPECT_EQ(PowerManagerProxyDelegate::kSuspending, - power_manager_.power_state()); + EXPECT_TRUE(power_manager_.suspending()); } - void OnSuspendTimeout() { - power_manager_.OnSuspendTimeout(); + void OnSuspendDone(int suspend_id) { + factory_.delegate()->OnSuspendDone(suspend_id); + EXPECT_FALSE(power_manager_.suspending()); } + void OnSuspendTimeout(int suspend_id) { + power_manager_.OnSuspendTimeout(suspend_id); + } + + // This is non-static since it's a non-POD type. + const base::TimeDelta kTimeout; + MockEventDispatcher dispatcher_; FakeProxyFactory factory_; PowerManager power_manager_; + MockPowerManagerProxy *const proxy_; PowerManagerProxyDelegate *const delegate_; - PowerManager::PowerStateCallback state_change_callback1_; - PowerManager::PowerStateCallback state_change_callback2_; - PowerManager::SuspendDelayCallback suspend_delay_callback1_; - PowerManager::SuspendDelayCallback suspend_delay_callback2_; + PowerManager::SuspendImminentCallback suspend_imminent_callback1_; + PowerManager::SuspendImminentCallback suspend_imminent_callback2_; + PowerManager::SuspendDoneCallback suspend_done_callback1_; + PowerManager::SuspendDoneCallback suspend_done_callback2_; }; const char PowerManagerTest::kKey1[] = "Zaphod"; const char PowerManagerTest::kKey2[] = "Beeblebrox"; +const char PowerManagerTest::kDescription1[] = "Foo"; +const char PowerManagerTest::kDescription2[] = "Bar"; -TEST_F(PowerManagerTest, OnPowerStateChanged) { - EXPECT_EQ(PowerManagerProxyDelegate::kUnknown, power_manager_.power_state()); - power_manager_.OnPowerStateChanged(PowerManagerProxyDelegate::kOn); - EXPECT_EQ(PowerManagerProxyDelegate::kOn, power_manager_.power_state()); +TEST_F(PowerManagerTest, SuspendingState) { + const int kSuspendId = 3; + EXPECT_FALSE(power_manager_.suspending()); + OnSuspendImminent(kSuspendId); + EXPECT_TRUE(power_manager_.suspending()); + OnSuspendDone(kSuspendId); + EXPECT_FALSE(power_manager_.suspending()); } -TEST_F(PowerManagerTest, AddStateChangeCallback) { - EXPECT_CALL(*this, StateChangeAction1(PowerManagerProxyDelegate::kOn)); - power_manager_.AddStateChangeCallback(kKey1, state_change_callback1_); - factory_.delegate()->OnPowerStateChanged(PowerManagerProxyDelegate::kOn); - power_manager_.RemoveStateChangeCallback(kKey1); -} +TEST_F(PowerManagerTest, MultipleSuspendDelays) { + AddProxyRegisterSuspendDelayExpectation( + kTimeout, kDescription1, kDelayId1, true); + EXPECT_TRUE(power_manager_.AddSuspendDelay( + kKey1, kDescription1, kTimeout, + suspend_imminent_callback1_, suspend_done_callback1_)); + Mock::VerifyAndClearExpectations(proxy_); -TEST_F(PowerManagerTest, AddSuspendDelayCallback) { - EXPECT_CALL(*this, SuspendDelayAction1(kSuspendId1)); - power_manager_.AddSuspendDelayCallback(kKey1, suspend_delay_callback1_); - EXPECT_EQ(PowerManagerProxyDelegate::kUnknown, power_manager_.power_state()); + AddProxyRegisterSuspendDelayExpectation( + kTimeout, kDescription2, kDelayId2, true); + EXPECT_TRUE(power_manager_.AddSuspendDelay( + kKey2, kDescription2, kTimeout, + suspend_imminent_callback2_, suspend_done_callback2_)); + Mock::VerifyAndClearExpectations(proxy_); + + EXPECT_CALL(*this, SuspendImminentAction1(kSuspendId1)); + EXPECT_CALL(*this, SuspendImminentAction2(kSuspendId1)); OnSuspendImminent(kSuspendId1); - power_manager_.RemoveSuspendDelayCallback(kKey1); -} + Mock::VerifyAndClearExpectations(this); -TEST_F(PowerManagerTest, AddMultipleStateChangeRunMultiple) { - EXPECT_CALL(*this, StateChangeAction1(PowerManagerProxyDelegate::kOn)); - EXPECT_CALL(*this, StateChangeAction1(PowerManagerProxyDelegate::kMem)); - power_manager_.AddStateChangeCallback(kKey1, state_change_callback1_); + AddProxyReportSuspendReadinessExpectation(kDelayId1, kSuspendId1, true); + EXPECT_TRUE(power_manager_.ReportSuspendReadiness(kKey1, kSuspendId1)); + Mock::VerifyAndClearExpectations(proxy_); - EXPECT_CALL(*this, StateChangeAction2(PowerManagerProxyDelegate::kOn)); - EXPECT_CALL(*this, StateChangeAction2(PowerManagerProxyDelegate::kMem)); - power_manager_.AddStateChangeCallback(kKey2, state_change_callback2_); + AddProxyReportSuspendReadinessExpectation(kDelayId2, kSuspendId2, true); + EXPECT_TRUE(power_manager_.ReportSuspendReadiness(kKey2, kSuspendId2)); + Mock::VerifyAndClearExpectations(proxy_); - factory_.delegate()->OnPowerStateChanged(PowerManagerProxyDelegate::kOn); - factory_.delegate()->OnPowerStateChanged(PowerManagerProxyDelegate::kMem); -} + EXPECT_CALL(*this, SuspendDoneAction1(kSuspendId1)); + EXPECT_CALL(*this, SuspendDoneAction2(kSuspendId1)); + OnSuspendDone(kSuspendId1); + Mock::VerifyAndClearExpectations(this); -TEST_F(PowerManagerTest, AddMultipleSuspendDelayRunMultiple) { - EXPECT_CALL(*this, SuspendDelayAction1(kSuspendId1)); - EXPECT_CALL(*this, SuspendDelayAction1(kSuspendId2)); - power_manager_.AddSuspendDelayCallback(kKey1, suspend_delay_callback1_); + AddProxyUnregisterSuspendDelayExpectation(kDelayId1, true); + EXPECT_TRUE(power_manager_.RemoveSuspendDelay(kKey1)); + Mock::VerifyAndClearExpectations(proxy_); - EXPECT_CALL(*this, SuspendDelayAction2(kSuspendId1)); - EXPECT_CALL(*this, SuspendDelayAction2(kSuspendId2)); - power_manager_.AddSuspendDelayCallback(kKey2, suspend_delay_callback2_); + AddProxyUnregisterSuspendDelayExpectation(kDelayId2, true); + EXPECT_TRUE(power_manager_.RemoveSuspendDelay(kKey2)); + Mock::VerifyAndClearExpectations(proxy_); - OnSuspendImminent(kSuspendId1); + // Start a second suspend attempt with no registered delays. OnSuspendImminent(kSuspendId2); + OnSuspendDone(kSuspendId1); + Mock::VerifyAndClearExpectations(this); } -TEST_F(PowerManagerTest, RemoveStateChangeCallback) { - EXPECT_CALL(*this, StateChangeAction1(PowerManagerProxyDelegate::kOn)); - EXPECT_CALL(*this, StateChangeAction1(PowerManagerProxyDelegate::kMem)); - power_manager_.AddStateChangeCallback(kKey1, state_change_callback1_); +TEST_F(PowerManagerTest, RegisterSuspendDelayFailure) { + AddProxyRegisterSuspendDelayExpectation( + kTimeout, kDescription1, kDelayId1, false); + EXPECT_FALSE(power_manager_.AddSuspendDelay( + kKey1, kDescription1, kTimeout, + suspend_imminent_callback1_, suspend_done_callback1_)); + Mock::VerifyAndClearExpectations(proxy_); - EXPECT_CALL(*this, StateChangeAction2(PowerManagerProxyDelegate::kOn)); - EXPECT_CALL(*this, StateChangeAction2(PowerManagerProxyDelegate::kMem)) - .Times(0); - power_manager_.AddStateChangeCallback(kKey2, state_change_callback2_); - - factory_.delegate()->OnPowerStateChanged(PowerManagerProxyDelegate::kOn); - - power_manager_.RemoveStateChangeCallback(kKey2); - factory_.delegate()->OnPowerStateChanged(PowerManagerProxyDelegate::kMem); - - power_manager_.RemoveStateChangeCallback(kKey1); - factory_.delegate()->OnPowerStateChanged(PowerManagerProxyDelegate::kOn); -} - -TEST_F(PowerManagerTest, RemoveSuspendDelayCallback) { - EXPECT_CALL(*this, SuspendDelayAction1(kSuspendId1)); - EXPECT_CALL(*this, SuspendDelayAction1(kSuspendId2)); - power_manager_.AddSuspendDelayCallback(kKey1, suspend_delay_callback1_); - - EXPECT_CALL(*this, SuspendDelayAction2(kSuspendId1)); - EXPECT_CALL(*this, SuspendDelayAction2(kSuspendId2)).Times(0); - power_manager_.AddSuspendDelayCallback(kKey2, suspend_delay_callback2_); - + // No callbacks should be invoked. OnSuspendImminent(kSuspendId1); + OnSuspendDone(kSuspendId1); + Mock::VerifyAndClearExpectations(this); +} - power_manager_.RemoveSuspendDelayCallback(kKey2); - OnSuspendImminent(kSuspendId2); +TEST_F(PowerManagerTest, ReportSuspendReadinessFailure) { + AddProxyRegisterSuspendDelayExpectation( + kTimeout, kDescription1, kDelayId1, true); + EXPECT_TRUE(power_manager_.AddSuspendDelay( + kKey1, kDescription1, kTimeout, + suspend_imminent_callback1_, suspend_done_callback1_)); + Mock::VerifyAndClearExpectations(proxy_); - power_manager_.RemoveSuspendDelayCallback(kKey1); + AddProxyReportSuspendReadinessExpectation(kDelayId1, kSuspendId1, false); + EXPECT_FALSE(power_manager_.ReportSuspendReadiness(kKey1, kSuspendId1)); + Mock::VerifyAndClearExpectations(proxy_); +} + +TEST_F(PowerManagerTest, UnregisterSuspendDelayFailure) { + AddProxyRegisterSuspendDelayExpectation( + kTimeout, kDescription1, kDelayId1, true); + EXPECT_TRUE(power_manager_.AddSuspendDelay( + kKey1, kDescription1, kTimeout, + suspend_imminent_callback1_, suspend_done_callback1_)); + Mock::VerifyAndClearExpectations(proxy_); + + // Make the proxy report failure for unregistering. + AddProxyUnregisterSuspendDelayExpectation(kDelayId1, false); + EXPECT_FALSE(power_manager_.RemoveSuspendDelay(kKey1)); + Mock::VerifyAndClearExpectations(proxy_); + + // As a result, the callback should still be invoked. + EXPECT_CALL(*this, SuspendImminentAction1(kSuspendId1)); OnSuspendImminent(kSuspendId1); + Mock::VerifyAndClearExpectations(this); + + // Let the unregister request complete successfully now. + AddProxyUnregisterSuspendDelayExpectation(kDelayId1, true); + EXPECT_TRUE(power_manager_.RemoveSuspendDelay(kKey1)); + Mock::VerifyAndClearExpectations(proxy_); } -typedef PowerManagerTest PowerManagerDeathTest; +TEST_F(PowerManagerTest, DuplicateKey) { + AddProxyRegisterSuspendDelayExpectation( + kTimeout, kDescription1, kDelayId1, true); + EXPECT_TRUE(power_manager_.AddSuspendDelay( + kKey1, kDescription1, kTimeout, + suspend_imminent_callback1_, suspend_done_callback1_)); + Mock::VerifyAndClearExpectations(proxy_); -TEST_F(PowerManagerDeathTest, AddStateChangeCallbackDuplicateKey) { - power_manager_.AddStateChangeCallback( - kKey1, Bind(&PowerManagerTest::StateChangeAction1, Unretained(this))); + // The new request should be ignored. + EXPECT_FALSE(power_manager_.AddSuspendDelay( + kKey1, kDescription2, kTimeout, + suspend_imminent_callback2_, suspend_done_callback2_)); -#ifndef NDEBUG - // Adding another callback with the same key is an error and causes a crash in - // debug mode. - EXPECT_DEATH(power_manager_.AddStateChangeCallback( - kKey1, Bind(&PowerManagerTest::StateChangeAction2, Unretained(this))), - "Inserting duplicate key"); -#else // NDEBUG - EXPECT_CALL(*this, StateChangeAction2(PowerManagerProxyDelegate::kOn)); - power_manager_.AddStateChangeCallback( - kKey1, Bind(&PowerManagerTest::StateChangeAction2, Unretained(this))); - factory_.delegate()->OnPowerStateChanged(PowerManagerProxyDelegate::kOn); -#endif // NDEBUG + // The first delay's callback should be invoked. + EXPECT_CALL(*this, SuspendImminentAction1(kSuspendId1)); + OnSuspendImminent(kSuspendId1); + Mock::VerifyAndClearExpectations(this); } -TEST_F(PowerManagerDeathTest, RemoveStateChangeCallbackUnknownKey) { - power_manager_.AddStateChangeCallback( - kKey1, Bind(&PowerManagerTest::StateChangeAction1, Unretained(this))); - -#ifndef NDEBUG - // Attempting to remove a callback key that was not added is an error and - // crashes in debug mode. - EXPECT_DEATH(power_manager_.RemoveStateChangeCallback(kKey2), - "Removing unknown key"); -#else // NDEBUG - EXPECT_CALL(*this, StateChangeAction1(PowerManagerProxyDelegate::kOn)); - - // In non-debug mode, removing an unknown key does nothing. - power_manager_.RemoveStateChangeCallback(kKey2); - factory_.delegate()->OnPowerStateChanged(PowerManagerProxyDelegate::kOn); -#endif // NDEBUG -} - -TEST_F(PowerManagerTest, RegisterSuspendDelay) { - const base::TimeDelta kTimeout = base::TimeDelta::FromMilliseconds(100); - const char kDescription[] = "description"; - int delay_id = 0; - EXPECT_CALL(*factory_.proxy(), RegisterSuspendDelay(kTimeout, kDescription, - &delay_id)) - .WillOnce(Return(true)); - EXPECT_TRUE(power_manager_.RegisterSuspendDelay(kTimeout, kDescription, - &delay_id)); -} - -TEST_F(PowerManagerTest, UnregisterSuspendDelay) { - const int kDelayId = 123; - EXPECT_CALL(*factory_.proxy(), UnregisterSuspendDelay(kDelayId)) - .WillOnce(Return(true)); - EXPECT_TRUE(power_manager_.UnregisterSuspendDelay(kDelayId)); -} - -TEST_F(PowerManagerTest, ReportSuspendReadiness) { - const int kDelayId = 678; - const int kSuspendId = 12345; - EXPECT_CALL(*factory_.proxy(), ReportSuspendReadiness(kDelayId, kSuspendId)) - .WillOnce(Return(true)); - EXPECT_TRUE(power_manager_.ReportSuspendReadiness(kDelayId, kSuspendId)); +TEST_F(PowerManagerTest, UnknownKey) { + EXPECT_FALSE(power_manager_.RemoveSuspendDelay(kKey1)); + Mock::VerifyAndClearExpectations(proxy_); } TEST_F(PowerManagerTest, OnSuspendTimeout) { - EXPECT_EQ(PowerManagerProxyDelegate::kUnknown, power_manager_.power_state()); - OnSuspendTimeout(); - EXPECT_EQ(PowerManagerProxyDelegate::kOn, power_manager_.power_state()); + AddProxyRegisterSuspendDelayExpectation( + kTimeout, kDescription1, kDelayId1, true); + EXPECT_TRUE(power_manager_.AddSuspendDelay( + kKey1, kDescription1, kTimeout, + suspend_imminent_callback1_, suspend_done_callback1_)); + Mock::VerifyAndClearExpectations(proxy_); + + EXPECT_CALL(*this, SuspendImminentAction1(kSuspendId1)); + OnSuspendImminent(kSuspendId1); + Mock::VerifyAndClearExpectations(this); + + // After the timeout, the "done" callback should be run. + EXPECT_CALL(*this, SuspendDoneAction1(kSuspendId1)); + OnSuspendTimeout(kSuspendId1); + EXPECT_FALSE(power_manager_.suspending()); + Mock::VerifyAndClearExpectations(this); } } // namespace shill
diff --git a/service.cc b/service.cc index a79837c..895970e 100644 --- a/service.cc +++ b/service.cc
@@ -884,11 +884,9 @@ SLOG(Service, 2) << "Disconnect while manager stopped ignored."; return; } - // Ignore the event if the power state is not on (e.g., when suspending). + // Ignore the event if the system is suspending. PowerManager *power_manager = manager_->power_manager(); - if (!power_manager || - (power_manager->power_state() != PowerManager::kOn && - power_manager->power_state() != PowerManager::kUnknown)) { + if (!power_manager || power_manager->suspending()) { SLOG(Service, 2) << "Disconnect in transitional power state ignored."; return; }
diff --git a/service_unittest.cc b/service_unittest.cc index e583b32..c4172d6 100644 --- a/service_unittest.cc +++ b/service_unittest.cc
@@ -102,8 +102,8 @@ void SetManagerRunning(bool running) { mock_manager_.running_ = running; } - void SetPowerState(PowerManager::SuspendState state) { - power_manager_->power_state_ = state; + void SetSuspending(bool suspending) { + power_manager_->suspending_ = suspending; } void SetExplicitlyDisconnected(bool explicitly) { @@ -1538,7 +1538,7 @@ // Disconnect while suspending is a non-event. SetManagerRunning(true); - SetPowerState(PowerManager::kSuspending); + SetSuspending(true); NoteDisconnectEvent(); EXPECT_TRUE(GetDisconnects()->empty()); EXPECT_TRUE(GetMisconnects()->empty());