Remote set volume command

chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h

+// Copyright 2017 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_CHROMEOS_POLICY_REMOTE_COMMANDS_DEVICE_COMMAND_SET_VOLUME_JOB_H_
+#define CHROME_BROWSER_CHROMEOS_POLICY_REMOTE_COMMANDS_DEVICE_COMMAND_SET_VOLUME_JOB_H_
+
+#include <memory>
+#include <string>
+
+#include "base/macros.h"
+#include "components/policy/core/common/remote_commands/remote_command_job.h"
+
+namespace policy {
+
+class DeviceCommandSetVolumeJob : public RemoteCommandJob {
+ public:
+  // Cras delegate used for setting the output volume.
+  class CrasDelegate {
+   public:
+    virtual ~CrasDelegate();
+    virtual void SetOutputVolumePercent(int volume_percent);

[Andrew T Wilson (Slow), 2017/03/07 11:45:14]

I'm guessing you only need this for testing, since you're defining default
behavior.

Two things:

1) Unless you expect to add more APIs to this class in the future, you should
use a callback instead.
2) In this specific case, I'd probably not take a param in the constructor, but
instead have something like "SetVolumeCallbackForTesting()" which sets an
internal callback. Then, you can have code like this:

if (callback_.is_null()) {
  CrasAudioHandler::Get()->SetOutputVolumePercent(vol)
else
  callback_.Run(vol)

[Ivan Šandrk, 2017/03/07 16:15:31]

Good suggestion, thanks.

+  };
+
+  explicit DeviceCommandSetVolumeJob(
+      std::unique_ptr<CrasDelegate> cras_delegate);
+  ~DeviceCommandSetVolumeJob() override;
+
+  // RemoteCommandJob:
+  enterprise_management::RemoteCommand_Type GetType() const override;
+
+ private:
+  // RemoteCommandJob:

[Andrew T Wilson (Slow), 2017/03/07 11:45:14]

In general, if you are publicly deriving from RemoteCommandJob, you should also
make all your overrides public. Making them private doesn't really help, since
anyone can just invoke these APIs on your object by casting to RemoteCommandJob
where the APIs are public, like so:

static_cast<RemoteCommandJob>(volume_job)->IsExpired(now)

[Ivan Šandrk, 2017/03/07 16:15:31]

Hm I followed what Chris was doing in the older code. Fixed it now (I've put
public in public, and protected in protected).

+  bool IsExpired(base::TimeTicks now) override;
+  bool ParseCommandPayload(const std::string& command_payload) override;
+  void RunImpl(const CallbackWithResult& succeeded_callback,
+               const CallbackWithResult& failed_callback) override;
+  base::TimeDelta GetCommmandTimeout() const override;
+
+  int volume_;

[Andrew T Wilson (Slow), 2017/03/07 11:45:14]

Explain what this is - a value from 0-100, I guess?

[Ivan Šandrk, 2017/03/07 16:15:31]

Done.

+
+  std::unique_ptr<CrasDelegate> cras_delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeviceCommandSetVolumeJob);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_CHROMEOS_POLICY_REMOTE_COMMANDS_DEVICE_COMMAND_SET_VOLUME_JOB_H_