Add support for device-change notifications to MediaStreamDispatcher and MediaStreamDispatcherHost

Add support for device-change notifications to MediaStreamDispatcher and MediaStreamDispatcherHost

This is a step towards supporting the "devicechange" event in Blink.

BUG=388648
Committed: https://crrev.com/ec334e1799ae25aaa1b0c66e25af641018a3a84a
Cr-Commit-Position: refs/heads/master@{#391114}

[Guido Urdaneta, 2016-04-26 08:56:25]

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

[Guido Urdaneta, 2016-04-26 08:56:26]

Hi, PTAL

[tommi (sloooow) - chröme, 2016-04-26 10:11:36]

lgtm with thread checks added

https://codereview.chromium.org/1918173002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc
(right):

https://codereview.chromium.org/1918173002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:251: void
MediaStreamDispatcherHost::OnSubscribeToDeviceChangeNotifications(
can you add thread checks to these methods?

https://codereview.chromium.org/1918173002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_dispatcher.cc (right):

https://codereview.chromium.org/1918173002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_dispatcher.cc:421: void
MediaStreamDispatcher::OnDevicesChanged() {
DCHECK(thread_checker_.CalledOnValidThread()); ?

[Guido Urdaneta, 2016-04-26 11:07:54]

https://codereview.chromium.org/1918173002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc
(right):

https://codereview.chromium.org/1918173002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:251: void
MediaStreamDispatcherHost::OnSubscribeToDeviceChangeNotifications(
On 2016/04/26 10:11:36, tommi-chrömium wrote:
> can you add thread checks to these methods?

Done.

https://codereview.chromium.org/1918173002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_dispatcher.cc (right):

https://codereview.chromium.org/1918173002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_dispatcher.cc:421: void
MediaStreamDispatcher::OnDevicesChanged() {
On 2016/04/26 10:11:36, tommi-chrömium wrote:
> DCHECK(thread_checker_.CalledOnValidThread()); ?

Done.

[Guido Urdaneta, 2016-04-26 11:08:55]

guidou@chromium.org changed reviewers:
+ bbudge@chromium.org, palmer@chromium.org

[Guido Urdaneta, 2016-04-26 11:08:56]

palmer@chromium.org: Please review changes in
content/common/media/media_stream_messages.h

bbudge@chromium.org: Please review changes in content/renderer/pepper

[Guido Urdaneta, 2016-04-26 11:09:39]

Description was changed from

==========
Add support for device-change notifications to MediaStreamDispatcher and
MediaStreamDispatcherHost

This is a step towards supporting the "devicechange" event in Blink.

BUG=388648
==========

to

==========
Add support for device-change notifications to MediaStreamDispatcher and
MediaStreamDispatcherHost

This is a step towards supporting the "devicechange" event in Blink.

BUG=388648
==========

[hta - Chromium, 2016-04-26 12:30:59]

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

[hta - Chromium, 2016-04-26 12:31:00]

lgtm as far as I can read.
Seems straightforward to me. Some nits.

https://codereview.chromium.org/1918173002/diff/60001/content/browser/rendere...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/1918173002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:148:
// In this method we dispatch the messages to the according handlers as if
Just because you're here: can you change "to the according handlers" into "to
the corresponding handlers"?

The English, it grates.

https://codereview.chromium.org/1918173002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1918173002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_dispatcher_unittest.cc:428:
dispatcher_->CancelDeviceChangeNotifications(handler2->AsWeakPtr());
Do you check the error cases of adding a handler twice and of deleting a handler
twice? (presumably EXPECT_DEATH or something like that)

https://codereview.chromium.org/1918173002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_dispatcher_unittest.cc:431: EXPECT_EQ(true,
handler2->did_receive_devices_changed_);
Any reason to not EXPECT_TRUE? Seems more readable.

[Guido Urdaneta, 2016-04-26 13:04:19]

https://codereview.chromium.org/1918173002/diff/60001/content/browser/rendere...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/1918173002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:148:
// In this method we dispatch the messages to the according handlers as if
On 2016/04/26 12:31:00, hta - Chromium wrote:
> Just because you're here: can you change "to the according handlers" into "to
> the corresponding handlers"?
> 
> The English, it grates.

Done.

https://codereview.chromium.org/1918173002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1918173002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_dispatcher_unittest.cc:428:
dispatcher_->CancelDeviceChangeNotifications(handler2->AsWeakPtr());
On 2016/04/26 12:31:00, hta - Chromium wrote:
> Do you check the error cases of adding a handler twice and of deleting a
handler
> twice? (presumably EXPECT_DEATH or something like that)
> 

Done.

https://codereview.chromium.org/1918173002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_dispatcher_unittest.cc:431: EXPECT_EQ(true,
handler2->did_receive_devices_changed_);
On 2016/04/26 12:31:00, hta - Chromium wrote:
> Any reason to not EXPECT_TRUE? Seems more readable.

Done.

[palmer, 2016-04-26 23:25:02]

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

https://codereview.chromium.org/1918173002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:7: #include
<algorithm>
Nit: I think you need a blank line after this.

https://codereview.chromium.org/1918173002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:282:
device_change_subscribers_.erase(it);
Wrap this in

  if (it != device_change_subscribers_.end())

Or, if you intend to crash in production, use CHECK instead of DCHECK.

https://codereview.chromium.org/1918173002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_dispatcher_host.h (right):

https://codereview.chromium.org/1918173002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:121: GURL
security_origin;
Here, too.

https://codereview.chromium.org/1918173002/diff/80001/content/common/media/me...
File content/common/media/media_stream_messages.h (right):

https://codereview.chromium.org/1918173002/diff/80001/content/common/media/me...
content/common/media/media_stream_messages.h:150: GURL /* security origin */)
Use url::Origin, not GURL, for this purpose.

[bbudge, 2016-04-26 23:48:05]

content/renderer/pepper LGTM

[Guido Urdaneta, 2016-05-01 11:08:40]

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

https://codereview.chromium.org/1918173002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:7: #include
<algorithm>
On 2016/04/26 23:25:02, palmer wrote:
> Nit: I think you need a blank line after this.

Done.

https://codereview.chromium.org/1918173002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:282:
device_change_subscribers_.erase(it);
On 2016/04/26 23:25:02, palmer wrote:
> Wrap this in
> 
>   if (it != device_change_subscribers_.end())
> 
> Or, if you intend to crash in production, use CHECK instead of DCHECK.

Done. Also in two other places that use the same idiom.

https://codereview.chromium.org/1918173002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_dispatcher_host.h (right):

https://codereview.chromium.org/1918173002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:121: GURL
security_origin;
On 2016/04/26 23:25:02, palmer wrote:
> Here, too.

Done.

https://codereview.chromium.org/1918173002/diff/80001/content/common/media/me...
File content/common/media/media_stream_messages.h (right):

https://codereview.chromium.org/1918173002/diff/80001/content/common/media/me...
content/common/media/media_stream_messages.h:150: GURL /* security origin */)
On 2016/04/26 23:25:02, palmer wrote:
> Use url::Origin, not GURL, for this purpose.

Done. Changed the rest of the file in a separate CL.

[palmer, 2016-05-02 22:39:00]

lgtm

[Guido Urdaneta, 2016-05-02 23:00:29]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-05-02 23:00:30]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
hta@chromium.org, bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/1918173002/#ps100001
(title: "rebase and palmer's comments")

