Skip to content

Commit

Permalink
Misc refactoring of SessionManagerClient.
Browse files Browse the repository at this point in the history 
- In ObjectProxy::CallMethod, in case of error, always LOG.
- SessionManagerClientImpl's dbus call error logging is now relying
  on ObjectProxy, so use EmptyResponseCallback wherever possible.
- Introduce OnNoOutputParamResponse(), for sharing the code (OnStorePolicy
  and OnArcMethod).
- Use nullptr instead of NULL.
- Use "= default" for ctor and dtor if possible.
- Initialize member var on declaration, if possible.

BUG=n/a
TEST=Ran trybots.

Review-Url: https://codereview.chromium.org/2889683006
Cr-Commit-Position: refs/heads/master@{#473513}
  • Loading branch information
hidehiko authored and Commit bot committed May 22, 2017
1 parent 5a3def7 commit 6c197ef
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 146 deletions.
165 changes: 46 additions & 119 deletions chromeos/dbus/session_manager_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,9 @@ void LogPolicyResponseUma(base::StringPiece method_name,
// The SessionManagerClient implementation used in production.
class SessionManagerClientImpl : public SessionManagerClient {
public:
SessionManagerClientImpl()
: session_manager_proxy_(NULL),
screen_is_locked_(false),
weak_ptr_factory_(this) {}
SessionManagerClientImpl() : weak_ptr_factory_(this) {}

~SessionManagerClientImpl() override {}
~SessionManagerClientImpl() override = default;

// SessionManagerClient overrides:
void SetStubDelegate(StubDelegate* delegate) override {
Expand Down Expand Up @@ -205,10 +202,8 @@ class SessionManagerClientImpl : public SessionManagerClient {
writer.AppendString(cryptohome_id.id());
writer.AppendString(""); // Unique ID is deprecated
session_manager_proxy_->CallMethod(
&method_call,
dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnStartSession,
weak_ptr_factory_.GetWeakPtr()));
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
dbus::ObjectProxy::EmptyResponseCallback());
}

void StopSession() override {
Expand All @@ -217,20 +212,13 @@ class SessionManagerClientImpl : public SessionManagerClient {
dbus::MessageWriter writer(&method_call);
writer.AppendString(""); // Unique ID is deprecated
session_manager_proxy_->CallMethod(
&method_call,
dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnStopSession,
weak_ptr_factory_.GetWeakPtr()));
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
dbus::ObjectProxy::EmptyResponseCallback());
}

void StartDeviceWipe() override {
dbus::MethodCall method_call(login_manager::kSessionManagerInterface,
login_manager::kSessionManagerStartDeviceWipe);
session_manager_proxy_->CallMethod(
&method_call,
dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnDeviceWipe,
weak_ptr_factory_.GetWeakPtr()));
SimpleMethodCallToSessionManager(
login_manager::kSessionManagerStartDeviceWipe);
}

void RequestLockScreen() override {
Expand Down Expand Up @@ -347,12 +335,9 @@ class SessionManagerClientImpl : public SessionManagerClient {
reinterpret_cast<const uint8_t*>(policy_blob.data()),
policy_blob.size());
session_manager_proxy_->CallMethod(
&method_call,
dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnStorePolicy,
weak_ptr_factory_.GetWeakPtr(),
login_manager::kSessionManagerStorePolicy,
callback));
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnNoOutputParamResponse,
weak_ptr_factory_.GetWeakPtr(), callback));
}

void StorePolicyForUser(const cryptohome::Identification& cryptohome_id,
Expand Down Expand Up @@ -440,9 +425,8 @@ class SessionManagerClientImpl : public SessionManagerClient {
login_manager::kSessionManagerStopArcInstance);
session_manager_proxy_->CallMethod(
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnArcMethod,
weak_ptr_factory_.GetWeakPtr(),
login_manager::kSessionManagerStopArcInstance, callback));
base::Bind(&SessionManagerClientImpl::OnNoOutputParamResponse,
weak_ptr_factory_.GetWeakPtr(), callback));
}

void SetArcCpuRestriction(
Expand All @@ -455,10 +439,8 @@ class SessionManagerClientImpl : public SessionManagerClient {
writer.AppendUint32(restriction_state);
session_manager_proxy_->CallMethod(
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnArcMethod,
weak_ptr_factory_.GetWeakPtr(),
login_manager::kSessionManagerSetArcCpuRestriction,
callback));
base::Bind(&SessionManagerClientImpl::OnNoOutputParamResponse,
weak_ptr_factory_.GetWeakPtr(), callback));
}

