Migrate MediaDevices.enumerateDevices to Mojo

Migrate MediaDevices.enumerateDevices to Mojo

Note that Mojo is not called directly from Blink because device enumerations
must also be called by content::PepperMediaDeviceManager.
The Pepper work will come in a follow-up CL.

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9357cc53ac0d187d3020a8dab9596f1e67db5abe
Cr-Commit-Position: refs/heads/master@{#424517}

[Guido Urdaneta, 2016-09-28 17:56:30]

Description was changed from

==========
Migrate MediaDevices.enumerateDevices to Mojo

Note that Mojo is not called directly from Blink because device enumerations
must also be called by content::PepperMediaDeviceManager.
The Pepper work will come in a follow-up CL.

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

to

==========
Migrate MediaDevices.enumerateDevices to Mojo

Note that Mojo is not called directly from Blink because device enumerations
must also be called by content::PepperMediaDeviceManager.
The Pepper work will come in a follow-up CL.

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Guido Urdaneta, 2016-09-28 18:01:05]

Patchset #1 (id:1) has been deleted

[Guido Urdaneta, 2016-09-28 18:01:24]

Description was changed from

==========
Migrate MediaDevices.enumerateDevices to Mojo

Note that Mojo is not called directly from Blink because device enumerations
must also be called by content::PepperMediaDeviceManager.
The Pepper work will come in a follow-up CL.

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Migrate MediaDevices.enumerateDevices to Mojo

Note that Mojo is not called directly from Blink because device enumerations
must also be called by content::PepperMediaDeviceManager.
The Pepper work will come in a follow-up CL.

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Guido Urdaneta, 2016-09-28 18:01:24]

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

[Guido Urdaneta, 2016-09-28 18:01:39]

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

[commit-bot: I haz the power, 2016-09-28 18:02:37]

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-09-28 18:04:17]

dcheng@: This is an IPC-to-mojo migration. Can you take a look at the general
Mojo/IPC work before I send this to the rest of the owners?

[commit-bot: I haz the power, 2016-09-28 18:08:01]

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

[commit-bot: I haz the power, 2016-09-28 18:08:02]

Dry run: Exceeded global retry quota

[Guido Urdaneta, 2016-09-29 09:32:49]

Patchset #1 (id:20001) has been deleted

[Guido Urdaneta, 2016-09-29 09:32:56]

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

[commit-bot: I haz the power, 2016-09-29 09:33:06]

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-09-29 09:36:51]

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

[commit-bot: I haz the power, 2016-09-29 09:36:52]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Guido Urdaneta, 2016-09-29 09:59:58]

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

[Guido Urdaneta, 2016-09-29 10:00:05]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-09-29 10:00:19]

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-09-29 10:32:07]

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

[commit-bot: I haz the power, 2016-09-29 10:32:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hta - Chromium, 2016-09-30 08:38:11]

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

[hta - Chromium, 2016-09-30 08:38:13]

Probably too early to review, but I did some commentary anyway.
What I understand seems mostly OK to me.

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

https://codereview.chromium.org/2380793002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:91:
bad_message::MDDH_INVALID_DEVICE_TYPE);
Given that this is what the error means, should it be called
MDDH_INVALID_DEVICE_TYPE_REQUEST?

https://codereview.chromium.org/2380793002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:116: //
audio input and output. TODO(guidou): use specific permission for audio
New line for TODO?

https://codereview.chromium.org/2380793002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_devices_dispatcher_host.h
(right):

https://codereview.chromium.org/2380793002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.h:54: friend
class MediaDevicesDispatcherHostTest;
If this is the only reason to friend the unit test, I'd rather expose a method
called SetFakeUiProxyForTesting(fake_proxy).

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
File content/common/media/media_devices.mojom (right):

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
content/common/media/media_devices.mojom:16: // requests. TODO(guidou): Add
support for device-change notifications.
Style: TODO on separate line.

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
content/common/media/media_devices.mojom:19: // Use MediaDeviceType values to
index the result.
Would it be more correct to say "The result is indexed by device type, as
defined in <header>"? As it stands, it's not that easy to see what it points to.

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
File content/common/media/media_devices_param_traits.h (right):

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
content/common/media/media_devices_param_traits.h:5: // Multiply-included IPC
traits file, hence no include guard.
I'm a bit surprised to see this IPC traits file in the mojom-based solution. Why
is it needed?

[Guido Urdaneta, 2016-10-04 16:43:53]

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

[Guido Urdaneta, 2016-10-04 16:45:30]

mkwst@: Can you do a preliminary review of the mojo/IPC parts of this CL before
I send it to the other owners?

[Mike West, 2016-10-06 06:36:36]

Looks like the right direction to me. Please let the functional owners sign off
on the behavior, and I'll come back and stamp the messaging bits once they've
solidified.

[Guido Urdaneta, 2016-10-07 17:00:46]

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

