Make media permission requests go through the PermissionManager

Make media permission requests go through the PermissionManager

This CL adds a feature flag which, when enabled, causes media requests to call
into PermissionManager::RequestPermissions in order to display a permission
prompt (among other things). The feature flag is added because this results in
the UI changing, and also won't work on Android until we have unified the
permission request codepaths (see https://crbug.com/606138).

BUG=596786
Review-Url: https://codereview.chromium.org/2829123003
Cr-Commit-Position: refs/heads/master@{#468265}
Committed: https://chromium.googlesource.com/chromium/src/+/96988faed343f078f05d0f364ce5de260d068be0

[raymes, 2017-04-21 05:33:18]

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

[Timothy Loh, 2017-04-24 04:18:57]

Looks good, just small questions about one bit.

https://codereview.chromium.org/2829123003/diff/20001/chrome/browser/media/we...
File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2829123003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/media_stream_devices_controller.cc:403: if
(audio_setting_ == CONTENT_SETTING_BLOCK ||
Should this be an &&? Looks like RunCallback() will ignore the denial reason for
ALLOW+BLOCK (i.e. one was already set).

Also it seems like we now we return DENIED for BLOCK+ASK (i.e. one was already
blocked) instead of DISMISSED, is one of these better than the other?

[raymes, 2017-04-26 00:10:30]

https://codereview.chromium.org/2829123003/diff/20001/chrome/browser/media/we...
File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2829123003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/media_stream_devices_controller.cc:403: if
(audio_setting_ == CONTENT_SETTING_BLOCK ||
We should only really be updating |denial_reason_| here based on |response| not
based on previous settings, so I've fixed that. But otherwise I don't think it
matters - previously there would only be one response to the prompt, but now
there are potentially 2. They should never really be ASK and BLOCK together
though.

[raymes, 2017-04-26 00:10:35]

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

[commit-bot: I haz the power, 2017-04-26 00:11:29]

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-04-26 01:03:32]

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

[commit-bot: I haz the power, 2017-04-26 01:03:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[raymes, 2017-04-26 01:10:03]

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

[commit-bot: I haz the power, 2017-04-26 01:10:43]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-04-26 01:40:25]

lgtm

https://codereview.chromium.org/2829123003/diff/20001/chrome/browser/media/we...
File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2829123003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/media_stream_devices_controller.cc:403: if
(audio_setting_ == CONTENT_SETTING_BLOCK ||
On 2017/04/26 00:10:30, raymes wrote:
> We should only really be updating |denial_reason_| here based on |response|
not
> based on previous settings, so I've fixed that. But otherwise I don't think it
> matters - previously there would only be one response to the prompt, but now
> there are potentially 2. They should never really be ASK and BLOCK together
> though. 

OK, sounds reasonable.

[commit-bot: I haz the power, 2017-04-26 01:41:21]

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

[commit-bot: I haz the power, 2017-04-26 01:41:21]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-04-26 01:43:59]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-04-26 01:44:32]

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-04-26 01:55:25]

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

[commit-bot: I haz the power, 2017-04-26 01:55:26]

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-04-26 02:27:05]

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

[raymes, 2017-04-26 02:27:06]

+isherman for histograms.xml

[Ilya Sherman, 2017-04-26 07:20:28]

histograms.xml lgtm

[raymes, 2017-04-30 22:54:07]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-04-30 22:54:21]

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

CQ is committing da patch.

Bot data: {"patchset_id": 50011, "attempt_start_ts": 1493592847521890,
"parent_rev": "9b549cec866068f02af088ed0eec0efea26cb150", "commit_rev":
"96988faed343f078f05d0f364ce5de260d068be0"}

[commit-bot: I haz the power, 2017-05-01 00:51:40]

Description was changed from

==========
Make media permission requests go through the PermissionManager

This CL adds a feature flag which, when enabled, causes media requests to call
into PermissionManager::RequestPermissions in order to display a permission
prompt (among other things). The feature flag is added because this results in
the UI changing, and also won't work on Android until we have unified the
permission request codepaths (see https://crbug.com/606138).

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

to

==========
Make media permission requests go through the PermissionManager

This CL adds a feature flag which, when enabled, causes media requests to call
into PermissionManager::RequestPermissions in order to display a permission
prompt (among other things). The feature flag is added because this results in
the UI changing, and also won't work on Android until we have unified the
permission request codepaths (see https://crbug.com/606138).

BUG=596786

Review-Url: https://codereview.chromium.org/2829123003
Cr-Commit-Position: refs/heads/master@{#468265}
Committed:
https://chromium.googlesource.com/chromium/src/+/96988faed343f078f05d0f364ce5...
==========

[commit-bot: I haz the power, 2017-05-01 00:51:41]

Committed patchset #4 (id:50011) as
https://chromium.googlesource.com/chromium/src/+/96988faed343f078f05d0f364ce5...