Change MediaStreamDevicesController tests to use RequestPermissions function

Make MediaStreamDevicesController tests go through the RequestPermissions API

In https://codereview.chromium.org/2706813004/ the consumers of
MediaStreamDevicesController were changed to use a static RequestPermissions
function instead of constructing the object directly. This facilitated moving
the prompting logic closer to the permission status checking logic which will
help us move to using PermissionManager to request permissions.

This CL does the same things for tests that are consumers of
MediaStreamDevicesController. This ensures that if we move any logic out of the
class and into the static entry point, that tests will continue to work
correctly.

In order to do this it means that tests still need a way to simulate
accepting permission prompts. Previously they did this by operating directly on
the MediaStreamDevicesController object, but that has been taken away. It has
been replaced by a private version of the entry point:
RequestPermissionsWithDelegate. This allows them to pass in a delegate which
can be used to determine whether prompts have been shown and to accept/deny
them.

The big benefit of this approach is that the delegate has been given the same
API as the MockPermissionPromptFactory. MockPermissionPromptFactory is the
class that is used to simulate/control permission prompts in tests of all other
permissions that use PermissionRequestManager. So, when the refactoring in
https://crbug.com/606138 is completed, we can replace the Delegate with a
MockPermissionPromptFactory and the tests themselves will not need to be
altered.

