Media Stream Image Capture (4): wire takePhoto and implement in FakeVCDevice

Media Stream Image Capture (4): wire takePhoto and implement in FakeVCDevice

This CL wires the takePhoto() call from ImageCaptureImpl,
via MediaStreamManager and VideoCaptureManager, to
VideoCaptureDevice. It also implements it for the Fake Video
CaptureDevice, and extends its unit test.

Note the multiple thread-hopping: MSM can only be
accessed on UI thread, where the Mojo callback has
to be Run back. VCManager and FakeVCD live on the
IO thread. (In the future, some platform-VCDevices
have even more threads!).

BUG=518807
TEST= run demo html in
https://rawgit.com/Miguelao/demos/master/imagecapture.html
with flag --enable-blink-features=ImageCapture
(click on "Open Camera 320x240", then on
"Create Image Capturer" and finally, repeatedly,
on "takePhoto").

Committed: https://crrev.com/c67c0bc29e2ce52567af8f769134bd067ba2615b
Cr-Commit-Position: refs/heads/master@{#392350}

[mcasas, 2016-05-04 02:00:09]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-05-04 02:00:16]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-05-04 02:00:26]

Patchset #1 (id:40001) has been deleted

[mcasas, 2016-05-04 18:16:19]

Description was changed from

==========
[WIP] Media Stream Image Capture (4): takePhoto for FakeVCD and FileVCD

BUG=518807
==========

to

==========
Media Stream Image Capture (4): wire takePhoto and implement in FakeVCDevice

This CL wires the takePhoto() call from ImageCaptureImpl,
via MediaStreamManager and VideoCaptureManager, to
VideoCaptureDevice. It also implements it for the Fake Video
CaptureDevice, and extends its unit test.

Note the multiple thread-hopping: MSM can only be 
accessed on UI thread, where the Mojo callback has
to be Run back. VCManager and FakeVCD live on the
IO thread. (In the future, some platform-VCDevices
have even more threads!).

BUG=518807
==========

[mcasas, 2016-05-04 18:21:38]

Patchset #1 (id:60001) has been deleted

[mcasas, 2016-05-04 18:21:46]

Patchset #1 (id:80001) has been deleted

[mcasas, 2016-05-04 18:21:53]

Patchset #1 (id:100001) has been deleted

[mcasas, 2016-05-04 18:22:29]

Description was changed from

==========
Media Stream Image Capture (4): wire takePhoto and implement in FakeVCDevice

This CL wires the takePhoto() call from ImageCaptureImpl,
via MediaStreamManager and VideoCaptureManager, to
VideoCaptureDevice. It also implements it for the Fake Video
CaptureDevice, and extends its unit test.

Note the multiple thread-hopping: MSM can only be 
accessed on UI thread, where the Mojo callback has
to be Run back. VCManager and FakeVCD live on the
IO thread. (In the future, some platform-VCDevices
have even more threads!).

BUG=518807
==========

to

==========
Media Stream Image Capture (4): wire takePhoto and implement in FakeVCDevice

This CL wires the takePhoto() call from ImageCaptureImpl,
via MediaStreamManager and VideoCaptureManager, to
VideoCaptureDevice. It also implements it for the Fake Video
CaptureDevice, and extends its unit test.

Note the multiple thread-hopping: MSM can only be 
accessed on UI thread, where the Mojo callback has
to be Run back. VCManager and FakeVCD live on the
IO thread. (In the future, some platform-VCDevices
have even more threads!).

BUG=518807


TEST= run demo html in
https://rawgit.com/Miguelao/demos/master/imagecapture.html
with flag --enable-blink-features=ImageCapture
(click on "Open Camera 320x240", then on
"Create Image Capturer" and finally, repeatedly,
on "takePhoto").
==========

[mcasas, 2016-05-04 18:25:11]

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

[mcasas, 2016-05-04 18:25:12]

miu@ PTAL at the content/ parts.

