[Mojo Video Capture] Introduce abstraction VideoCaptureSystem

[Mojo Video Capture] Introduce abstraction VideoCaptureSystem

This CL is supposed to be a pure refactoring. There should be no changes to
existing behavior.

This is part of a series of CLs with the goal of separating the class
VideoCaptureManager from knowledge of things that are going to be private
implementation details of the video capture service. The end goal is to have
VideoCaptureManager talk to an abstraction that can be implemented by either the
video capture Mojo service or the existing (legacy) in-process implementation.

Changes in this CL:
* Move nested struct VideoCaptureManager::DeviceInfo to struct
  media::VideoCaptureDeviceInfo.
* Create abstraction VideoCaptureSystem and move caching of
  VideoCaptureDeviceInfo to there. This allows us to use this functionality in
  both InProcess and MojoService code paths.
* Simplify BuildableDeviceCallbacks interface. With the mapping from device_id
  to descriptor being moved into VideoCaptureSystem, BuildableDeviceCallbacks no
  longer needs to offer a LookupDeviceDescriptor() method.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL18.

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

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

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

[chfremer, 2017-03-21 23:47:26]

Patchset #1 (id:1) has been deleted

[chfremer, 2017-03-21 23:47:37]

Patchset #2 (id:40001) has been deleted

[chfremer, 2017-03-21 23:47:47]

Patchset #2 (id:60001) has been deleted

[chfremer, 2017-03-21 23:48:12]

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

[commit-bot: I haz the power, 2017-03-21 23:48:32]

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

[chfremer, 2017-03-21 23:48:33]

chfremer@chromium.org changed reviewers:
+ miu@chromium.org

[chfremer, 2017-03-21 23:48:33]

miu@: PTAL

[commit-bot: I haz the power, 2017-03-21 23:52:42]

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

[commit-bot: I haz the power, 2017-03-21 23:52:43]

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...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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_...)

[miu, 2017-03-22 22:51:24]

Comments on PS1:

https://codereview.chromium.org/2769543002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/in_process_buildable_video_capture_device.h
(right):

https://codereview.chromium.org/2769543002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.h:26:
media::VideoCaptureSystem* device_factory);
arg rename too?

https://codereview.chromium.org/2769543002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.h:96:
media::VideoCaptureSystem* const device_factory_;
ditto: rename variable too?

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
File media/capture/video/video_capture_device_info.h (right):

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_device_info.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
2017

