Initial RemoteCommandService

components/policy/core/common/remote_commands/remote_commands_service.cc

Index: components/policy/core/common/remote_commands/remote_commands_service.cc
diff --git a/components/policy/core/common/remote_commands/remote_commands_service.cc b/components/policy/core/common/remote_commands/remote_commands_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a23400c7f1a0be75a335a293a87d3b6286e03177
--- /dev/null
+++ b/components/policy/core/common/remote_commands/remote_commands_service.cc
@@ -0,0 +1,115 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/policy/core/common/remote_commands/remote_commands_service.h"
+
+#include <algorithm>
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/time/time.h"
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
+
+namespace policy {
+
+namespace em = enterprise_management;
+
+RemoteCommandsService::RemoteCommandsService(
+    scoped_ptr<RemoteCommandJob::Factory> factory,
+    CloudPolicyClient* client)
+    : request_job_in_progress_(false),

[bartfab (slow), 2015/02/12 14:29:20]

Nit: C++11 allows you to initialize this member and the next two in the header
file.

[binjin, 2015/02/16 22:46:24]

Done.

+      last_fetched_command_id_(0),
+      last_executed_command_id_(0),
+      factory_(factory.Pass()),
+      client_(client) {
+  queue_.AddObserver(this);
+}
+
+RemoteCommandsService::~RemoteCommandsService() {
+  queue_.RemoveObserver(this);
+}
+
+void RemoteCommandsService::FetchRemoteCommands() {
+  if (request_job_in_progress_)
+    return;
+
+  request_job_in_progress_ = true;
+
+  client_->FetchRemoteCommands(
+      last_executed_command_id_, unsent_results_,
+      base::Bind(&RemoteCommandsService::OnRemoteCommandsFetched,
+                 base::Unretained(this)));

[bartfab (slow), 2015/02/12 14:29:20]

You must use a weak pointer here. Otherwise, the callback may be invoked after
|this| has been destroyed.

[binjin, 2015/02/16 22:46:24]

Done.

+  unsent_results_.clear();

[bartfab (slow), 2015/02/12 14:29:20]

Nit: It would be nice to make this method reentrant by clearing
|unsent_results_| before issuing the fetch.

[binjin, 2015/02/16 22:46:24]

Done.

+}
+
+void RemoteCommandsService::AddCommand(
+    const enterprise_management::RemoteCommand& command) {
+  if (!command.has_type() || !command.has_unique_id()) {
+    LOG(WARNING) << "Invalid remote command from server.";
+    return;
+  }
+
+  if (command.unique_id() <= last_fetched_command_id_)

[bartfab (slow), 2015/02/12 14:29:20]

You must never compare the command IDs to each other. They are opaque
identifiers that can increas and decrease at will.

[binjin, 2015/02/16 22:46:24]

Done.

+    return;
+
+  last_fetched_command_id_ = command.unique_id();
+
+  scoped_ptr<RemoteCommandJob> job = factory_->BuildJobForType(command.type());
+
+  if (!job || !job->Init(command)) {
+    em::RemoteCommandResult ignored_result;
+    ignored_result.set_result(
+        em::RemoteCommandResult_ResultType_RESULT_IGNORED);
+    ignored_result.set_unique_id(command.unique_id());
+    unsent_results_.push_back(ignored_result);
+    return;
+  }
+
+  queue_.AddJob(job.Pass());
+}
+
+void RemoteCommandsService::OnJobStarted(RemoteCommandJob* command) {
+  last_executed_command_id_ =
+      std::max(last_executed_command_id_, command->unique_id());

[bartfab (slow), 2015/02/12 14:29:20]

Always set it to command->unique_id(). Never compare the opaque command IDs to
each other.

[binjin, 2015/02/16 22:46:24]

Done.

+  // XXX: sync |last_executed_command_id_| to persistent source, and reload
+  //      later.
+}
+
+void RemoteCommandsService::OnJobFinished(RemoteCommandJob* command) {
+  em::RemoteCommandResult result;
+  result.set_unique_id(command->unique_id());
+  result.set_timestamp((command->execution_started_time() -
+                        base::Time::UnixEpoch()).InMilliseconds());
+
+  if (command->status() == RemoteCommandJob::SUCCEEDED) {
+    result.set_result(em::RemoteCommandResult_ResultType_RESULT_SUCCESS);
+    scoped_ptr<std::string> result_payload = command->GetResultPayload();
+    if (result_payload)
+      result.set_payload(*result_payload);
+  } else if (command->status() == RemoteCommandJob::EXPIRED ||
+             command->status() == RemoteCommandJob::INVALID) {
+    result.set_result(em::RemoteCommandResult_ResultType_RESULT_IGNORED);
+  } else {
+    result.set_result(em::RemoteCommandResult_ResultType_RESULT_FAILURE);
+  }
+
+  unsent_results_.push_back(result);
+
+  FetchRemoteCommands();
+}
+
+void RemoteCommandsService::OnRemoteCommandsFetched(
+    const std::vector<enterprise_management::RemoteCommand>& commands) {
+  request_job_in_progress_ = false;

[bartfab (slow), 2015/02/12 14:29:20]

Nit: Add a DCHECK(request_job_in_progress_);

[binjin, 2015/02/16 22:46:24]

Done.

+
+  for (const auto& command : commands)
+    AddCommand(command);
+
+  if (!unsent_results_.empty())
+    FetchRemoteCommands();
+}
+
+}  // namespace policy