Add a ChromeOS implementation of VideoCaptureDevice

Add a ChromeOS implementation of VideoCaptureDevice that is aware of the rotation of the internal display, and updates the rotation of captured frames to match.

BUG=360048
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271560

[Zachary Kuznia, 2014-05-08 21:17:03]

Please take a look.

Thanks,
-Zach

[vrk (LEFT CHROMIUM), 2014-05-08 21:43:33]

lgtm

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:73: void
SetDisplayRotation(gfx::Display::Rotation rotation) {
nit: this should be set_display_rotation.

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:108:
display_rotation_ = rotation;
Set this field on IO thread as well so as to keep the setting/getting of the
field simple.

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.h:250:
gfx::Display::Rotation display_rotation_;
Add comment saying this should be on the device thread only.

[Zachary Kuznia, 2014-05-08 21:51:22]

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:73: void
SetDisplayRotation(gfx::Display::Rotation rotation) {
On 2014/05/08 21:43:34, Victoria Kirst wrote:
> nit: this should be set_display_rotation.

Done.

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:108:
display_rotation_ = rotation;
On 2014/05/08 21:43:34, Victoria Kirst wrote:
> Set this field on IO thread as well so as to keep the setting/getting of the
> field simple.

Done.

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/270263008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.h:250:
gfx::Display::Rotation display_rotation_;
On 2014/05/08 21:43:34, Victoria Kirst wrote:
> Add comment saying this should be on the device thread only.

Done.

[vrk (LEFT CHROMIUM), 2014-05-09 01:11:31]

not lgtm

Zach's updated the code since my initial review, and I think now I need to defer
this review to Tommi and Per.

Tommi/Per: Could you take a look, especially at where the observer gets added in
MediaStreamManager? That seems like the wrong place to me, but I don't know
where the right place is. Zach mentioned that there's some trickiness because
MSM is created after the screen is accessible from
gfx::Screen::GetScreenByType(). This also seems to be behavior that we really
only need for CrOS, so I'm wondering if there's a place where CrOS-specific
behavior can be consolidated.

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:460: if
(!observing_screen_) {
Hmm.. I don't think this is the right place to add the observer. I'm also
wondering if VideoCaptureManager should observe this directly, instead of having
MediaStreamManager observe it... also, this is really only useful with CrOS,
right? So there's possibly a CrOS-specific place where you can add this code.

Unfortunately this goes beyond my familiarity with this code, so I'm deferring
this to tommi@ and perkj@.

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:465:
OnDisplayBoundsChanged(screen->GetPrimaryDisplay());
Generally it's best let observer functions be called solely by the object it's
observing, instead of calling it directly. That makes it easier to reason about
the code.

So modify SetDisplayRotationOnIOThread() to include the "If
display.IsInternal()" check and call that instead of OnDisplayBoundsChange.

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1972: if
(display.IsInternal()) {
Move the "if (display.IsInternal())" check to SetDisplayRotationOnIOThread, and
pass |display| instead of |rotation|. See comment above.

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1981: void
MediaStreamManager::OnDisplayAdded(const gfx::Display& new_display) {
Nit: {} on one line, and comment out the unused param, so
void MediaStreamManager::OnDisplayAdded(const gfx::Display& /*new_display*/) {}

(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1984: void
MediaStreamManager::OnDisplayRemoved(const gfx::Display& old_display) {
ditto above

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:46: void
SetDisplayRotation(gfx::Display::Rotation rotation);
nit: Comment that this must be called on the IO thread.

[Zachary Kuznia, 2014-05-09 07:42:21]

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:460: if
(!observing_screen_) {
On 2014/05/09 01:11:31, Victoria Kirst wrote:
> Hmm.. I don't think this is the right place to add the observer. I'm also
> wondering if VideoCaptureManager should observe this directly, instead of
having
> MediaStreamManager observe it... also, this is really only useful with CrOS,
> right? So there's possibly a CrOS-specific place where you can add this code.
> 
> Unfortunately this goes beyond my familiarity with this code, so I'm deferring
> this to tommi@ and perkj@.

I will discuss this with them.

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:465:
OnDisplayBoundsChanged(screen->GetPrimaryDisplay());
On 2014/05/09 01:11:31, Victoria Kirst wrote:
> Generally it's best let observer functions be called solely by the object it's
> observing, instead of calling it directly. That makes it easier to reason
about
> the code.
> 
> So modify SetDisplayRotationOnIOThread() to include the "If
> display.IsInternal()" check and call that instead of OnDisplayBoundsChange.

Done.

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1972: if
(display.IsInternal()) {
On 2014/05/09 01:11:31, Victoria Kirst wrote:
> Move the "if (display.IsInternal())" check to SetDisplayRotationOnIOThread,
and
> pass |display| instead of |rotation|. See comment above.

Done.

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1981: void
MediaStreamManager::OnDisplayAdded(const gfx::Display& new_display) {
On 2014/05/09 01:11:31, Victoria Kirst wrote:
> Nit: {} on one line, and comment out the unused param, so
> void MediaStreamManager::OnDisplayAdded(const gfx::Display& /*new_display*/)
{}
> 
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

Done.

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1984: void
MediaStreamManager::OnDisplayRemoved(const gfx::Display& old_display) {
On 2014/05/09 01:11:31, Victoria Kirst wrote:
> ditto above

Done, save that the } doesn't fit on the same line.

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/270263008/diff/50001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:46: void
SetDisplayRotation(gfx::Display::Rotation rotation);
On 2014/05/09 01:11:31, Victoria Kirst wrote:
> nit: Comment that this must be called on the IO thread.

Done.

[perkj_chrome, 2014-05-09 08:28:11]

Discussed off line- Will try to move this to the VideoCaptuereDeviceLinux to
work similar to Android.

[Zachary Kuznia, 2014-05-09 09:55:37]

Moved the implementation to VideoCaptureDeviceLinux.

[perkj_chrome, 2014-05-09 10:11:27]

Much better I think.

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:277: gfx::Screen* screen
=
This will be called on a special thread used only for media devices? Is it ok to
run the display observer from any thread?

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:287:
gfx::Screen::GetScreenByType(gfx::SCREEN_TYPE_ALTERNATE);
Can there be a race here? ie, is it ok to not call RemoveObserver even if it was
added?

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:332:
v4l2_thread_.message_loop()->PostTask(
This thread is started in AllocateAndStart. So you must verify that you are in
the correct state before posting. How about moving registration to
AllocateAndStart to ensure that this is safe. And deregister in the stop method.

[perkj_chrome, 2014-05-09 10:18:51]

Adding Miguel as well. He as actually worked on this code lately...

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:348: display_rotation_ =
display.rotation();
Add a todo and a comment that this will in fact rotate all cameras and why this
is done.

[perkj_chrome, 2014-05-09 10:28:43]

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:332:
v4l2_thread_.message_loop()->PostTask(
oh- btw - this need a thread check to make sure its called on the same thread as
we do AllocateAndStart and Stop.... -- ie it must be called on the same thread
as we register and deregister the observer.

[mcasas, 2014-05-09 10:32:16]

I don't fully agree with the approach here based on the fact that we'd be
extending the Capture Device, whose tasks are opening/closing the device and
pushing frames, to listen to some external entity mostly  unrelated. (Android,
truly, grabs the orientation from the system, but that's also bad). 

Plus, I don't feel entirely sure that media/video/capture depending on UI is a
good thing (I know it works now due to VideoCaptureFormat pulling gfx::Size...).

Finally I'm not entirely sure if this would work as intended with the Chromebook
listed as use-case in the bug: would it be OK to rotate the video feed if using
an external Webcam?

My proposal could be to:
a) extend the VCDeviceLinux and create a specific one for the internal webcam,
which would do the same thing as VCDL plus the observation.
b) Create your special device driver in the Factory method (that soon will be
Factory class method) [1].

Makes sense?


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.h (right):

https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.h:57: // Once v4l2_thread_
is started, only called on that thread.
|v4l2_thread_|

[Zachary Kuznia, 2014-05-09 22:04:51]

On 2014/05/09 10:32:16, mcasas wrote:
> I don't fully agree with the approach here based on the fact that we'd be
> extending the Capture Device, whose tasks are opening/closing the device and
> pushing frames, to listen to some external entity mostly  unrelated. (Android,
> truly, grabs the orientation from the system, but that's also bad). 
> 
> Plus, I don't feel entirely sure that media/video/capture depending on UI is a
> good thing (I know it works now due to VideoCaptureFormat pulling
gfx::Size...).
> 
> Finally I'm not entirely sure if this would work as intended with the
Chromebook
> listed as use-case in the bug: would it be OK to rotate the video feed if
using
> an external Webcam?
> 
> My proposal could be to:
> a) extend the VCDeviceLinux and create a specific one for the internal webcam,
> which would do the same thing as VCDL plus the observation.
> b) Create your special device driver in the Factory method (that soon will be
> Factory class method) [1].
> 
> Makes sense?
> 
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
> 
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> File media/video/capture/linux/video_capture_device_linux.h (right):
> 
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> media/video/capture/linux/video_capture_device_linux.h:57: // Once
v4l2_thread_
> is started, only called on that thread.
> |v4l2_thread_|

I've updated it to use this design, as you suggest.  Limiting this behavior to
the internal webcam will be handled in a future patch, as we discussed offline.

[mcasas, 2014-05-10 10:12:30]

On 2014/05/09 22:04:51, Zachary Kuznia wrote:
> On 2014/05/09 10:32:16, mcasas wrote:
> > I don't fully agree with the approach here based on the fact that we'd be
> > extending the Capture Device, whose tasks are opening/closing the device and
> > pushing frames, to listen to some external entity mostly  unrelated.
(Android,
> > truly, grabs the orientation from the system, but that's also bad). 
> > 
> > Plus, I don't feel entirely sure that media/video/capture depending on UI is
a
> > good thing (I know it works now due to VideoCaptureFormat pulling
> gfx::Size...).
> > 
> > Finally I'm not entirely sure if this would work as intended with the
> Chromebook
> > listed as use-case in the bug: would it be OK to rotate the video feed if
> using
> > an external Webcam?
> > 
> > My proposal could be to:
> > a) extend the VCDeviceLinux and create a specific one for the internal
webcam,
> > which would do the same thing as VCDL plus the observation.
> > b) Create your special device driver in the Factory method (that soon will
be
> > Factory class method) [1].
> > 
> > Makes sense?
> > 
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
> > 
> >
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> > File media/video/capture/linux/video_capture_device_linux.h (right):
> > 
> >
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> > media/video/capture/linux/video_capture_device_linux.h:57: // Once
> v4l2_thread_
> > is started, only called on that thread.
> > |v4l2_thread_|
> 
> I've updated it to use this design, as you suggest.  Limiting this behavior to
> the internal webcam will be handled in a future patch, as we discussed
offline.

Looks much better. Suggestion: you could move the |rotation_|, 
SetRotation(int rotation) etc out of the VideoCaptureDeviceLinux
and into VideoCaptureDeviceChromeOS, since the latter would be
the only used for CrOS, right? 

Another tip: the "media thread" is usually called the "Device Thread".

Then this CL would be just about adding the VCDChromeOS and
the modifications to the VideoCaptureDevice::Create().

[Zachary Kuznia, 2014-05-10 13:58:36]

On 2014/05/10 10:12:30, mcasas wrote:
> On 2014/05/09 22:04:51, Zachary Kuznia wrote:
> > On 2014/05/09 10:32:16, mcasas wrote:
> > > I don't fully agree with the approach here based on the fact that we'd be
> > > extending the Capture Device, whose tasks are opening/closing the device
and
> > > pushing frames, to listen to some external entity mostly  unrelated.
> (Android,
> > > truly, grabs the orientation from the system, but that's also bad). 
> > > 
> > > Plus, I don't feel entirely sure that media/video/capture depending on UI
is
> a
> > > good thing (I know it works now due to VideoCaptureFormat pulling
> > gfx::Size...).
> > > 
> > > Finally I'm not entirely sure if this would work as intended with the
> > Chromebook
> > > listed as use-case in the bug: would it be OK to rotate the video feed if
> > using
> > > an external Webcam?
> > > 
> > > My proposal could be to:
> > > a) extend the VCDeviceLinux and create a specific one for the internal
> webcam,
> > > which would do the same thing as VCDL plus the observation.
> > > b) Create your special device driver in the Factory method (that soon will
> be
> > > Factory class method) [1].
> > > 
> > > Makes sense?
> > > 
> > > 
> > > [1]
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
> > > 
> > >
> >
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> > > File media/video/capture/linux/video_capture_device_linux.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> > > media/video/capture/linux/video_capture_device_linux.h:57: // Once
> > v4l2_thread_
> > > is started, only called on that thread.
> > > |v4l2_thread_|
> > 
> > I've updated it to use this design, as you suggest.  Limiting this behavior
to
> > the internal webcam will be handled in a future patch, as we discussed
> offline.
> 
> Looks much better. Suggestion: you could move the |rotation_|, 
> SetRotation(int rotation) etc out of the VideoCaptureDeviceLinux
> and into VideoCaptureDeviceChromeOS, since the latter would be
> the only used for CrOS, right? 
> 
> Another tip: the "media thread" is usually called the "Device Thread".
> 
> Then this CL would be just about adding the VCDChromeOS and
> the modifications to the VideoCaptureDevice::Create().

The capture device needs to know about the rotation when it calls into the
client.  To keep this info in a subclass, I'd need to rearrange OnCaptureTask to
call into a virtual function.  Putting rotation_ in VCDLinux seems like a much
smaller and cleaner change to me.

[mcasas, 2014-05-11 17:50:33]

On 2014/05/10 13:58:36, Zachary Kuznia wrote:
> On 2014/05/10 10:12:30, mcasas wrote:
> > On 2014/05/09 22:04:51, Zachary Kuznia wrote:
> > > On 2014/05/09 10:32:16, mcasas wrote:
> > > > I don't fully agree with the approach here based on the fact that we'd
be
> > > > extending the Capture Device, whose tasks are opening/closing the device
> and
> > > > pushing frames, to listen to some external entity mostly  unrelated.
> > (Android,
> > > > truly, grabs the orientation from the system, but that's also bad). 
> > > > 
> > > > Plus, I don't feel entirely sure that media/video/capture depending on
UI
> is
> > a
> > > > good thing (I know it works now due to VideoCaptureFormat pulling
> > > gfx::Size...).
> > > > 
> > > > Finally I'm not entirely sure if this would work as intended with the
> > > Chromebook
> > > > listed as use-case in the bug: would it be OK to rotate the video feed
if
> > > using
> > > > an external Webcam?
> > > > 
> > > > My proposal could be to:
> > > > a) extend the VCDeviceLinux and create a specific one for the internal
> > webcam,
> > > > which would do the same thing as VCDL plus the observation.
> > > > b) Create your special device driver in the Factory method (that soon
will
> > be
> > > > Factory class method) [1].
> > > > 
> > > > Makes sense?
> > > > 
> > > > 
> > > > [1]
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> > > > File media/video/capture/linux/video_capture_device_linux.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> > > > media/video/capture/linux/video_capture_device_linux.h:57: // Once
> > > v4l2_thread_
> > > > is started, only called on that thread.
> > > > |v4l2_thread_|
> > > 
> > > I've updated it to use this design, as you suggest.  Limiting this
behavior
> to
> > > the internal webcam will be handled in a future patch, as we discussed
> > offline.
> > 
> > Looks much better. Suggestion: you could move the |rotation_|, 
> > SetRotation(int rotation) etc out of the VideoCaptureDeviceLinux
> > and into VideoCaptureDeviceChromeOS, since the latter would be
> > the only used for CrOS, right? 
> > 
> > Another tip: the "media thread" is usually called the "Device Thread".
> > 
> > Then this CL would be just about adding the VCDChromeOS and
> > the modifications to the VideoCaptureDevice::Create().
> 
> The capture device needs to know about the rotation when it calls into the
> client.  To keep this info in a subclass, I'd need to rearrange OnCaptureTask
to
> call into a virtual function.  Putting rotation_ in VCDLinux seems like a much
> smaller and cleaner change to me.

That's all right. The mutators SetRotation() and 
SetRotationInV4L2Thread(), can they belong to VCDChromeOS?

[mcasas, 2014-05-11 17:56:25]

On 2014/05/11 17:50:33, mcasas wrote:
> On 2014/05/10 13:58:36, Zachary Kuznia wrote:
> > On 2014/05/10 10:12:30, mcasas wrote:
> > > On 2014/05/09 22:04:51, Zachary Kuznia wrote:
> > > > On 2014/05/09 10:32:16, mcasas wrote:
> > > > > I don't fully agree with the approach here based on the fact that we'd
> be
> > > > > extending the Capture Device, whose tasks are opening/closing the
device
> > and
> > > > > pushing frames, to listen to some external entity mostly  unrelated.
> > > (Android,
> > > > > truly, grabs the orientation from the system, but that's also bad). 
> > > > > 
> > > > > Plus, I don't feel entirely sure that media/video/capture depending on
> UI
> > is
> > > a
> > > > > good thing (I know it works now due to VideoCaptureFormat pulling
> > > > gfx::Size...).
> > > > > 
> > > > > Finally I'm not entirely sure if this would work as intended with the
> > > > Chromebook
> > > > > listed as use-case in the bug: would it be OK to rotate the video feed
> if
> > > > using
> > > > > an external Webcam?
> > > > > 
> > > > > My proposal could be to:
> > > > > a) extend the VCDeviceLinux and create a specific one for the internal
> > > webcam,
> > > > > which would do the same thing as VCDL plus the observation.
> > > > > b) Create your special device driver in the Factory method (that soon
> will
> > > be
> > > > > Factory class method) [1].
> > > > > 
> > > > > Makes sense?
> > > > > 
> > > > > 
> > > > > [1]
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> > > > > File media/video/capture/linux/video_capture_device_linux.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/270263008/diff/110001/media/video/capture/lin...
> > > > > media/video/capture/linux/video_capture_device_linux.h:57: // Once
> > > > v4l2_thread_
> > > > > is started, only called on that thread.
> > > > > |v4l2_thread_|
> > > > 
> > > > I've updated it to use this design, as you suggest.  Limiting this
> behavior
> > to
> > > > the internal webcam will be handled in a future patch, as we discussed
> > > offline.
> > > 
> > > Looks much better. Suggestion: you could move the |rotation_|, 
> > > SetRotation(int rotation) etc out of the VideoCaptureDeviceLinux
> > > and into VideoCaptureDeviceChromeOS, since the latter would be
> > > the only used for CrOS, right? 
> > > 
> > > Another tip: the "media thread" is usually called the "Device Thread".
> > > 
> > > Then this CL would be just about adding the VCDChromeOS and
> > > the modifications to the VideoCaptureDevice::Create().
> > 
> > The capture device needs to know about the rotation when it calls into the
> > client.  To keep this info in a subclass, I'd need to rearrange
OnCaptureTask
> to
> > call into a virtual function.  Putting rotation_ in VCDLinux seems like a
much
> > smaller and cleaner change to me.
> 
> That's all right. The mutators SetRotation() and 
> SetRotationInV4L2Thread(), can they belong to VCDChromeOS?

Sorry, I didn't mean the last comment: SetRotation() and
SetRotationInV4L2Thread() are in the right place :)
in the right

[mcasas, 2014-05-11 18:41:24]

Almost there, looking good.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:40: friend class
base::RefCountedThreadSafe<ScreenObserverDelegate>;
According to base/memory/ref_counted.h, if you use the 
friend idiom the destructor should be private. Does it 
apply here?

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:48: virtual void
OnDisplayRemoved(const gfx::Display& /*old_display*/) OVERRIDE {}
Shouldn't these 3 methods from gfx::DisplayObserver be public?

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:52:
gfx::Screen::GetScreenByType(gfx::SCREEN_TYPE_ALTERNATE);
Why not gfx::SCREEN_TYPE_NATIVE?

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:77:
VideoCaptureDeviceChromeOS* capture_device_;
Ugh naked pointer. Maybe const?

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:78:
base::MessageLoop* media_message_loop_;
const?

ultra-nit: would you mind terribly calling it |capture_thread_loop_|?
And changing "Media" -> "Capture".

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:89: 
nit: no empty line.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.h (right):

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.h:9: #include
"ui/gfx/display_observer.h"
Shouldn't you include be "ui/gfx/display.h" here 
and "ui/gfx/display_observer.h" in 
video_capture_device_chromeos.cc ?

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:339:
v4l2_thread_.message_loop() == base::MessageLoop::current());
The method would not be executed in |v4l2_thread_| if it is not running (i.e.
the capture is not active), but the method name says "OnV4L2Thread". Why not
assign directly |rotation_| in l.333 and write a comment there? Then here you
can 
DCHECK(v4l2_thread_.IsRunning() &&
       v4l2_thread_.message_loop() == base::MessageLoop::current());

which is more "expected".

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.h (right):

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.h:76: int rotation_;  //
Clockwise rotation.
Comment that |rotation_| is used in |v4l2_thread_|
when this is active or in the constructor thread
(which is the IO thread) otherwise?

[Zachary Kuznia, 2014-05-12 02:54:17]

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:40: friend class
base::RefCountedThreadSafe<ScreenObserverDelegate>;
On 2014/05/11 18:41:25, mcasas wrote:
> According to base/memory/ref_counted.h, if you use the 
> friend idiom the destructor should be private. Does it 
> apply here?

Done.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:48: virtual void
OnDisplayRemoved(const gfx::Display& /*old_display*/) OVERRIDE {}
On 2014/05/11 18:41:25, mcasas wrote:
> Shouldn't these 3 methods from gfx::DisplayObserver be public?

This is a controversial topic.  See:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/CwzjcWd9cYM

I prefer keeping them private, personally, but I'm more than happy to follow
whichever convention is preferred by you.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:52:
gfx::Screen::GetScreenByType(gfx::SCREEN_TYPE_ALTERNATE);
On 2014/05/11 18:41:25, mcasas wrote:
> Why not gfx::SCREEN_TYPE_NATIVE?

Because this is the pattern established in ash/shell.cc.  They're only different
on Windows, so either would work.  As such, following ash seems best.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:77:
VideoCaptureDeviceChromeOS* capture_device_;
On 2014/05/11 18:41:25, mcasas wrote:
> Ugh naked pointer. Maybe const?

Unfortunately, that would require marking rotation_ as mutable in
VideoCaptureDeviceLinux.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:78:
base::MessageLoop* media_message_loop_;
On 2014/05/11 18:41:25, mcasas wrote:
> const?
> 
> ultra-nit: would you mind terribly calling it |capture_thread_loop_|?
> And changing "Media" -> "Capture".

Nits Done.  The MessageLoop pointer can't be const, as PostTask is not const.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:89: 
On 2014/05/11 18:41:25, mcasas wrote:
> nit: no empty line.

Done.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.h (right):

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.h:9: #include
"ui/gfx/display_observer.h"
On 2014/05/11 18:41:25, mcasas wrote:
> Shouldn't you include be "ui/gfx/display.h" here 
> and "ui/gfx/display_observer.h" in 
> video_capture_device_chromeos.cc ?

Actually, I can just forward declare Display.  Done.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:339:
v4l2_thread_.message_loop() == base::MessageLoop::current());
On 2014/05/11 18:41:25, mcasas wrote:
> The method would not be executed in |v4l2_thread_| if it is not running (i.e.
> the capture is not active), but the method name says "OnV4L2Thread". Why not
> assign directly |rotation_| in l.333 and write a comment there? Then here you
> can 
> DCHECK(v4l2_thread_.IsRunning() &&
>        v4l2_thread_.message_loop() == base::MessageLoop::current());
> 
> which is more "expected".

Done.

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.h (right):

https://codereview.chromium.org/270263008/diff/150001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.h:76: int rotation_;  //
Clockwise rotation.
On 2014/05/11 18:41:25, mcasas wrote:
> Comment that |rotation_| is used in |v4l2_thread_|
> when this is active or in the constructor thread
> (which is the IO thread) otherwise?

Done.

[mcasas, 2014-05-12 06:29:38]

LGTM pending either perkj@ or tommi@ review.

[mcasas, 2014-05-12 07:42:43]

On 2014/05/12 06:29:38, mcasas wrote:
> LGTM pending either perkj@ or tommi@ review.

nit: please update the description.

[perkj_chrome, 2014-05-12 07:57:12]

lgtm provides the below comments are addressed.

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:29: void
RemoveObserver() {
add thread check

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:75:
capture_device_->SetDisplayRotation(display);
add thread check

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:79:
base::MessageLoop* capture_thread_loop_;
scoped_refptr<MessageLoopProxy> capture_thread_loop_;

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:93: const
gfx::Display& display) {
nit: Fits on line above?

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.h (right):

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.h:16: class
VideoCaptureDeviceChromeOS : public VideoCaptureDeviceLinux {
Please add a short description what this class is and does. Whats the difference
to VideoCaptureDeviceLinux.

[Zachary Kuznia, 2014-05-12 08:56:21]

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:29: void
RemoveObserver() {
On 2014/05/12 07:57:13, perkj wrote:
> add thread check

Done.

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:75:
capture_device_->SetDisplayRotation(display);
On 2014/05/12 07:57:13, perkj wrote:
> add thread check

Done.

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:79:
base::MessageLoop* capture_thread_loop_;
On 2014/05/12 07:57:13, perkj wrote:
> scoped_refptr<MessageLoopProxy> capture_thread_loop_;

Unfortunately, MessageLoopProxy is not threadsafe, and can only be used to post
to the current thread.

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:93: const
gfx::Display& display) {
On 2014/05/12 07:57:13, perkj wrote:
> nit: Fits on line above?

2 characters too long.

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.h (right):

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.h:16: class
VideoCaptureDeviceChromeOS : public VideoCaptureDeviceLinux {
On 2014/05/12 07:57:13, perkj wrote:
> Please add a short description what this class is and does. Whats the
difference
> to VideoCaptureDeviceLinux.

Done.

[perkj_chrome, 2014-05-13 09:46:37]

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/270263008/diff/170001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:79:
base::MessageLoop* capture_thread_loop_;
On 2014/05/12 08:56:22, Zachary Kuznia wrote:
> On 2014/05/12 07:57:13, perkj wrote:
> > scoped_refptr<MessageLoopProxy> capture_thread_loop_;
> 
> Unfortunately, MessageLoopProxy is not threadsafe, and can only be used to
post
> to the current thread.

but if you set it in ctor and and only use it on the UI thread you are safe
right.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:25: FROM_HERE,
nit: indentation

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:49: virtual void
OnDisplayAdded(const gfx::Display& /*new_display*/) OVERRIDE {}
style nit - name all arguments even if they are not used.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:69:
capture_thread_loop_->PostTask(
Can you add thread check here as well.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:70: FROM_HERE,
nit: intendation

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:79: 
Can you add a threadchecker and verify that you are on the correct thread.

[tommi (sloooow) - chröme, 2014-05-13 11:02:50]

looks good for the most part. Some questions and minor comments.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:52: void
AddObserverOnUIThread() {
nit: add a dcheck that we're indeed on the ui thread.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:62: gfx::Screen*
screen =
dcheck

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:81:
base::MessageLoop* capture_thread_loop_;
if you use this variable on a thread other than the capture thread, then you
should be using MessageLoopProxy.  Maybe this is fine but it's not clear to me
from the code as is.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:261: if (!self)
nit: we don't check the return value from new()

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:341:
v4l2_thread_.message_loop() == base::MessageLoop::current());
BelongsToCurrentThread()?

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:342: rotation_ =
rotation;
DCHECK valid values of rotation?

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.h (right):

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.h:79: int rotation_;
what are valid values? is it degrees of rotation or an enum?

[Zachary Kuznia, 2014-05-13 13:54:28]

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:25: FROM_HERE,
On 2014/05/13 09:46:40, perkj wrote:
> nit: indentation

Done.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:49: virtual void
OnDisplayAdded(const gfx::Display& /*new_display*/) OVERRIDE {}
On 2014/05/13 09:46:40, perkj wrote:
> style nit - name all arguments even if they are not used.

See:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:52: void
AddObserverOnUIThread() {
On 2014/05/13 11:02:51, tommi wrote:
> nit: add a dcheck that we're indeed on the ui thread.

Done.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:62: gfx::Screen*
screen =
On 2014/05/13 11:02:51, tommi wrote:
> dcheck

Done.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:69:
capture_thread_loop_->PostTask(
On 2014/05/13 09:46:40, perkj wrote:
> Can you add thread check here as well. 

Done.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:70: FROM_HERE,
On 2014/05/13 09:46:40, perkj wrote:
> nit: intendation

Done.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:79: 
On 2014/05/13 09:46:40, perkj wrote:
> Can you add a threadchecker and verify that you are on the correct thread.

I don't think a threadchecker would be useful, because the constructor and
destructor are called on different threads.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:81:
base::MessageLoop* capture_thread_loop_;
On 2014/05/13 11:02:51, tommi wrote:
> if you use this variable on a thread other than the capture thread, then you
> should be using MessageLoopProxy.  Maybe this is fine but it's not clear to me
> from the code as is.

I actually used a MessageLoopProxy initially, but there is a presubmit error
that asserts:
"MessageLoopProxy should only be used for accessing the current loop."

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:261: if (!self)
On 2014/05/13 11:02:51, tommi wrote:
> nit: we don't check the return value from new()

Done.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:341:
v4l2_thread_.message_loop() == base::MessageLoop::current());
On 2014/05/13 11:02:51, tommi wrote:
> BelongsToCurrentThread()?

Done.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:342: rotation_ =
rotation;
On 2014/05/13 11:02:51, tommi wrote:
> DCHECK valid values of rotation?

Done.

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.h (right):

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.h:79: int rotation_;
On 2014/05/13 11:02:51, tommi wrote:
> what are valid values? is it degrees of rotation or an enum?

Done.

[tommi (sloooow) - chröme, 2014-05-13 14:35:16]

lgtm with the below change

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/270263008/diff/190001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_chromeos.cc:81:
base::MessageLoop* capture_thread_loop_;
On 2014/05/13 13:54:29, Zachary Kuznia wrote:
> On 2014/05/13 11:02:51, tommi wrote:
> > if you use this variable on a thread other than the capture thread, then you
> > should be using MessageLoopProxy.  Maybe this is fine but it's not clear to
me
> > from the code as is.
> 
> I actually used a MessageLoopProxy initially, but there is a presubmit error
> that asserts:
> "MessageLoopProxy should only be used for accessing the current loop."

Ah, I'm guessing this is the error you ran into:
https://code.google.com/p/chromium/codesearch#chromium/src/media/PRESUBMIT.py...

If so, then I think scoped_refptr<base::SingleThreadTaskRunner> is what you
want.  Sounds like you were going down the right path.  Where you call
MessageLoop::current() above, you should still call MessageLoopProxy::current().
 The only difference is the type of the variable that holds the pointer.

[Zachary Kuznia, 2014-05-14 18:24:14]

Hi John,

I've updated the CL to inject the UI message loop from content as you suggested.
 Could you take a look?

Thanks,
-Zach

[jam, 2014-05-14 23:54:38]

content lgtm

[Zachary Kuznia, 2014-05-18 02:43:13]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-05-18 02:43:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zork@chromium.org/270263008/310001

[commit-bot: I haz the power, 2014-05-18 02:44:00]

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

[commit-bot: I haz the power, 2014-05-18 02:44:01]

A disapproval has been posted by following reviewers: vrk@chromium.org.
Please make sure to get positive review before another CQ attempt.

[Zachary Kuznia, 2014-05-19 14:06:41]

vrk@:  Could you take another look at this?

Thanks,
-Zach

[vrk (LEFT CHROMIUM), 2014-05-19 20:28:20]

lgtm

Sorry about that!

[Zachary Kuznia, 2014-05-19 20:56:25]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-05-19 20:57:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zork@chromium.org/270263008/330001

[commit-bot: I haz the power, 2014-05-20 01:08:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-20 04:06:56]

Change committed as 271560