Make sure we don't open audio or video device if blocked by policy.

Make sure we don't open audio or video device if blocked by policy.

Adding a check if the type (audio, video) was requested and allowed before adding the device to the list.

BUG=259567
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220559

[Henrik Grunell, 2013-08-28 15:36:46]

There should be a test for this too. I'll take a look at that, whether a unit
test or browser test makes more sense.

How do I manually test open device request?

Wei: added you too since I think you've been involved in this code. Just let me
know if you don't have anything to contribute.

Julian: can you test that your case works with the patch?

[tommi (sloooow) - chröme, 2013-08-28 16:13:47]

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
chrome/browser/media/media_stream_devices_controller.cc:229: const
content::MediaStreamDevice* video_device =
Wouldn't it make more sense to check the allowed bools here as well as above?

[tommi (sloooow) - chröme, 2013-08-28 16:14:20]

On 2013/08/28 16:13:47, tommi wrote:
>
https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
> File chrome/browser/media/media_stream_devices_controller.cc (right):
> 
>
https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
> chrome/browser/media/media_stream_devices_controller.cc:229: const
> content::MediaStreamDevice* video_device =
> Wouldn't it make more sense to check the allowed bools here as well as above?

Lgtm with that change applied.

[tommi (sloooow) - chröme, 2013-08-28 16:14:26]

On 2013/08/28 16:13:47, tommi wrote:
>
https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
> File chrome/browser/media/media_stream_devices_controller.cc (right):
> 
>
https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
> chrome/browser/media/media_stream_devices_controller.cc:229: const
> content::MediaStreamDevice* video_device =
> Wouldn't it make more sense to check the allowed bools here as well as above?

Lgtm with that change applied.

[wjia(left Chromium), 2013-08-28 16:57:35]

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
chrome/browser/media/media_stream_devices_controller.cc:214: } case
content::MEDIA_GENERATE_STREAM: {
nit: drop the case to a new line. Somehow, this slipped through.

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
chrome/browser/media/media_stream_devices_controller.cc:216: bool
needs_video_device = video_allowed;
Should these be the following?
  needs_*_device = *_allowed && request_.*_type ==
content::MEDIA_DEVICE_*_CAPTURE;

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
chrome/browser/media/media_stream_devices_controller.cc:248: } case
content::MEDIA_DEVICE_ACCESS:
ditto.

[Henrik Grunell, 2013-08-29 08:19:51]

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
chrome/browser/media/media_stream_devices_controller.cc:214: } case
content::MEDIA_GENERATE_STREAM: {
On 2013/08/28 16:57:35, wjia wrote:
> nit: drop the case to a new line. Somehow, this slipped through.

Done. Also added {} to the other case blocks to be consistent.

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
chrome/browser/media/media_stream_devices_controller.cc:216: bool
needs_video_device = video_allowed;
On 2013/08/28 16:57:35, wjia wrote:
> Should these be the following?
>   needs_*_device = *_allowed && request_.*_type ==
> content::MEDIA_DEVICE_*_CAPTURE;

For request_.request_type == content::MEDIA_GENERATE_STREAM:

If request_.*_type == content::MEDIA_DEVICE_*_CAPTURE then
content::MEDIA_DEVICE_*_CAPTURE permission will be in request_permissions_. (See
ctor.) That is checked in IsDevice*CaptureRequestedAndAllowed() so it's not
needed here.

Above it's needed because for content::MEDIA_OPEN_DEVICE case, we set the
content::MEDIA_DEVICE_*_CAPTURE permission. This is ugly and confusing, etc.
There's a todo comment for fixing this in the ctor.

Preferably, most of this class should be refactored/rewritten at some point.

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
chrome/browser/media/media_stream_devices_controller.cc:229: const
content::MediaStreamDevice* video_device =
On 2013/08/28 16:13:47, tommi wrote:
> Wouldn't it make more sense to check the allowed bools here as well as above? 

Agree. Then I also renamed the needs_* bools to get_default_* since it reflects
better what they're for.

https://codereview.chromium.org/23453012/diff/1/chrome/browser/media/media_st...
chrome/browser/media/media_stream_devices_controller.cc:248: } case
content::MEDIA_DEVICE_ACCESS:
On 2013/08/28 16:57:35, wjia wrote:
> ditto.

Done.

[wjia(left Chromium), 2013-08-29 17:31:44]

lgtm

[commit-bot: I haz the power, 2013-08-30 09:35:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/23453012/26001

[commit-bot: I haz the power, 2013-08-30 12:14:46]

Change committed as 220559