Add mojo-based support for media device-change notifications.

Add mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and PepperMediaDeviceManager to this and removal of the corresponding code in MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183
Committed: https://crrev.com/57409acb300fa06da3679b20d3474436fadb68c5
Cr-Commit-Position: refs/heads/master@{#430310}

[Guido Urdaneta, 2016-11-01 19:47:56]

Patchset #1 (id:1) has been deleted

[Guido Urdaneta, 2016-11-01 19:48:04]

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

[commit-bot: I haz the power, 2016-11-01 19:48:26]

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, 2016-11-01 20:13:35]

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

[commit-bot: I haz the power, 2016-11-01 20:13:36]

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_...)

[Guido Urdaneta, 2016-11-01 21:12:44]

Patchset #1 (id:20001) has been deleted

[Guido Urdaneta, 2016-11-01 21:13:00]

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

[commit-bot: I haz the power, 2016-11-01 21:13:39]

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

[Guido Urdaneta, 2016-11-01 22:37:16]

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

[commit-bot: I haz the power, 2016-11-01 22:37: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, 2016-11-01 23:32:50]

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

[commit-bot: I haz the power, 2016-11-01 23:32:51]

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

[Guido Urdaneta, 2016-11-02 07:44:33]

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

[Guido Urdaneta, 2016-11-02 07:44:34]

Hi, PTAL.

In the meantime I'm resolving a leak in a unit test.

[Guido Urdaneta, 2016-11-02 10:37:37]

Description was changed from

==========
Implement mojo-based infrastructure for media device-change notifications.

Users of device-change notifications are not yet wired into this infrastructure.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager.

BUG=648183
==========

to

==========
Implement mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the old
MediaStreamDispatcher/MediaStreamDispatcherHost code.

BUG=648183
==========

[Guido Urdaneta, 2016-11-02 10:38:00]

Description was changed from

==========
Implement mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the old
MediaStreamDispatcher/MediaStreamDispatcherHost code.

BUG=648183
==========

to

==========
Implement mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the old code in
MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183
==========

[Guido Urdaneta, 2016-11-02 10:38:13]

Description was changed from

==========
Implement mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the old code in
MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183
==========

to

==========
Implement mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the corresponding code in
MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183
==========

[Guido Urdaneta, 2016-11-02 10:38:28]

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

[commit-bot: I haz the power, 2016-11-02 10:38:43]

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

[Guido Urdaneta, 2016-11-02 10:51:41]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-11-02 11:48:21]

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

[commit-bot: I haz the power, 2016-11-02 11:48:22]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2016-11-02 11:49:50]

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

[Guido Urdaneta, 2016-11-02 11:50:39]

mkwst@chromium.org: Please review IPC and histograms.xml

[Guido Urdaneta, 2016-11-02 13:27:01]

Description was changed from

==========
Implement mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the corresponding code in
MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183
==========

to

==========
Add mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the corresponding code in
MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183
==========

[Mike West, 2016-11-03 12:58:50]

Looks reasonable; I'll hold off on stamping the IPC change until tommi@ is happy
with the rest of the patch.

[tommi (sloooow) - chröme, 2016-11-03 15:21:24]

lgtm. I'm assuming that the cases below are handled. If they're not, let me know
and I'll take another look

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/media_devices_dispatcher_host.cc
(right):

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:51:
output->clear();
is this the right place to do this?  Generally we have a DCHECK(foo->empty());
check if it's a requirement that the array is empty (and in practice, it usually
is), and in the odd case where it isn't, appending might be desired by the
caller.

If the situation is that this array is always empty on entry, I'd go with a
DCHECK.

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:189: if
(device_change_subscriptions_[type].empty())
{}

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:222:
mojo::GetProxy(&media_devices_listener));
is there no chance that media_devices_listener is null after this call?

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:226:
MediaDeviceInfoArray translated_device_infos;
nit: move to where the call to TranslateMediaDeviceInfoArray is

https://codereview.chromium.org/2471543003/diff/80001/content/renderer/media/...
File content/renderer/media/media_devices_event_dispatcher.cc (right):

https://codereview.chromium.org/2471543003/diff/80001/content/renderer/media/...
content/renderer/media/media_devices_event_dispatcher.cc:46: SubscriptionList&
subscriptions = device_change_subscriptions_[type];
nit: move below SubscribeDeviceChangeNotifications

[Guido Urdaneta, 2016-11-03 16:20:24]

Patchset #3 (id:100001) has been deleted

[Guido Urdaneta, 2016-11-03 16:25:24]

tommi@: PTA(nother)L

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/media_devices_dispatcher_host.cc
(right):

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:51:
output->clear();
On 2016/11/03 15:21:24, tommi (chrömium) wrote:
> is this the right place to do this?  Generally we have a DCHECK(foo->empty());
> check if it's a requirement that the array is empty (and in practice, it
usually
> is), and in the odd case where it isn't, appending might be desired by the
> caller.
> 
> If the situation is that this array is always empty on entry, I'd go with a
> DCHECK.

