Change class VideoCaptureDevice::Name to struct VideoCaptureDeviceDescriptor

Change class VideoCaptureDevice::Name to struct VideoCaptureDeviceDescriptor.

The purpose of this change is to simplify the VideoCaptureDeviceFactory interface in preparation for making it a Mojo interface in a future step. In particular, we want to remove the platform-dependency of the interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor
  with a platform-independent interface. Moved out of
  video_capture_device.h/cc into new files
  video_capture_device_descriptor.h/cc.
* Instead of a method VideoCaptureDevice::Name::GetModel(), have |model_id|
  be a public field in struct VideoCaptureDeviceDescriptor. Moved the logic
  for determining the |model_id| into the platform-specific factory
  implementations. As a side-effect, this change eliminates the need for
  code that was added in issue 532272 [2].
* Rename VideoCaptureDeviceFactory::Create() to
  VideoCaptureDeviceFactory::CreateDevice()
* Moved struct VideoCaptureDeviceInfo closer to its usage. Moved from
  media/capture/video to content/browser/media.

[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified
     that issue 532272 does not regress by trying the repro steps in the crbug entry. Successfully conducted an apprtc loopback session on Linux.

Committed: https://crrev.com/1c38eb46ec548d21536fbbda949cc598fdc263e9
Cr-Commit-Position: refs/heads/master@{#409199}

[chfremer, 2016-07-21 18:05:43]

Description was changed from

==========
Let VideoCaptureDeviceFactory obtain VideoCaptureDeviceInfos including both
formats and model ids.


Change VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor


* Changed VideoCaptureDevice::Name to VideoCaptureDeviceDescriptor with a
platform-independent interface

BUG=
==========

to

==========
Remove platform-dependency from VideoCaptureDeviceFactory interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Let VideoCaptureDeviceFactory obtain VideoCaptureDeviceInfos including 
  both the supported formats and the model ids. Previously, the two would 
  be obtained seperately. The model ids were previously obtained through 
  VideoCaptureDevice::Name::GetModel() with implementations being located
  in somewhat arbitrary *.cc files for each platform. The implementations 
  have now moved to the platform-specific implementations of 
  VideoCaptureDeviceFactory.
  As a side-effect, this change should eliminate the need for code added 
  in issue 532272 [2]. (Need to confirm this)

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=TODO: Should verify that existing tests still pass. Should verify 
     that issue 532272 does not regress.
==========

[chfremer, 2016-07-21 18:09:49]

chfremer@chromium.org changed reviewers:
+ emircan@chromium.org, mcasas@chromium.org

[chfremer, 2016-07-21 18:09:50]

emircan@ and mcasas@, I am already adding you as reviewers to this
not-yet-complete CL to give you the chance for early feedback.

I still need to update the code for Mac and Android and then verify that tests
on all platforms still pass.

Please let me know if you agree with the overall approach.

[chfremer, 2016-07-22 16:45:50]

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

[commit-bot: I haz the power, 2016-07-22 16:46:11]

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-07-22 16:49:08]

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

[commit-bot: I haz the power, 2016-07-22 16:49:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chfremer, 2016-07-22 17:18:09]

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

[commit-bot: I haz the power, 2016-07-22 17:18:34]

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-07-22 17:28:47]

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

[commit-bot: I haz the power, 2016-07-22 17:28:49]

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

[chfremer, 2016-07-22 20:09:28]

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

[commit-bot: I haz the power, 2016-07-22 20:10:02]

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-07-22 20:22:23]

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

[commit-bot: I haz the power, 2016-07-22 20:22:24]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chfremer, 2016-07-22 21:27:59]

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

[commit-bot: I haz the power, 2016-07-22 21:28:47]

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-07-22 22:12:42]

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

[commit-bot: I haz the power, 2016-07-22 22:12:43]

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

[chfremer, 2016-07-22 23:55:47]

Patchset #8 (id:140001) has been deleted

[chfremer, 2016-07-22 23:59:42]

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

[commit-bot: I haz the power, 2016-07-23 00:00:09]

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-07-23 00:03:11]

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

[commit-bot: I haz the power, 2016-07-23 00:03:12]

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

[chfremer, 2016-07-25 16:19:23]

Patchset #1 (id:1) has been deleted

[chfremer, 2016-07-25 16:19:44]

Patchset #6 (id:120001) has been deleted

[chfremer, 2016-07-25 16:19:57]

Patchset #5 (id:100001) has been deleted

[chfremer, 2016-07-25 16:20:06]

Patchset #4 (id:80001) has been deleted

[chfremer, 2016-07-25 16:20:17]

Patchset #3 (id:60001) has been deleted

[chfremer, 2016-07-25 16:20:30]

Patchset #2 (id:40001) has been deleted

[chfremer, 2016-07-25 16:20:42]

Patchset #1 (id:20001) has been deleted

[chfremer, 2016-07-25 16:57:47]

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

[commit-bot: I haz the power, 2016-07-25 16:58:14]

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-07-25 17:11:34]

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

[commit-bot: I haz the power, 2016-07-25 17:11:35]

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

[chfremer, 2016-07-25 17:36:39]

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

[commit-bot: I haz the power, 2016-07-25 17:36:59]

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-07-25 18:02:42]

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

[commit-bot: I haz the power, 2016-07-25 18:02:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)

[chfremer, 2016-07-25 18:08:32]

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

[commit-bot: I haz the power, 2016-07-25 18:09:13]

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

[chfremer, 2016-07-25 18:43:26]

Description was changed from

==========
Remove platform-dependency from VideoCaptureDeviceFactory interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Let VideoCaptureDeviceFactory obtain VideoCaptureDeviceInfos including 
  both the supported formats and the model ids. Previously, the two would 
  be obtained seperately. The model ids were previously obtained through 
  VideoCaptureDevice::Name::GetModel() with implementations being located
  in somewhat arbitrary *.cc files for each platform. The implementations 
  have now moved to the platform-specific implementations of 
  VideoCaptureDeviceFactory.
  As a side-effect, this change should eliminate the need for code added 
  in issue 532272 [2]. (Need to confirm this)

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=TODO: Should verify that existing tests still pass. Should verify 
     that issue 532272 does not regress.
==========

to

==========
Remove platform-dependency from VideoCaptureDeviceFactory interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Let VideoCaptureDeviceFactory obtain VideoCaptureDeviceInfos including 
  both the supported formats and the model ids. Previously, the two would 
  be obtained seperately. The model ids were previously obtained through 
  VideoCaptureDevice::Name::GetModel() with implementations being located
  in somewhat arbitrary *.cc files for each platform. The implementations 
  have now moved to the platform-specific implementations of 
  VideoCaptureDeviceFactory.
  As a side-effect, this change should eliminate the need for code added 
  in issue 532272 [2]. (Need to confirm this)

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified 
     that issue 532272 does not regress by trying the repro steps in the crbug