[commit-bot: I haz the power, 2016-10-07 17:01:17]

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-10-07 17:07:13]

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

https://codereview.chromium.org/2380793002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:91:
bad_message::MDDH_INVALID_DEVICE_TYPE);
On 2016/09/30 08:38:12, hta - Chromium wrote:
> Given that this is what the error means, should it be called
> MDDH_INVALID_DEVICE_TYPE_REQUEST?

Done.

https://codereview.chromium.org/2380793002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:116: //
audio input and output. TODO(guidou): use specific permission for audio
On 2016/09/30 08:38:12, hta - Chromium wrote:
> New line for TODO?

Done.

https://codereview.chromium.org/2380793002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_devices_dispatcher_host.h
(right):

https://codereview.chromium.org/2380793002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.h:54: friend
class MediaDevicesDispatcherHostTest;
On 2016/09/30 08:38:12, hta - Chromium wrote:
> If this is the only reason to friend the unit test, I'd rather expose a method
> called SetFakeUiProxyForTesting(fake_proxy).

Done.

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
File content/common/media/media_devices.mojom (right):

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
content/common/media/media_devices.mojom:16: // requests. TODO(guidou): Add
support for device-change notifications.
On 2016/09/30 08:38:12, hta - Chromium wrote:
> Style: TODO on separate line.

Done.

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
content/common/media/media_devices.mojom:19: // Use MediaDeviceType values to
index the result.
On 2016/09/30 08:38:12, hta - Chromium wrote:
> Would it be more correct to say "The result is indexed by device type, as
> defined in <header>"? As it stands, it's not that easy to see what it points
to.

Done.

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
File content/common/media/media_devices_param_traits.h (right):

https://codereview.chromium.org/2380793002/diff/60001/content/common/media/me...
content/common/media/media_devices_param_traits.h:5: // Multiply-included IPC
traits file, hence no include guard.
On 2016/09/30 08:38:12, hta - Chromium wrote:
> I'm a bit surprised to see this IPC traits file in the mojom-based solution.
Why
> is it needed?

Because the MediaDeviceInfo type we are using is a regular C++ struct and we
need to define a way to serialize it for IPC purposes.

The choices are:
1. Define a Mojo struct that replaces MediaDeviceInfo and use it everywhere,
even in things not concerned with IPC.
2. Define a Mojo struct as in (1), but instead of replacing
content::MediaDeviceInfo,  define a mapping from the native version to the mojo
one.
3. Use IPC_ macros to generate serialization code for MediaDeviceInfo.
4. Manually write the serialization code for MediaDeviceInfo (same code
generated by the IPC_ macros)

As of now, the most straightforward choice is (3).
We might want to invoke this directly from Blink at some point. In that case
we'll probably have to go with (1) or (2).

[Guido Urdaneta, 2016-10-07 17:19:54]

Patchset #3 (id:100001) has been deleted

[Guido Urdaneta, 2016-10-07 17:25:40]

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

[Guido Urdaneta, 2016-10-07 17:25:41]

alexmos@chromium.org: Please review changes in content/browser/frame_host

tommi@chromium.org: Please review changes in content/common/media,
content/renderer/media and content/browser/renderer_host/media

isherman@chromium.org: Please review changes in histograms.xml

[tommi (sloooow) - chröme, 2016-10-07 19:59:40]

lgtm