BUG=596786
Review-Url: https://codereview.chromium.org/2711883003
Cr-Commit-Position: refs/heads/master@{#454974}
Committed: https://chromium.googlesource.com/chromium/src/+/2d2d7d754045d63fd75aaee27026d27ae1e2fdab

[raymes, 2017-02-23 04:48:11]

Description was changed from

==========
Change MediaStreamDevicesController tests to use RequestPermissions function

BUG=
==========

to

==========
Make MediaStreamDevicesController tests go through the RequestPermissions API

In https://codereview.chromium.org/2706813004/ the consumers of
MediaStreamDevicesController were changed to use a static RequestPermissions
function instead of constructing the object directly. This facilitated moving
the prompting logic closer to the permission status checking logic which will
help us move to using PermissionManager to request permissions.

This CL does the same things for tests that are consumers of
MediaStreamDevicesController. This ensures that if we move any logic out of the
class and into the static entry point, that tests will continue to work
correctly.

In order to do this it means that tests need to still need a way to simulate
accepting permission prompts. Previously they did this by operating directly on
the MediaStreamDevicesController object, but that has been taken away. It has
been replaced by a private version of the entry point:
RequestPermissionsWithDelegate. This allows them to pass in a delegate which
can be used to determine whether prompts have been shown and to accept/deny
them.

The big benefit of this approach is that the delegate has been given the same
API as the MockPermissionPromptFactory. MockPermissionPromptFactory is the
class that is used to simulate/control permission prompts in tests of all other
permissions that use PermissionRequestManager. So, when the refactoring in
https://crbug.com/606138 is completed, we can replace the Delegate with a
MockPermissionPromptFactory and the tests themselves will not need to be
altered.

BUG=596786
==========

[raymes, 2017-02-23 04:54:17]

Description was changed from

==========
Make MediaStreamDevicesController tests go through the RequestPermissions API

In https://codereview.chromium.org/2706813004/ the consumers of
MediaStreamDevicesController were changed to use a static RequestPermissions
function instead of constructing the object directly. This facilitated moving
the prompting logic closer to the permission status checking logic which will
help us move to using PermissionManager to request permissions.

This CL does the same things for tests that are consumers of
MediaStreamDevicesController. This ensures that if we move any logic out of the
class and into the static entry point, that tests will continue to work
correctly.

In order to do this it means that tests need to still need a way to simulate
accepting permission prompts. Previously they did this by operating directly on
the MediaStreamDevicesController object, but that has been taken away. It has
been replaced by a private version of the entry point:
RequestPermissionsWithDelegate. This allows them to pass in a delegate which
can be used to determine whether prompts have been shown and to accept/deny
them.

The big benefit of this approach is that the delegate has been given the same
API as the MockPermissionPromptFactory. MockPermissionPromptFactory is the
class that is used to simulate/control permission prompts in tests of all other
permissions that use PermissionRequestManager. So, when the refactoring in
https://crbug.com/606138 is completed, we can replace the Delegate with a
MockPermissionPromptFactory and the tests themselves will not need to be
altered.

BUG=596786
==========

to

==========
Make MediaStreamDevicesController tests go through the RequestPermissions API

In https://codereview.chromium.org/2706813004/ the consumers of
MediaStreamDevicesController were changed to use a static RequestPermissions
function instead of constructing the object directly. This facilitated moving
the prompting logic closer to the permission status checking logic which will
help us move to using PermissionManager to request permissions.

This CL does the same things for tests that are consumers of
MediaStreamDevicesController. This ensures that if we move any logic out of the
class and into the static entry point, that tests will continue to work
correctly.

In order to do this it means that tests still need a way to simulate
accepting permission prompts. Previously they did this by operating directly on
the MediaStreamDevicesController object, but that has been taken away. It has
been replaced by a private version of the entry point:
RequestPermissionsWithDelegate. This allows them to pass in a delegate which
can be used to determine whether prompts have been shown and to accept/deny
them.

The big benefit of this approach is that the delegate has been given the same
API as the MockPermissionPromptFactory. MockPermissionPromptFactory is the
class that is used to simulate/control permission prompts in tests of all other
permissions that use PermissionRequestManager. So, when the refactoring in
https://crbug.com/606138 is completed, we can replace the Delegate with a
MockPermissionPromptFactory and the tests themselves will not need to be
altered.

BUG=596786
==========

[raymes, 2017-02-27 06:42:39]

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

[Timothy Loh, 2017-02-28 00:39:20]

https://codereview.chromium.org/2711883003/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc/media_stream_devices_controller_browsertest.cc
(right):

https://codereview.chromium.org/2711883003/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc/media_stream_devices_controller_browsertest.cc:696:
ASSERT_EQ(test.NumExpectedPrompts(),
This makes the test a bit less thorough, can we retain separate checks for which
permissions we get prompted for?

[raymes, 2017-03-01 02:08:01]

https://codereview.chromium.org/2711883003/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc/media_stream_devices_controller_browsertest.cc
(right):

https://codereview.chromium.org/2711883003/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc/media_stream_devices_controller_browsertest.cc:696:
ASSERT_EQ(test.NumExpectedPrompts(),
On 2017/02/28 00:39:20, Timothy Loh wrote:
> This makes the test a bit less thorough, can we retain separate checks for
which
> permissions we get prompted for?

That's true. I kept the checks back in by storing a list of the request types.
That means this won't exactly match mock_permission_prompt_factory.h but when we
convert this over it should be easy to make that API slightly more general to
cover this use case.

[Timothy Loh, 2017-03-01 06:45:27]

On 2017/03/01 02:08:01, raymes wrote:
>
https://codereview.chromium.org/2711883003/diff/1/chrome/browser/media/webrtc...
> File
chrome/browser/media/webrtc/media_stream_devices_controller_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2711883003/diff/1/chrome/browser/media/webrtc...
>
chrome/browser/media/webrtc/media_stream_devices_controller_browsertest.cc:696:
> ASSERT_EQ(test.NumExpectedPrompts(),
> On 2017/02/28 00:39:20, Timothy Loh wrote:
> > This makes the test a bit less thorough, can we retain separate checks for
> which
> > permissions we get prompted for?
> 
> That's true. I kept the checks back in by storing a list of the request types.
> That means this won't exactly match mock_permission_prompt_factory.h but when
we
> convert this over it should be easy to make that API slightly more general to
> cover this use case.

lgtm

[raymes, 2017-03-02 01:47:16]

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

[commit-bot: I haz the power, 2017-03-02 01:48:25]

Dry run: 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-02 03:22:57]

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

[commit-bot: I haz the power, 2017-03-02 03:22:57]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-03-06 00:45:09]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-03-06 00:45:17]

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-06 00:50:46]

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