void EmitArcBooted(const cryptohome::Identification& cryptohome_id,
Expand All @@ -469,9 +451,8 @@ class SessionManagerClientImpl : public SessionManagerClient {
writer.AppendString(cryptohome_id.id());
session_manager_proxy_->CallMethod(
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnArcMethod,
weak_ptr_factory_.GetWeakPtr(),
login_manager::kSessionManagerEmitArcBooted, callback));
base::Bind(&SessionManagerClientImpl::OnNoOutputParamResponse,
weak_ptr_factory_.GetWeakPtr(), callback));
}

void GetArcStartTime(const GetArcStartTimeCallback& callback) override {
Expand All @@ -493,9 +474,8 @@ class SessionManagerClientImpl : public SessionManagerClient {
writer.AppendString(cryptohome_id.id());
session_manager_proxy_->CallMethod(
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnArcMethod,
weak_ptr_factory_.GetWeakPtr(),
login_manager::kSessionManagerRemoveArcData, callback));
base::Bind(&SessionManagerClientImpl::OnNoOutputParamResponse,
weak_ptr_factory_.GetWeakPtr(), callback));
}

protected:
Expand Down Expand Up @@ -556,6 +536,15 @@ class SessionManagerClientImpl : public SessionManagerClient {
dbus::ObjectProxy::EmptyResponseCallback());
}

// Calls given callback (if non-null), with the |success| boolean
// representing the dbus call was successful or not.
void OnNoOutputParamResponse(
const base::Callback<void(bool success)>& callback,
dbus::Response* response) {
if (!callback.is_null())
callback.Run(response != nullptr);
}

// Helper for RetrieveDeviceLocalAccountPolicy and RetrievePolicyForUser.
void CallRetrievePolicyByUsername(const std::string& method_name,
const std::string& account_id,
Expand Down Expand Up @@ -612,13 +601,9 @@ class SessionManagerClientImpl : public SessionManagerClient {
reinterpret_cast<const uint8_t*>(policy_blob.data()),
policy_blob.size());
session_manager_proxy_->CallMethod(
&method_call,
dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(
&SessionManagerClientImpl::OnStorePolicy,
weak_ptr_factory_.GetWeakPtr(),
method_name,
callback));
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&SessionManagerClientImpl::OnNoOutputParamResponse,
weak_ptr_factory_.GetWeakPtr(), callback));
}

// Called when kSessionManagerRestartJob method is complete.
Expand All @@ -631,48 +616,26 @@ class SessionManagerClientImpl : public SessionManagerClient {
: DBUS_METHOD_CALL_FAILURE);
}

// Called when kSessionManagerStartSession method is complete.
void OnStartSession(dbus::Response* response) {
LOG_IF(ERROR, !response)
<< "Failed to call "
<< login_manager::kSessionManagerStartSession;
}

// Called when kSessionManagerStopSession method is complete.
void OnStopSession(dbus::Response* response) {
LOG_IF(ERROR, !response)
<< "Failed to call "
<< login_manager::kSessionManagerStopSession;
}

// Called when kSessionManagerStopSession method is complete.
void OnDeviceWipe(dbus::Response* response) {
LOG_IF(ERROR, !response)
<< "Failed to call "
<< login_manager::kSessionManagerStartDeviceWipe;
}

