Check Feature Policy for Mic/Camera requests

Check Feature Policy for Mic/Camera requests

Mic/Camera requests don't currently go through the permissions service so we
need to implement additional checks. These happen in
MediaDevicesPermissionChecker. Tests have also been added for these.

BUG=689802
Review-Url: https://codereview.chromium.org/2931783002
Cr-Commit-Position: refs/heads/master@{#479177}
Committed: https://chromium.googlesource.com/chromium/src/+/7938f5d3da218f7d59feddd5cebda6079fd8f09b

[raymes, 2017-06-08 05:24:12]

raymes@chromium.org changed reviewers:
+ guidou@chromium.org

[raymes, 2017-06-08 05:31:47]

Oh I just realized I also need to add checks in media_stream_ui_proxy.cc. Feel
free to take a look or to wait until I add the checks there too :)

[Guido Urdaneta, 2017-06-08 12:08:23]

The code looks good, but take a look at the comment about ScopedFeatureList.

https://codereview.chromium.org/2931783002/diff/1/content/browser/media/media...
File content/browser/media/media_devices_permission_checker_unittest.cc (right):

https://codereview.chromium.org/2931783002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker_unittest.cc:103:
base::test::ScopedFeatureList feature_list;
Can you test this without having ScopedFeatureList as local variables in the
test?

There will be problems if you add another test that also uses a local
ScopedFeatureList. ScopedFeatureList manipulates a global variable and two tests
running in parallel setting different values for ScopedFeatureList will cause a
race.

A possible workaround is to make the ScopedFeatureList a field , and make the
test parameterized (using TEST_P), with the ScopedFeatureList initialized in the
constructor or SetUp method with different values depending on the parameter.

[raymes, 2017-06-13 04:12:19]

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

[commit-bot: I haz the power, 2017-06-13 04:12:36]

Dry run: CQ is trying da patch.

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

[raymes, 2017-06-13 04:13:21]

Thanks Guido! ptal.

I will put up the MediaStreamUIProxy change in a separate CL (this is necessary
to gate calls to RequestPermission).

https://codereview.chromium.org/2931783002/diff/1/content/browser/media/media...
File content/browser/media/media_devices_permission_checker_unittest.cc (right):

https://codereview.chromium.org/2931783002/diff/1/content/browser/media/media...
content/browser/media/media_devices_permission_checker_unittest.cc:103:
base::test::ScopedFeatureList feature_list;
On 2017/06/08 12:08:22, Guido Urdaneta wrote:
> Can you test this without having ScopedFeatureList as local variables in the
> test?
> 
> There will be problems if you add another test that also uses a local
> ScopedFeatureList. ScopedFeatureList manipulates a global variable and two
tests
> running in parallel setting different values for ScopedFeatureList will cause
a
> race.
> 
> A possible workaround is to make the ScopedFeatureList a field , and make the
> test parameterized (using TEST_P), with the ScopedFeatureList initialized in
the
> constructor or SetUp method with different values depending on the parameter.

Hmm, is that really true? What would be the difference between having the scoped
feature list as a field vs a local variable? I believe a new instance of the
test harness would be created for each test? 

Many of the tests I see in code search use a local ScopedFeatureList:
https://cs.chromium.org/search/?q=scopedfeaturelist&type=cs

[commit-bot: I haz the power, 2017-06-13 05:34:23]

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

[commit-bot: I haz the power, 2017-06-13 05:34:24]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-06-13 19:47:03]

On 2017/06/13 04:13:21, raymes wrote:
> Thanks Guido! ptal.
> 
> I will put up the MediaStreamUIProxy change in a separate CL (this is
necessary
> to gate calls to RequestPermission).
> 
>
https://codereview.chromium.org/2931783002/diff/1/content/browser/media/media...
> File content/browser/media/media_devices_permission_checker_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2931783002/diff/1/content/browser/media/media...
> content/browser/media/media_devices_permission_checker_unittest.cc:103:
> base::test::ScopedFeatureList feature_list;
> On 2017/06/08 12:08:22, Guido Urdaneta wrote:
> > Can you test this without having ScopedFeatureList as local variables in the
> > test?
> > 
> > There will be problems if you add another test that also uses a local
> > ScopedFeatureList. ScopedFeatureList manipulates a global variable and two
> tests
> > running in parallel setting different values for ScopedFeatureList will
cause
> a
> > race.
> > 
> > A possible workaround is to make the ScopedFeatureList a field , and make
the
> > test parameterized (using TEST_P), with the ScopedFeatureList initialized in
> the
> > constructor or SetUp method with different values depending on the
parameter.
> 
> Hmm, is that really true? What would be the difference between having the
scoped
> feature list as a field vs a local variable? I believe a new instance of the
> test harness would be created for each test? 
> 
> Many of the tests I see in code search use a local ScopedFeatureList:
> https://cs.chromium.org/search/?q=scopedfeaturelist&type=cs

OK. Let's land it and fix it later if it gives problems when other tests are
added.
lgtm

[raymes, 2017-06-13 22:12:25]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-06-13 22:12:55]

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-06-13 22:19:17]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1497391945968430, "parent_rev":
"c3ae0fb25a81eb3807096daccd5554f2d020f3d5", "commit_rev":
"7938f5d3da218f7d59feddd5cebda6079fd8f09b"}

[commit-bot: I haz the power, 2017-06-13 22:19:28]

Description was changed from

==========
Check Feature Policy for Mic/Camera requests

Mic/Camera requests don't currently go through the permissions service so we
need to implement additional checks. These happen in
MediaDevicesPermissionChecker. Tests have also been added for these.

BUG=689802
==========

to

==========
Check Feature Policy for Mic/Camera requests

Mic/Camera requests don't currently go through the permissions service so we
need to implement additional checks. These happen in
MediaDevicesPermissionChecker. Tests have also been added for these.

BUG=689802

Review-Url: https://codereview.chromium.org/2931783002
Cr-Commit-Position: refs/heads/master@{#479177}
Committed:
https://chromium.googlesource.com/chromium/src/+/7938f5d3da218f7d59feddd5cebd...
==========

[commit-bot: I haz the power, 2017-06-13 22:19:29]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/7938f5d3da218f7d59feddd5cebd...