...and in other new files.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
File media/capture/video/video_capture_system.cc (right):

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:89: // simply copy them from
|devices_info_cache_|.
nit: Add: "If |descriptors| does not contain a description for an entry in
|devices_info_cache_|, it will be effectively removed from |devices_info_cache_|
later on."

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:91: for (const auto& device_info :
devices_info_cache_) {
On a second review of this code, it seems this whole method could be simplified
if the nesting of the loops were reversed (and then LookupDescriptorFromId()
could be re-scoped to a LookupDeviceInfo() that returns a
VideoCaptureDeviceInfo*). Meaning:

  std::vector<VideoCaptureDeviceInfo> new_devices_info_cache;
  new_devices_info_cache.reserve(descriptors->size());
  for (const auto& descriptor : *descriptors) {
    if (auto* cached_info = LookupDeviceInfo(descriptor->device_id)) {
      new_devices_info_cache.push_back(*cached_info);
    } else {
      // Query for supported formats in order to create the entry.
      VideoCaptureDeviceInfo device_info(descriptor);
      ...GetSupportedFromats(), Consolidate() here...
      new_devices_info_cache.push_back(device_info);
    }
  }

  devices_info_cache_.swap(new_devices_info_cache);
  result_callback.Run(devices_info_cache_);

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:110: devices_info_cache_ =
new_devices_info_cache;
nit: devices_info_cache_.swap(new_devices_info_cache);

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:111:
result_callback.Run(new_devices_info_cache);
s/new_devices_info_cache/devices_info_cache_/ (because of swap() call; see
previous comment)

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:114: bool
VideoCaptureSystem::LookupDescriptorFromId(
Along the lines of improvements made on prior CLs, can we change this to:

  const media::VideoCaptureDeviceDescriptor*
VideoCaptureSystem::LookupDescriptor(const std::string& device_id) const {
    auto iter = std::find_if(...);
    return iter == devices_info_cache_.end() ? nullptr : &(iter->descriptor);
  }

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
File media/capture/video/video_capture_system.h (right):

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:18:
VideoCaptureSystem(std::unique_ptr<VideoCaptureDeviceFactory> factory);
explicit

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:22: const base::Callback<void(const
std::vector<VideoCaptureDeviceInfo>&)>&
nit: Since the callback is used in more than one place, I'd suggest:

  using DeviceInfoCallback = base::Callback<void(const
std::vector<VideoCaptureDeviceInfo>&)>;
  void GetDeviceInfosAsync(const DeviceInfoCallback& result_callback);

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:28: media::VideoCaptureDeviceFactory*
video_capture_device_factory() {
nit:

  media::VideoCaptureDeviceFactory* video_capture_device_factory() const {
                                                                   ^^^^^

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:28: media::VideoCaptureDeviceFactory*
video_capture_device_factory() {
Is the plan to remove this accessor in later CLs, and just do everything via
VideoCaptureSystem?

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:34: const base::Callback<void(const
std::vector<VideoCaptureDeviceInfo>&)>&
DeviceInfoCallback here. (and in the methods in the .cc file)

[chfremer, 2017-03-23 00:34:14]

miu@: PTAL

https://codereview.chromium.org/2769543002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/in_process_buildable_video_capture_device.h
(right):

https://codereview.chromium.org/2769543002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.h:26:
media::VideoCaptureSystem* device_factory);
On 2017/03/22 22:51:23, miu wrote:
> arg rename too?

Done.

https://codereview.chromium.org/2769543002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.h:96:
media::VideoCaptureSystem* const device_factory_;
On 2017/03/22 22:51:23, miu wrote:
> ditto: rename variable too?

Done.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
File media/capture/video/video_capture_device_info.h (right):

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_device_info.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/03/22 22:51:23, miu wrote:
> 2017
> 
> ...and in other new files.

Done.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
File media/capture/video/video_capture_system.cc (right):

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:89: // simply copy them from
|devices_info_cache_|.
On 2017/03/22 22:51:23, miu wrote:
> nit: Add: "If |descriptors| does not contain a description for an entry in
> |devices_info_cache_|, it will be effectively removed from
|devices_info_cache_|
> later on."

With your suggested improvement from below, I think we no longer need this.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:91: for (const auto& device_info :
devices_info_cache_) {
On 2017/03/22 22:51:23, miu wrote:
> On a second review of this code, it seems this whole method could be
simplified
> if the nesting of the loops were reversed (and then LookupDescriptorFromId()
> could be re-scoped to a LookupDeviceInfo() that returns a
> VideoCaptureDeviceInfo*). Meaning:
> 
>   std::vector<VideoCaptureDeviceInfo> new_devices_info_cache;
>   new_devices_info_cache.reserve(descriptors->size());
>   for (const auto& descriptor : *descriptors) {
>     if (auto* cached_info = LookupDeviceInfo(descriptor->device_id)) {
>       new_devices_info_cache.push_back(*cached_info);
>     } else {
>       // Query for supported formats in order to create the entry.
>       VideoCaptureDeviceInfo device_info(descriptor);
>       ...GetSupportedFromats(), Consolidate() here...
>       new_devices_info_cache.push_back(device_info);
>     }
>   }
> 
>   devices_info_cache_.swap(new_devices_info_cache);
>   result_callback.Run(devices_info_cache_);

Thanks. That makes things much easier to read.
Done.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:110: devices_info_cache_ =
new_devices_info_cache;
On 2017/03/22 22:51:23, miu wrote:
> nit: devices_info_cache_.swap(new_devices_info_cache);

Done.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:111:
result_callback.Run(new_devices_info_cache);
On 2017/03/22 22:51:23, miu wrote:
> s/new_devices_info_cache/devices_info_cache_/ (because of swap() call; see
> previous comment)

Done.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:114: bool
VideoCaptureSystem::LookupDescriptorFromId(
On 2017/03/22 22:51:23, miu wrote:
> Along the lines of improvements made on prior CLs, can we change this to:
> 
>   const media::VideoCaptureDeviceDescriptor*
> VideoCaptureSystem::LookupDescriptor(const std::string& device_id) const {
>     auto iter = std::find_if(...);
>     return iter == devices_info_cache_.end() ? nullptr : &(iter->descriptor);
>   }

Done.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
File media/capture/video/video_capture_system.h (right):

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:18:
VideoCaptureSystem(std::unique_ptr<VideoCaptureDeviceFactory> factory);
On 2017/03/22 22:51:24, miu wrote:
> explicit

Done.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:22: const base::Callback<void(const
std::vector<VideoCaptureDeviceInfo>&)>&
On 2017/03/22 22:51:23, miu wrote:
> nit: Since the callback is used in more than one place, I'd suggest:
> 
>   using DeviceInfoCallback = base::Callback<void(const
> std::vector<VideoCaptureDeviceInfo>&)>;
>   void GetDeviceInfosAsync(const DeviceInfoCallback& result_callback);

Done.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:28: media::VideoCaptureDeviceFactory*
video_capture_device_factory() {
On 2017/03/22 22:51:24, miu wrote:
> nit:
> 
>   media::VideoCaptureDeviceFactory* video_capture_device_factory() const {
>                                                                    ^^^^^

Done.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:28: media::VideoCaptureDeviceFactory*
video_capture_device_factory() {
On 2017/03/22 22:51:24, miu wrote:
> Is the plan to remove this accessor in later CLs, and just do everything via
> VideoCaptureSystem?

Yes. This accessor is going to go away in CL19.

https://codereview.chromium.org/2769543002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_system.h:34: const base::Callback<void(const
std::vector<VideoCaptureDeviceInfo>&)>&
On 2017/03/22 22:51:24, miu wrote:
> DeviceInfoCallback here. (and in the methods in the .cc file)

Done.

[miu, 2017-03-24 21:24:14]

PS2 lgtm

[chfremer, 2017-03-31 17:26:14]

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

[commit-bot: I haz the power, 2017-03-31 17:26:36]

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

[chfremer, 2017-03-31 17:28:39]

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

[chfremer, 2017-03-31 17:28:40]

emircan@: PTAL
mcasas@: optional review

[chfremer, 2017-03-31 18:08:17]

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

[commit-bot: I haz the power, 2017-03-31 18:09:05]

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

[commit-bot: I haz the power, 2017-03-31 19:09:31]

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

[commit-bot: I haz the power, 2017-03-31 19:09:32]

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

[chfremer, 2017-03-31 22:49:59]

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

[commit-bot: I haz the power, 2017-03-31 22:50: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, 2017-04-01 00:40:07]

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

[commit-bot: I haz the power, 2017-04-01 00:40:08]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-04-03 19:04:28]

lgtm % nits.

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
File media/capture/video/video_capture_system.cc (right):

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_system.cc:68:
factory_->EnumerateDeviceDescriptors(media::BindToCurrentLoop(
It seems like VideoCaptureDeviceFactory::EnumerateDeviceDescriptors() runs
synchronously and can return the result immediately. That would make these extra
posttasks unnecessary. Consider refactoring this method and the callers. 
https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device...

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_system.cc:70: base::Unretained(this),
result_callback)));
Can you add a comment explaining base::Unretained(this) is safe?

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_system.cc:84:
std::unique_ptr<VideoCaptureDeviceDescriptors> descriptors) {
Can you add thread_checker to this class? It would make it clear that these
methods are not overlapping and |devices_info_cache_| access is ok.

[mcasas, 2017-04-04 18:38:42]

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

https://codereview.chromium.org/2769543002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:294: //
TODO(chfremer): Check if any production code actually depends on this
Add a crbug.com for this plz.

https://codereview.chromium.org/2769543002/diff/140001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2769543002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:238: void
OnDevicesInfoEnumerated(
Is this method still defined somewhere? 
Regardless, it's confusing to have 
OnDeviceInfosReceived() and
OnDevicesInfoEnumerated() with
the same parameter signature, perhaps we 
should stick with one?

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

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_device_info.cc:21: const
VideoCaptureDeviceInfo& other) = default;
Since all this methods are =default, why not
moving them to the header file and nuking this
file?

[chfremer, 2017-04-04 19:05:33]

Incorporated suggestions from PatchSet 5.
Thanks for the review!

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

https://codereview.chromium.org/2769543002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.cc:294: //
TODO(chfremer): Check if any production code actually depends on this
On 2017/04/04 18:38:41, mcasas wrote:
> Add a http://crbug.com for this plz.

Done.

https://codereview.chromium.org/2769543002/diff/140001/content/browser/render...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2769543002/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:238: void
OnDevicesInfoEnumerated(
On 2017/04/04 18:38:41, mcasas wrote:
> Is this method still defined somewhere? 
> Regardless, it's confusing to have 
> OnDeviceInfosReceived() and
> OnDevicesInfoEnumerated() with
> the same parameter signature, perhaps we 
> should stick with one?

This is no longer defined. Thanks for catching this.
Removed.

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

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_device_info.cc:21: const
VideoCaptureDeviceInfo& other) = default;
On 2017/04/04 18:38:41, mcasas wrote:
> Since all this methods are =default, why not
> moving them to the header file and nuking this
> file?

Because this struct is considered "complex" (because of its members) our build
requires non-inline constructor, destructor, copy constructor and assignment
operator.

I don't know any way to do this without a .cc file, but would love it if there
was.

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
File media/capture/video/video_capture_system.cc (right):

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_system.cc:68:
factory_->EnumerateDeviceDescriptors(media::BindToCurrentLoop(
On 2017/04/03 19:04:28, emircan wrote:
> It seems like VideoCaptureDeviceFactory::EnumerateDeviceDescriptors() runs
> synchronously and can return the result immediately. That would make these
extra
> posttasks unnecessary. Consider refactoring this method and the callers. 
>
https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device...

Agreed that this is the case. I'd like to defer this work. Will open a bug entry
for tracking this.

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_system.cc:70: base::Unretained(this),
result_callback)));
On 2017/04/03 19:04:28, emircan wrote:
> Can you add a comment explaining base::Unretained(this) is safe?

Done.

https://codereview.chromium.org/2769543002/diff/140001/media/capture/video/vi...
media/capture/video/video_capture_system.cc:84:
std::unique_ptr<VideoCaptureDeviceDescriptors> descriptors) {
On 2017/04/03 19:04:28, emircan wrote:
> Can you add thread_checker to this class? It would make it clear that these
> methods are not overlapping and |devices_info_cache_| access is ok.

Done.

[chfremer, 2017-04-04 19:05:43]

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

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

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

[commit-bot: I haz the power, 2017-04-04 21:09:10]

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

[commit-bot: I haz the power, 2017-04-04 21:09:13]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-04-04 21:16:43]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-04-04 21:16:43]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
emircan@chromium.org
Link to the patchset: https://codereview.chromium.org/2769543002/#ps160001
(title: "Incorporated suggestions from PatchSet 5")

[commit-bot: I haz the power, 2017-04-04 21:18:04]

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

[commit-bot: I haz the power, 2017-04-04 21:33:42]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491340603062100,
"parent_rev": "81033db0364139db8ee059623d8892f2b00a39ba", "commit_rev":
"8d8a5d278461d098d3481acf52275bb931f834f3"}

[commit-bot: I haz the power, 2017-04-04 21:49:40]

Description was changed from

==========
[Mojo Video Capture] Introduce abstraction VideoCaptureSystem

This CL is supposed to be a pure refactoring. There should be no changes to
existing behavior.

This is part of a series of CLs with the goal of separating the class
VideoCaptureManager from knowledge of things that are going to be private
implementation details of the video capture service. The end goal is to have
VideoCaptureManager talk to an abstraction that can be implemented by either the
video capture Mojo service or the existing (legacy) in-process implementation.

Changes in this CL:
* Move nested struct VideoCaptureManager::DeviceInfo to struct
  media::VideoCaptureDeviceInfo.
* Create abstraction VideoCaptureSystem and move caching of
  VideoCaptureDeviceInfo to there. This allows us to use this functionality in
  both InProcess and MojoService code paths.
* Simplify BuildableDeviceCallbacks interface. With the mapping from device_id
  to descriptor being moved into VideoCaptureSystem, BuildableDeviceCallbacks no
  longer needs to offer a LookupDeviceDescriptor() method.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL18.

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Introduce abstraction VideoCaptureSystem

This CL is supposed to be a pure refactoring. There should be no changes to
existing behavior.

This is part of a series of CLs with the goal of separating the class
VideoCaptureManager from knowledge of things that are going to be private
implementation details of the video capture service. The end goal is to have
VideoCaptureManager talk to an abstraction that can be implemented by either the
video capture Mojo service or the existing (legacy) in-process implementation.

Changes in this CL:
* Move nested struct VideoCaptureManager::DeviceInfo to struct
  media::VideoCaptureDeviceInfo.
* Create abstraction VideoCaptureSystem and move caching of
  VideoCaptureDeviceInfo to there. This allows us to use this functionality in
  both InProcess and MojoService code paths.
* Simplify BuildableDeviceCallbacks interface. With the mapping from device_id
  to descriptor being moved into VideoCaptureSystem, BuildableDeviceCallbacks no
  longer needs to offer a LookupDeviceDescriptor() method.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL18.

BUG=584797
TEST=
  content_unittests --gtest_filter="*Video*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

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

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

[commit-bot: I haz the power, 2017-04-04 21:49:41]

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

[tasak, 2017-04-05 02:44:41]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/2802553002/ by tasak@google.com.

The reason for reverting is: I think, this patch makes
MediaDevicesDispatcherHostTest.GetVideoInputCapabilities (Win7 Tests) flaky.

e.g.
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...

MediaDevicesDispatcherHostTest.GetVideoInputCapabilities (run #1):
[ RUN      ] MediaDevicesDispatcherHostTest.GetVideoInputCapabilities
c:\b\c\b\win\src\content\browser\renderer_host\media\media_devices_dispatcher_host_unittest.cc(373):
error: Actual function call count doesn't match EXPECT_CALL(*this,
MockVideoInputCapabilitiesCallback())...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[  FAILED  ] MediaDevicesDispatcherHostTest.GetVideoInputCapabilities (48 ms)
.