Initial RemoteCommandService

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

Index: components/policy/core/common/remote_commands/testing_remote_commands_server.h
diff --git a/components/policy/core/common/remote_commands/testing_remote_commands_server.h b/components/policy/core/common/remote_commands/testing_remote_commands_server.h
new file mode 100644
index 0000000000000000000000000000000000000000..14b0fa0efd24605c2599b003788172121e9b95f6
--- /dev/null
+++ b/components/policy/core/common/remote_commands/testing_remote_commands_server.h
@@ -0,0 +1,123 @@
+// 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.
+
+#ifndef COMPONENTS_POLICY_CORE_COMMON_REMOTE_COMMANDS_TESTING_REMOTE_COMMANDS_SERVER_H_
+#define COMPONENTS_POLICY_CORE_COMMON_REMOTE_COMMANDS_TESTING_REMOTE_COMMANDS_SERVER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
+#include "components/policy/core/common/remote_commands/remote_command_job.h"
+#include "policy/proto/device_management_backend.pb.h"
+
+namespace base {
+class Clock;
+class SingleThreadTaskRunner;
+}  // namespace base
+
+namespace policy {
+
+// This class implements server side logic for remote commands service. It

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

Nit 1: s/server side/server-side/
Nit 2: s/service/service tests/

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

Done.

+// acts just like a queue, and there are mainly two exposed methods for this
+// purpose. Test authors are expected to call IssueCommand() to add a command

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

Nit: s/a command/commands/

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

Done.

+// to the queue in their tests. The use of FetchCommands() is internal only for

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

Nit: It is not that internal anymore now that we have at least two very
different (external) users of this method. How about something like:
"FetchCommands() should be called when serving a request to the server
intercepted e.g. via a net::URLRequestInterceptor or a mock CloudPolicyClient."

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

Done.

+// classes like subclasses of net::URLRequestInterceptor, or Mocked

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

Nit 1: s/, or Mocked/ or a mocked/

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

Done.

+// CloudPolicyClient.  In addition, this class also supports verifying command
+// result sent back to server, and test authors should provide a callback for
+// this purpose when issue a command.

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

Nit: s/issue/issuing/

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

Done.

+// All method (including ctor/dtor) except FetchCommands should be called

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

Nit 1: s/method/methods/
Nit 2: No need to explicitly list the constructor and destructor. All method
means all methods.
Nit 3: s/FetchCommands/FetchCommands()/

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

Done.

+// from the same thread (like UI thread), and FetchCommands() can be called from

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

Nit: s/ (like UI thread)// - This class is entirely agnostic to what thread it
runs on, as long as it is the same thread throughout its entire lifetime.

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

Done.

+// any thread.

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

Nit: Document that the test author is responsible for ensuring that
FetchCommands() is not called from another thread after |this| has been
destroyed.

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

Done.

+class TestingRemoteCommandsServer {
+ public:
+  TestingRemoteCommandsServer();
+  virtual ~TestingRemoteCommandsServer();
+
+  using RemoteCommands = std::vector<enterprise_management::RemoteCommand>;
+  using RemoteCommandResults =
+      std::vector<enterprise_management::RemoteCommandResult>;
+
+  // Callback called when the command job result is reported back to server.

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

Nit 1: s/the command job/a command's/
Nit 2: s/to/to the/

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

Done.

+  using ResultReportedCallback =
+      base::Callback<void(const enterprise_management::RemoteCommandResult&)>;
+
+  // Create and add a command with |type| as command type and |payload| as
+  // command payload if it's not empty. |clock_| will be used to get the
+  // command issued time. |reported_callback| will be called from the same

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

Nit: s/issued/issue/

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

Done.

+  // thread when a command result is reported back to server.

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

Nit: s/to/to the/

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

Done.

+  // |skip_next_fetch| should be set default to false, and if it's set to true

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

Nit: I think we can omit the specification of what should be default. How about
"If |skip_next_fetch| is true, the command will only actually be added to the
queue after the next fetch completes?"

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

Done.

+  // instead, it will only be added to the main queue after next fetch

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

Nit 1: s/it/the command/
Nit 2: s/after/after the/

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

Acknowledged.

+  // completes. Both |fetched_callback|s and |reported_callback|s are expected

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

Nit: There is no |fetched_callback|.

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

Done.

+  // to be called if they are not null and will be considered as failure

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

This sentence is unclear to me. Are you saying the test should be considered
failed if the callback is never invoked?

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

Rewrite this piece of comments.

+  // otherwise.
+  void IssueCommand(enterprise_management::RemoteCommand_Type type,
+                    const std::string& payload,
+                    const ResultReportedCallback& reported_callback,
+                    bool skip_next_fetch);
+
+  // Fetch commands with |last_command_id| and provide |previous_job_results|

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

Nit: s/with |last_command_id|/, acknowledging all commands up to and including
|last_command_id|,/

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

Done.

+  // as results for previous commands, see protobuf definition for protocol

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

Nit 1: s/, see/. See the/
Nit 2: s/protocol definition/a definition of the protocol/

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

Done.

+  // definition between server and client for remote commands fetching.

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

Nit: s/commands/command/

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

Done.

+  // Unlike every other methods in the class, this method can be called from
+  // any thread.
+  RemoteCommands FetchCommands(
+      RemoteCommandJob::UniqueIDType last_command_id,
+      const RemoteCommandResults& previous_job_results);
+
+  // Set alternative Clock for command issued time. The default clock uses

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

Nit 1: s/Clock/clock/
Nit 2: s/for command issued/for obtaining the command issue/

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

Done.

+  // system clock.

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

Nit: s/system/the system/

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

Done.

+  void SetClock(scoped_ptr<base::Clock> clock);
+
+  // Get the number of commands which are still expecting command results for

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

Nit: s/which are still expecting command results for verifying/for which no
results have been reported yet/

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

Done.

+  // verifying. Note that it's possible that some command is not even fetched.

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

Nit 1: s/is/have/
Nit 2: s/even/even been/
Nit 3: It is not clear to me how these unfetched commands affect the value
returned here. If there are 3 fetched and 2 unfetched commands missing results,
will this method return 3 or 5?

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

5 in the case. The server don't have records of fetched commands, so it's
difficult to distinguish between fetched and unfetched comments.

Updated comments accordingly.

+  size_t NumberOfCommandsPendingResult() const;

[bartfab (slow), 2015/02/28 00:32:42]

Nit: #include <stddef.h>

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

Done.

+
+ private:
+  struct RemoteCommandWithCallback;
+
+  void ReportJobResult(
+      const ResultReportedCallback& reported_callback,
+      const enterprise_management::RemoteCommandResult& job_result) const;
+
+  // The main commands queue.

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

Nit: s/commands/command/

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

Done.

+  std::vector<RemoteCommandWithCallback> commands_;
+
+  // Commands which will be added to the main queue after the next fetch.
+  std::vector<RemoteCommandWithCallback> commands_issued_after_next_fetch_;
+
+  // The latest acknowledged command ID from client, should be non-decreasing.

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

Nit 1: s/from/from the/

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

Done.

+  RemoteCommandJob::UniqueIDType last_acknowledged_unique_id_ = 0;
+
+  // The latest command ID generated by server. This test server generates
+  // strictly monotonically increasing unique IDs.

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

Nit: If you swap the |last_generated_unique_id_| and
|last_acknowledged_unique_id_| members, the command explaining that the IDs are
strictly monotonically increasing will come earlier, making it clear why the
|last_acknowledged_unique_id_| is expected to be non-decreasing.

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

Done.

+  RemoteCommandJob::UniqueIDType last_generated_unique_id_ = 0;
+
+  // Clock used to generate command issued time when IssueCommand() is called.

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

Nit: s/command issued/the command issze/

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

Done.

+  scoped_ptr<base::Clock> clock_;
+
+  // A lock protecting the command queues and recorded IDs.

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

Nit: s/recorded/generated and acknowledged/

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

Done.

+  base::Lock lock_;
+
+  // The thread on which this test server is created, usually UI thread.

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

Nit: s/usually/usually the/

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

Done.

+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

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

Nit: You said why you need a |SingleThreadTaskRunner| but I still do not
understand. Even though ThreadTaskRunnerHandle::Get() returns a
|SingleThreadTaskRunner|, that is a subclass of |TaskRunner|, so you are not
obliged to store it in a |SingleThreadTaskRunner| member.

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

Yes. But I want to state that this task runner runs on single thread, and it
will make code easier to read.

+
+  // A weak pointer created early so that it can be accessed from other thread.

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

Nit: s/thread/threads/

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

Done.

+  base::WeakPtr<TestingRemoteCommandsServer> weak_ptr_to_this_;
+
+  base::ThreadChecker thread_checker_;
+  base::WeakPtrFactory<TestingRemoteCommandsServer> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestingRemoteCommandsServer);
+};
+
+}  // namespace policy
+
+#endif  // COMPONENTS_POLICY_CORE_COMMON_REMOTE_COMMANDS_TESTING_REMOTE_COMMANDS_SERVER_H_