Initial RemoteCommandService

chrome/browser/policy/remote_commands/testing_remote_commands_server.h

Index: chrome/browser/policy/remote_commands/testing_remote_commands_server.h
diff --git a/chrome/browser/policy/remote_commands/testing_remote_commands_server.h b/chrome/browser/policy/remote_commands/testing_remote_commands_server.h
new file mode 100644
index 0000000000000000000000000000000000000000..259a7519a61307efde7564b1b55e9ce4c54cb8e9
--- /dev/null
+++ b/chrome/browser/policy/remote_commands/testing_remote_commands_server.h
@@ -0,0 +1,66 @@
+// 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 CHROME_BROWSER_POLICY_REMOTE_COMMANDS_TESTING_REMOTE_COMMANDS_SERVER_H_
+#define CHROME_BROWSER_POLICY_REMOTE_COMMANDS_TESTING_REMOTE_COMMANDS_SERVER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/lock.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 {
+
+class TestingRemoteCommandsServer {

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

Without any documentation, it is hard to understand what the methods do and how
they work together.

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

Acknowledged.

+ public:
+  TestingRemoteCommandsServer();
+  virtual ~TestingRemoteCommandsServer();
+
+  // XXX
+  void IssueCommand(enterprise_management::RemoteCommand_Type type,
+                    const std::string& payload);
+
+  // XXX
+  void SetClockForTesting(scoped_ptr<base::Clock> clock);

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

No need for the "ForTesting" prefix. This is testing code after all.

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

Done.

+
+  // XXX
+  size_t GetUnreportedCommandCount() const;

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

Nit: "Unreported" is not clear to me. This seems to return the number of
commands for which no results have been sent back yet. I think it would be good
to find a better name that reflects this.

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

Done.

+
+  // XXX
+  void FetchCommandsFromIO(
+      RemoteCommandJob::UniqueIDType last_command_id,
+      const std::vector<enterprise_management::RemoteCommandResult>&
+          previous_job_results,
+      std::vector<enterprise_management::RemoteCommand>* fetched_commands);

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

Why not have |std::vector<enterprise_management::RemoteCommand> as the return
value?

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

Done.

+
+ protected:
+  // XXX
+  virtual void JobResultReported(
+      const enterprise_management::RemoteCommandResult& job_result) const {}

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

Nit: Never inline virtual methods. The style guide forbids it.

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

Done.

+
+ private:
+  std::vector<enterprise_management::RemoteCommand> commands_;
+  RemoteCommandJob::UniqueIDType last_acknowledged_id_;

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

Nit: s/id/unique_id/ for consistency.

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

Done.

+  RemoteCommandJob::UniqueIDType last_generated_unique_id_;

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

Nit: Document that this test server generates strictly monotonically increasing
unique IDs. In the general case, the IDs are opaque and do not have to follow
any specific pattern.

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

Done.

+
+  scoped_ptr<base::Clock> clock_;
+  base::Lock lock_;

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

Locks are avoided in Chrome code whenever possible. In your case, the lock is
definitely avoidable. The task queue should live on one thread, either UI or IO.
When the other thread wishes to access the queue, it must post a task and wait
for the reply to come back.

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

I will keep the lock as we discussed offline.

+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

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

Nit 1: base::SequencedTaskRunner or even base::TaskRunner is sufficient here.
Nit 2: Document what this is used for.

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

SingleThreadTaskRunner is required single I need to ensure that task runner
bounds to the same thread from constructor.

+
+  DISALLOW_COPY_AND_ASSIGN(TestingRemoteCommandsServer);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_POLICY_REMOTE_COMMANDS_TESTING_REMOTE_COMMANDS_SERVER_H_