[Mojo Video Capture] Stop service when last client disconnects.

[Mojo Video Capture] Stop service when last client disconnects.

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

Purpose of this CL:
Stop service when last client disconnects. Once we host it in a separate process, this will allow the service to automatically restart when a new request comes in.

Changes in this CL:
* Split class video_capture::ServiceImpl into two classes
  ServiceManagerServiceImpl and VideoCaptureServiceImpl.
* In class ServiceManagerServiceImpl, following the example of ImageDecoder [2],
  use a ServiceContextRefFactory to manage service lifetime and implement a
  delayed service shutdown when there are no clients.
* Add tests ServiceQuitsWhenNoClientConnected and
  ServiceQuitsWhenClientDisconnectsWhileUsingDevice
* In service.mojom, add a method for setting the service shutdown delay.
  For the automated tests, set the delay to zero.
* In DeviceFactoryMediaToMojoAdapter, make sure that
  |capture_system_->GetDeviceInfosAsync()| is called before
  |capture_system_->CreateDevice()|. This is required by the implementation
  VideoCaptureSystemImpl in order to fill the cache |devices_info_cache_|.
* In DeviceMediaToMojoAdapter, unsubscibe from
  |receiver.set_connection_error_handler()| in order to avoid use after free
  on service shutdown.
* In ReceiverMojoToMediaAdapter, introduce class ReceiverOnTaskRunner in order
  to ensure that mojom.Receiver methods are posted to the correct thread.
* In VideoCaptureDeviceClient, allow |external_jpeg_decoder_| to be nullptr,
  since in the context of the video_capture service, this will (for now) be the
  case.

BUG=584797
TEST=
  service_unittests --gtest_filter="*Video*"
  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
[2] https://cs.chromium.org/chromium/src/services/data_decoder/data_decoder_service.cc?dr=CSs

Review-Url: https://codereview.chromium.org/2824883005
Cr-Commit-Position: refs/heads/master@{#468358}
Committed: https://chromium.googlesource.com/chromium/src/+/9fe5b74189b804423d7fe66182a3c07dd6c61dd1

[chfremer, 2017-04-18 21:37:26]

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

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

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-18 23:53:18]

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

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

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chfremer, 2017-04-19 23:17:39]

Patchset #1 (id:1) has been deleted

[chfremer, 2017-04-19 23:17:52]

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

[commit-bot: I haz the power, 2017-04-19 23:18:27]

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

[chfremer, 2017-04-19 23:19:19]

Description was changed from

==========
[Mojo Video Capture] Stop service when last client disconnects.

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

Purpose of this CL:
Stop service (and its hosting process) when last client disconnects. This will
allow the service to automatically restart when a new request comes in.

Changes in this CL:
* Split class video_capture::ServiceImpl into two classes
  ServiceManagerServiceImpl and VideoCaptureServiceImpl.
* In class ServiceManagerServiceImpl, following the example of ImageDecoder [2],
  use a ServiceContextRefFactory to manage service lifetime and implement a
  delayed service shutdown when there are no clients.
* Add tests ServiceQuitsWhenNoClientConnected and
  ServiceQuitsWhenClientDisconnectsWhileUsingDevice
* In service.mojom, add a method for setting the service shutdown delay.
  For the automated tests, set the delay to zero.
* In DeviceFactoryMediaToMojoAdapter, make sure that
  |capture_system_->GetDeviceInfosAsync()| is called before
  |capture_system_->CreateDevice()|. This is required by the implementation
  VideoCaptureSystemImpl in order to fill the cache |devices_info_cache_|.
* In DeviceMediaToMojoAdapter, unsubscibe from
  |receiver.set_connection_error_handler()| in order to avoid use after free
  on service shutdown.
* In ReceiverMojoToMediaAdapter, introduce class ReceiverOnTaskRunner in order
  to ensure that mojom.Receiver methods are posted to the correct thread.
* In VideoCaptureDeviceClient, allow |external_jpeg_decoder_| to be nullptr,
  since in the context of the video_capture service, this will (for now) be the
  case.

BUG=584797
TEST=
  service_unittests --gtest_filter="*Video*"
  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] Stop service when last client disconnects.

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

