Pull code associated with a Media PermissionRequest out of MediaStreamDevicesController

Pull code associated with a Media PermissionRequest out of MediaStreamDevicesController

This pulls the code associated with a Media PermissionRequest out of
MediaStreamDevicesController. This code is only used for PermissionRequests
that are made using the legacy media permission prompting code. Permission
requests that will go through the PermissionManager will not use this code so
it can be deleted once crbug.com/606138 is fixed.

BUG=596786
TBR=sergeyu@chromium.org,pastarmovj@chromium.org

Review-Url: https://codereview.chromium.org/2811913002
Cr-Commit-Position: refs/heads/master@{#465904}
Committed: https://chromium.googlesource.com/chromium/src/+/8a32f1b172e3ee37f98766eac4f8de5dc58dba8f

[raymes, 2017-04-12 03:15:58]

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

[commit-bot: I haz the power, 2017-04-12 03:16:20]

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

[raymes, 2017-04-12 03:17:51]

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

[commit-bot: I haz the power, 2017-04-12 03:37:35]

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

[commit-bot: I haz the power, 2017-04-12 03:37:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[raymes, 2017-04-12 04:50:41]

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

[commit-bot: I haz the power, 2017-04-12 04:51:03]

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-12 05:13:40]

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

[commit-bot: I haz the power, 2017-04-12 05:13:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[raymes, 2017-04-12 05:21:11]

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

[commit-bot: I haz the power, 2017-04-12 05:21: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-12 05:43:05]

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

[commit-bot: I haz the power, 2017-04-12 05:43:06]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[raymes, 2017-04-13 06:24:32]

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

[commit-bot: I haz the power, 2017-04-13 06:24:57]

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-13 07:28:37]

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

[commit-bot: I haz the power, 2017-04-13 07:28:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2017-04-20 00:35:08]

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

[commit-bot: I haz the power, 2017-04-20 00:35:55]

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-20 02:05:43]

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

[commit-bot: I haz the power, 2017-04-20 02:05:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Timothy Loh, 2017-04-20 03:13:20]

https://codereview.chromium.org/2811913002/diff/140001/chrome/browser/media/w...
File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2811913002/diff/140001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_stream_devices_controller.cc:206:
RecordPermissionAction(is_asking_for_audio_, is_asking_for_video_,
I think this needs to be guarded somehow otherwise we're going to record ignores
for every request.

[raymes, 2017-04-20 04:23:37]

https://codereview.chromium.org/2811913002/diff/140001/chrome/browser/media/w...
File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2811913002/diff/140001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_stream_devices_controller.cc:206:
RecordPermissionAction(is_asking_for_audio_, is_asking_for_video_,
On 2017/04/20 03:13:20, Timothy Loh wrote:
> I think this needs to be guarded somehow otherwise we're going to record
ignores
> for every request.

Thanks, good catch! Done.

[raymes, 2017-04-20 04:23:42]

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

[commit-bot: I haz the power, 2017-04-20 04:24:10]

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-20 04:34:04]

lgtm

[commit-bot: I haz the power, 2017-04-20 05:09:25]

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

[commit-bot: I haz the power, 2017-04-20 05:09:26]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-04-20 05:13:30]

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

[raymes, 2017-04-20 05:13:31]

TBR OWNERS

sergeyu for trivial changes in
chrome/browser/media/webrtc/media_stream_infobar_delegate_android*
pastarmovj for trivial changes in chrome/browser/policy/policy_browsertest.cc

[raymes, 2017-04-20 05:13:59]

Description was changed from

==========
Pull code associated with a Media PermissionRequest out of
MediaStreamDevicesController

This pulls the code associated with a Media PermissionRequest out of
MediaStreamDevicesController. This code is only used for PermissionRequests
that are made using the legacy media permission prompting code. Permission
requests that will go through the PermissionManager will not use this code so
it can be deleted once crbug.com/606138 is fixed.

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

to

==========
Pull code associated with a Media PermissionRequest out of
MediaStreamDevicesController

This pulls the code associated with a Media PermissionRequest out of
MediaStreamDevicesController. This code is only used for PermissionRequests
that are made using the legacy media permission prompting code. Permission
requests that will go through the PermissionManager will not use this code so
it can be deleted once crbug.com/606138 is fixed.