[commit-bot: I haz the power, 2017-03-06 00:50:47]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[raymes, 2017-03-06 01:12:29]

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

[raymes, 2017-03-06 01:12:30]

+pastarmovj for policy_browsertest.cc

[pastarmovj, 2017-03-06 08:02:25]

lgtm

[raymes, 2017-03-06 21:41:54]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-03-06 21:42:30]

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-06 22:16:39]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488836514871130,
"parent_rev": "476af767372c72fd5e71f365e78481af9ce9a1d9", "commit_rev":
"2d2d7d754045d63fd75aaee27026d27ae1e2fdab"}

[commit-bot: I haz the power, 2017-03-06 22:17:23]

Description was changed from

==========
Make MediaStreamDevicesController tests go through the RequestPermissions API

In https://codereview.chromium.org/2706813004/ the consumers of
MediaStreamDevicesController were changed to use a static RequestPermissions
function instead of constructing the object directly. This facilitated moving
the prompting logic closer to the permission status checking logic which will
help us move to using PermissionManager to request permissions.

This CL does the same things for tests that are consumers of
MediaStreamDevicesController. This ensures that if we move any logic out of the
class and into the static entry point, that tests will continue to work
correctly.

In order to do this it means that tests still need a way to simulate
accepting permission prompts. Previously they did this by operating directly on
the MediaStreamDevicesController object, but that has been taken away. It has
been replaced by a private version of the entry point:
RequestPermissionsWithDelegate. This allows them to pass in a delegate which
can be used to determine whether prompts have been shown and to accept/deny
them.

The big benefit of this approach is that the delegate has been given the same
API as the MockPermissionPromptFactory. MockPermissionPromptFactory is the
class that is used to simulate/control permission prompts in tests of all other
permissions that use PermissionRequestManager. So, when the refactoring in
https://crbug.com/606138 is completed, we can replace the Delegate with a
MockPermissionPromptFactory and the tests themselves will not need to be
altered.

BUG=596786
==========

to

==========
Make MediaStreamDevicesController tests go through the RequestPermissions API

In https://codereview.chromium.org/2706813004/ the consumers of
MediaStreamDevicesController were changed to use a static RequestPermissions
function instead of constructing the object directly. This facilitated moving
the prompting logic closer to the permission status checking logic which will
help us move to using PermissionManager to request permissions.

This CL does the same things for tests that are consumers of
MediaStreamDevicesController. This ensures that if we move any logic out of the
class and into the static entry point, that tests will continue to work
correctly.

In order to do this it means that tests still need a way to simulate
accepting permission prompts. Previously they did this by operating directly on
the MediaStreamDevicesController object, but that has been taken away. It has
been replaced by a private version of the entry point:
RequestPermissionsWithDelegate. This allows them to pass in a delegate which
can be used to determine whether prompts have been shown and to accept/deny
them.

The big benefit of this approach is that the delegate has been given the same
API as the MockPermissionPromptFactory. MockPermissionPromptFactory is the
class that is used to simulate/control permission prompts in tests of all other
permissions that use PermissionRequestManager. So, when the refactoring in
https://crbug.com/606138 is completed, we can replace the Delegate with a
MockPermissionPromptFactory and the tests themselves will not need to be
altered.

BUG=596786

Review-Url: https://codereview.chromium.org/2711883003
Cr-Commit-Position: refs/heads/master@{#454974}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d2d7d754045d63fd75aaee27026...
==========

[commit-bot: I haz the power, 2017-03-06 22:17:24]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2d2d7d754045d63fd75aaee27026...