Video Capture Mojo (1.4b): Implement ability to use fake device instance

Video Capture Mojo (1.4b): Implement ability to use fake device instance

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review.

* Add DEPS file to allow dependency on existing video capture (media/capture/video) and media Mojo (media/mojo) code.
* Add adapter class DeviceClientMojoToMediaAdapter for being able to connect a Mojo-style VideoCaptureDeviceClient to
  a media::VideoCaptureDevice expecting a media::VideoCaptureDevice::Client. The class is not yet fully implemented.
  Only enough implementation is added to pass the current set of tests.
* Added service test:
  + ErrorCodeOnCreateDeviceForInvalidDescriptor
  + FakeDeviceVideoCaptureServiceTest.FrameCallbacksArrive
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass.

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

Committed: https://crrev.com/3cfa98dff3f78af0da0e2b8f22d0f77e6fde0c28
Cr-Commit-Position: refs/heads/master@{#416029}

[chfremer, 2016-08-12 17:53:38]

Description was changed from

==========
Merge branch 'FillServicePart1' into FillServicePart2


Fill Service Part 2

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

to

==========
Video Capture Mojo: Implement ability to add fake device instances

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Add DEPS file to allow dependency on existing video capture
(media/capture/video) and media Mojo (media/mojo) code.
* Add adapter class for being able to connect a Mojo-style
VideoCaptureDeviceClient to 
  a media::VideoCaptureDevice expecting a media::VideoCaptureDevice::Client. The
class is not yet fully implemented.
  Only enough implementation is added to pass the current set of tests.
* Added service test:
  + FrameCallbacksFromFakeCaptureDeviceArrive
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

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

[chfremer, 2016-08-12 17:55:30]

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

[chfremer, 2016-08-12 17:55:31]

mcasas@: Please review general approach and style
yzshen@: Please review Mojo aspects
emircan@ & rockot@: FYI

[chfremer, 2016-08-19 17:09:35]

Description was changed from

==========
Video Capture Mojo: Implement ability to add fake device instances

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Add DEPS file to allow dependency on existing video capture
(media/capture/video) and media Mojo (media/mojo) code.
* Add adapter class for being able to connect a Mojo-style
VideoCaptureDeviceClient to 
  a media::VideoCaptureDevice expecting a media::VideoCaptureDevice::Client. The
class is not yet fully implemented.
  Only enough implementation is added to pass the current set of tests.
* Added service test:
  + FrameCallbacksFromFakeCaptureDeviceArrive
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

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

to

==========
Video Capture Mojo (1.4b): Implement ability to use fake device instance

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Add DEPS file to allow dependency on existing video capture
(media/capture/video) and media Mojo (media/mojo) code.
* Add adapter class DeviceClientMojoToMediaAdapter for being able to connect a
Mojo-style VideoCaptureDeviceClient to 
  a media::VideoCaptureDevice expecting a media::VideoCaptureDevice::Client. The
class is not yet fully implemented.
  Only enough implementation is added to pass the current set of tests.
* Added service test:
  + ErrorCodeOnCreateDeviceForInvalidDescriptor
  + FakeDeviceVideoCaptureServiceTest.FrameCallbacksArrive
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

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

[chfremer, 2016-08-19 17:09:45]

Patchset #1 (id:1) has been deleted

[chfremer, 2016-08-19 17:09:52]

Patchset #1 (id:20001) has been deleted

[chfremer, 2016-08-19 17:10:00]

Patchset #1 (id:40001) has been deleted

[chfremer, 2016-08-19 17:11:54]

On 2016/08/12 17:55:31, chfremer wrote:
> mcasas@: Please review general approach and style
> yzshen@: Please review Mojo aspects
> emircan@ & rockot@: FYI

mcasas@: PTAL
rockot@: PTAL Mojo aspects
emircan@ & yzshen@: FYI

Merged with latest version of upstream changes from preceding reviews.

[Ken Rockot(use gerrit already), 2016-08-19 17:49:00]

lgtm

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/video_capture_device_factory_impl.h (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/video_capture_device_factory_impl.h:60:
std::unique_ptr<mojo::Binding<mojom::VideoCaptureDeviceProxy>> binding_;
I don't see any reason to use a unique_ptr here. Just use a mojo::Binding
directly?

[chfremer, 2016-08-19 18:11:19]

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/video_capture_device_factory_impl.h (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/video_capture_device_factory_impl.h:60:
std::unique_ptr<mojo::Binding<mojom::VideoCaptureDeviceProxy>> binding_;
On 2016/08/19 17:49:00, Ken Rockot wrote:
> I don't see any reason to use a unique_ptr here. Just use a mojo::Binding
> directly?

I tried that initially, but found that an inconvenient amount of complexity was
needed to implement the move operators, since mojo::Binding does not offer them.
Decided to fall back to using a unique_ptr<>. 

Should we consider adding move operators for mojo::Binding?

[chfremer, 2016-08-29 17:39:50]

Patchset #2 (id:80001) has been deleted

[chfremer, 2016-08-29 17:41:17]

mcasas@: PTAL
emircan@: FYI

[mcasas, 2016-08-30 01:20:27]

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/device_client_mojo_to_media_adapter.h (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/device_client_mojo_to_media_adapter.h:19: class
DeviceClientMojoToMediaAdapter
Hmm this name doesn't sound 100% right somehow.
Name wise, I see there are few _shim, and about
the same _adapter and _bridge, albeit >50% of the
adapters are in media related code. But I guess
adapter is fine. 
However, what this class does is to adapt the data
flow coming from a media::VCD towards a 
mojom::VideoCaptureDeviceClient, so I'd call it
MediaVideoCaptureDeviceToMojomAdapter :-) :-)
or some shortened version of it, WDYT?

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/device_client_mojo_to_media_adapter.h:58:
scoped_refptr<media::MojoSharedBufferVideoFrame> dummy_frame_;
nit: I'd add here the comment about this variable being 
temporary instead of l.31-32 of the .cc file and, just in 
case, quote the bug number 584797.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/mock_video_capture_device_client.h (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/mock_video_capture_device_client.h:22:
MOCK_METHOD1(OnFrameAvailablePtr, void(media::mojom::VideoFramePtr* frame));
Sometimes I've seen this "upper-half"/"bottom-half"
micro pattern methods being called X() and DoX(),
respectively. Your call.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/mock_video_capture_device_client.h:26:
mojo::Binding<mojom::VideoCaptureDeviceClient> binding_;
const?

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/service_unittest.cc (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/service_unittest.cc:109: class
FakeDeviceVideoCaptureServiceTest : public VideoCaptureServiceTest {
Separate this new class and associated test cases into
a file on its own, and make the file names follow the 
class plz.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/video_capture_device_factory_impl.cc (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/video_capture_device_factory_impl.cc:37: bool
VideoCaptureDeviceFactoryImpl::DeviceEntry::DescriptorEquals(
Why not adding a friend bool operator== ?

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/video_capture_device_factory_impl.cc:88:
entry.Bind(std::move(proxy_request));
nit: no {} for one-line bodies.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/video_capture_device_proxy_impl.h (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/video_capture_device_proxy_impl.h:36:
mojom::PowerLineFrequency frequency);
All these conversion methods are shouting for a typemap,
although perhaps not in this CL. Please add a TODO+bug.

https://www.chromium.org/developers/design-documents/mojo/type-mapping

[chfremer, 2016-08-30 18:21:35]

Patchset #2 (id:100001) has been deleted

[chfremer, 2016-08-30 18:22:55]

mcasas@: PTAL

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/device_client_mojo_to_media_adapter.h (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/device_client_mojo_to_media_adapter.h:19: class
DeviceClientMojoToMediaAdapter
On 2016/08/30 01:20:26, mcasas wrote:
> Hmm this name doesn't sound 100% right somehow.
> Name wise, I see there are few _shim, and about
> the same _adapter and _bridge, albeit >50% of the
> adapters are in media related code. But I guess
> adapter is fine. 
> However, what this class does is to adapt the data
> flow coming from a media::VCD towards a 
> mojom::VideoCaptureDeviceClient, so I'd call it
> MediaVideoCaptureDeviceToMojomAdapter :-) :-)
> or some shortened version of it, WDYT?

It seems that the terms "shim", "adapter", "bridge", and "wrapper" could all be
used here. I prefer "adapter", because it refers to the Adapter Pattern [1] as
described in "standard literature" of software engineering [2][3]. In addition
to just expressing that calls are redirected, it expresses the purpose of the
redirection, which is "The Adapter design pattern allows otherwise incompatible
classes to work together by converting the interface of one class into an
interface expected by the clients". I would avoid the term "bridge" as it could
cause confusion with the Bridge Pattern [4].

As for the naming of the adapter, there are two opposite but equivalent ways of
thinking about it. Your suggestion takes the perspective of client calls
arriving at the outer interface (media) and being adapted to the inner interface
(mojo). My naming choice takes the perspective of factory code providing an
instance exposing the inner interface (mojo) and using an adapter to convert it
to the outer interface (media). I prefer the second perspective, because the
factory code is the place where the name of the adapter class appears, and
clients of the outer interface should know nothing about the adapter.

[1] https://en.wikipedia.org/wiki/Adapter_pattern
[2] https://en.wikipedia.org/wiki/Design_Patterns
[3] http://www.headfirstlabs.com/books/hfdp/
[4] https://en.wikipedia.org/wiki/Bridge_pattern

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/device_client_mojo_to_media_adapter.h:58:
scoped_refptr<media::MojoSharedBufferVideoFrame> dummy_frame_;
On 2016/08/30 01:20:26, mcasas wrote:
> nit: I'd add here the comment about this variable being 
> temporary instead of l.31-32 of the .cc file and, just in 
> case, quote the bug number 584797.

Done.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/mock_video_capture_device_client.h (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/mock_video_capture_device_client.h:22:
MOCK_METHOD1(OnFrameAvailablePtr, void(media::mojom::VideoFramePtr* frame));
On 2016/08/30 01:20:26, mcasas wrote:
> Sometimes I've seen this "upper-half"/"bottom-half"
> micro pattern methods being called X() and DoX(),
> respectively. Your call.

Acknowledged.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/mock_video_capture_device_client.h:26:
mojo::Binding<mojom::VideoCaptureDeviceClient> binding_;
On 2016/08/30 01:20:26, mcasas wrote:
> const?

Done.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/service_unittest.cc (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/service_unittest.cc:109: class
FakeDeviceVideoCaptureServiceTest : public VideoCaptureServiceTest {
On 2016/08/30 01:20:26, mcasas wrote:
> Separate this new class and associated test cases into
> a file on its own, and make the file names follow the 
> class plz.

Done.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/video_capture_device_factory_impl.cc (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/video_capture_device_factory_impl.cc:37: bool
VideoCaptureDeviceFactoryImpl::DeviceEntry::DescriptorEquals(
On 2016/08/30 01:20:26, mcasas wrote:
> Why not adding a friend bool operator== ?

Not sure I understand. Do you mean adding DeviceEntry::operator==()? In the
usage, I don't need such an operator. I only need comparison with the
|descriptor_| field.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/video_capture_device_factory_impl.cc:88:
entry.Bind(std::move(proxy_request));
On 2016/08/30 01:20:27, mcasas wrote:
> nit: no {} for one-line bodies.

Done.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/video_capture_device_proxy_impl.h (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/video_capture_device_proxy_impl.h:36:
mojom::PowerLineFrequency frequency);
On 2016/08/30 01:20:27, mcasas wrote:
> All these conversion methods are shouting for a typemap,
> although perhaps not in this CL. Please add a TODO+bug.
> 
> https://www.chromium.org/developers/design-documents/mojo/type-mapping

Done.

[mcasas, 2016-08-31 21:10:36]

Almost there, just a few comments.

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
File services/video_capture/video_capture_device_factory_impl.cc (right):

https://codereview.chromium.org/2238083004/diff/60001/services/video_capture/...
services/video_capture/video_capture_device_factory_impl.cc:37: bool
VideoCaptureDeviceFactoryImpl::DeviceEntry::DescriptorEquals(
On 2016/08/30 18:22:54, chfremer wrote:
> On 2016/08/30 01:20:26, mcasas wrote:
> > Why not adding a friend bool operator== ?
> 
> Not sure I understand. Do you mean adding DeviceEntry::operator==()? In the
> usage, I don't need such an operator. I only need comparison with the
> |descriptor_| field.

I meant a VideoCaptureDeviceDescriptor's 
operator==, but now I see that there's just one field
compared indeed.

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
File services/video_capture/fake_device_video_capture_service_test.h (right):

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/fake_device_video_capture_service_test.h:14: class
FakeDeviceVideoCaptureServiceTest : public VideoCaptureServiceTest {
Sorry, I meant to put the class declaration & definition
& test cases in a file of its own:
fake_device_video_capture_service_unittest.cc,
containing the usual class declaration+definition (it's
commonplace to inline the SetUp() method) and then
the test cases. Sorry for the confusion :)

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
File services/video_capture/mock_device_descriptor_receiver.h (right):

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/mock_device_descriptor_receiver.h:16:
~MockDeviceDescriptorReceiver();
Since this class has no member variables, consider moving the .cc
definitions in here.

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
File services/video_capture/video_capture_device_factory_impl.cc (right):

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/video_capture_device_factory_impl.cc:20: device_proxy_ =
std::move(bindable_target);
Move l.18-19 to the member initializer list.

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/video_capture_device_factory_impl.cc:86: entry.Unbind();
Question: if there is a VCDeviceProxy correctly bound, 
wouldn't this hit the CHECK in [1]? 

// This method may only be called if the object has been bound to a message
// pipe and there are no associated interfaces running.

If this were the case, we could not re-Bind() the Proxy 
and l.85-86 should just be a CHECK(!entry.is_bound());
which would be superfluous as is enforced by [1].


[1]
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/binding.h?q=moj...

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
File services/video_capture/video_capture_service_unittest.cc (right):

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/video_capture_service_unittest.cc:57:
mojom::VideoCaptureDeviceProxyPtr fake_device_proxy_;
No underscore for non-members.

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/video_capture_service_unittest.cc:67: &wait_loop,
&result_code));
Simplify this test case by removing l.58 and moving l.69
inside the lambda:

  factory_->CreateDeviceProxy(
      std::move(invalid_descriptor), mojo::GetProxy(&fake_device_proxy_),
      base::Bind(
          [](base::RunLoop* wait_loop, mojom::DeviceAccessResultCode code) {
            ASSERT_EQ(mojom::DeviceAccessResultCode::ERROR_DEVICE_NOT_FOUND,
                      code);
            wait_loop->Quit();
          },
          &wait_loop));

[chfremer, 2016-08-31 21:49:43]

mcasas@: PTAL

Also, will ping you regarding one of the questions.

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
File services/video_capture/fake_device_video_capture_service_test.h (right):

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/fake_device_video_capture_service_test.h:14: class
FakeDeviceVideoCaptureServiceTest : public VideoCaptureServiceTest {
On 2016/08/31 21:10:35, mcasas wrote:
> Sorry, I meant to put the class declaration & definition
> & test cases in a file of its own:
> fake_device_video_capture_service_unittest.cc,
> containing the usual class declaration+definition (it's
> commonplace to inline the SetUp() method) and then
> the test cases. Sorry for the confusion :)

No worries. I actually tried that first, but then realized that I need to
separate-out the declarations, because there is a inheritance chain going on
here: FakeDeviceVideoCaptureServiceTest extends VideoCaptureServiceTest. This is
to avoid duplication of the setup code that is common for both.

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
File services/video_capture/mock_device_descriptor_receiver.h (right):

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/mock_device_descriptor_receiver.h:16:
~MockDeviceDescriptorReceiver();
On 2016/08/31 21:10:35, mcasas wrote:
> Since this class has no member variables, consider moving the .cc
> definitions in here.

Tried this, and compiler complains that Complex class/struct needs an explicit
out-of-line constructor and destructor.
Not sure why, but I assume that the MOCK_METHOD1 somehow causes the class to be
considered "Complex".

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
File services/video_capture/video_capture_device_factory_impl.cc (right):

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/video_capture_device_factory_impl.cc:20: device_proxy_ =
std::move(bindable_target);
On 2016/08/31 21:10:35, mcasas wrote:
> Move l.18-19 to the member initializer list.

Done.

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/video_capture_device_factory_impl.cc:86: entry.Unbind();
On 2016/08/31 21:10:35, mcasas wrote:
> Question: if there is a VCDeviceProxy correctly bound, 
> wouldn't this hit the CHECK in [1]? 
> 
> // This method may only be called if the object has been bound to a message
> // pipe and there are no associated interfaces running.
> 
> If this were the case, we could not re-Bind() the Proxy 
> and l.85-86 should just be a CHECK(!entry.is_bound());
> which would be superfluous as is enforced by [1].
> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/binding.h?q=moj...

I'll ping you to discuss about this.

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
File services/video_capture/video_capture_service_unittest.cc (right):

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/video_capture_service_unittest.cc:57:
mojom::VideoCaptureDeviceProxyPtr fake_device_proxy_;
On 2016/08/31 21:10:35, mcasas wrote:
> No underscore for non-members.

Done.

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/video_capture_service_unittest.cc:67: &wait_loop,
&result_code));
On 2016/08/31 21:10:35, mcasas wrote:
> Simplify this test case by removing l.58 and moving l.69
> inside the lambda:
> 
>   factory_->CreateDeviceProxy(
>       std::move(invalid_descriptor), mojo::GetProxy(&fake_device_proxy_),
>       base::Bind(
>           [](base::RunLoop* wait_loop, mojom::DeviceAccessResultCode code) {
>             ASSERT_EQ(mojom::DeviceAccessResultCode::ERROR_DEVICE_NOT_FOUND,
>                       code);
>             wait_loop->Quit();
>           },
>           &wait_loop));

I did that originally, but ran into the downside that |wait_loop| is never quit
if the assert fails.

[chfremer, 2016-08-31 22:30:52]

mcasas@: PTAL

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
File services/video_capture/video_capture_device_factory_impl.cc (right):

https://codereview.chromium.org/2238083004/diff/120001/services/video_capture...
services/video_capture/video_capture_device_factory_impl.cc:86: entry.Unbind();
On 2016/08/31 21:49:43, chfremer wrote:
> On 2016/08/31 21:10:35, mcasas wrote:
> > Question: if there is a VCDeviceProxy correctly bound, 
> > wouldn't this hit the CHECK in [1]? 
> > 
> > // This method may only be called if the object has been bound to a message
> > // pipe and there are no associated interfaces running.
> > 
> > If this were the case, we could not re-Bind() the Proxy 
> > and l.85-86 should just be a CHECK(!entry.is_bound());
> > which would be superfluous as is enforced by [1].
> > 
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/binding.h?q=moj...

I got clarification from yzshen. "Associated interfaces" [1] is a Mojo concept I
didn't know yet, and we are not using it here. Therefore it is legal to call
Unbind() here.

The follow-up CL will also contain tests that verify that requesting a device
proxy for a second time behaves as expected.

[1]
https://www.chromium.org/developers/design-documents/mojo/associated-interfaces

[chfremer, 2016-08-31 23:21:05]

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

[chfremer, 2016-08-31 23:21:06]

dalecurtis@: PTAL, I am adding a DEPS on media/mojo and media/capture/video.

[mcasas, 2016-08-31 23:22:36]

after some offline discussion, lgtm

[DaleCurtis, 2016-09-01 00:09:09]

dalecurtis@chromium.org changed reviewers:
+ sandersd@chromium.org

[DaleCurtis, 2016-09-01 00:09:10]

=>sandersd since I'm not sure what the latest state of mojo deps plan is.

[sandersd (OOO until July 31), 2016-09-01 00:14:19]

On 2016/09/01 00:09:10, DaleCurtis wrote:
> =>sandersd since I'm not sure what the latest state of mojo deps plan is.

As far as I know, the only rule is that it must be possible to build //media
without a mojo dependency. Since this dep is in the opposite direction, I don't
think there is a problem. (I'm assuming here that //media does not depend on
//services/video_capture.)

lgtm

[chfremer, 2016-09-01 13:48:06]

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

[commit-bot: I haz the power, 2016-09-01 13:48: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-09-01 13:50:26]

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

[commit-bot: I haz the power, 2016-09-01 13:50:28]

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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chfremer, 2016-09-01 14:57:56]

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

[commit-bot: I haz the power, 2016-09-01 14:58:18]

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

[commit-bot: I haz the power, 2016-09-01 15:56:55]

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

[commit-bot: I haz the power, 2016-09-01 15:56:56]

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

[chfremer, 2016-09-01 17:34:49]

Patchset #5 (id:180001) has been deleted

[chfremer, 2016-09-01 18:11:44]

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

[commit-bot: I haz the power, 2016-09-01 18:12: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-09-01 19:18:42]

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

[commit-bot: I haz the power, 2016-09-01 19:18:43]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-09-01 20:03:47]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2016-09-01 20:03:48]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
sandersd@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2238083004/#ps200001
(title: "Fix control reached end of non-void function.")

[commit-bot: I haz the power, 2016-09-01 20:04:49]

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

[commit-bot: I haz the power, 2016-09-01 20:10:13]

Description was changed from

==========
Video Capture Mojo (1.4b): Implement ability to use fake device instance

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Add DEPS file to allow dependency on existing video capture
(media/capture/video) and media Mojo (media/mojo) code.
* Add adapter class DeviceClientMojoToMediaAdapter for being able to connect a
Mojo-style VideoCaptureDeviceClient to 
  a media::VideoCaptureDevice expecting a media::VideoCaptureDevice::Client. The
class is not yet fully implemented.
  Only enough implementation is added to pass the current set of tests.
* Added service test:
  + ErrorCodeOnCreateDeviceForInvalidDescriptor
  + FakeDeviceVideoCaptureServiceTest.FrameCallbacksArrive
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

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

to

==========
Video Capture Mojo (1.4b): Implement ability to use fake device instance

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Add DEPS file to allow dependency on existing video capture
(media/capture/video) and media Mojo (media/mojo) code.
* Add adapter class DeviceClientMojoToMediaAdapter for being able to connect a
Mojo-style VideoCaptureDeviceClient to 
  a media::VideoCaptureDevice expecting a media::VideoCaptureDevice::Client. The
class is not yet fully implemented.
  Only enough implementation is added to pass the current set of tests.
* Added service test:
  + ErrorCodeOnCreateDeviceForInvalidDescriptor
  + FakeDeviceVideoCaptureServiceTest.FrameCallbacksArrive
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

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

[commit-bot: I haz the power, 2016-09-01 20:10:14]

Committed patchset #5 (id:200001)

[commit-bot: I haz the power, 2016-09-01 20:12:13]

Description was changed from

==========
Video Capture Mojo (1.4b): Implement ability to use fake device instance

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Add DEPS file to allow dependency on existing video capture
(media/capture/video) and media Mojo (media/mojo) code.
* Add adapter class DeviceClientMojoToMediaAdapter for being able to connect a
Mojo-style VideoCaptureDeviceClient to 
  a media::VideoCaptureDevice expecting a media::VideoCaptureDevice::Client. The
class is not yet fully implemented.
  Only enough implementation is added to pass the current set of tests.
* Added service test:
  + ErrorCodeOnCreateDeviceForInvalidDescriptor
  + FakeDeviceVideoCaptureServiceTest.FrameCallbacksArrive
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

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

to

==========
Video Capture Mojo (1.4b): Implement ability to use fake device instance

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review.

* Add DEPS file to allow dependency on existing video capture
(media/capture/video) and media Mojo (media/mojo) code.
* Add adapter class DeviceClientMojoToMediaAdapter for being able to connect a
Mojo-style VideoCaptureDeviceClient to
  a media::VideoCaptureDevice expecting a media::VideoCaptureDevice::Client. The
class is not yet fully implemented.
  Only enough implementation is added to pass the current set of tests.
* Added service test:
  + ErrorCodeOnCreateDeviceForInvalidDescriptor
  + FakeDeviceVideoCaptureServiceTest.FrameCallbacksArrive
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

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

Committed: https://crrev.com/3cfa98dff3f78af0da0e2b8f22d0f77e6fde0c28
Cr-Commit-Position: refs/heads/master@{#416029}
==========

[commit-bot: I haz the power, 2016-09-01 20:12:14]

Patchset 5 (id:??) landed as
https://crrev.com/3cfa98dff3f78af0da0e2b8f22d0f77e6fde0c28
Cr-Commit-Position: refs/heads/master@{#416029}