entry. Successfully conducted an apprtc loopback session on Linux.
==========

[chfremer, 2016-07-25 18:48:28]

This is now ready for review.
Since lots of files had to be touched I'll suggest the following:

emircan@ Please focus on the platform-specific files for Android, Mac, and
Windows.
mcasas@ Please focus on Linux and platform-independent files.

[chfremer, 2016-07-25 18:48:47]

Patchset #3 (id:200001) has been deleted

[chfremer, 2016-07-25 18:48:59]

Patchset #2 (id:180001) has been deleted

[chfremer, 2016-07-25 18:49:11]

Patchset #1 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-07-25 19:28:48]

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

[commit-bot: I haz the power, 2016-07-25 19:28:49]

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

[emircan, 2016-07-25 22:56:40]

https://codereview.chromium.org/2169013002/diff/220001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2169013002/diff/220001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:930:
devices.push_back(StreamDeviceInfo(stream_type, it.GetNameAndModel(),
Use emplace_back().

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/li...
File media/capture/video/linux/v4l2_capture_delegate.h (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/li...
media/capture/video/linux/v4l2_capture_delegate.h:29: // Class doing the actual
Linux capture using V4L2 API. V4L2 SPLANE/MPLANE
There is no derived classes; this is marked final. Please update the comment.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/ma...
File media/capture/video/mac/video_capture_device_mac.h (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/ma...
media/capture/video/mac/video_capture_device_mac.h:58: const
VideoCaptureDeviceDescriptor& device_descriptor);
Move this below ctor/dtor. Also, make sure it appears in the same order in .cc

https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:34: OTHER_TRANSPORT
Can we split OTHER_TRANSPORT into two: MACOSX_OTHER_TRANSPORT, UNKNOWN? UNKNOWN
would be more clear in covering all the other platform cases.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:41: // disambiguates the
API.
Earlier there was a comment about using VideoCaptureDevice::GetDeviceNames to
create an instance. Does that apply for GetDeviceDescriptors?

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:62: std::string
friendly_name;
s/friendly_name/name/? Or drop a comment about why you call it friendly_name.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:67: // Allow generated
copy constructor and assignment.
This comment is unnecessary.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
File media/capture/video/video_capture_device_factory.h (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:40: // formats array will be
empty.
Please update the comment.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:43: VideoCaptureDeviceInfo*
device_info) = 0;
VideoCaptureDeviceInfo* const device_info

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:51:
VideoCaptureDeviceDescriptors* device_descriptors) = 0;
VideoCaptureDeviceDescriptors* const device_descriptors

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
File media/capture/video/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:131: DVLOG(1) << "
GetDeviceDescriptorsDirectShow";
Replace hard coded function names with __FUNCTION__ macro.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:182:
device_descriptors->push_back(descriptor);
Use std::list::emplace_back to avoid extra copy.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:467: std::string
id_vendor = device_descriptor.device_id.substr(
const

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:469: std::string
id_product = device_descriptor.device_id.substr(
const

[chfremer, 2016-07-26 16:56:35]

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/ma...
File media/capture/video/mac/video_capture_device_mac.h (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/ma...
media/capture/video/mac/video_capture_device_mac.h:58: const
VideoCaptureDeviceDescriptor& device_descriptor);
On 2016/07/25 22:56:40, emircan wrote:
> Move this below ctor/dtor. Also, make sure it appears in the same order in .cc
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:34: OTHER_TRANSPORT
I initially had the same thought, but then I couldn't find a reason why we would
want to distinguish the different platforms. Note that the situation is similar
with API_TYPE_UNKNOWN, where we also don't distinguish different platforms. 

And if we do distinguish, we would need an #idfef in the constructor
implementation to initialize the Mac OSX case differently from the other
platforms.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:41: // disambiguates the
API.
I believe you are referring to this:
  // You should not create an instance of this class directly by e.g. setting
  // various properties directly.  Instead use
  // VideoCaptureDevice::GetDeviceNames to do this for you and if you need to
  // cache your own copy of a name, you can do so via the copy constructor.
  // The reason for this is that a device name might contain platform specific
  // settings that are relevant only to the platform specific implementation of
  // VideoCaptureDevice::Create.

This description seems to be more an explanation of the intended use of the
VideoCaptureDeviceFactory API than an description of the struct itself. I
revised my description now and am trying to convey the same information. I also
put more explanations into the description of VideoDeviceCaptureFactory.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:62: std::string
friendly_name;
I chose this, because in the original code in VideoCaptureDevice::Name the
accessor method name() had a comment explaining that this is the "Friendly name
of a device". By choosing this name I was hoping to eliminate the need for the
comment, which apparently did not work out :-).

Is there any name I can choose for this field that is self-explaning enough to
not require a comment?
How would you feel about "display_name"?

For now, I am adding a comment back in.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:67: // Allow generated
copy constructor and assignment.
removed

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
File media/capture/video/video_capture_device_factory.h (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:40: // formats array will be
empty.
On 2016/07/25 22:56:40, emircan wrote:
> Please update the comment.

Done.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:43: VideoCaptureDeviceInfo*
device_info) = 0;
Discussed offline. 
I don't see much value in making the pointer const since it (the pointer) is
passed by value, which already guarantees that callers would not be affected by
the method implementation trying to modify the pointer.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:51:
VideoCaptureDeviceDescriptors* device_descriptors) = 0;
On 2016/07/25 22:56:40, emircan wrote:
> VideoCaptureDeviceDescriptors* const device_descriptors

Acknowledged.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
File media/capture/video/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:131: DVLOG(1) << "
GetDeviceDescriptorsDirectShow";
On 2016/07/25 22:56:40, emircan wrote:
> Replace hard coded function names with __FUNCTION__ macro.

Done.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:182:
device_descriptors->push_back(descriptor);
On 2016/07/25 22:56:40, emircan wrote:
> Use std::list::emplace_back to avoid extra copy.

Done.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:467: std::string
id_vendor = device_descriptor.device_id.substr(
On 2016/07/25 22:56:40, emircan wrote:
> const

Done.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:469: std::string
id_product = device_descriptor.device_id.substr(
On 2016/07/25 22:56:40, emircan wrote:
> const

Done.

[mcasas, 2016-07-26 23:52:04]

Looking good, just a few comments/questions.

https://codereview.chromium.org/2169013002/diff/240001/content/browser/media/...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/content/browser/media/...
content/browser/media/media_internals_unittest.cc:117: //
https://crbug.com/630694
What's the TODO? :)

The original meaning of the test was to make sure,
by breaking, taht developers would update media_internals
code when they updated other parts of media/capture,
e.g. [1]. nisse@ had a go at it but couldn't do much.


[1]
https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc...

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/an...
File media/capture/video/android/video_capture_device_android.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/an...
media/capture/video/android/video_capture_device_android.h:124:
VideoCaptureDeviceDescriptor device_descriptor_;
nit: can be made const?

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:17: enum class
VideoCaptureApiType {
s/VideoCaptureApiType/VideoCaptureAPI/ (or Api)

There's no need to further identify with "Type", IMHO.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:28: API_TYPE_UNKNOWN
Since it's an enum class, the name will
always prefix this entry, and
VideoCaptureApi::API_TYPE_UNKNOWN, is a
bit verbose, so just make it
VideoCaptureApi::UNKNOWN

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:42: struct CAPTURE_EXPORT
VideoCaptureDeviceDescriptor {
I'd err on the side of making this a class [1]

[1] https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:48:
VideoCaptureDeviceDescriptor(const VideoCaptureDeviceDescriptor& other);
We used to have a ctor with a |transport_type|
argument, used in Mac, right? [1]

[1]
https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device...

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:67: VideoCaptureApiType
capture_api;
Make this three const?

And, if possible, |transport_type| too (if
we bring back the constructor with it).

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:71: typedef
std::list<VideoCaptureDeviceDescriptor> VideoCaptureDeviceDescriptors;
using VideoCaptureDeviceDescriptors = std::list<VideoCaptureDeviceDescriptor>;

(typedef is the old thing).

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_factory.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.cc:45:
VideoCaptureDeviceFactory::~VideoCaptureDeviceFactory() {
nit: I might be wrong, but I believe empty function/method
bodies can be squeezed into one line:

VideoCaptureDeviceFactory::~VideoCaptureDeviceFactory() {}

(Or so does clang-format locally)

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_factory.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:21: // used to either get
extended information about a device usng GetDeviceInfo(),
nit: s/usng/using/

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:49: // Gets descriptors of
all video capture devices connected to this computer.
Heh, maybe remove "to this computer"?

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_info.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_info.cc:21: }
No need for {} for one-line bodies (might
not be fully enforced in media/ but it's a
pervasive enough rule that we should follow 
it).

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_info.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_info.h:13: struct CAPTURE_EXPORT
VideoCaptureDeviceInfo {
As a note, I was reading the changes in 
VideoCaptureManager and found confusing the
difference between VCDDescriptor and VCDInfo
(whereas on ToT this is Name and Info, and
Name sounds like less informational than
Info...).

So, I like VCDDescriptor, but Info is a bit
less evident. But really, VCDInfo is just a
convenience for VideoCaptureManager (later
was hijacked by media_internals stuff as well),
so could we just nuke VCDInfo ?

Also, shouldn't |model_id| live in VCDDescriptor?

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/wi...
File media/capture/video/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:36: const size_t
kVidPidSize = 4;
Maybe we can use arraysize() here? (It might
need |kVidPrefix|, |kPidPrefix| to be static).

Otherwise, add DCHECK_EQ(sizeof(bla) - 1, bla);
where you use these, to catch regressions down the
line.

https://cs.chromium.org/chromium/src/third_party/webrtc/base/arraysize.h?dr=S...

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:448: const size_t
vid_prefix_size = sizeof(kVidPrefix) - 1;
I thought sizeof(kVidPrefix) - 1 was called
|kVidPidSize| ?

[mcasas, 2016-07-27 14:37:17]

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

[commit-bot: I haz the power, 2016-07-27 14:37:31]

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-07-27 16:54:35]

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

[commit-bot: I haz the power, 2016-07-27 16:54:37]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-07-27 23:10:18]

PTAL

https://codereview.chromium.org/2169013002/diff/240001/content/browser/media/...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/content/browser/media/...
content/browser/media/media_internals_unittest.cc:117: //
https://crbug.com/630694
The TODO is to remove the test (see also https://crbug.com/630694) unless we can
convince ourselves that there is value in keeping it. I am currently not quite
convinced of its value.

I don't understand what to look at at the code you referenced in [1].
I see there is an invocation of GetCaptureApiTypeString(), but I don't see the
connection to this test. Could you please elaborate?

[1]
https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc...

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/an...
File media/capture/video/android/video_capture_device_android.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/an...
media/capture/video/android/video_capture_device_android.h:124:
VideoCaptureDeviceDescriptor device_descriptor_;
On 2016/07/26 23:52:03, mcasas wrote:
> nit: can be made const?

Done.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:17: enum class
VideoCaptureApiType {
On 2016/07/26 23:52:03, mcasas wrote:
> s/VideoCaptureApiType/VideoCaptureAPI/ (or Api)
> 
> There's no need to further identify with "Type", IMHO.

Done.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:28: API_TYPE_UNKNOWN
On 2016/07/26 23:52:04, mcasas wrote:
> Since it's an enum class, the name will
> always prefix this entry, and
> VideoCaptureApi::API_TYPE_UNKNOWN, is a
> bit verbose, so just make it
> VideoCaptureApi::UNKNOWN

Done.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:42: struct CAPTURE_EXPORT
VideoCaptureDeviceDescriptor {
After reviewing the styleguide, I still feel struct is the right choice here.
The only behavior implemented here is GetCaptureApiTypeString(), and this does
not really belong here.
Would you feel more comfortable with it being a struct if I moved that method
out?

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:48:
VideoCaptureDeviceDescriptor(const VideoCaptureDeviceDescriptor& other);
True. I had removed all constructors for initializing the struct in order to
treat it as "plain old data" and have clients use the empty constructor followed
by assigning to fields by name.

Since emircan@ already suggested the use of emplace_back(), which requires
constructors, I am now adding the other constructors back in and use them
wherever possible.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:67: VideoCaptureApiType
capture_api;
We need the ability to assign values when we fill default-constructed instances
that are passed into VideoCaptureDeviceFactory::GetDeviceInfo(). This is done in
order to avoid the need for returning a copy of VideoCaptureDeviceInfo from that
method.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_factory.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.cc:45:
VideoCaptureDeviceFactory::~VideoCaptureDeviceFactory() {
On 2016/07/26 23:52:04, mcasas wrote:
> nit: I might be wrong, but I believe empty function/method
> bodies can be squeezed into one line:
> 
> VideoCaptureDeviceFactory::~VideoCaptureDeviceFactory() {}
> 
> (Or so does clang-format locally)

Done.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_factory.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:21: // used to either get
extended information about a device usng GetDeviceInfo(),
On 2016/07/26 23:52:04, mcasas wrote:
> nit: s/usng/using/

Done.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_factory.h:49: // Gets descriptors of
all video capture devices connected to this computer.
On 2016/07/26 23:52:04, mcasas wrote:
> Heh, maybe remove "to this computer"?

Done.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_info.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_info.cc:21: }
On 2016/07/26 23:52:04, mcasas wrote:
> No need for {} for one-line bodies (might
> not be fully enforced in media/ but it's a
> pervasive enough rule that we should follow 
> it).

Done.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_info.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_info.h:13: struct CAPTURE_EXPORT
VideoCaptureDeviceInfo {
I agree that the difference between ...Descriptor and ...Info is not clear from
the names. Unfortunately, I don't have any great ideas for better naming here.
What does ToT stand for?

I am assuming that the reason to keep the two separate is that it is supposed to
be "cheap" to query for the descriptors, but it can be expensive to query for
the model_id and supported formats. Because of that, I decided to keep them
separate.

How would you feel about renaming VideoCaptureDeviceInfo to
VideoCaptureDeviceExpensiveInfo? Or VideoCaptureDeviceProperties?

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/wi...
File media/capture/video/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:36: const size_t
kVidPidSize = 4;
By "here" do you mean on the right hand sight of line 36?
It seems the number 4 is referring to the size of the identifiers, not the size
of the prefixes (which also happen to be 4 bytes long).

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:448: const size_t
vid_prefix_size = sizeof(kVidPrefix) - 1;
That also confused me at first. It is because there is a size for the prefixes
and a size for the actual vid/pid.

[mcasas, 2016-07-28 00:05:42]

https://codereview.chromium.org/2169013002/diff/240001/content/browser/media/...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/content/browser/media/...
content/browser/media/media_internals_unittest.cc:117: //
https://crbug.com/630694
On 2016/07/27 23:10:17, chfremer wrote:
> The TODO is to remove the test (see also https://crbug.com/630694) unless we
can
> convince ourselves that there is value in keeping it. I am currently not quite
> convinced of its value.

Then add some action to the TODO :) like "consider removing...".
> 
> I don't understand what to look at at the code you referenced in [1].
> I see there is an invocation of GetCaptureApiTypeString(), but I don't see the
> connection to this test. Could you please elaborate?
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc...
>

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:42: struct CAPTURE_EXPORT
VideoCaptureDeviceDescriptor {
On 2016/07/27 23:10:18, chfremer wrote:
> After reviewing the styleguide, I still feel struct is the right choice here.
> The only behavior implemented here is GetCaptureApiTypeString(), and this does
> not really belong here.
> Would you feel more comfortable with it being a struct if I moved that method
> out?

Hmm I'll leave the decision to you.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:67: VideoCaptureApiType
capture_api;
On 2016/07/27 23:10:18, chfremer wrote:
> We need the ability to assign values when we fill default-constructed
instances
> that are passed into VideoCaptureDeviceFactory::GetDeviceInfo(). This is done
in
> order to avoid the need for returning a copy of VideoCaptureDeviceInfo from
that
> method.

Acknowledged.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:71: typedef
std::list<VideoCaptureDeviceDescriptor> VideoCaptureDeviceDescriptors;
On 2016/07/26 23:52:03, mcasas wrote:
> using VideoCaptureDeviceDescriptors = std::list<VideoCaptureDeviceDescriptor>;
> 
> (typedef is the old thing).

ping

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_info.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_info.h:13: struct CAPTURE_EXPORT
VideoCaptureDeviceInfo {
On 2016/07/27 23:10:18, chfremer wrote:
> I agree that the difference between ...Descriptor and ...Info is not clear
from
> the names. Unfortunately, I don't have any great ideas for better naming here.
> What does ToT stand for?
Tip of the Tree, i.e. master standpoint.

> 
> I am assuming that the reason to keep the two separate is that it is supposed
to
> be "cheap" to query for the descriptors, but it can be expensive to query for
> the model_id and supported formats. Because of that, I decided to keep them
> separate.

Not really: Linux vid:pid implies reading from the /dev/ file, but
that has to happen anyhow, and for Win/Mac, the vid:pid is just
extracted out of the device id. (Android has no vid:pid). I guess 
is just historical, so I'd say just move |model_id| to VCDIngo if 
at all possible, so VCDInfo keeps all the naming information 
together.



> 
> How would you feel about renaming VideoCaptureDeviceInfo to
> VideoCaptureDeviceExpensiveInfo? Or VideoCaptureDeviceProperties?

With my comment before, could we consider nuking
VideoCaptureDeviceInfo and using a
using std::pair<>or an ad-hoc struct inside VideoCaptureManager
to tie together |name| and |supported_formats| ?

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/wi...
File media/capture/video/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:36: const size_t
kVidPidSize = 4;
On 2016/07/27 23:10:18, chfremer wrote:
> By "here" do you mean on the right hand sight of line 36?
> It seems the number 4 is referring to the size of the identifiers, not the
size
> of the prefixes (which also happen to be 4 bytes long).

Oh I see :)
What about calling l.36 |kVidPidFieldSize| ?

Also, consider using arraysize(kVidPrefix) (resp., |kPidPrefix|)
since arraysize is a compile-time constant (but might need
the variable to be static, don't remember exactly).

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:448: const size_t
vid_prefix_size = sizeof(kVidPrefix) - 1;
On 2016/07/27 23:10:18, chfremer wrote:
> That also confused me at first. It is because there is a size for the prefixes
> and a size for the actual vid/pid.

Acknowledged.

[chfremer, 2016-07-28 19:17:50]

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

[commit-bot: I haz the power, 2016-07-28 19:18:13]

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-07-28 19:29:35]

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

[commit-bot: I haz the power, 2016-07-28 19:29:36]

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

[emircan, 2016-07-28 21:23:02]

lgtm % nits below. Also, please address the bot issues.

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:62: std::string
friendly_name;
On 2016/07/26 16:56:35, chfremer wrote:
> I chose this, because in the original code in VideoCaptureDevice::Name the
> accessor method name() had a comment explaining that this is the "Friendly
name
> of a device". By choosing this name I was hoping to eliminate the need for the
> comment, which apparently did not work out :-).
> 
> Is there any name I can choose for this field that is self-explaning enough to
> not require a comment?
> How would you feel about "display_name"?
> 
> For now, I am adding a comment back in.

|display_name| sgtm

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/an...
File media/capture/video/android/video_capture_device_factory_android.h (right):

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/an...
media/capture/video/android/video_capture_device_factory_android.h:36:
VideoCaptureFormats* supported_formats);
override

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/ma...
File media/capture/video/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/ma...
media/capture/video/mac/video_capture_device_mac.mm:344: std::string
device_model = GetDeviceModelId(
const std::string& device_model?

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:45:
VideoCaptureDeviceDescriptor(const std::string friendly_name,
const std::string& friendly_name and below.
Also, using default arg may be helpful in reducing the number of constructors.
https://google.github.io/styleguide/cppguide.html

[chfremer, 2016-07-28 21:58:41]

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

[commit-bot: I haz the power, 2016-07-28 21:59:04]

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-07-28 22:08:55]

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

[commit-bot: I haz the power, 2016-07-28 22:08:57]

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

[chfremer, 2016-07-28 22:37:57]

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

[commit-bot: I haz the power, 2016-07-28 22:38:16]

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

[chfremer, 2016-07-28 22:38:20]

Patchset #6 (id:320001) has been deleted

[chfremer, 2016-07-28 22:38:52]

Patchset #4 (id:280001) has been deleted

[chfremer, 2016-07-28 22:39:14]

Patchset #3 (id:260001) has been deleted

[chfremer, 2016-07-28 22:47:34]

Description was changed from

==========
Remove platform-dependency from VideoCaptureDeviceFactory interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Let VideoCaptureDeviceFactory obtain VideoCaptureDeviceInfos including 
  both the supported formats and the model ids. Previously, the two would 
  be obtained seperately. The model ids were previously obtained through 
  VideoCaptureDevice::Name::GetModel() with implementations being located
  in somewhat arbitrary *.cc files for each platform. The implementations 
  have now moved to the platform-specific implementations of 
  VideoCaptureDeviceFactory.
  As a side-effect, this change should eliminate the need for code added 
  in issue 532272 [2]. (Need to confirm this)

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified 
     that issue 532272 does not regress by trying the repro steps in the crbug
entry. Successfully conducted an apprtc loopback session on Linux.
==========

to

==========
Remove platform-dependency from VideoCaptureDeviceFactory interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Instead of a method VideoCaptureDevice::Name::GetModel(), have |model_id| 
  be a public field in struct VideoCaptureDeviceDescriptor. Moved the logic 
  for determining the |model_id| into the platform-specific factory 
  implementations. As a side-effect, this change eliminates the need for 
  code that was added in issue 532272 [2].
* Rename VideoCaptureDeviceFactory::Create() to 
  VideoCaptureDeviceFactory::CreateDevice()
* Moved struct VideoCaptureDeviceInfo closer to its usage. Moved from 
  media/capture/video to content/browser/media. 
  
[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified 
     that issue 532272 does not regress by trying the repro steps in the crbug
entry. Successfully conducted an apprtc loopback session on Linux.
==========

[commit-bot: I haz the power, 2016-07-28 22:51:21]

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

[commit-bot: I haz the power, 2016-07-28 22:51:22]

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

[chfremer, 2016-07-28 22:53:40]

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

[commit-bot: I haz the power, 2016-07-28 22:53:54]

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

[chfremer, 2016-07-28 22:54:17]

Patchset #4 (id:340001) has been deleted

[commit-bot: I haz the power, 2016-07-28 23:15:29]

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

[commit-bot: I haz the power, 2016-07-28 23:15:30]

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

[chfremer, 2016-07-28 23:28:02]

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

[commit-bot: I haz the power, 2016-07-28 23:28:24]

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-07-28 23:58:23]

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

[commit-bot: I haz the power, 2016-07-28 23:58:24]

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

[chfremer, 2016-07-29 18:05:44]

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

[commit-bot: I haz the power, 2016-07-29 18:06:06]

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

[chfremer, 2016-07-29 18:08:22]

Patchset #5 (id:380001) has been deleted

[chfremer, 2016-07-29 18:08:39]

Patchset #4 (id:360001) has been deleted

[chfremer, 2016-07-29 18:09:51]

@mcasas: PTAL

Addressed comments by mcasas and emircan

* Move |model_id| from VideoCaptureDeviceInfo to VideoCaptureDeviceDescriptor.
* Rename VideoCaptureDeviceFactory::Create() to
VideoCaptureDeviceFactory::CreateDevice()
* Move struct VideoCaptureDeviceInfo closer to its usage. Moved from
media/capture/video to content/browser/media
* Instead of VideoCaptureDeviceInfo, let VideoCaptureDeviceFactories return
supported formats (as it did before
  this CL) and let VideoDeviceManager pack them into VideoCaptureDeviceInfo.
* Update CL description to match latest Patch Set

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/220001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:62: std::string
friendly_name;
On 2016/07/28 21:23:01, emircan wrote:
> On 2016/07/26 16:56:35, chfremer wrote:
> > I chose this, because in the original code in VideoCaptureDevice::Name the
> > accessor method name() had a comment explaining that this is the "Friendly
> name
> > of a device". By choosing this name I was hoping to eliminate the need for
the
> > comment, which apparently did not work out :-).
> > 
> > Is there any name I can choose for this field that is self-explaning enough
to
> > not require a comment?
> > How would you feel about "display_name"?
> > 
> > For now, I am adding a comment back in.
> 
> |display_name| sgtm

Done.

https://codereview.chromium.org/2169013002/diff/240001/content/browser/media/...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/2169013002/diff/240001/content/browser/media/...
content/browser/media/media_internals_unittest.cc:117: //
https://crbug.com/630694
On 2016/07/28 00:05:42, mcasas wrote:
> On 2016/07/27 23:10:17, chfremer wrote:
> > The TODO is to remove the test (see also https://crbug.com/630694) unless we
> can
> > convince ourselves that there is value in keeping it. I am currently not
quite
> > convinced of its value.
> 
> Then add some action to the TODO :) like "consider removing...".
> > 
> > I don't understand what to look at at the code you referenced in [1].
> > I see there is an invocation of GetCaptureApiTypeString(), but I don't see
the
> > connection to this test. Could you please elaborate?
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/content/browser/media/media_internals.cc...
> > 
> 

Done.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:42: struct CAPTURE_EXPORT
VideoCaptureDeviceDescriptor {
Okay, then I'll leave it a struct for now.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:71: typedef
std::list<VideoCaptureDeviceDescriptor> VideoCaptureDeviceDescriptors;
On 2016/07/28 00:05:42, mcasas wrote:
> On 2016/07/26 23:52:03, mcasas wrote:
> > using VideoCaptureDeviceDescriptors =
std::list<VideoCaptureDeviceDescriptor>;
> > 
> > (typedef is the old thing).
> 
> ping

Done.

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
File media/capture/video/video_capture_device_info.h (right):

https://codereview.chromium.org/2169013002/diff/240001/media/capture/video/vi...
media/capture/video/video_capture_device_info.h:13: struct CAPTURE_EXPORT
VideoCaptureDeviceInfo {
On 2016/07/28 00:05:42, mcasas wrote:
> On 2016/07/27 23:10:18, chfremer wrote:
> > I am assuming that the reason to keep the two separate is that it is
supposed
> to
> > be "cheap" to query for the descriptors, but it can be expensive to query
for
> > the model_id and supported formats. Because of that, I decided to keep them
> > separate.
> 
> Not really: Linux vid:pid implies reading from the /dev/ file, but
> that has to happen anyhow, and for Win/Mac, the vid:pid is just
> extracted out of the device id. (Android has no vid:pid). I guess 
> is just historical, so I'd say just move |model_id| to VCDIngo if 
> at all possible, so VCDInfo keeps all the naming information 
> together.

I think you meant to suggest to put |model_id| into VCDDescriptor. Considering
that obtaining the |model_ids| is cheap, I agree that it should go there. I will
post a new PatchSet with that change.

> > How would you feel about renaming VideoCaptureDeviceInfo to
> > VideoCaptureDeviceExpensiveInfo? Or VideoCaptureDeviceProperties?
> 
> With my comment before, could we consider nuking
> VideoCaptureDeviceInfo and using a
> using std::pair<>or an ad-hoc struct inside VideoCaptureManager
> to tie together |name| and |supported_formats| ?

Yes. Will do that.

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/ma...
File media/capture/video/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/ma...
media/capture/video/mac/video_capture_device_mac.mm:344: std::string
device_model = GetDeviceModelId(
On 2016/07/28 21:23:02, emircan wrote:
> const std::string& device_model?

I will add const, but const reference is not possible here, since the method
returns a copy of a string.

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/300001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:45:
VideoCaptureDeviceDescriptor(const std::string friendly_name,
On 2016/07/28 21:23:02, emircan wrote:
> const std::string& friendly_name and below.
> Also, using default arg may be helpful in reducing the number of constructors.
> https://google.github.io/styleguide/cppguide.html 

Done.

[commit-bot: I haz the power, 2016-07-29 19:14:24]

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

[commit-bot: I haz the power, 2016-07-29 19:14:25]

Dry run: This issue passed the CQ dry run.

[mcasas, 2016-07-30 00:06:12]

LGTM % comments below 
(most prominently, try hard to nuke VideoCaptureDeviceInfo).

Also, recommend changing the CL short description
to something stating what it does, ISO what it is for,
e.g. "VideoCaptureDevice: change VCD::Name to struct VCDDescriptor "

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
content/browser/media/media_internals_unittest.cc:136: for
(CaptureApiTypeStringMap::iterator it = m.begin(); it != m.end(); ++it) {
nit: for (const auto& capture_api : m) ...

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
content/browser/media/media_internals_unittest.cc:139: EXPECT_EQ(it->second,
descriptor.GetCaptureApiTypeString());
EXPECT_STREQ ...?

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
File content/browser/media/video_capture_device_info.cc (right):

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
content/browser/media/video_capture_device_info.cc:9:
VideoCaptureDeviceInfo::VideoCaptureDeviceInfo() {}
nit: you can also add =default in the
declaration, and spare these two lines.

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
File content/browser/media/video_capture_device_info.h (right):

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
content/browser/media/video_capture_device_info.h:16: struct CONTENT_EXPORT
VideoCaptureDeviceInfo {
Last proposal:
- Move this class as internal to VideoCaptureManager, i.e.
 forward declare in the .h file and implement in the .cc

- The only other use of this class is in [1]:
MediaInternals::UpdateVideoCaptureDeviceCapabilities()
but IMHO is not worth creating this extra public class just
for that use, that eventually might be removed anyhow (was
intended to be a debug feature...); instead, I propose changing
that method's signature to sth like

  void UpdateVideoCaptureDeviceCapabilities(
      const std::vector<std::tuple<media:VideoCaptureDeviceDescriptor,
      media::VideoCaptureFormats>>& devices_and_formats);        

and composing this vector in l.927's for loop below [2], WDYT?


[1]
https://cs.chromium.org/chromium/src/content/browser/media/media_internals.h?...
[2]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
File media/capture/video/android/video_capture_device_factory_android.cc
(right):

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
media/capture/video/android/video_capture_device_factory_android.cc:85:
display_name, device_id,
format? Looks like it fits in one line.

Pro-tip: before uploading your CL, run 
git cl format
and see what clang-format has done.

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
File media/capture/video/android/video_capture_device_factory_android.h (right):

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
media/capture/video/android/video_capture_device_factory_android.h:38:
DISALLOW_COPY_AND_ASSIGN(VideoCaptureDeviceFactoryAndroid);
this should still be private
https://cs.chromium.org/chromium/src/base/macros.h?q=DISALLOW_COPY_AND_ASSIGN...

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:20: WINDOWS_DIRECT_SHOW,
nit: to follow the conventions of OS_bla
preprocessor definitions, please s/WINDOWS/WIN/

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/wi...
File media/capture/video/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:458: }
No {} if (both) the bodies are one-liners.

[chfremer, 2016-08-01 16:32:49]

Description was changed from

==========
Remove platform-dependency from VideoCaptureDeviceFactory interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Instead of a method VideoCaptureDevice::Name::GetModel(), have |model_id| 
  be a public field in struct VideoCaptureDeviceDescriptor. Moved the logic 
  for determining the |model_id| into the platform-specific factory 
  implementations. As a side-effect, this change eliminates the need for 
  code that was added in issue 532272 [2].
* Rename VideoCaptureDeviceFactory::Create() to 
  VideoCaptureDeviceFactory::CreateDevice()
* Moved struct VideoCaptureDeviceInfo closer to its usage. Moved from 
  media/capture/video to content/browser/media. 
  
[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified 
     that issue 532272 does not regress by trying the repro steps in the crbug
entry. Successfully conducted an apprtc loopback session on Linux.
==========

to

==========
Change class VideoCaptureDevice::Name to struct VideoCaptureDeviceDescriptor.

The purpose of this change is to simplify the VideoCaptureDeviceFactory
interface in preparation for making it a Mojo interface in a future step. In
particular, we want to remove the platform-dependency of the interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Instead of a method VideoCaptureDevice::Name::GetModel(), have |model_id| 
  be a public field in struct VideoCaptureDeviceDescriptor. Moved the logic 
  for determining the |model_id| into the platform-specific factory 
  implementations. As a side-effect, this change eliminates the need for 
  code that was added in issue 532272 [2].
* Rename VideoCaptureDeviceFactory::Create() to 
  VideoCaptureDeviceFactory::CreateDevice()
* Moved struct VideoCaptureDeviceInfo closer to its usage. Moved from 
  media/capture/video to content/browser/media. 
  
[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified 
     that issue 532272 does not regress by trying the repro steps in the crbug
entry. Successfully conducted an apprtc loopback session on Linux.
==========

[chfremer, 2016-08-01 18:47:32]

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

[commit-bot: I haz the power, 2016-08-01 18:48:39]

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

[chfremer, 2016-08-01 18:53:10]

chfremer@chromium.org changed reviewers:
+ rockot@chromium.org,

[chfremer, 2016-08-01 18:53:12]

Added patch set to address mcasas' comments.

@rockot Could you please have a quick look as well?

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
File content/browser/media/media_internals_unittest.cc (right):

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
content/browser/media/media_internals_unittest.cc:136: for
(CaptureApiTypeStringMap::iterator it = m.begin(); it != m.end(); ++it) {
On 2016/07/30 00:06:11, mcasas wrote:
> nit: for (const auto& capture_api : m) ...

Done.

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
content/browser/media/media_internals_unittest.cc:139: EXPECT_EQ(it->second,
descriptor.GetCaptureApiTypeString());
On 2016/07/30 00:06:11, mcasas wrote:
> EXPECT_STREQ ...?

Done.

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
File content/browser/media/video_capture_device_info.cc (right):

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
content/browser/media/video_capture_device_info.cc:9:
VideoCaptureDeviceInfo::VideoCaptureDeviceInfo() {}
On 2016/07/30 00:06:11, mcasas wrote:
> nit: you can also add =default in the
> declaration, and spare these two lines.

Done.

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
File content/browser/media/video_capture_device_info.h (right):

https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
content/browser/media/video_capture_device_info.h:16: struct CONTENT_EXPORT
VideoCaptureDeviceInfo {
Done.

Even though I think that nested classes/structs are a bad idea in general, this
approach seems consistent with the rest of the Chromium code base.

The other downside is the extra copying that has to happen to package the stuff
into the tuple type ad-hoc.

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
File media/capture/video/android/video_capture_device_factory_android.cc
(right):

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
media/capture/video/android/video_capture_device_factory_android.cc:85:
display_name, device_id,
Thanks for the tip. I tried re-applying the formatting tool and in this case it
seems the format is already what clang-format wants.

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
File media/capture/video/android/video_capture_device_factory_android.h (right):

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
media/capture/video/android/video_capture_device_factory_android.h:38:
DISALLOW_COPY_AND_ASSIGN(VideoCaptureDeviceFactoryAndroid);
On 2016/07/30 00:06:11, mcasas wrote:
> this should still be private
>
https://cs.chromium.org/chromium/src/base/macros.h?q=DISALLOW_COPY_AND_ASSIGN...

Done.

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/vi...
File media/capture/video/video_capture_device_descriptor.h (right):

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/vi...
media/capture/video/video_capture_device_descriptor.h:20: WINDOWS_DIRECT_SHOW,
On 2016/07/30 00:06:11, mcasas wrote:
> nit: to follow the conventions of OS_bla
> preprocessor definitions, please s/WINDOWS/WIN/

Done.

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/wi...
File media/capture/video/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/wi...
media/capture/video/win/video_capture_device_factory_win.cc:458: }
On 2016/07/30 00:06:11, mcasas wrote:
> No {} if (both) the bodies are one-liners.

Done.

[commit-bot: I haz the power, 2016-08-01 19:00:25]

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

[commit-bot: I haz the power, 2016-08-01 19:00:31]

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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chfremer, 2016-08-01 19:07:54]

chfremer@chromium.org changed reviewers:
+ avi@chromium.org, dalecurtis@chromium.org

[chfremer, 2016-08-01 19:07:56]

avi@chromium.org: Please review changes in 
  content/browser/*

dalecurtis@chromium.org: Please review changes in 
  media/capture/BUILD.gn
  media/capture/capture.gyp

[Ken Rockot(use gerrit already), 2016-08-01 19:08:44]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org
- rockot@chromium.org,

[Ken Rockot(use gerrit already), 2016-08-01 19:10:06]

On 2016/08/01 at 18:53:12, chfremer wrote:
> Added patch set to address mcasas' comments.
> 
> @rockot Could you please have a quick look as well?

I'm happy to look, but I don't own any of these files. What specifically would
you like me to look at?

> 
>
https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
> File content/browser/media/media_internals_unittest.cc (right):
> 
>
https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
> content/browser/media/media_internals_unittest.cc:136: for
(CaptureApiTypeStringMap::iterator it = m.begin(); it != m.end(); ++it) {
> On 2016/07/30 00:06:11, mcasas wrote:
> > nit: for (const auto& capture_api : m) ...
> 
> Done.
> 
>
https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
> content/browser/media/media_internals_unittest.cc:139: EXPECT_EQ(it->second,
descriptor.GetCaptureApiTypeString());
> On 2016/07/30 00:06:11, mcasas wrote:
> > EXPECT_STREQ ...?
> 
> Done.
> 
>
https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
> File content/browser/media/video_capture_device_info.cc (right):
> 
>
https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
> content/browser/media/video_capture_device_info.cc:9:
VideoCaptureDeviceInfo::VideoCaptureDeviceInfo() {}
> On 2016/07/30 00:06:11, mcasas wrote:
> > nit: you can also add =default in the
> > declaration, and spare these two lines.
> 
> Done.
> 
>
https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
> File content/browser/media/video_capture_device_info.h (right):
> 
>
https://codereview.chromium.org/2169013002/diff/400001/content/browser/media/...
> content/browser/media/video_capture_device_info.h:16: struct CONTENT_EXPORT
VideoCaptureDeviceInfo {
> Done.
> 
> Even though I think that nested classes/structs are a bad idea in general,
this approach seems consistent with the rest of the Chromium code base.
> 
> The other downside is the extra copying that has to happen to package the
stuff into the tuple type ad-hoc.
> 
>
https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
> File media/capture/video/android/video_capture_device_factory_android.cc
(right):
> 
>
https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
> media/capture/video/android/video_capture_device_factory_android.cc:85:
display_name, device_id,
> Thanks for the tip. I tried re-applying the formatting tool and in this case
it seems the format is already what clang-format wants.
> 
>
https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
> File media/capture/video/android/video_capture_device_factory_android.h
(right):
> 
>
https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/an...
> media/capture/video/android/video_capture_device_factory_android.h:38:
DISALLOW_COPY_AND_ASSIGN(VideoCaptureDeviceFactoryAndroid);
> On 2016/07/30 00:06:11, mcasas wrote:
> > this should still be private
> >
https://cs.chromium.org/chromium/src/base/macros.h?q=DISALLOW_COPY_AND_ASSIGN...
> 
> Done.
> 
>
https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/vi...
> File media/capture/video/video_capture_device_descriptor.h (right):
> 
>
https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/vi...
> media/capture/video/video_capture_device_descriptor.h:20: WINDOWS_DIRECT_SHOW,
> On 2016/07/30 00:06:11, mcasas wrote:
> > nit: to follow the conventions of OS_bla
> > preprocessor definitions, please s/WINDOWS/WIN/
> 
> Done.
> 
>
https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/wi...
> File media/capture/video/win/video_capture_device_factory_win.cc (right):
> 
>
https://codereview.chromium.org/2169013002/diff/400001/media/capture/video/wi...
> media/capture/video/win/video_capture_device_factory_win.cc:458: }
> On 2016/07/30 00:06:11, mcasas wrote:
> > No {} if (both) the bodies are one-liners.
> 
> Done.

[Avi (use Gerrit), 2016-08-01 19:11:52]

content/browser lgtm

[chfremer, 2016-08-01 21:00:40]

On 2016/08/01 19:10:06, Ken Rockot wrote:
> On 2016/08/01 at 18:53:12, chfremer wrote:
> > Added patch set to address mcasas' comments.
> > 
> > @rockot Could you please have a quick look as well?
> 
> I'm happy to look, but I don't own any of these files. What specifically would
> you like me to look at?

@rockot: Apologies, I meant to include you in a follow-up review that contains
actual Mojo stuff.
No need to review anything on this refactoring CL, which is just a preparation
step.
Sorry for the confusion.

[chfremer, 2016-08-01 21:37:03]

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

[commit-bot: I haz the power, 2016-08-01 21:37:30]

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

[chfremer, 2016-08-01 22:26:40]

chfremer@chromium.org changed reviewers:
- dalecurtis@chromium.org, rockot@chromium.org

[commit-bot: I haz the power, 2016-08-01 23:49:06]

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

[commit-bot: I haz the power, 2016-08-01 23:49:08]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-08-02 15:49:19]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2016-08-02 15:49:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
emircan@chromium.org, mcasas@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2169013002/#ps440001
(title: "Fix build errors")

[commit-bot: I haz the power, 2016-08-02 15:49:47]

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-08-02 15:54:49]

Description was changed from

==========
Change class VideoCaptureDevice::Name to struct VideoCaptureDeviceDescriptor.

The purpose of this change is to simplify the VideoCaptureDeviceFactory
interface in preparation for making it a Mojo interface in a future step. In
particular, we want to remove the platform-dependency of the interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Instead of a method VideoCaptureDevice::Name::GetModel(), have |model_id| 
  be a public field in struct VideoCaptureDeviceDescriptor. Moved the logic 
  for determining the |model_id| into the platform-specific factory 
  implementations. As a side-effect, this change eliminates the need for 
  code that was added in issue 532272 [2].
* Rename VideoCaptureDeviceFactory::Create() to 
  VideoCaptureDeviceFactory::CreateDevice()
* Moved struct VideoCaptureDeviceInfo closer to its usage. Moved from 
  media/capture/video to content/browser/media. 
  
[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified 
     that issue 532272 does not regress by trying the repro steps in the crbug
entry. Successfully conducted an apprtc loopback session on Linux.
==========

to

==========
Change class VideoCaptureDevice::Name to struct VideoCaptureDeviceDescriptor.

The purpose of this change is to simplify the VideoCaptureDeviceFactory
interface in preparation for making it a Mojo interface in a future step. In
particular, we want to remove the platform-dependency of the interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Instead of a method VideoCaptureDevice::Name::GetModel(), have |model_id| 
  be a public field in struct VideoCaptureDeviceDescriptor. Moved the logic 
  for determining the |model_id| into the platform-specific factory 
  implementations. As a side-effect, this change eliminates the need for 
  code that was added in issue 532272 [2].
* Rename VideoCaptureDeviceFactory::Create() to 
  VideoCaptureDeviceFactory::CreateDevice()
* Moved struct VideoCaptureDeviceInfo closer to its usage. Moved from 
  media/capture/video to content/browser/media. 
  
[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified 
     that issue 532272 does not regress by trying the repro steps in the crbug
entry. Successfully conducted an apprtc loopback session on Linux.
==========

[commit-bot: I haz the power, 2016-08-02 15:54:51]

Committed patchset #6 (id:440001)

[commit-bot: I haz the power, 2016-08-02 15:57:52]

Description was changed from

==========
Change class VideoCaptureDevice::Name to struct VideoCaptureDeviceDescriptor.

The purpose of this change is to simplify the VideoCaptureDeviceFactory
interface in preparation for making it a Mojo interface in a future step. In
particular, we want to remove the platform-dependency of the interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor 
  with a platform-independent interface. Moved out of 
  video_capture_device.h/cc into new files 
  video_capture_device_descriptor.h/cc.
* Instead of a method VideoCaptureDevice::Name::GetModel(), have |model_id| 
  be a public field in struct VideoCaptureDeviceDescriptor. Moved the logic 
  for determining the |model_id| into the platform-specific factory 
  implementations. As a side-effect, this change eliminates the need for 
  code that was added in issue 532272 [2].
* Rename VideoCaptureDeviceFactory::Create() to 
  VideoCaptureDeviceFactory::CreateDevice()
* Moved struct VideoCaptureDeviceInfo closer to its usage. Moved from 
  media/capture/video to content/browser/media. 
  
[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
 
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified 
     that issue 532272 does not regress by trying the repro steps in the crbug
entry. Successfully conducted an apprtc loopback session on Linux.
==========

to

==========
Change class VideoCaptureDevice::Name to struct VideoCaptureDeviceDescriptor.

The purpose of this change is to simplify the VideoCaptureDeviceFactory
interface in preparation for making it a Mojo interface in a future step. In
particular, we want to remove the platform-dependency of the interface.

No new functionality added. This is a refactoring.

This CL is a step towards the larger goal of Mojofication of Video Capture.
For the bigger picture, please refer to the design document at [1].

* Changed VideoCaptureDevice::Name to a struct VideoCaptureDeviceDescriptor
  with a platform-independent interface. Moved out of
  video_capture_device.h/cc into new files
  video_capture_device_descriptor.h/cc.
* Instead of a method VideoCaptureDevice::Name::GetModel(), have |model_id|
  be a public field in struct VideoCaptureDeviceDescriptor. Moved the logic
  for determining the |model_id| into the platform-specific factory
  implementations. As a side-effect, this change eliminates the need for
  code that was added in issue 532272 [2].
* Rename VideoCaptureDeviceFactory::Create() to
  VideoCaptureDeviceFactory::CreateDevice()
* Moved struct VideoCaptureDeviceInfo closer to its usage. Moved from
  media/capture/video to content/browser/media.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] http://crbug.com/532272

BUG=584797
TEST=Existing tests still pass (trybots). Verified
     that issue 532272 does not regress by trying the repro steps in the crbug
entry. Successfully conducted an apprtc loopback session on Linux.

Committed: https://crrev.com/1c38eb46ec548d21536fbbda949cc598fdc263e9
Cr-Commit-Position: refs/heads/master@{#409199}
==========

[commit-bot: I haz the power, 2016-08-02 15:57:54]

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