[miu, 2016-05-04 23:45:50]

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:8: #include
"content/browser/browser_main_loop.h"
Doesn't look like this include is needed.

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:21:
std::unique_ptr<std::string> data) {
Any particular reason you're mixing use of std::string and std::vector<uint8_t>?
Seems that copying could be avoided if you just used the vector throughout.

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:50:
base::Bind(&ImageCaptureImpl::TakePhotoOnIOThread, base::Unretained(this),
base::Unretained(this) is safe, right?  (I'm not that familiar with Mojo's
lifecycle management.)

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:69:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
IMHO, just call RunTakePhotoCallback() directly (i.e., without the PostTask())
here.

https://codereview.chromium.org/1952463002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/1952463002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:600: int
MediaStreamManager::VideoDeviceIdToSessionId(
The device_id is mapped to a session_id, and then in VideoCaptureManager, the
session_id isn't needed to get at the DeviceEntry. Instead of making any changes
to MediaStreamManager, just add a search-for-DeviceEntry-by-device_id to
VideoCaptureManager::TakePhoto().

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

https://codereview.chromium.org/1952463002/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:161: int session_id,
Following up on my comment above, can this take a device_id instead of a
session_id?

https://codereview.chromium.org/1952463002/diff/120001/media/capture/video/vi...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/1952463002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device.h:308: virtual bool TakePhoto(const
TakePhotoCallback& photo_callback);
Consider adding WARN_UNUSED_RESULT (just before the ending semicolon) to make
sure callers check and handle the "fail" boolean result.

[mcasas, 2016-05-05 00:55:41]

Patchset #2 (id:140001) has been deleted

[mcasas, 2016-05-05 00:57:13]

PTAL

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:8: #include
"content/browser/browser_main_loop.h"
On 2016/05/04 23:45:50, miu wrote:
> Doesn't look like this include is needed.

Is needed for BrowserMainLoop::GetInstance()
in l.52.

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:21:
std::unique_ptr<std::string> data) {
On 2016/05/04 23:45:50, miu wrote:
> Any particular reason you're mixing use of std::string and
std::vector<uint8_t>?
> Seems that copying could be avoided if you just used the vector throughout.

Good point, yeah. The previous CL shifted between std::string
and std::vector<uint8_t> and this is a remnant. Seeing the
next CL in line, std::vector<uint8_t> looks like a better type
throughout, so moving in that direction.

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:50:
base::Bind(&ImageCaptureImpl::TakePhotoOnIOThread, base::Unretained(this),
On 2016/05/04 23:45:50, miu wrote:
> base::Unretained(this) is safe, right?  (I'm not that familiar with Mojo's
> lifecycle management.)

Phew, it might be hard to reason as to if it's really 
safe or not, instead I made TakePhotoOnIOThread()
file-static, so there's no confusion.

https://codereview.chromium.org/1952463002/diff/120001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:69:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2016/05/04 23:45:50, miu wrote:
> IMHO, just call RunTakePhotoCallback() directly (i.e., without the PostTask())
> here.

Done.

https://codereview.chromium.org/1952463002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/1952463002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:600: int
MediaStreamManager::VideoDeviceIdToSessionId(
On 2016/05/04 23:45:50, miu wrote:
> The device_id is mapped to a session_id, and then in VideoCaptureManager, the
> session_id isn't needed to get at the DeviceEntry. Instead of making any
changes
> to MediaStreamManager, just add a search-for-DeviceEntry-by-device_id to
> VideoCaptureManager::TakePhoto().

I wish it was that simple. The MSM has a map from
renderer device ID to sessions, which I use in VCM 
to get to the VCM device ID associated to the session,
_but_ these are different device ids ! Also, the session
in MSM is different from the session in VCM !

E.g. the Renderer Device ID can be called a 128 uuid ID:
ea6a5c2fa17cdee8ff26b13681a3d42b2083cae66ce647e73450ad38a18c6c16
whereas VCM calls this guy
DJH5266R8QBG1HPBV

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

https://codereview.chromium.org/1952463002/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_manager.h:161: int session_id,
On 2016/05/04 23:45:50, miu wrote:
> Following up on my comment above, can this take a device_id instead of a
> session_id?

See above: unfortunately, |device_id| is a different thing for 
MSM and for VCM.

https://codereview.chromium.org/1952463002/diff/120001/media/capture/video/vi...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/1952463002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device.h:308: virtual bool TakePhoto(const
TakePhotoCallback& photo_callback);
On 2016/05/04 23:45:50, miu wrote:
> Consider adding WARN_UNUSED_RESULT (just before the ending semicolon) to make
> sure callers check and handle the "fail" boolean result.

Done.

[miu, 2016-05-05 22:04:24]

https://codereview.chromium.org/1952463002/diff/120001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/1952463002/diff/120001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:600: int
MediaStreamManager::VideoDeviceIdToSessionId(
On 2016/05/05 00:57:12, mcasas wrote:
> On 2016/05/04 23:45:50, miu wrote:
> > The device_id is mapped to a session_id, and then in VideoCaptureManager,
the
> > session_id isn't needed to get at the DeviceEntry. Instead of making any
> changes
> > to MediaStreamManager, just add a search-for-DeviceEntry-by-device_id to
> > VideoCaptureManager::TakePhoto().
> 
> I wish it was that simple. The MSM has a map from
> renderer device ID to sessions, which I use in VCM 
> to get to the VCM device ID associated to the session,
> _but_ these are different device ids ! Also, the session
> in MSM is different from the session in VCM !
> 
> E.g. the Renderer Device ID can be called a 128 uuid ID:
> ea6a5c2fa17cdee8ff26b13681a3d42b2083cae66ce647e73450ad38a18c6c16
> whereas VCM calls this guy
> DJH5266R8QBG1HPBV

OIC. One more thought: Could the renderer provide the device_id the
VideoCaptureManager wants? On the renderer-side, I imagine you'll be ultimately
calling into content::VideoCaptureImpl to request the photo be taken, right? The
needed device ID should be there.

This thought actually brings up another point: Perhaps there should not be an
ImageCapture mojo service to begin with? All the infrastructure you need for the
RPC to the browser process is available in VideoCaptureHost (browser-side) +
VideoCaptureImpl (renderer-side). Why not just add a new IPC message to do this?

https://codereview.chromium.org/1952463002/diff/140002/content/browser/media/...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/1952463002/diff/140002/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:23: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
FWICT, the trampoline to run the callback on the UI thread will always be
needed. Thus, can this whole RunTakePhotoCallback() method just be simply:

  DCHECK(data.get());
  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                          base::Bind(callback, mime_type,
                                     mojo::Array<uint8_t>::From(*data)));

(And, even if there is a way RunTakePhotoCallback() is already running on the UI
thread, it wouldn't hurt to PostTask() anyway.)

https://codereview.chromium.org/1952463002/diff/140002/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:50: DCHECK(result);
This feels brittle. It's possible for the TakePhoto() call on the UI thread to
be made on a device that is shutdown by the time TakePhotoOnIOThread() runs
later on the IO thread.

To keep things sane, I would suggest you guarantee the
ImageCaptureImpl::TakePhotoCallback will be run regardless of success/failure at
any point in the process. The if-statement above could be:

  if (session_id == StreamDeviceInfo::kNoId ||
      !media_stream_manager->video_capture_manager()->TakePhoto(...)) {
    // run callback w/ empty vector
  }

https://codereview.chromium.org/1952463002/diff/140002/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/1952463002/diff/140002/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:180: // Findd
|device_id| in the list of |requests_|, and returns its session id,
s/Findd/Find/

https://codereview.chromium.org/1952463002/diff/140002/media/capture/video/fa...
File media/capture/video/fake_video_capture_device.cc (right):

https://codereview.chromium.org/1952463002/diff/140002/media/capture/video/fa...
media/capture/video/fake_video_capture_device.cc:78: void
RunTakePhotoCallback(const VideoCaptureDevice::TakePhotoCallback& callback,
naming: RunTakePhotoCallback() doesn't really tell the whole story here.
Suggestion: DoTakeFakePhoto()

https://codereview.chromium.org/1952463002/diff/140002/media/capture/video/fa...
File media/capture/video/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/1952463002/diff/140002/media/capture/video/fa...
media/capture/video/fake_video_capture_device_unittest.cc:147:
ASSERT_TRUE(gfx::PNGCodec::Decode(data->data(), data->size(),
Your call, but it feels a bit heavyweight for the unit test to try and decode
the image data. (The layout tests can do this, and also examine the content of
the image.) IMO, just check for a non-empty vector here.

[mcasas, 2016-05-05 23:05:50]

PTAL

https://codereview.chromium.org/1952463002/diff/140002/content/browser/media/...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/1952463002/diff/140002/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:23: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2016/05/05 22:04:24, miu wrote:
> FWICT, the trampoline to run the callback on the UI thread will always be
> needed. Thus, can this whole RunTakePhotoCallback() method just be simply:
> 
>   DCHECK(data.get());
>   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
>                           base::Bind(callback, mime_type,
>                                      mojo::Array<uint8_t>::From(*data)));
> 
> (And, even if there is a way RunTakePhotoCallback() is already running on the
UI
> thread, it wouldn't hurt to PostTask() anyway.)

Done.

https://codereview.chromium.org/1952463002/diff/140002/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:50: DCHECK(result);
On 2016/05/05 22:04:24, miu wrote:
> This feels brittle. It's possible for the TakePhoto() call on the UI thread to
> be made on a device that is shutdown by the time TakePhotoOnIOThread() runs
> later on the IO thread.
> 
> To keep things sane, I would suggest you guarantee the
> ImageCaptureImpl::TakePhotoCallback will be run regardless of success/failure
at
> any point in the process. The if-statement above could be:
> 
>   if (session_id == StreamDeviceInfo::kNoId ||
>       !media_stream_manager->video_capture_manager()->TakePhoto(...)) {
>     // run callback w/ empty vector
>   }

Done.

https://codereview.chromium.org/1952463002/diff/140002/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/1952463002/diff/140002/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:180: // Findd
|device_id| in the list of |requests_|, and returns its session id,
On 2016/05/05 22:04:24, miu wrote:
> s/Findd/Find/

Done.

https://codereview.chromium.org/1952463002/diff/140002/media/capture/video/fa...
File media/capture/video/fake_video_capture_device.cc (right):

https://codereview.chromium.org/1952463002/diff/140002/media/capture/video/fa...
media/capture/video/fake_video_capture_device.cc:78: void
RunTakePhotoCallback(const VideoCaptureDevice::TakePhotoCallback& callback,
On 2016/05/05 22:04:24, miu wrote:
> naming: RunTakePhotoCallback() doesn't really tell the whole story here.
> Suggestion: DoTakeFakePhoto()

Done.

https://codereview.chromium.org/1952463002/diff/140002/media/capture/video/fa...
File media/capture/video/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/1952463002/diff/140002/media/capture/video/fa...
media/capture/video/fake_video_capture_device_unittest.cc:147:
ASSERT_TRUE(gfx::PNGCodec::Decode(data->data(), data->size(),
On 2016/05/05 22:04:24, miu wrote:
> Your call, but it feels a bit heavyweight for the unit test to try and decode
> the image data. (The layout tests can do this, and also examine the content of
> the image.) IMO, just check for a non-empty vector here.

Done.

[mcasas, 2016-05-06 00:52:07]

Patchset #3 (id:170001) has been deleted

[miu, 2016-05-06 18:24:02]

lgtm % nit:

https://codereview.chromium.org/1952463002/diff/190001/content/browser/media/...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/1952463002/diff/190001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:31:
base::Bind(&RunMojoCallback, callback, mime_type,
This is fine, but do you need the RunMojoCallback function? Is the compiler
complaining if you:

  BrowserThread::PostTask(
      BrowserThread::UI, FROM_HERE,
      base::Bind(callback, mime_type,
                 base::Passed(mojo::Array<uint8_t>::From(*data))));

[mcasas, 2016-05-06 19:20:44]

mcasas@chromium.org changed reviewers:
+ haraken@chromium.org, mlamouri@chromium.org

[mcasas, 2016-05-06 19:20:44]

haraken@ or mlamouri@ can you please RS the
micro changes in third_party/WebKit? Thanks!

https://codereview.chromium.org/1952463002/diff/190001/content/browser/media/...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/1952463002/diff/190001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:31:
base::Bind(&RunMojoCallback, callback, mime_type,
On 2016/05/06 18:24:01, miu wrote:
> This is fine, but do you need the RunMojoCallback function? Is the compiler
> complaining if you:
> 
>   BrowserThread::PostTask(
>       BrowserThread::UI, FROM_HERE,
>       base::Bind(callback, mime_type,
>                  base::Passed(mojo::Array<uint8_t>::From(*data))));

I tried it too but it doesn't know how to Bind the
params to the callback.

[mcasas, 2016-05-06 19:21:26]

mcasas@chromium.org changed reviewers:
+ tommi@chromium.org

[mcasas, 2016-05-06 19:21:26]

tommi@: PTAL at media_stream_manager and
video_capture_manager changes or forward 
appropriately, thanks!

[mcasas, 2016-05-06 19:55:25]

Patchset #4 (id:210001) has been deleted

[mcasas, 2016-05-06 19:55:37]

Patchset #4 (id:230001) has been deleted

[haraken, 2016-05-07 02:46:04]

WebKit LGTM.

[tommi (sloooow) - chröme, 2016-05-09 11:17:33]

lgtm % some minor comments.  please check if we're copying the data
unnecessarily there.

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:21: mojo::Array<uint8_t>
data) {
just checking - this is moved and not copied, right? (have you verified?)

I'm not actually sure what happens in this case, but my gut instinct was that
the Array would be owned by the Bind() and therefore not moved when the callback
is made, but rather copied.  It would be a shame to have to copy the array
twice.

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:32:
base::Passed(mojo::Array<uint8_t>::From(*data))));
if you find that the array is actually copied, consider just moving |data| here
and calling the From() method in the RunMojoCallback function.

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
File content/browser/media/capture/image_capture_impl.h (right):

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
content/browser/media/capture/image_capture_impl.h:15: class MediaStreamManager;
doesn't look like this is needed

https://codereview.chromium.org/1952463002/diff/250001/media/capture/video/fa...
File media/capture/video/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/1952463002/diff/250001/media/capture/video/fa...
media/capture/video/fake_video_capture_device_unittest.cc:145:
EXPECT_GT(data->size(), 4u);
ASSERT_GT?

https://codereview.chromium.org/1952463002/diff/250001/media/capture/video/vi...
File media/capture/video/video_capture_device.cc (right):

https://codereview.chromium.org/1952463002/diff/250001/media/capture/video/vi...
media/capture/video/video_capture_device.cc:145: const TakePhotoCallback& /*
photo_callback */) {
since we generally don't use /* */ comment style, suggest to just leave the
variable in there.

[mlamouri (slow - plz ping), 2016-05-09 12:44:35]

Blink changes lgtm

https://codereview.chromium.org/1952463002/diff/250001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-takePhoto.html
(right):

https://codereview.chromium.org/1952463002/diff/250001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-takePhoto.html:28:
// promise will be rejected.
Are you planning to fix this? Should you have a TODO pointing to a bug in order
to add a mock in the test runner?

[mcasas, 2016-05-09 16:33:44]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-05-09 16:33:45]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
haraken@chromium.org, tommi@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1952463002/#ps270001
(title: "tommi@ and mlamouri@ comments")

[commit-bot: I haz the power, 2016-05-09 16:34:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1952463002/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1952463002/270001

[mcasas, 2016-05-09 16:48:41]

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:21: mojo::Array<uint8_t>
data) {
On 2016/05/09 11:17:32, tommi-chrömium wrote:
> just checking - this is moved and not copied, right? (have you verified?)
> 
> I'm not actually sure what happens in this case, but my gut instinct was that
> the Array would be owned by the Bind() and therefore not moved when the
callback
> is made, but rather copied.  It would be a shame to have to copy the array
> twice.

Yeah, mojo::Array is a moveable-not-copyable type [1]
(and, for reference, mojo types ending in Ptr behave
like unique_ptrs).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
content/browser/media/capture/image_capture_impl.cc:32:
base::Passed(mojo::Array<uint8_t>::From(*data))));
On 2016/05/09 11:17:32, tommi-chrömium wrote:
> if you find that the array is actually copied, consider just moving |data|
here
> and calling the From() method in the RunMojoCallback function.

Acknowledged.

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
File content/browser/media/capture/image_capture_impl.h (right):

https://codereview.chromium.org/1952463002/diff/250001/content/browser/media/...
content/browser/media/capture/image_capture_impl.h:15: class MediaStreamManager;
On 2016/05/09 11:17:33, tommi-chrömium wrote:
> doesn't look like this is needed

Done.

https://codereview.chromium.org/1952463002/diff/250001/media/capture/video/fa...
File media/capture/video/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/1952463002/diff/250001/media/capture/video/fa...
media/capture/video/fake_video_capture_device_unittest.cc:145:
EXPECT_GT(data->size(), 4u);
On 2016/05/09 11:17:33, tommi-chrömium wrote:
> ASSERT_GT?

Done.

https://codereview.chromium.org/1952463002/diff/250001/media/capture/video/vi...
File media/capture/video/video_capture_device.cc (right):

https://codereview.chromium.org/1952463002/diff/250001/media/capture/video/vi...
media/capture/video/video_capture_device.cc:145: const TakePhotoCallback& /*
photo_callback */) {
On 2016/05/09 11:17:33, tommi-chrömium wrote:
> since we generally don't use /* */ comment style, suggest to just leave the
> variable in there.

Done.

https://codereview.chromium.org/1952463002/diff/250001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-takePhoto.html
(right):

https://codereview.chromium.org/1952463002/diff/250001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-takePhoto.html:28:
// promise will be rejected.
On 2016/05/09 12:44:35, Mounir Lamouri wrote:
> Are you planning to fix this? Should you have a TODO pointing to a bug in
order
> to add a mock in the test runner?

Done. It essentially means tapping the source id
being passed around, and then having a mojo
mock interface implementation a la USB (with a
mojo mock "service" in JS).

[commit-bot: I haz the power, 2016-05-09 17:27:05]

Description was changed from

==========
Media Stream Image Capture (4): wire takePhoto and implement in FakeVCDevice

This CL wires the takePhoto() call from ImageCaptureImpl,
via MediaStreamManager and VideoCaptureManager, to
VideoCaptureDevice. It also implements it for the Fake Video
CaptureDevice, and extends its unit test.

Note the multiple thread-hopping: MSM can only be 
accessed on UI thread, where the Mojo callback has
to be Run back. VCManager and FakeVCD live on the
IO thread. (In the future, some platform-VCDevices
have even more threads!).

BUG=518807


TEST= run demo html in
https://rawgit.com/Miguelao/demos/master/imagecapture.html
with flag --enable-blink-features=ImageCapture
(click on "Open Camera 320x240", then on
"Create Image Capturer" and finally, repeatedly,
on "takePhoto").
==========

to

==========
Media Stream Image Capture (4): wire takePhoto and implement in FakeVCDevice

This CL wires the takePhoto() call from ImageCaptureImpl,
via MediaStreamManager and VideoCaptureManager, to
VideoCaptureDevice. It also implements it for the Fake Video
CaptureDevice, and extends its unit test.

Note the multiple thread-hopping: MSM can only be 
accessed on UI thread, where the Mojo callback has
to be Run back. VCManager and FakeVCD live on the
IO thread. (In the future, some platform-VCDevices
have even more threads!).

BUG=518807


TEST= run demo html in
https://rawgit.com/Miguelao/demos/master/imagecapture.html
with flag --enable-blink-features=ImageCapture
(click on "Open Camera 320x240", then on
"Create Image Capturer" and finally, repeatedly,
on "takePhoto").
==========

[commit-bot: I haz the power, 2016-05-09 17:27:07]

Committed patchset #5 (id:270001)

[commit-bot: I haz the power, 2016-05-09 17:28:39]

Description was changed from

==========
Media Stream Image Capture (4): wire takePhoto and implement in FakeVCDevice

This CL wires the takePhoto() call from ImageCaptureImpl,
via MediaStreamManager and VideoCaptureManager, to
VideoCaptureDevice. It also implements it for the Fake Video
CaptureDevice, and extends its unit test.

Note the multiple thread-hopping: MSM can only be 
accessed on UI thread, where the Mojo callback has
to be Run back. VCManager and FakeVCD live on the
IO thread. (In the future, some platform-VCDevices
have even more threads!).

BUG=518807


TEST= run demo html in
https://rawgit.com/Miguelao/demos/master/imagecapture.html
with flag --enable-blink-features=ImageCapture
(click on "Open Camera 320x240", then on
"Create Image Capturer" and finally, repeatedly,
on "takePhoto").
==========

to

==========
Media Stream Image Capture (4): wire takePhoto and implement in FakeVCDevice

This CL wires the takePhoto() call from ImageCaptureImpl,
via MediaStreamManager and VideoCaptureManager, to
VideoCaptureDevice. It also implements it for the Fake Video
CaptureDevice, and extends its unit test.

Note the multiple thread-hopping: MSM can only be
accessed on UI thread, where the Mojo callback has
to be Run back. VCManager and FakeVCD live on the
IO thread. (In the future, some platform-VCDevices
have even more threads!).

BUG=518807

TEST= run demo html in
https://rawgit.com/Miguelao/demos/master/imagecapture.html
with flag --enable-blink-features=ImageCapture
(click on "Open Camera 320x240", then on
"Create Image Capturer" and finally, repeatedly,
on "takePhoto").

Committed: https://crrev.com/c67c0bc29e2ce52567af8f769134bd067ba2615b
Cr-Commit-Position: refs/heads/master@{#392350}
==========

[commit-bot: I haz the power, 2016-05-09 17:28:40]

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