// Called when kSessionManagerRetrieveActiveSessions method is complete.
void OnRetrieveActiveSessions(const std::string& method_name,
const ActiveSessionsCallback& callback,
dbus::Response* response) {
ActiveSessionsMap sessions;
bool success = false;
if (!response) {
LOG(ERROR) << "Failed to call " << method_name;
callback.Run(sessions, success);
return;
}

dbus::MessageReader reader(response);
dbus::MessageReader array_reader(NULL);
dbus::MessageReader array_reader(nullptr);

if (!reader.PopArray(&array_reader)) {
LOG(ERROR) << method_name << " response is incorrect: "
<< response->ToString();
} else {
while (array_reader.HasMoreData()) {
dbus::MessageReader dict_entry_reader(NULL);
dbus::MessageReader dict_entry_reader(nullptr);
std::string key;
std::string value;
if (!array_reader.PopDictEntry(&dict_entry_reader) ||
Expand All @@ -697,7 +660,7 @@ class SessionManagerClientImpl : public SessionManagerClient {
return;
}
dbus::MessageReader reader(response);
const uint8_t* values = NULL;
const uint8_t* values = nullptr;
size_t length = 0;
if (!reader.PopArrayOfBytes(&values, &length)) {
LOG(ERROR) << "Invalid response: " << response->ToString();
Expand Down Expand Up @@ -731,16 +694,6 @@ class SessionManagerClientImpl : public SessionManagerClient {
LogPolicyResponseUma(method_name, response_type);
}

// Called when kSessionManagerStorePolicy or kSessionManagerStorePolicyForUser
// method is complete.
void OnStorePolicy(const std::string& method_name,
const StorePolicyCallback& callback,
dbus::Response* response) {
bool success = response != nullptr;
LOG_IF(ERROR, !success) << "Failed to call " << method_name;
callback.Run(success);
}

// Called when the owner key set signal is received.
void OwnerKeySetReceived(dbus::Signal* signal) {
dbus::MessageReader reader(signal);
Expand Down Expand Up @@ -803,26 +756,22 @@ class SessionManagerClientImpl : public SessionManagerClient {
void OnGetServerBackedStateKeys(const StateKeysCallback& callback,
dbus::Response* response) {
std::vector<std::string> state_keys;
if (!response) {
LOG(ERROR) << "Failed to call "
<< login_manager::kSessionManagerGetServerBackedStateKeys;
} else {
if (response) {
dbus::MessageReader reader(response);
dbus::MessageReader array_reader(NULL);
dbus::MessageReader array_reader(nullptr);

if (!reader.PopArray(&array_reader)) {
LOG(ERROR) << "Bad response: " << response->ToString();
} else {
while (array_reader.HasMoreData()) {
const uint8_t* data = NULL;
const uint8_t* data = nullptr;
size_t size = 0;
if (!array_reader.PopArrayOfBytes(&data, &size)) {
LOG(ERROR) << "Bad response: " << response->ToString();
state_keys.clear();
break;
}
state_keys.push_back(
std::string(reinterpret_cast<const char*>(data), size));
state_keys.emplace_back(reinterpret_cast<const char*>(data), size);
}
}
}
Expand All @@ -835,10 +784,7 @@ class SessionManagerClientImpl : public SessionManagerClient {
void OnCheckArcAvailability(const ArcCallback& callback,
dbus::Response* response) {
bool available = false;
if (!response) {
LOG(ERROR) << "Failed to call "
<< login_manager::kSessionManagerCheckArcAvailability;
} else {
if (response) {
dbus::MessageReader reader(response);
if (!reader.PopBool(&available))
LOG(ERROR) << "Invalid response: " << response->ToString();
Expand All @@ -851,10 +797,7 @@ class SessionManagerClientImpl : public SessionManagerClient {
dbus::Response* response) {
bool success = false;
base::TimeTicks arc_start_time;
if (!response) {
LOG(ERROR) << "Failed to call "
<< login_manager::kSessionManagerGetArcStartTimeTicks;
} else {
if (response) {
dbus::MessageReader reader(response);
int64_t ticks = 0;
if (reader.PopInt64(&ticks)) {
Expand All @@ -867,22 +810,6 @@ class SessionManagerClientImpl : public SessionManagerClient {
callback.Run(success, arc_start_time);
}

// Called when kSessionManagerStartArcInstance or
// kSessionManagerStopArcInstance methods complete.
void OnArcMethod(const std::string& method_name,
const ArcCallback& callback,
dbus::Response* response) {
bool success = false;
if (!response) {
LOG(ERROR) << "Failed to call " << method_name;
} else {
success = true;
}

if (!callback.is_null())
callback.Run(success);
}

void OnStartArcInstanceSucceeded(const StartArcInstanceCallback& callback,
dbus::Response* response) {
if (!callback.is_null())
Expand All @@ -901,12 +828,12 @@ class SessionManagerClientImpl : public SessionManagerClient {
}
}

dbus::ObjectProxy* session_manager_proxy_;
dbus::ObjectProxy* session_manager_proxy_ = nullptr;
std::unique_ptr<BlockingMethodCaller> blocking_method_caller_;
base::ObserverList<Observer> observers_;

// Most recent screen-lock state received from session_manager.
bool screen_is_locked_;
bool screen_is_locked_ = false;

// Note: This should remain the last member so it'll be destroyed and
// invalidate its weak pointers before any other members are destroyed.
Expand All @@ -919,8 +846,8 @@ class SessionManagerClientImpl : public SessionManagerClient {
// which does nothing.
class SessionManagerClientStubImpl : public SessionManagerClient {
public:
SessionManagerClientStubImpl() : delegate_(NULL), screen_is_locked_(false) {}
~SessionManagerClientStubImpl() override {}
SessionManagerClientStubImpl() = default;
~SessionManagerClientStubImpl() override = default;

// SessionManagerClient overrides
void Init(dbus::Bus* bus) override {}
Expand Down Expand Up @@ -1140,10 +1067,10 @@ class SessionManagerClientStubImpl : public SessionManagerClient {
}

private:
StubDelegate* delegate_; // Weak pointer; may be NULL.
StubDelegate* delegate_ = nullptr; // Weak pointer; may be nullptr.
base::ObserverList<Observer> observers_;
std::string device_policy_;
bool screen_is_locked_;
bool screen_is_locked_ = false;

DISALLOW_COPY_AND_ASSIGN(SessionManagerClientStubImpl);
};
Expand Down
Loading

0 comments on commit 6c197ef

Please sign in to comment.