Initial RemoteCommandService

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

Index: components/policy/core/common/remote_commands/remote_commands_service.h
diff --git a/components/policy/core/common/remote_commands/remote_commands_service.h b/components/policy/core/common/remote_commands/remote_commands_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..7d2afba163c20dc842eaa6e06c82f51e8f54f4bc
--- /dev/null
+++ b/components/policy/core/common/remote_commands/remote_commands_service.h
@@ -0,0 +1,96 @@
+// 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_REMOTE_COMMANDS_SERVICE_H_
+#define COMPONENTS_POLICY_CORE_COMMON_REMOTE_COMMANDS_REMOTE_COMMANDS_SERVICE_H_
+
+#include <set>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "components/policy/core/common/remote_commands/remote_command_job.h"
+#include "components/policy/core/common/remote_commands/remote_commands_queue.h"
+#include "components/policy/policy_export.h"
+#include "policy/proto/device_management_backend.pb.h"
+
+namespace base {
+class Clock;
+}  // namespace base
+
+namespace policy {
+
+class CloudPolicyClient;
+class RemoteCommandsFactory;
+
+// Service class which will connect to a CloudPolicyClient in order to fetch
+// remote commands from DMServer and send results for executed commands
+// back to server.

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

Nit: s/to/to the/

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

Done.

+class POLICY_EXPORT RemoteCommandsService
+    : public RemoteCommandsQueue::Observer {
+ public:
+  RemoteCommandsService(scoped_ptr<RemoteCommandsFactory> factory,
+                        CloudPolicyClient* client);
+  ~RemoteCommandsService() override;
+
+  // Attempts to fetch remote commands, mainly supposed to be called by
+  // invalidation service. Note that there will be at most one ongoing fetch
+  // request and all other fetch request will be enqueued if another fetch
+  // request is in-progress. And in such case, a following up request will be

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

Nit 1: s/such/such a/
Nit 2: a/a following/another/

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

Done.

+  // made immediately after current ongoing requests finishes. Returns true if a

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

Nit 1: s/current/the current/
Nit 2: s/requests/request/

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

Done.

+  // new command fetch request is created.

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

Nit: It was not clear to me what "Returns true if a new command fetch request is
created" means without looking at the code. How about "Returns true if the new
request was started immediately. Returns false if another request was in
progress already and the new request got enqueued?"

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

Done.

+  bool FetchRemoteCommands();
+
+  // Returns whether a command fetch request is in progress or not.
+  bool IsCommandFetchInProgressForTesting() const {
+    return command_fetch_in_progress_;
+  }
+
+  // Set an alternative clock for testing.
+  void SetClockForTesting(scoped_ptr<base::Clock> clock);
+
+ private:
+  // Helper function to add an command which we get from server.

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

Nit 1: How about s/add/enqueue/?
Nit 2: s/an/a/

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

Done. (Changed function name as well)

+  void AddCommand(const enterprise_management::RemoteCommand& command);
+
+  // RemoteCommandsQueue::Observer:
+  void OnJobStarted(RemoteCommandJob* command) override;
+  void OnJobFinished(RemoteCommandJob* command) override;
+
+  // Callback to handle commands we get from server.

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

Nit: s/from/from the/

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

Done.

+  void OnRemoteCommandsFetched(
+      const std::vector<enterprise_management::RemoteCommand>& commands);
+
+  // Whether there is a command fetch on going or not.
+  bool command_fetch_in_progress_ = false;
+
+  // Whether there is an enqueued fetch request, which indicates there are

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

Nit: s/are/were/

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

Done.

+  // additional FetchRemoteCommands() calls while a fetch request is ongoing.

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

Nit: s/is/was/

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

Done.

+  bool has_enqueued_fetch_request_ = false;
+
+  // Commands results have not been sent back to server yet.

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

Nit 1: s/Commands/Command/
Nit 2: s/have/that have/
Nit 3: s/server/the server/

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

Done.

+  std::vector<enterprise_management::RemoteCommandResult> unsent_results_;
+
+  // ID for the latest command which has started execution. We will acknowledge

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

Nit: s/for/of/

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

Done.

+  // the server with this ID so that we can re-fetch some commands not

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

Nit: s/some commands not/commands that have not been/

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

Done.

+  // executed yet after a crash or browser restart.
+  RemoteCommandJob::UniqueIDType last_executed_command_id_ = 0;

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

1: We cannot use 0 as a magical value here. Command IDs are entirely opaque and
0 may just as well be a valid ID that the server uses. We need to send *no* ID
instead when no commands have been fetched and processed yet.

2: As discussed offline, I want to make sure we know how persistence will work
for the reboot command before we land this CL. If we acknowledge a command the
moment it starts running, a reboot command will *not* be re-run after a reboot.
This is OK, but it means we need to figure out another way to figure out a
reboot just happened and there is a result to send back to the server.

[binjin, 2015/03/09 16:31:56]

1. Done.
2. We will introduce commands specified timing for job report, on
Started/Finished/Stop And Blocking the queue.

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

How will 2. work across reboots? How will we know after a reboot that a reboot
command was in the queue and just finished?

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

We won't acknowledge the commands before rebooting and thus we will re-fetch it
after the reboot with help of timestamps.

+
+  // Collects all IDs for fetched commands. We need this since the command id

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

Nit 1: s/all IDs for/the IDs of all/
Nit 2: Document that we do not keep the IDs of all commands we ever fetched in
this set. We only store an ID until the command is executed and acknowledged.

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

1. Done.
2. Actually the current implementation will not remove IDs from this set after
executed and acknowledged. IDs have low memory footprint and keeping all IDs
will also help avoiding the case that server send commands regardless of what we
acknowledged before.

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

I think we should fix 2. Once a command has been acknowledged, the server is
free to reuse that command ID in the future. We should remove it from our set of
fetched commands.

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

Done.

+  // is opaque.
+  std::set<RemoteCommandJob::UniqueIDType> fetched_command_ids_;
+
+  RemoteCommandsQueue queue_;
+  scoped_ptr<RemoteCommandsFactory> factory_;
+  CloudPolicyClient* client_;

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

Nit: const.

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

Done. Mark the pointer itself constant instead.

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

Thanks, this is what I meant.

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

Acknowledged.

+
+  base::WeakPtrFactory<RemoteCommandsService> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemoteCommandsService);
+};
+
+}  // namespace policy
+
+#endif  // COMPONENTS_POLICY_CORE_COMMON_REMOTE_COMMANDS_REMOTE_COMMANDS_SERVICE_H_