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..94210c86c8427ea38b0294430be12133c8b88309
--- /dev/null
+++ b/components/policy/core/common/remote_commands/testing_remote_commands_server.cc
@@ -0,0 +1,127 @@
+// 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>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/run_loop.h"
+#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 {
+
+using RemoteCommandVector = TestingRemoteCommandsServer::RemoteCommandVector;

[bartfab (slow), 2015/02/17 18:13:53]

Nit: Use the definitions from the header file instead of repeating them here.

[binjin, 2015/02/18 15:37:51]

Done.

+using RemoteCommandResultVector =
+    TestingRemoteCommandsServer::RemoteCommandResultVector;
+
+TestingRemoteCommandsServer::TestingRemoteCommandsServer()
+    : clock_(new base::DefaultClock()),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_factory_(this) {
+}
+
+TestingRemoteCommandsServer::~TestingRemoteCommandsServer() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+void TestingRemoteCommandsServer::IssueCommand(em::RemoteCommand_Type type,
+                                               const std::string& payload,
+                                               bool skip_next_fetch) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  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());
+
+  if (skip_next_fetch)
+    commands_issued_after_next_fetch_.push_back(command);
+  else
+    commands_.push_back(command);
+}
+
+RemoteCommandVector TestingRemoteCommandsServer::FetchCommandsFromIO(

[bartfab (slow), 2015/02/17 18:13:53]

1: You accidentally removed the locking. This method races against e.g.
IssueCommand() now.
2: How do you know that |this| still exists?

[binjin, 2015/02/18 15:37:50]

1. Done.
2. I don't think it's necessary to add a comments here requiring this to be
valid when calling this method. Instead, I have comments for
TestRequestInterceptor::FetchRemoteCommandsJob which requiring this test server
to be valid when the request is intercepted.

+    RemoteCommandJob::UniqueIDType last_command_id,
+    const RemoteCommandResultVector& previous_job_results) {
+  DCHECK(!thread_checker_.CalledOnValidThread());

[bartfab (slow), 2015/02/17 18:13:53]

Why do you want this method to be *only* called from other threads? With proper
locking in place, it can be called from any thread, including the main one.

[binjin, 2015/02/18 15:37:50]

Done. Also Changed name/comments.

+
+  // Verify that acknowledged ID from client is non-decreasing.
+  EXPECT_LE(last_acknowledged_unique_id_, last_command_id);

[bartfab (slow), 2015/02/17 18:13:53]

You are mixing DCHECK and EXPECT. If this class is to be used in tests only, you
can use EXPECT throughout. Otherwise, use DCHECK throughout.

[binjin, 2015/02/18 15:37:50]

Done. But left thread checking with DCHECK().

+  last_acknowledged_unique_id_ = last_command_id;
+
+  for (const auto& job_result : previous_job_results) {
+    DCHECK(job_result.has_unique_id());
+    DCHECK(job_result.has_result());
+
+    EXPECT_GE(last_command_id, job_result.unique_id());
+
+    bool found_command = false;
+    for (auto it = commands_.begin(); it != commands_.end(); ++it) {
+      if (it->unique_id() == job_result.unique_id()) {
+        commands_.erase(it);
+        found_command = true;
+        break;
+      }
+    }
+
+    // Verify that command result is for an existing commands actually expecting
+    // command result.
+    EXPECT_TRUE(found_command);
+
+    // Post task to original thread for result reporting. Note that
+    // |task_runner| is on the same thread |weak_factory_| was created and thus
+    // it's safe to use weak pointer here.
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&TestingRemoteCommandsServer::ReportJobResult,
+                              weak_factory_.GetWeakPtr(), job_result));

[bartfab (slow), 2015/02/17 18:13:53]

This is not correct. If you create a weak pointer on UI, you can pass the
pointer to IO and use it in tasks posted back to UI. But you cannot create the
pointer on IO.

[binjin, 2015/02/18 15:37:51]

Done.

+  }
+
+  RemoteCommandVector fetched_commands;
+  for (const auto& command : commands_) {
+    if (command.unique_id() > last_command_id)
+      fetched_commands.push_back(command);
+  }
+
+  // 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::OnJobResultReported(
+    const em::RemoteCommandResult& job_result) const {
+}
+
+void TestingRemoteCommandsServer::ReportJobResult(
+    const em::RemoteCommandResult& job_result) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  OnJobResultReported(job_result);
+}
+
+}  // namespace policy