https://codereview.chromium.org/2380793002/diff/120001/content/renderer/media...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2380793002/diff/120001/content/renderer/media...
content/renderer/media/user_media_client_impl.cc:145: if
(device_label.find("front") != std::string::npos) {
is this behavior copied from somewhere?  Would it make sense for us to be a
little more strict here? e.g. what should "backfront" match with?

[Guido Urdaneta, 2016-10-08 10:09:56]

https://codereview.chromium.org/2380793002/diff/120001/content/renderer/media...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2380793002/diff/120001/content/renderer/media...
content/renderer/media/user_media_client_impl.cc:145: if
(device_label.find("front") != std::string::npos) {
On 2016/10/07 19:59:40, tommi (chrömium) wrote:
> is this behavior copied from somewhere?  Would it make sense for us to be a
> little more strict here? e.g. what should "backfront" match with?

This is copied from here:
https://cs.chromium.org/chromium/src/content/public/common/media_stream_reque...

The reason it works is because, in Android, camera labels include the
orientation and can only include one of the strings. See
https://cs.chromium.org/chromium/src/third_party/webrtc/api/android/java/src/...

MediaStreamTrack.GetSources() is going away anyway, but we will have to decide
how to deal with the orientation once we implement capabilities and
applyConstraints.

[Guido Urdaneta, 2016-10-08 10:15:05]

Patchset #2 (id:80001) has been deleted

[Guido Urdaneta, 2016-10-09 08:29:10]

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

[Guido Urdaneta, 2016-10-09 08:29:11]

isherman@: please review histograms.xml

[dcheng, 2016-10-10 17:51:53]

(drive-by)

https://codereview.chromium.org/2380793002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2380793002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2223: //
BrowserMainLoop::GetInstance() may be null on unit tests.
Out of curiosity, which unit tests does this trip up?

It'd be nicer not to have to do this, since it generally makes code a bit harder
to understand.

[alexmos, 2016-10-10 17:56:13]

https://codereview.chromium.org/2380793002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2380793002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2233:
base::Unretained(media_stream_manager),
Can you please explain (and add a comment about) why base::Unretained is safe
here?  E.g., is it similar to the explanation for time_zone_monitor here? 
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...

[Guido Urdaneta, 2016-10-10 18:01:05]

On 2016/10/10 17:51:53, dcheng wrote:
> (drive-by)
> 
>
https://codereview.chromium.org/2380793002/diff/120001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2380793002/diff/120001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_impl.cc:2223: //
> BrowserMainLoop::GetInstance() may be null on unit tests.
> Out of curiosity, which unit tests does this trip up?
> 
> It'd be nicer not to have to do this, since it generally makes code a bit
harder
> to understand.

A lot of unit tests fail. Maybe all of them. 
From
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_iso...
this is a count of failed unit tests:
 122 component_unittests
 357 content_unittests
 952 unit_tests

[Guido Urdaneta, 2016-10-10 18:34:42]

https://codereview.chromium.org/2380793002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2380793002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2233:
base::Unretained(media_stream_manager),
On 2016/10/10 17:56:13, alexmos wrote:
> Can you please explain (and add a comment about) why base::Unretained is safe
> here?  E.g., is it similar to the explanation for time_zone_monitor here? 
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...

Done.

[alexmos, 2016-10-10 20:51:03]

Thanks, frame_host LGTM

[Ilya Sherman, 2016-10-10 23:35:35]

histograms.xml lgtm

[Guido Urdaneta, 2016-10-11 08:25:06]

mkwst@: functional owners have given approval. Please continue with the
IPC/security review.

[Mike West, 2016-10-11 08:34:41]

The port of the existing message LGTM.

[Guido Urdaneta, 2016-10-11 08:55:29]

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

[Guido Urdaneta, 2016-10-11 08:55:30]

jochen@chromium.org: Can you rs top-level content/browser and content/common?

[Guido Urdaneta, 2016-10-11 15:41:32]

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

[Guido Urdaneta, 2016-10-11 15:41:55]

avi@: Can you rs?

[Avi (use Gerrit), 2016-10-11 15:44:29]

lgtm

stampity stamp

[Guido Urdaneta, 2016-10-11 15:44:59]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-10-11 15:45:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2380793002/#ps140001
(title: "address alexmos@ comments")

[commit-bot: I haz the power, 2016-10-11 15:45:23]

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-10-11 15:47:49]

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

[commit-bot: I haz the power, 2016-10-11 15:47:50]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Guido Urdaneta, 2016-10-11 15:57:07]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-10-11 15:57:08]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
tommi@chromium.org, mkwst@chromium.org, alexmos@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2380793002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-11 15:57: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, 2016-10-11 16:08:08]

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

[commit-bot: I haz the power, 2016-10-11 16:08:11]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Guido Urdaneta, 2016-10-11 19:00:27]

The CQ bit was checked by guidou@chromium.org

[commit-bot: I haz the power, 2016-10-11 19:01:07]

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-10-11 19:53:58]

Description was changed from

==========
Migrate MediaDevices.enumerateDevices to Mojo

Note that Mojo is not called directly from Blink because device enumerations
must also be called by content::PepperMediaDeviceManager.
The Pepper work will come in a follow-up CL.

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Migrate MediaDevices.enumerateDevices to Mojo

Note that Mojo is not called directly from Blink because device enumerations
must also be called by content::PepperMediaDeviceManager.
The Pepper work will come in a follow-up CL.

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-11 19:54:00]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-10-11 19:55:59]

Description was changed from

==========
Migrate MediaDevices.enumerateDevices to Mojo

Note that Mojo is not called directly from Blink because device enumerations
must also be called by content::PepperMediaDeviceManager.
The Pepper work will come in a follow-up CL.

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Migrate MediaDevices.enumerateDevices to Mojo

Note that Mojo is not called directly from Blink because device enumerations
must also be called by content::PepperMediaDeviceManager.
The Pepper work will come in a follow-up CL.

BUG=648183
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9357cc53ac0d187d3020a8dab9596f1e67db5abe
Cr-Commit-Position: refs/heads/master@{#424517}
==========

[commit-bot: I haz the power, 2016-10-11 19:56:01]

Patchset 4 (id:??) landed as
https://crrev.com/9357cc53ac0d187d3020a8dab9596f1e67db5abe
Cr-Commit-Position: refs/heads/master@{#424517}