Initial RemoteCommandService

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

Index: components/policy/core/common/remote_commands/testing_remote_commands_server.cc
diff --git a/components/policy/core/common/remote_commands/testing_remote_commands_server.cc b/components/policy/core/common/remote_commands/testing_remote_commands_server.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d7fd634880d2903283fa38f89ff71650ae18587d
--- /dev/null
+++ b/components/policy/core/common/remote_commands/testing_remote_commands_server.cc
@@ -0,0 +1,147 @@
+// 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/testing_remote_commands_server.h"
+
+#include <algorithm>

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

Nit: What is this used for?

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

Removed. It's used for std::max() before.

+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/run_loop.h"

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

Nit: Not used.

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

Done.

+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/time/clock.h"
+#include "base/time/default_clock.h"
+#include "base/time/time.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace em = enterprise_management;
+
+namespace policy {
+
+struct TestingRemoteCommandsServer::RemoteCommandWithCallback {
+  RemoteCommandWithCallback(const em::RemoteCommand& command_proto,
+                            const ResultReportedCallback& reported_callback)
+      : command_proto(command_proto), reported_callback(reported_callback) {}
+  virtual ~RemoteCommandWithCallback() {}

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

Nit: Why virtual?

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

Removed.

+
+  em::RemoteCommand command_proto;
+  ResultReportedCallback reported_callback;

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

Nit: #include "base/callback.h"

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

Done.

+};
+
+TestingRemoteCommandsServer::TestingRemoteCommandsServer()
+    : clock_(new base::DefaultClock()),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_factory_(this) {
+  weak_ptr_to_this_ = weak_factory_.GetWeakPtr();
+}
+
+TestingRemoteCommandsServer::~TestingRemoteCommandsServer() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // All reported callbacks explicitly stated must be called.

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

Nit: This is hard to understand. How about: "Commands are removed from the queue
when a result is reported. Only commands for which no result was expected should
remain in the queue?"

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

Done.

+  for (const auto& command_with_callback : commands_)
+    EXPECT_TRUE(command_with_callback.reported_callback.is_null());
+}
+
+void TestingRemoteCommandsServer::IssueCommand(
+    em::RemoteCommand_Type type,
+    const std::string& payload,
+    const ResultReportedCallback& reported_callback,
+    bool skip_next_fetch) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  base::AutoLock auto_lock(lock_);
+
+  em::RemoteCommand command;
+  command.set_type(type);
+  command.set_unique_id(++last_generated_unique_id_);
+  if (!payload.empty())
+    command.set_payload(payload);
+  command.set_timestamp(
+      (clock_->Now() - base::Time::UnixEpoch()).InMilliseconds());
+
+  const RemoteCommandWithCallback command_with_callback(command,
+                                                        reported_callback);
+  if (skip_next_fetch)
+    commands_issued_after_next_fetch_.push_back(command_with_callback);
+  else
+    commands_.push_back(command_with_callback);
+}
+
+TestingRemoteCommandsServer::RemoteCommands

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

Nit: Is the TestingRemoteCommandsServer:: prefix really necessary here?

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

The compiler tells yes :(

+TestingRemoteCommandsServer::FetchCommands(
+    RemoteCommandJob::UniqueIDType last_command_id,
+    const RemoteCommandResults& previous_job_results) {
+  base::AutoLock auto_lock(lock_);
+
+  // Verify that acknowledged ID from client is non-decreasing, since we are

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

Nit: s/that/that the/

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

Done.

+  // generating commands with increasing IDs.
+  EXPECT_LE(last_acknowledged_unique_id_, last_command_id);
+  last_acknowledged_unique_id_ = last_command_id;
+
+  for (const auto& job_result : previous_job_results) {
+    EXPECT_TRUE(job_result.has_unique_id());
+    EXPECT_TRUE(job_result.has_result());
+
+    EXPECT_GE(last_command_id, job_result.unique_id());
+
+    bool found_command = false;
+    ResultReportedCallback reported_callback;
+
+    for (auto it = commands_.begin(); it != commands_.end(); ++it) {
+      if (it->command_proto.unique_id() == job_result.unique_id()) {
+        reported_callback = it->reported_callback;
+        commands_.erase(it);
+        found_command = true;
+        break;
+      }
+    }
+
+    // Verify that command result is for an existing commands actually expecting

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

Nit 1: s/that/that the/
Nit 2: s/commands/command/

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

Done.

+    // command result.

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

1: Nit: s/command/a/
2: You are not actually verifying that the command expects a result. Even if the
command was found in the queue, if |reported_callback| is null, the command
expected no result.

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

Done.

+    EXPECT_TRUE(found_command);
+
+    if (!reported_callback.is_null()) {
+      // Post task to original thread for result reporting.

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

Nit 1: s/task to/a task to the/
Nit 2: s/for result reporting/which will report the result/

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

Done.

+      task_runner_->PostTask(
+          FROM_HERE,
+          base::Bind(&TestingRemoteCommandsServer::ReportJobResult,
+                     weak_ptr_to_this_, reported_callback, job_result));
+    }
+  }
+
+  RemoteCommands fetched_commands;
+  for (const auto& command_with_callback : commands_) {
+    if (command_with_callback.command_proto.unique_id() > last_command_id)
+      fetched_commands.push_back(command_with_callback.command_proto);
+  }
+
+  // Push delayed commands into the main queue.
+  commands_.insert(commands_.end(), commands_issued_after_next_fetch_.begin(),
+                   commands_issued_after_next_fetch_.end());
+  commands_issued_after_next_fetch_.clear();
+
+  return fetched_commands;
+}
+
+void TestingRemoteCommandsServer::SetClock(scoped_ptr<base::Clock> clock) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  clock_ = clock.Pass();
+}
+
+size_t TestingRemoteCommandsServer::NumberOfCommandsPendingResult() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return commands_.size();
+}
+
+void TestingRemoteCommandsServer::ReportJobResult(
+    const ResultReportedCallback& reported_callback,
+    const em::RemoteCommandResult& job_result) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  reported_callback.Run(job_result);
+}
+
+}  // namespace policy