Rewritten to return a new translated vector.
The clear() was an unnecessary leftover from an older version which I forgot to
remove.

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:189: if
(device_change_subscriptions_[type].empty())
On 2016/11/03 15:21:24, tommi (chrömium) wrote:
> {}

Done.

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:222:
mojo::GetProxy(&media_devices_listener));
On 2016/11/03 15:21:24, tommi (chrömium) wrote:
> is there no chance that media_devices_listener is null after this call?

Now that you mention it, I guess it could happen that a malicious renderer
process doesn't register the interface in order to crash the browser.
Now checking if |media_devices_listener| is null in order to protect against
this.

mkwst@: Is this reasoning correct?

https://codereview.chromium.org/2471543003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:226:
MediaDeviceInfoArray translated_device_infos;
On 2016/11/03 15:21:24, tommi (chrömium) wrote:
> nit: move to where the call to TranslateMediaDeviceInfoArray is

Done.

https://codereview.chromium.org/2471543003/diff/80001/content/renderer/media/...
File content/renderer/media/media_devices_event_dispatcher.cc (right):

https://codereview.chromium.org/2471543003/diff/80001/content/renderer/media/...
content/renderer/media/media_devices_event_dispatcher.cc:46: SubscriptionList&
subscriptions = device_change_subscriptions_[type];
On 2016/11/03 15:21:24, tommi (chrömium) wrote:
> nit: move below SubscribeDeviceChangeNotifications

Done.

[Guido Urdaneta, 2016-11-03 17:09:55]

mkwst@: Please start security/IPC (and histograms) review. I discussed the
latest changes with tommi@ offline and his lgtm stands.

[Mike West, 2016-11-07 09:14:32]

mojo LGTM. Thanks for adding the BAD_MESSAGE crash for origin mismatch.

[Guido Urdaneta, 2016-11-07 12:17:34]

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

[commit-bot: I haz the power, 2016-11-07 12:17:38]

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

[Guido Urdaneta, 2016-11-07 12:20:43]

The CQ bit was unchecked by guidou@chromium.org

[Guido Urdaneta, 2016-11-07 13:58:33]

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

[commit-bot: I haz the power, 2016-11-07 13:58:44]

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

[Guido Urdaneta, 2016-11-07 14:28:03]

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

[Guido Urdaneta, 2016-11-07 14:28:04]

avi@chromium.org: Can you rs?

[commit-bot: I haz the power, 2016-11-07 15:21:02]

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

[commit-bot: I haz the power, 2016-11-07 15:21:03]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-11-07 16:09:48]

LGTM with nit

https://codereview.chromium.org/2471543003/diff/200001/content/browser/render...
File content/browser/renderer_host/media/media_devices_dispatcher_host.h
(right):

https://codereview.chromium.org/2471543003/diff/200001/content/browser/render...
content/browser/renderer_host/media/media_devices_dispatcher_host.h:8: #include
<array>
I don't see use of <array> here.

[Guido Urdaneta, 2016-11-07 16:35:54]

https://codereview.chromium.org/2471543003/diff/200001/content/browser/render...
File content/browser/renderer_host/media/media_devices_dispatcher_host.h
(right):

https://codereview.chromium.org/2471543003/diff/200001/content/browser/render...
content/browser/renderer_host/media/media_devices_dispatcher_host.h:8: #include
<array>
On 2016/11/07 16:09:48, Avi wrote:
> I don't see use of <array> here.

Nice catch! I forgot to remove it from older version.

[Guido Urdaneta, 2016-11-07 16:36:02]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-11-07 16:36:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
mkwst@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2471543003/#ps220001
(title: "remove unused #include")

[commit-bot: I haz the power, 2016-11-07 16:36:28]

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, 2016-11-07 17:40:12]

Description was changed from

==========
Add mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the corresponding code in
MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183
==========

to

==========
Add mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the corresponding code in
MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183
==========

[commit-bot: I haz the power, 2016-11-07 17:40:13]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2016-11-07 18:14:25]

Description was changed from

==========
Add mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the corresponding code in
MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183
==========

to

==========
Add mojo-based support for media device-change notifications.

Users of device-change notifications are not yet using this.
An  upcoming CL will include wiring of the Blink devicechange event and
PepperMediaDeviceManager to this and removal of the corresponding code in
MediaStreamDispatcher and MediaStreamDispatcherHost.

BUG=648183

Committed: https://crrev.com/57409acb300fa06da3679b20d3474436fadb68c5
Cr-Commit-Position: refs/heads/master@{#430310}
==========

[commit-bot: I haz the power, 2016-11-07 18:14:27]

Patchset 8 (id:??) landed as
https://crrev.com/57409acb300fa06da3679b20d3474436fadb68c5
Cr-Commit-Position: refs/heads/master@{#430310}