Purpose of this CL:
Stop service (and its hosting process) when last client disconnects. This will
allow the service to automatically restart when a new request comes in.

Changes in this CL:
* Split class video_capture::ServiceImpl into two classes
  ServiceManagerServiceImpl and VideoCaptureServiceImpl.
* In class ServiceManagerServiceImpl, following the example of ImageDecoder [2],
  use a ServiceContextRefFactory to manage service lifetime and implement a
  delayed service shutdown when there are no clients.
* Add tests ServiceQuitsWhenNoClientConnected and
  ServiceQuitsWhenClientDisconnectsWhileUsingDevice
* In service.mojom, add a method for setting the service shutdown delay.
  For the automated tests, set the delay to zero.
* In DeviceFactoryMediaToMojoAdapter, make sure that
  |capture_system_->GetDeviceInfosAsync()| is called before
  |capture_system_->CreateDevice()|. This is required by the implementation
  VideoCaptureSystemImpl in order to fill the cache |devices_info_cache_|.
* In DeviceMediaToMojoAdapter, unsubscibe from
  |receiver.set_connection_error_handler()| in order to avoid use after free
  on service shutdown.
* In ReceiverMojoToMediaAdapter, introduce class ReceiverOnTaskRunner in order
  to ensure that mojom.Receiver methods are posted to the correct thread.
* In VideoCaptureDeviceClient, allow |external_jpeg_decoder_| to be nullptr,
  since in the context of the video_capture service, this will (for now) be the
  case.

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

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://cs.chromium.org/chromium/src/services/data_decoder/data_decoder_servi...
==========

[chfremer, 2017-04-19 23:22:11]

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

[chfremer, 2017-04-19 23:22:12]

rockot@: PTAL
emircan@: PTAL
mcasas@: FYI
miu@: FYI

[chfremer, 2017-04-19 23:23:19]

Description was changed from

==========
[Mojo Video Capture] Stop service when last client disconnects.

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

Purpose of this CL:
Stop service (and its hosting process) when last client disconnects. This will
allow the service to automatically restart when a new request comes in.

Changes in this CL:
* Split class video_capture::ServiceImpl into two classes
  ServiceManagerServiceImpl and VideoCaptureServiceImpl.
* In class ServiceManagerServiceImpl, following the example of ImageDecoder [2],
  use a ServiceContextRefFactory to manage service lifetime and implement a
  delayed service shutdown when there are no clients.
* Add tests ServiceQuitsWhenNoClientConnected and
  ServiceQuitsWhenClientDisconnectsWhileUsingDevice
* In service.mojom, add a method for setting the service shutdown delay.
  For the automated tests, set the delay to zero.
* In DeviceFactoryMediaToMojoAdapter, make sure that
  |capture_system_->GetDeviceInfosAsync()| is called before
  |capture_system_->CreateDevice()|. This is required by the implementation
  VideoCaptureSystemImpl in order to fill the cache |devices_info_cache_|.
* In DeviceMediaToMojoAdapter, unsubscibe from
  |receiver.set_connection_error_handler()| in order to avoid use after free
  on service shutdown.
* In ReceiverMojoToMediaAdapter, introduce class ReceiverOnTaskRunner in order
  to ensure that mojom.Receiver methods are posted to the correct thread.
* In VideoCaptureDeviceClient, allow |external_jpeg_decoder_| to be nullptr,
  since in the context of the video_capture service, this will (for now) be the
  case.

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

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://cs.chromium.org/chromium/src/services/data_decoder/data_decoder_servi...
==========

to

==========
[Mojo Video Capture] Stop service when last client disconnects.

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

Purpose of this CL:
Stop service when last client disconnects. Once we host it in a separate
process, this will allow the service to automatically restart when a new request
comes in.

Changes in this CL:
* Split class video_capture::ServiceImpl into two classes
  ServiceManagerServiceImpl and VideoCaptureServiceImpl.
* In class ServiceManagerServiceImpl, following the example of ImageDecoder [2],
  use a ServiceContextRefFactory to manage service lifetime and implement a
  delayed service shutdown when there are no clients.
* Add tests ServiceQuitsWhenNoClientConnected and
  ServiceQuitsWhenClientDisconnectsWhileUsingDevice