BUG=596786
TBR=sergeyu@chromium.org,pastarmovj@chromium.org
==========

[raymes, 2017-04-20 05:14:39]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-04-20 05:15:04]

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-20 05:20:29]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1492665279347370,
"parent_rev": "e1e7dfb3555b5e5b248a84899a2c99ef9ba2c6e9", "commit_rev":
"8a32f1b172e3ee37f98766eac4f8de5dc58dba8f"}

[commit-bot: I haz the power, 2017-04-20 05:21:01]

Description was changed from

==========
Pull code associated with a Media PermissionRequest out of
MediaStreamDevicesController

This pulls the code associated with a Media PermissionRequest out of
MediaStreamDevicesController. This code is only used for PermissionRequests
that are made using the legacy media permission prompting code. Permission
requests that will go through the PermissionManager will not use this code so
it can be deleted once crbug.com/606138 is fixed.

BUG=596786
TBR=sergeyu@chromium.org,pastarmovj@chromium.org
==========

to

==========
Pull code associated with a Media PermissionRequest out of
MediaStreamDevicesController

This pulls the code associated with a Media PermissionRequest out of
MediaStreamDevicesController. This code is only used for PermissionRequests
that are made using the legacy media permission prompting code. Permission
requests that will go through the PermissionManager will not use this code so
it can be deleted once crbug.com/606138 is fixed.

BUG=596786
TBR=sergeyu@chromium.org,pastarmovj@chromium.org

Review-Url: https://codereview.chromium.org/2811913002
Cr-Commit-Position: refs/heads/master@{#465904}
Committed:
https://chromium.googlesource.com/chromium/src/+/8a32f1b172e3ee37f98766eac4f8...
==========

[commit-bot: I haz the power, 2017-04-20 05:21:02]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/8a32f1b172e3ee37f98766eac4f8...

[Nico, 2017-04-24 13:51:52]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2017-04-24 13:51:53]

https://codereview.chromium.org/2811913002/diff/160001/chrome/browser/media/w...
File chrome/browser/media/webrtc/media_stream_devices_controller.h (right):

https://codereview.chromium.org/2811913002/diff/160001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_stream_devices_controller.h:43:
base::Callback<void(ContentSetting, bool /* persist */)>;
nit: spell this like `/*persist=*/bool` -- clang-format knows about that
convention, and it makes it clear that `/*persist=*/false` at a call site means
"set persist to false" (`false /*persist*/` could also mean "false here means we
persist").

Better yet, use enums instead of bools.

https://codereview.chromium.org/2811913002/diff/160001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_stream_devices_controller.h:88: class
PermissionPromptDelegate {
std::unique_ptr<PermissionPromptDelegate> delegate_; below deletes this class
through the base pointer, so it must have a virtual dtor, else the dtor of
subclasses won't be called. (Subclasses don't have dtors from what I can tell,
but it's still a latent bug.)

[raymes, 2017-04-26 00:28:15]

https://codereview.chromium.org/2811913002/diff/160001/chrome/browser/media/w...
File chrome/browser/media/webrtc/media_stream_devices_controller.h (right):

https://codereview.chromium.org/2811913002/diff/160001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_stream_devices_controller.h:43:
base::Callback<void(ContentSetting, bool /* persist */)>;
On 2017/04/24 13:51:53, Nico wrote:
> nit: spell this like `/*persist=*/bool` -- clang-format knows about that
> convention, and it makes it clear that `/*persist=*/false` at a call site
means
> "set persist to false" (`false /*persist*/` could also mean "false here means
we
> persist").
> 
> Better yet, use enums instead of bools.

Thanks I didn't know that. That was the convention that I had always known and
seen used in Chrome but I'll keep this in mind for the future.

https://codereview.chromium.org/2811913002/diff/160001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_stream_devices_controller.h:88: class
PermissionPromptDelegate {
On 2017/04/24 13:51:53, Nico wrote:
> std::unique_ptr<PermissionPromptDelegate> delegate_; below deletes this class
> through the base pointer, so it must have a virtual dtor, else the dtor of
> subclasses won't be called. (Subclasses don't have dtors from what I can tell,
> but it's still a latent bug.)

Thanks for catching this. Fixed in: https://codereview.chromium.org/2836093006#