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..0a43b752075c9ad633b3e52a6377cb34671faf98
--- /dev/null
+++ b/components/policy/core/common/remote_commands/remote_commands_service.cc
@@ -0,0 +1,129 @@
+// 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>

[bartfab (slow), 2015/02/28 00:01:23]

Nit: Is this used anywhere?

[binjin, 2015/02/28 02:18:05]

Removed. Used previously for std::max()

+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"

[bartfab (slow), 2015/02/28 00:01:23]

Nit: Not used.

[binjin, 2015/02/28 02:18:05]

Done.

+#include "base/logging.h"
+#include "base/time/clock.h"
+#include "base/time/time.h"
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
+#include "components/policy/core/common/remote_commands/remote_commands_factory.h"
+
+namespace policy {
+
+namespace em = enterprise_management;
+
+RemoteCommandsService::RemoteCommandsService(
+    scoped_ptr<RemoteCommandsFactory> factory,
+    CloudPolicyClient* client)
+    : factory_(factory.Pass()), client_(client), weak_factory_(this) {
+  queue_.AddObserver(this);
+}
+
+RemoteCommandsService::~RemoteCommandsService() {
+  queue_.RemoveObserver(this);
+}
+
+bool RemoteCommandsService::FetchRemoteCommands() {
+  if (command_fetch_in_progress_) {
+    has_enqueued_fetch_request_ = true;
+    return false;
+  }
+
+  command_fetch_in_progress_ = true;
+
+  std::vector<em::RemoteCommandResult> previous_results;
+  unsent_results_.swap(previous_results);
+
+  client_->FetchRemoteCommands(

[bartfab (slow), 2015/02/28 00:01:23]

You should reset has_enqueued_fetch_request_ to false before starting the fetch.
Otherwise, OnRemoteCommandsFetched() will see has_enqueued_fetch_request_ = true
and will start another fetch

[binjin, 2015/02/28 02:18:05]

Start another fetch immediately is actually intended, and it won't introduce
infinite loop since "has_enqueued_fetch_request_" will be set to false before
the following fetch.

Followed your suggestion, since it's easier to understand.

[bartfab (slow), 2015/03/10 16:09:20]

I know there would not have been an infinite loop, but you would have done two
fetches in a row instead of just one.

[binjin, 2015/03/12 11:03:15]

Acknowledged.

+      last_executed_command_id_, previous_results,
+      base::Bind(&RemoteCommandsService::OnRemoteCommandsFetched,
+                 weak_factory_.GetWeakPtr()));
+  return true;
+}
+
+void RemoteCommandsService::SetClockForTesting(scoped_ptr<base::Clock> clock) {
+  queue_.SetClockForTesting(clock.Pass());
+}
+
+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 the command is already fetched, ignore it instead.

[bartfab (slow), 2015/02/28 00:01:23]

Nit: s/ instead//

[binjin, 2015/02/28 02:18:05]

Done.

+  if (!fetched_command_ids_.insert(command.unique_id()).second)
+    return;
+
+  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_ = command->unique_id();
+  // TODO(binjin): Evaluate the security risks and attempts to sync

[bartfab (slow), 2015/02/28 00:01:23]

1: We agreed on a plan, no need to evaluate anymore.
1: Nit: s/attempts/attempt/

[binjin, 2015/02/28 02:18:05]

Done.

+  // |last_executed_command_id_| to some persistent source, so that we can
+  // reload it later without relying solely on server to keep our last

[bartfab (slow), 2015/02/28 00:01:23]

Nit: s/on/on the/

[binjin, 2015/02/28 02:18:05]

Done.

+  // acknowledged command id.

[bartfab (slow), 2015/02/28 00:01:23]

Nit: s/id/ID/

[binjin, 2015/02/28 02:18:05]

Done.

+}
+
+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();

[bartfab (slow), 2015/02/28 00:01:23]

Nit: const.

[binjin, 2015/02/28 02:18:05]

Done.

+    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);

[bartfab (slow), 2015/02/28 00:01:23]

Christoph had the good point that it may be useful to send up an error message
for a failed command. How about including a payload whenever GetResultPayload()
returns a non-nullptr value?

[binjin, 2015/02/28 02:18:05]

It will require changes to interface of RemoteCommandJob, and actual
implementation of commands,
I would rather do in in a seperated CL.

[bartfab (slow), 2015/03/10 16:09:20]

Ack, this is definitely a task for a separate CL. But it is something to keep in
mind for the future as it would be useful.

[binjin, 2015/03/12 11:03:15]

Acknowledged.

+  }
+
+  unsent_results_.push_back(result);
+
+  FetchRemoteCommands();
+}
+
+void RemoteCommandsService::OnRemoteCommandsFetched(
+    const std::vector<enterprise_management::RemoteCommand>& commands) {
+  DCHECK(command_fetch_in_progress_);
+  command_fetch_in_progress_ = false;
+
+  for (const auto& command : commands)
+    AddCommand(command);
+
+  // Start another fetch request job immediately if there are unsent command
+  // results or enqueued fetch requests.
+  if (!unsent_results_.empty() || has_enqueued_fetch_request_) {
+    has_enqueued_fetch_request_ = false;
+    FetchRemoteCommands();
+  } else {
+    has_enqueued_fetch_request_ = false;

[bartfab (slow), 2015/02/28 00:01:23]

No need for this. If |has_enqueued_fetch_request_| was |true|, we would have
taken the other branch.

[binjin, 2015/02/28 02:18:05]

Done.

+  }
+}
+
+}  // namespace policy