* In service.mojom, add a method for setting the service shutdown delay.
  For the automated tests, set the delay to zero.
* In DeviceFactoryMediaToMojoAdapter, make sure that
  |capture_system_->GetDeviceInfosAsync()| is called before
  |capture_system_->CreateDevice()|. This is required by the implementation
  VideoCaptureSystemImpl in order to fill the cache |devices_info_cache_|.
* In DeviceMediaToMojoAdapter, unsubscibe from
  |receiver.set_connection_error_handler()| in order to avoid use after free
  on service shutdown.
* In ReceiverMojoToMediaAdapter, introduce class ReceiverOnTaskRunner in order
  to ensure that mojom.Receiver methods are posted to the correct thread.
* In VideoCaptureDeviceClient, allow |external_jpeg_decoder_| to be nullptr,
  since in the context of the video_capture service, this will (for now) be the
  case.

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

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://cs.chromium.org/chromium/src/services/data_decoder/data_decoder_servi...
==========

[commit-bot: I haz the power, 2017-04-20 01:45:39]

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

[commit-bot: I haz the power, 2017-04-20 01:45:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ken Rockot(use gerrit already), 2017-04-26 19:40:20]

All Mojo and Service Manager stuff (e.g. usage of ServiceContextRef) LGTM

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/service_manager_service_impl.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.h:19: class
ServiceManagerServiceImpl : public service_manager::Service {
nit: Hmm, is this rename really necessary?

[chfremer, 2017-04-26 20:01:54]

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/service_manager_service_impl.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.h:19: class
ServiceManagerServiceImpl : public service_manager::Service {
On 2017/04/26 19:40:19, Ken Rockot wrote:
> nit: Hmm, is this rename really necessary?

Hmm, not absolutely necessary, but I feel it is useful to disambiguate from
VideoCaptureServiceImpl, since both classes would equally qualify for being
named just ServiceImpl.

[Ken Rockot(use gerrit already), 2017-04-26 20:05:48]

I see. I think it's probably confusing to have a video_capture::Service
interface. I realize that was a pattern that was maybe being established at
some point, i.e. having a top-level Service interface to vend other
interfaces... But I guess I'd just as soon rename that Service that
DeviceFactoryProvider or something.

Anyway, no action required, just thinking out loud.

On Wed, Apr 26, 2017 at 1:01 PM, <chfremer@chromium.org> wrote:

>
> https://codereview.chromium.org/2824883005/diff/20001/
> services/video_capture/service_manager_service_impl.h
> File services/video_capture/service_manager_service_impl.h (right):
>
> https://codereview.chromium.org/2824883005/diff/20001/
> services/video_capture/service_manager_service_impl.h#newcode19
> services/video_capture/service_manager_service_impl.h:19: class
> ServiceManagerServiceImpl : public service_manager::Service {
> On 2017/04/26 19:40:19, Ken Rockot wrote:
> > nit: Hmm, is this rename really necessary?
>
> Hmm, not absolutely necessary, but I feel it is useful to disambiguate
> from VideoCaptureServiceImpl, since both classes would equally qualify
> for being named just ServiceImpl.
>
> https://codereview.chromium.org/2824883005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mcasas, 2017-04-26 22:12:32]

Please try to keep CLs smaller (e.g. leave renames
or class-splitting for separate CLs etc)

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:129: }
This if-else reads complex, what about:

  const auto create_and_add_new_device_cb =
      base::Bind(&DeviceFactoryMediaToMojoAdapter::CreateAndAddNewDevice,
                 weak_factory_.GetWeakPtr(), device_id,
                 base::Passed(&device_request), callback);

  if (has_called_get_device_infos_) {
    create_and_add_new_device_cb.Run();
    return;
  }

  capture_system_->GetDeviceInfosAsync(base::Bind(
      &DiscardDeviceInfosAndCallContinuation, create_and_add_new_device_cb));


Which includes my next comment on making
DiscardDeviceInfosAndCallContinuation file-static and not member.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:132: void
DeviceFactoryMediaToMojoAdapter::DiscardDeviceInfosAndCallContinuation(
This doesn't need to be a member: make it file-static.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.h:65: bool
has_called_get_device_infos_ = false;
nit: initialize this to false in the ctor initialization list
with all the others. (Otherwise initialization is in two
places).

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/device_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_media_to_mojo_adapter.cc:29: :
service_ref_(std::move(service_ref)),
nit: I'm not sure we need std::move() here, since there
is a constructor that takes ownership IIRC.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_media_to_mojo_adapter.cc:34:
DeviceMediaToMojoAdapter::~DeviceMediaToMojoAdapter() {
DCHECK(thread_checker_.CalledOnValidThread());

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/public/cpp/BUILD.gn:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
s/2016/2017/

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/public/cpp/BUILD.gn:15: "//components/prefs:prefs",
Components? 
Probably not needed

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/public/interfaces/constants.mojom (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/public/interfaces/constants.mojom:1: // Copyright 2016
The Chromium Authors. All rights reserved.
s/2016/2017/

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/receiver_mojo_to_media_adapter.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/receiver_mojo_to_media_adapter.h:19: //
media::VideoFrameReceiver:
nit: this is written customarily as:
// media::VideoFrameReceiver implementation.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/receiver_mojo_to_media_adapter.h:39:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
const.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/receiver_mojo_to_media_adapter.h:49: void
ResetConnectionErrorHandler();
const method?

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/receiver_mojo_to_media_adapter.h:51: //
media::VideoFrameReceiver:
nit: // media::VideoFrameReceiver implementation.

I counted how many such cases there are in content:

grep -rn "implementation\." `find content/ | grep '\.h' ` | grep "//" | wc -l
     527

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/service_manager_service_impl.cc (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
s/2016/2017/

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/service_manager_service_impl.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
s/2016/2017/

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.h:19: class
ServiceManagerServiceImpl : public service_manager::Service {
On 2017/04/26 20:01:54, chfremer wrote:
> On 2017/04/26 19:40:19, Ken Rockot wrote:
> > nit: Hmm, is this rename really necessary?
> 
> Hmm, not absolutely necessary, but I feel it is useful to disambiguate from
> VideoCaptureServiceImpl, since both classes would equally qualify for being
> named just ServiceImpl.
> 

namespace disambiguates, right?
video_capture::Servicelmpl sounds quite clear to me.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.h:24: //
service_manager::Service:
// service_manager::Service implementation.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/test/mock_device_test.cc (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/test/mock_device_test.cc:69:
MockDeviceTest::MockDeviceTest() : ref_factory_(base::Bind([]() {})) {}
base::DoNothing() ?

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/test/service_test.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/test/service_test.h:26: // mojom::ServiceManagerListener:
s/:/implementation.

[chfremer, 2017-04-26 23:30:48]

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

[commit-bot: I haz the power, 2017-04-26 23:33:17]

Dry run: CQ is trying da patch.

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

[chfremer, 2017-04-26 23:33:36]

PTAL

Apologies for the repeatedly large review. Will work harder to keep them more
reasonable.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:129: }
On 2017/04/26 22:12:31, mcasas wrote:
> This if-else reads complex, what about:
> 
>   const auto create_and_add_new_device_cb =
>       base::Bind(&DeviceFactoryMediaToMojoAdapter::CreateAndAddNewDevice,
>                  weak_factory_.GetWeakPtr(), device_id,
>                  base::Passed(&device_request), callback);
> 
>   if (has_called_get_device_infos_) {
>     create_and_add_new_device_cb.Run();
>     return;
>   }
> 
>   capture_system_->GetDeviceInfosAsync(base::Bind(
>       &DiscardDeviceInfosAndCallContinuation, create_and_add_new_device_cb));
> 
> 
> Which includes my next comment on making
> DiscardDeviceInfosAndCallContinuation file-static and not member.
> 

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:132: void
DeviceFactoryMediaToMojoAdapter::DiscardDeviceInfosAndCallContinuation(
On 2017/04/26 22:12:31, mcasas wrote:
> This doesn't need to be a member: make it file-static.

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.h:65: bool
has_called_get_device_infos_ = false;
On 2017/04/26 22:12:31, mcasas wrote:
> nit: initialize this to false in the ctor initialization list
> with all the others. (Otherwise initialization is in two
> places).

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/device_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_media_to_mojo_adapter.cc:29: :
service_ref_(std::move(service_ref)),
On 2017/04/26 22:12:31, mcasas wrote:
> nit: I'm not sure we need std::move() here, since there
> is a constructor that takes ownership IIRC.

Removing the std::move() gives the following error:
Error - call to deleted constructor of 'const
std::unique_ptr<service_manager::ServiceContextRef>

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/device_media_to_mojo_adapter.cc:34:
DeviceMediaToMojoAdapter::~DeviceMediaToMojoAdapter() {
On 2017/04/26 22:12:31, mcasas wrote:
> DCHECK(thread_checker_.CalledOnValidThread());

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/public/cpp/BUILD.gn:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/04/26 22:12:31, mcasas wrote:
> s/2016/2017/

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/public/cpp/BUILD.gn:15: "//components/prefs:prefs",
On 2017/04/26 22:12:31, mcasas wrote:
> Components? 
> Probably not needed

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/public/interfaces/constants.mojom (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/public/interfaces/constants.mojom:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/04/26 22:12:31, mcasas wrote:
> s/2016/2017/

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/receiver_mojo_to_media_adapter.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/receiver_mojo_to_media_adapter.h:19: //
media::VideoFrameReceiver:
On 2017/04/26 22:12:31, mcasas wrote:
> nit: this is written customarily as:
> // media::VideoFrameReceiver implementation.

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/receiver_mojo_to_media_adapter.h:39:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2017/04/26 22:12:31, mcasas wrote:
> const.

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/receiver_mojo_to_media_adapter.h:49: void
ResetConnectionErrorHandler();
On 2017/04/26 22:12:31, mcasas wrote:
> const method?

Not possible here, because it requires non-const access to member |receiver_|.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/receiver_mojo_to_media_adapter.h:51: //
media::VideoFrameReceiver:
On 2017/04/26 22:12:31, mcasas wrote:
> nit: // media::VideoFrameReceiver implementation.
> 
> I counted how many such cases there are in content:
> 
> grep -rn "implementation\." `find content/ | grep '\.h' ` | grep "//" | wc -l
>      527

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/service_manager_service_impl.cc (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/04/26 22:12:31, mcasas wrote:
> s/2016/2017/

No longer needed in PatchSet 2, because file name no longer changes.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/service_manager_service_impl.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/04/26 22:12:31, mcasas wrote:
> s/2016/2017/

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.h:19: class
ServiceManagerServiceImpl : public service_manager::Service {
On 2017/04/26 22:12:31, mcasas wrote:
> On 2017/04/26 20:01:54, chfremer wrote:
> > On 2017/04/26 19:40:19, Ken Rockot wrote:
> > > nit: Hmm, is this rename really necessary?
> > 
> > Hmm, not absolutely necessary, but I feel it is useful to disambiguate from
> > VideoCaptureServiceImpl, since both classes would equally qualify for being
> > named just ServiceImpl.
> > 
> 
> namespace disambiguates, right?
> video_capture::Servicelmpl sounds quite clear to me.

Yes, to me too, but unfortunately both classes live in the video_capture
namespace.

I think Ken's suggestion to avoid using a top-level Mojo interface named
"Service" is good.
I'll go an rename mojom.Service -> mojom.DeviceFactoryProvider and then the rest
follows.

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/service_manager_service_impl.h:24: //
service_manager::Service:
On 2017/04/26 22:12:31, mcasas wrote:
> // service_manager::Service implementation.

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/test/mock_device_test.cc (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/test/mock_device_test.cc:69:
MockDeviceTest::MockDeviceTest() : ref_factory_(base::Bind([]() {})) {}
On 2017/04/26 22:12:31, mcasas wrote:
> base::DoNothing() ?

Done.

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
File services/video_capture/test/service_test.h (right):

https://codereview.chromium.org/2824883005/diff/20001/services/video_capture/...
services/video_capture/test/service_test.h:26: // mojom::ServiceManagerListener:
On 2017/04/26 22:12:31, mcasas wrote:
> s/:/implementation.

Done.

[commit-bot: I haz the power, 2017-04-26 23:40:22]

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

[commit-bot: I haz the power, 2017-04-26 23:40:23]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mcasas, 2017-04-27 00:22:16]

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
File services/video_capture/public/cpp/device_to_feedback_observer_adapter.h
(right):

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
services/video_capture/public/cpp/device_to_feedback_observer_adapter.h:25:
mojom::DevicePtr* device_;
I'm surprised by this change, it seems unrelated to other
changes in this CL and it's in itself peculiar: mojom Ptrs
are move-only types, so holding a weak reference to one
such is non-intuitive.  The previous behaviour looks more
correct to me.

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
File services/video_capture/public/interfaces/service.mojom (left):

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/service.mojom:19:
ConnectToFakeDeviceFactory(DeviceFactory& request);
This file is very similar to device_factory_provider.mojom
but git diff was confused, try uploading with 
git cl upload ...  --similarity=25 
for example, and see if Rietveld improves.

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
File services/video_capture/test/service_unittest.cc (left):

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
services/video_capture/test/service_unittest.cc:70: }  // namespace
video_capture
I'm not sure if Rietveld lost it or PS2 is corrupt, but I see 
e.g. service_test.h, service_test.cc and 
service_test_unittest.cc as "M" for modified, but all lines
are gone :?  (As in deleted).

Also, bizarrely, service_impl.h and service_impl.cc
are marked as "D" but they are just modified.

Since the bots are borked in Patch Failure, probably
PS2 didn't upload properly.

[chfremer, 2017-04-27 19:44:19]

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

[commit-bot: I haz the power, 2017-04-27 19:44:50]

Dry run: CQ is trying da patch.

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

[chfremer, 2017-04-27 19:52:50]

mcasas@: PTAL

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
File services/video_capture/public/cpp/device_to_feedback_observer_adapter.h
(right):

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
services/video_capture/public/cpp/device_to_feedback_observer_adapter.h:25:
mojom::DevicePtr* device_;
On 2017/04/27 00:22:15, mcasas wrote:
> I'm surprised by this change, it seems unrelated to other
> changes in this CL and it's in itself peculiar: mojom Ptrs
> are move-only types, so holding a weak reference to one
> such is non-intuitive.  The previous behaviour looks more
> correct to me.

Thanks for catching this. You are right, this change should not actually belong
to this CL. I believe it must somehow have propagated itself here through the
countless merges and reshuffles that the CLs have gone through on their way to
review. The change may make sense at a later CL. I guess if it is still needed,
we will see it when the time comes.

Reverted.

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
File services/video_capture/public/interfaces/service.mojom (left):

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/service.mojom:19:
ConnectToFakeDeviceFactory(DeviceFactory& request);
On 2017/04/27 00:22:15, mcasas wrote:
> This file is very similar to device_factory_provider.mojom
> but git diff was confused, try uploading with 
> git cl upload ...  --similarity=25 
> for example, and see if Rietveld improves.

Agreed that this is confusing. I am using similarity 25 in PatchSet #4 but it
did not improve. My guess is that because the file was modified in PatchSet #1
and then renamed in PatchSet #2, Rietveld shows it as modified but with all
lines deleted.

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
File services/video_capture/test/service_unittest.cc (left):

https://codereview.chromium.org/2824883005/diff/40001/services/video_capture/...
services/video_capture/test/service_unittest.cc:70: }  // namespace
video_capture
On 2017/04/27 00:22:15, mcasas wrote:
> I'm not sure if Rietveld lost it or PS2 is corrupt, but I see 
> e.g. service_test.h, service_test.cc and 
> service_test_unittest.cc as "M" for modified, but all lines
> are gone :?  (As in deleted).
> 
> Also, bizarrely, service_impl.h and service_impl.cc
> are marked as "D" but they are just modified.
> 
> Since the bots are borked in Patch Failure, probably
> PS2 didn't upload properly.

I believe this is due to the files being renamed as part of PatchSet #2. The
bots may be due to changes that happened on tip of tree. I hope it works with
PatchSet #4.

[mcasas, 2017-04-27 20:10:02]

my parts LGTM w/ some comments.

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

https://codereview.chromium.org/2824883005/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_system.h:14: // CreateDevice().
nit: Shouldn't this be enforced somehow?

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:65: base::Closure
continuation,
nit: totally not your or this CL's issue, but wouldn't
it be cool to have some kind of ScopedClosure
that would .Run() itself on destruction? Couldn't
find one such though.

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/service_impl.cc (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/service_impl.cc:22: DVLOG(3) << "Service Starting";
nit: consider removing these debugging DVLOG()s

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/service_impl.h (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/service_impl.h:19: class ServiceImpl : public
service_manager::Service {
Is this class single threaded? If so, consider adding
a base::ThreadChecker.

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/test/device_factory_provider_test.h (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/test/device_factory_provider_test.h:18: class
ServiceManagerListenerImpl
I think we should forward declare this class and 
implement it in device_factory_provider_test.cc
if possible.  Probably also move inside 
DeviceFactoryProviderTest.

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/test/mock_device_test.h (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/test/mock_device_test.h:86:
std::unique_ptr<base::MessageLoop> message_loop_;
In subsequent CLs we should remove MessageLoop
in favour of base::test::ScopedTaskEnvironment ,see:

https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

[commit-bot: I haz the power, 2017-04-27 20:10:23]

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

[commit-bot: I haz the power, 2017-04-27 20:10:24]

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

[chfremer, 2017-04-27 23:48:14]

Thanks for the review!

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

https://codereview.chromium.org/2824883005/diff/80001/media/capture/video/vid...
media/capture/video/video_capture_system.h:14: // CreateDevice().
On 2017/04/27 20:10:02, mcasas wrote:
> nit: Shouldn't this be enforced somehow?

Hmm, maybe I worded this too strongly. Actually, CreateDevice() can be called
before GetDeviceInfosAsync(), but it will always return nullptr.
I will update the comment.

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:65: base::Closure
continuation,
On 2017/04/27 20:10:02, mcasas wrote:
> nit: totally not your or this CL's issue, but wouldn't
> it be cool to have some kind of ScopedClosure
> that would .Run() itself on destruction? Couldn't
> find one such though.

Sure, but what purpose would that serve here? What would it improve?

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/service_impl.cc (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/service_impl.cc:22: DVLOG(3) << "Service Starting";
On 2017/04/27 20:10:02, mcasas wrote:
> nit: consider removing these debugging DVLOG()s

Done.

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/service_impl.h (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/service_impl.h:19: class ServiceImpl : public
service_manager::Service {
On 2017/04/27 20:10:02, mcasas wrote:
> Is this class single threaded? If so, consider adding
> a base::ThreadChecker.

Done.

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/test/device_factory_provider_test.h (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/test/device_factory_provider_test.h:18: class
ServiceManagerListenerImpl
On 2017/04/27 20:10:02, mcasas wrote:
> I think we should forward declare this class and 
> implement it in device_factory_provider_test.cc
> if possible.  Probably also move inside 
> DeviceFactoryProviderTest.

This class is used by the code in device_factory_provider_unittest.cc. If we
forward declare here we get 
device_factory_provider_unittest.cc:80,3 - Error - member access into incomplete
type 'video_capture::ServiceManagerListenerImpl'

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
File services/video_capture/test/mock_device_test.h (right):

https://codereview.chromium.org/2824883005/diff/80001/services/video_capture/...
services/video_capture/test/mock_device_test.h:86:
std::unique_ptr<base::MessageLoop> message_loop_;
On 2017/04/27 20:10:02, mcasas wrote:
> In subsequent CLs we should remove MessageLoop
> in favour of base::test::ScopedTaskEnvironment ,see:
> 
>
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

Acknowledged.

[chfremer, 2017-04-28 16:44:18]

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

[commit-bot: I haz the power, 2017-04-28 16:44:42]

Dry run: CQ is trying da patch.

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

[chfremer, 2017-04-28 16:45:14]

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

[chfremer, 2017-04-28 16:45:14]

tsepez@: Please RS *.mojom

[Tom Sepez, 2017-04-28 16:48:49]

RS LGTM Stampity Stamp.

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

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

[commit-bot: I haz the power, 2017-04-28 18:30:22]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-05-01 16:27:44]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-05-01 16:27:44]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2824883005/#ps100001
(title: "Incorporate suggestions from PatchSet #4")

[commit-bot: I haz the power, 2017-05-01 16:28:12]

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-05-01 18:24:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493656064147910,
"parent_rev": "ced3697c2cfa3e1d5814f8bbf9456aaf4a75543b", "commit_rev":
"9fe5b74189b804423d7fe66182a3c07dd6c61dd1"}

[commit-bot: I haz the power, 2017-05-01 18:24:29]

Description was changed from

==========
[Mojo Video Capture] Stop service when last client disconnects.

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

Purpose of this CL:
Stop service when last client disconnects. Once we host it in a separate
process, this will allow the service to automatically restart when a new request
comes in.

Changes in this CL:
* Split class video_capture::ServiceImpl into two classes
  ServiceManagerServiceImpl and VideoCaptureServiceImpl.
* In class ServiceManagerServiceImpl, following the example of ImageDecoder [2],
  use a ServiceContextRefFactory to manage service lifetime and implement a
  delayed service shutdown when there are no clients.
* Add tests ServiceQuitsWhenNoClientConnected and
  ServiceQuitsWhenClientDisconnectsWhileUsingDevice
* In service.mojom, add a method for setting the service shutdown delay.
  For the automated tests, set the delay to zero.
* In DeviceFactoryMediaToMojoAdapter, make sure that
  |capture_system_->GetDeviceInfosAsync()| is called before
  |capture_system_->CreateDevice()|. This is required by the implementation
  VideoCaptureSystemImpl in order to fill the cache |devices_info_cache_|.
* In DeviceMediaToMojoAdapter, unsubscibe from
  |receiver.set_connection_error_handler()| in order to avoid use after free
  on service shutdown.
* In ReceiverMojoToMediaAdapter, introduce class ReceiverOnTaskRunner in order
  to ensure that mojom.Receiver methods are posted to the correct thread.
* In VideoCaptureDeviceClient, allow |external_jpeg_decoder_| to be nullptr,
  since in the context of the video_capture service, this will (for now) be the
  case.

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

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://cs.chromium.org/chromium/src/services/data_decoder/data_decoder_servi...
==========

to

==========
[Mojo Video Capture] Stop service when last client disconnects.

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

Purpose of this CL:
Stop service when last client disconnects. Once we host it in a separate
process, this will allow the service to automatically restart when a new request
comes in.

Changes in this CL:
* Split class video_capture::ServiceImpl into two classes
  ServiceManagerServiceImpl and VideoCaptureServiceImpl.
* In class ServiceManagerServiceImpl, following the example of ImageDecoder [2],
  use a ServiceContextRefFactory to manage service lifetime and implement a
  delayed service shutdown when there are no clients.
* Add tests ServiceQuitsWhenNoClientConnected and
  ServiceQuitsWhenClientDisconnectsWhileUsingDevice
* In service.mojom, add a method for setting the service shutdown delay.
  For the automated tests, set the delay to zero.
* In DeviceFactoryMediaToMojoAdapter, make sure that
  |capture_system_->GetDeviceInfosAsync()| is called before
  |capture_system_->CreateDevice()|. This is required by the implementation
  VideoCaptureSystemImpl in order to fill the cache |devices_info_cache_|.
* In DeviceMediaToMojoAdapter, unsubscibe from
  |receiver.set_connection_error_handler()| in order to avoid use after free
  on service shutdown.
* In ReceiverMojoToMediaAdapter, introduce class ReceiverOnTaskRunner in order
  to ensure that mojom.Receiver methods are posted to the correct thread.
* In VideoCaptureDeviceClient, allow |external_jpeg_decoder_| to be nullptr,
  since in the context of the video_capture service, this will (for now) be the
  case.

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

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://cs.chromium.org/chromium/src/services/data_decoder/data_decoder_servi...

Review-Url: https://codereview.chromium.org/2824883005
Cr-Commit-Position: refs/heads/master@{#468358}
Committed:
https://chromium.googlesource.com/chromium/src/+/9fe5b74189b804423d7fe66182a3...
==========

[commit-bot: I haz the power, 2017-05-01 18:24:30]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9fe5b74189b804423d7fe66182a3...