[commit-bot: I haz the power, 2016-05-02 23:01:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1918173002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1918173002/100001

[commit-bot: I haz the power, 2016-05-03 00:12:58]

Description was changed from

==========
Add support for device-change notifications to MediaStreamDispatcher and
MediaStreamDispatcherHost

This is a step towards supporting the "devicechange" event in Blink.

BUG=388648
==========

to

==========
Add support for device-change notifications to MediaStreamDispatcher and
MediaStreamDispatcherHost

This is a step towards supporting the "devicechange" event in Blink.

BUG=388648
==========

[commit-bot: I haz the power, 2016-05-03 00:13:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-03 00:14:07]

Description was changed from

==========
Add support for device-change notifications to MediaStreamDispatcher and
MediaStreamDispatcherHost

This is a step towards supporting the "devicechange" event in Blink.

BUG=388648
==========

to

==========
Add support for device-change notifications to MediaStreamDispatcher and
MediaStreamDispatcherHost

This is a step towards supporting the "devicechange" event in Blink.

BUG=388648

Committed: https://crrev.com/ec334e1799ae25aaa1b0c66e25af641018a3a84a
Cr-Commit-Position: refs/heads/master@{#391114}
==========

[commit-bot: I haz the power, 2016-05-03 00:14:08]

Patchset 6 (id:??) landed as
https://crrev.com/ec334e1799ae25aaa1b0c66e25af641018a3a84a
Cr-Commit-Position: refs/heads/master@{#391114}

[Dirk Pranke, 2016-05-03 01:21:02]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1939313003/ by dpranke@chromium.org.

The reason for reverting is: This has caused content_browsertests to fail on
multiple builders, e.g.:

https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/23972
https://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/34086.

[Dirk Pranke, 2016-05-03 02:02:00]

On 2016/05/03 01:21:02, Dirk Pranke wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/1939313003/ by mailto:dpranke@chromium.org.
> 
> The reason for reverting is: This has caused content_browsertests to fail on
> multiple builders, e.g.:
> 
> https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/23972
>
https://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/34086.

Whoops, sorry, that should've been content_unittests.