Remote set volume command

Remote set volume command

This CL introduces a remote command that enables device administrators to remotely set device volume on CrOS devices.

More context can be found by reading the design doc go/remote-audio-management

BUG=682792
Review-Url: https://codereview.chromium.org/2733903002
Cr-Commit-Position: refs/heads/master@{#455469}
Committed: https://chromium.googlesource.com/chromium/src/+/0f397d38f3023f90d0765ac41416cb1fb4226609

[Ivan Šandrk, 2017-03-06 17:38:28]

isandrk@chromium.org changed reviewers:
+ atwilson@chromium.org

[Ivan Šandrk, 2017-03-06 17:38:28]

Hey Drew, please take a look!

Ivan

[Ivan Šandrk, 2017-03-06 17:39:01]

Description was changed from

==========
Remote set volume command

This CL introduces a remote command that enables device administrators to
remotely set device volume.

More context can be found by reading the design doc go/remote-audio-management

BUG=682792
==========

to

==========
Remote set volume command

This CL introduces a remote command that enables device administrators to
remotely set device volume on CrOS devices.

More context can be found by reading the design doc go/remote-audio-management

BUG=682792
==========

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

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc
(right):

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc:49:
kCommandExpirationTimeInMinutes);
I wonder if these commands should ever expire (or should have a longer
expiration time) - maybe ask Raj + Bartosz for their opinion around this
specific command

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc:61:
if (!payload->GetInteger(kVolumeFieldName, &volume_))
Do you want to validate the value (make sure it is in the right range?)

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h
(right):

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h:22:
virtual void SetOutputVolumePercent(int volume_percent);
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)

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h:33:
// RemoteCommandJob:
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)

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h:40:
int volume_;
Explain what this is - a value from 0-100, I guess?

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job_unittest.cc
(right):

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job_unittest.cc:108:
TEST_F(DeviceCommandSetVolumeTest, Success) {
Are there other cases we should test for - malformed payload, expired command,
etc?

[Ivan Šandrk, 2017-03-07 16:14:04]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 16:14:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Bartosz thinks the default (used in other commands) is fine.

I think it doesn't matter too much actually because the command gets executed in
under 10s pretty much every time.

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc
(right):

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc:49:
kCommandExpirationTimeInMinutes);
On 2017/03/07 11:45:14, Andrew T Wilson (Slow) wrote:
> I wonder if these commands should ever expire (or should have a longer
> expiration time) - maybe ask Raj + Bartosz for their opinion around this
> specific command

Will shoot them an email.

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc:61:
if (!payload->GetInteger(kVolumeFieldName, &volume_))
On 2017/03/07 11:45:14, Andrew T Wilson (Slow) wrote:
> Do you want to validate the value (make sure it is in the right range?)

Done.

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h
(right):

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h:22:
virtual void SetOutputVolumePercent(int volume_percent);
On 2017/03/07 11:45:14, Andrew T Wilson (Slow) wrote:
> 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)

Good suggestion, thanks.

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h:33:
// RemoteCommandJob:
On 2017/03/07 11:45:14, Andrew T Wilson (Slow) wrote:
> 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)

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

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h:40:
int volume_;
On 2017/03/07 11:45:14, Andrew T Wilson (Slow) wrote:
> Explain what this is - a value from 0-100, I guess?

Done.

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job_unittest.cc
(right):

https://codereview.chromium.org/2733903002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job_unittest.cc:108:
TEST_F(DeviceCommandSetVolumeTest, Success) {
On 2017/03/07 11:45:14, Andrew T Wilson (Slow) wrote:
> Are there other cases we should test for - malformed payload, expired command,
> etc?

Added a couple more.

[commit-bot: I haz the power, 2017-03-07 19:27:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 19:27:34]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2017-03-08 12:50:56]

lgtm with nits you can either address or ignore as you like.

https://codereview.chromium.org/2733903002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc
(right):

https://codereview.chromium.org/2733903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc:58:
if (volume_ < 0 || volume_ > 100)
I would probably log an error in this case?

https://codereview.chromium.org/2733903002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h
(right):

https://codereview.chromium.org/2733903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h:22:
void SetVolumeCallbackForTesting(const base::Callback<void(int)>& callback);
Maybe create a typedef for this callback?

[Ivan Šandrk, 2017-03-08 15:06:47]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 15:06:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2017-03-08 15:07:11]

https://codereview.chromium.org/2733903002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc
(right):

https://codereview.chromium.org/2733903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.cc:58:
if (volume_ < 0 || volume_ > 100)
On 2017/03/08 12:50:56, Andrew T Wilson (Slow) wrote:
> I would probably log an error in this case?

Added a more generic logging code at callsite (RemoteCommandJob::Init).

https://codereview.chromium.org/2733903002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h
(right):

https://codereview.chromium.org/2733903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_set_volume_job.h:22:
void SetVolumeCallbackForTesting(const base::Callback<void(int)>& callback);
On 2017/03/08 12:50:56, Andrew T Wilson (Slow) wrote:
> Maybe create a typedef for this callback?

Done.

[Ivan Šandrk, 2017-03-08 15:07:17]

The CQ bit was unchecked by isandrk@chromium.org

[Ivan Šandrk, 2017-03-08 15:07:18]

The CQ bit was checked by isandrk@chromium.org

[Ivan Šandrk, 2017-03-08 15:07:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2733903002/#ps40001
(title: "Nits + mute/unmute logic")

[commit-bot: I haz the power, 2017-03-08 15:07:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 16:41:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488985638687020,
"parent_rev": "67b81bc38c7e2164bc77f0c00d11d26d620edc18", "commit_rev":
"0f397d38f3023f90d0765ac41416cb1fb4226609"}

[commit-bot: I haz the power, 2017-03-08 16:41:50]

Description was changed from

==========
Remote set volume command

This CL introduces a remote command that enables device administrators to
remotely set device volume on CrOS devices.

More context can be found by reading the design doc go/remote-audio-management

BUG=682792
==========

to

==========
Remote set volume command

This CL introduces a remote command that enables device administrators to
remotely set device volume on CrOS devices.

More context can be found by reading the design doc go/remote-audio-management

BUG=682792

Review-Url: https://codereview.chromium.org/2733903002
Cr-Commit-Position: refs/heads/master@{#455469}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f397d38f3023f90d0765ac41416...
==========

[commit-bot: I haz the power, 2017-03-08 16:41:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0f397d38f3023f90d0765ac41416...