Show camera and microphone status indicators.

Show camera and microphone status indicators.
Some cameras do not have a visual indicator light. Some implementations of this visual indicator light can be programatically bypassed. Therefore, we are planning to add an on screen visual indicator that mimics the Speech Recognition implementation.
Chrome-UI-Leads have given us a red LED type icon to add to the windows / mac / linux system trays whenever getUserMedia is used.

BUG=120126, 123767
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135198

[no longer working on chromium, 2012-04-24 10:14:43]

please take a look.

[tommi (sloooow) - chröme, 2012-04-24 11:36:17]

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:27: return;
uhm... this doesn't make any sense :)

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:64: break;
NOTREACHED()? or at least DLOG(ERROR)?

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:72: DCHECK(!url.empty()
&& !devices.empty());
nit: separate DCHECKs so that it's clear which one failed.

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:92: DCHECK(!url.empty()
&& !devices.empty());
separate dchecks

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:118:
DCHECK(status_icon_);
this DCHECK isn't necessary since you deref the pointer anyway  on the next
line.

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.h (right):

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:28: int command_id,
ui::Accelerator* accelerator) OVERRIDE;
nit: use a consistent way of wrapping (see e.g. below)

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:40: struct
CaptureDeviceUser {
Please add some documentation.  It's not immediately obvious what this struct is
for (and there's no 'user' information being stored).

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/status_icon...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

http://codereview.chromium.org/10168008/diff/15001/chrome/browser/status_icon...
chrome/browser/status_icons/desktop_notification_balloon.cc:22: // The bwowser
process may have gone away during shutting down, in this case
s/bwowser/browser

[no longer working on chromium, 2012-04-25 13:52:50]

I addressed Tommi's first round's comments.

Mad and Nico, could you please review this CL?
Thanks a lot.

BR,
/SX

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/med...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:27: return;
On 2012/04/24 11:36:18, tommi wrote:
> uhm... this doesn't make any sense :)

Done.

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:64: break;
On 2012/04/24 11:36:18, tommi wrote:
> NOTREACHED()? or at least DLOG(ERROR)?

Done.

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:72: DCHECK(!url.empty()
&& !devices.empty());
On 2012/04/24 11:36:18, tommi wrote:
> nit: separate DCHECKs so that it's clear which one failed.

Done.

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:92: DCHECK(!url.empty()
&& !devices.empty());
On 2012/04/24 11:36:18, tommi wrote:
> separate dchecks

Done.

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:118:
DCHECK(status_icon_);
On 2012/04/24 11:36:18, tommi wrote:
> this DCHECK isn't necessary since you deref the pointer anyway  on the next
> line.

Done.

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/med...
File chrome/browser/media/media_stream_capture_indicator.h (right):

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.h:28: int command_id,
ui::Accelerator* accelerator) OVERRIDE;
On 2012/04/24 11:36:18, tommi wrote:
> nit: use a consistent way of wrapping (see e.g. below)

Done.

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.h:40: struct
CaptureDeviceUser {
On 2012/04/24 11:36:18, tommi wrote:
> Please add some documentation.  It's not immediately obvious what this struct
is
> for (and there's no 'user' information being stored).

Done.

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/sta...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

https://chromiumcodereview.appspot.com/10168008/diff/15001/chrome/browser/sta...
chrome/browser/status_icons/desktop_notification_balloon.cc:22: // The bwowser
process may have gone away during shutting down, in this case
On 2012/04/24 11:36:18, tommi wrote:
> s/bwowser/browser

Done.

[MAD, 2012-04-25 15:34:53]

Here are my comments...
Let me know if some are not clear...

BYE
MAD

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/app/generat...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/app/generat...
chrome/app/generated_resources.grd:15766: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_TITLE" desc="Title of the capture
notification status tray shown Media Stream when using capture device">
Is there a word missing here? I don't think the phrasing is correct... " tray
shown Media Stream when "

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/app/generat...
chrome/app/generated_resources.grd:15769: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_TITLE" desc="Title of the balloon
shown by the tray notification icon evert time media stream starts capturing
using a camera and/or a microphone">
evert -> every

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/app/generat...
chrome/app/generated_resources.grd:15772: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_BODY" desc="Contents of the balloon
shown by the tray notification icon evert time media stream starts captureing
audio using a microphone">
evert -> every

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/app/generat...
chrome/app/generated_resources.grd:15772: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_BODY" desc="Contents of the balloon
shown by the tray notification icon evert time media stream starts captureing
audio using a microphone">
captureing -> capturing

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/app/generat...
chrome/app/generated_resources.grd:15772: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_BODY" desc="Contents of the balloon
shown by the tray notification icon evert time media stream starts captureing
audio using a microphone">
Only audio mic? The title says cam/mic... Feels weird...

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/bro...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/bro...
chrome/browser/browser_process_impl.cc:562:
BrowserProcessImpl::media_stream_capture_indicator() {
The coding style wants the name of the function on the same line as the return
type, so I would prefer this format:
MediaStreamCaptureIndicator* BrowserProcessImpl::media_stream_capture_indicator(
    ) {

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:105:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Nit: You already DCHECKed on that in the caller method... Just as you don't need
to check g_browser_process again, I don't think you need to DCHECK again...

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:111: LOG(WARNING) <<
"This platform doesn't support notification icons";
VLOG maybe? Or do you really want this LOG to always show up?

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:136:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Nit: Again, not needed...

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:143: string16 message=
l10n_util::GetStringFUTF16(
message= -> message =

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:193: // The icon will
take the ownership of the passed context menu.
I would still use a scoped_ptr<> in case a return statement eventually gets
added within this function... You can simply call .release() to transfer the
ownership here.

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:200: dev !=
devices.end(); ++dev) {
Nit: consistency, above you align to 4 columns, and here to the (, 

One way I like to use to avoid for loops on multiple line is to declare the
variable outside the loop. Like as follows:

content::MediaStreamDevices::const_iterator dev = devices.begin();
for (; dev != devices.end(); ++dev) {

or


content::MediaStreamDevices::const_iterator dev;
for (dev = devices.begin(); dev != devices.end(); ++dev) {

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:231: if (!users_.empty())
Don't you want to clear the context menu when users_ is empty?

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
File chrome/browser/media/media_stream_capture_indicator.h (right):

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.h:21: explicit
MediaStreamCaptureIndicator();
I think you only need explicit when there is a single argument.

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.h:29: OVERRIDE;
OVERRIDE should be indented 4 columns... Or you could change the previous lines
to have this:

virtual bool GetAcceleratorForCommandId(
    int command_id, ui::Accelerator* accelerator) OVERRIDE;

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.h:44: const std::string
device,
Why not a & reference here?

[Nico, 2012-04-25 15:52:51]

I only skimmed. Looks like a good CL, so I think with MAD's comments addressed
this will look good to me :-)

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/bro...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/bro...
chrome/browser/browser_process_impl.cc:562:
BrowserProcessImpl::media_stream_capture_indicator() {
On 2012/04/25 15:34:53, MAD wrote:
> The coding style wants the name of the function on the same line as the return
> type, so I would prefer this format:
> MediaStreamCaptureIndicator*
BrowserProcessImpl::media_stream_capture_indicator(
>     ) {

line 584 (20 lines below) has

returntype
  BrowserProcessImpl::method_name() {

so I'd be consistent with that. Just indent the method name 4.

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.cc:111: LOG(WARNING) <<
"This platform doesn't support notification icons";
On 2012/04/25 15:34:53, MAD wrote:
> VLOG maybe? Or do you really want this LOG to always show up?

No log needed at all

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
File chrome/browser/media/media_stream_capture_indicator.h (right):

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/med...
chrome/browser/media/media_stream_capture_indicator.h:18: 
class comment that explains why this exists / what it's used for

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/sta...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

https://chromiumcodereview.appspot.com/10168008/diff/21006/chrome/browser/sta...
chrome/browser/status_icons/desktop_notification_balloon.cc:22: // The browser
process may have gone away during shutting down, in this case
nit: "during shutdown"

[no longer working on chromium, 2012-04-30 09:59:38]

Thanks Mad and Nico.

I think I have addressed all the comments from Mad and Nico, please point it out
in case I miss any of them.

Also I merged this CL with mflodman's CL 10161024, which connected the UI with
MediaStreamManager. John suggested that we should use the MediaObserver to
connect the UI with the MediaStreamManager, so that we can keep
ContentBrowserClient small and reuse the media interface.

Tommi, Nico and John, could you please take another round.

BR,
/SX

http://codereview.chromium.org/10168008/diff/21006/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10168008/diff/21006/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15766: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_TITLE" desc="Title of the capture
notification status tray shown Media Stream when using capture device">
On 2012/04/25 15:34:53, MAD wrote:
> Is there a word missing here? I don't think the phrasing is correct... " tray
> shown Media Stream when "

Sorry, it was just some prototype text for testing and demo, I was waiting for
the official version which will be used for the production.

Done now.

http://codereview.chromium.org/10168008/diff/21006/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15769: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_TITLE" desc="Title of the balloon
shown by the tray notification icon evert time media stream starts capturing
using a camera and/or a microphone">
On 2012/04/25 15:34:53, MAD wrote:
> evert -> every

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15772: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_BODY" desc="Contents of the balloon
shown by the tray notification icon evert time media stream starts captureing
audio using a microphone">
On 2012/04/25 15:34:53, MAD wrote:
> evert -> every

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15772: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_BODY" desc="Contents of the balloon
shown by the tray notification icon evert time media stream starts captureing
audio using a microphone">
On 2012/04/25 15:34:53, MAD wrote:
> captureing -> capturing

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15772: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_BODY" desc="Contents of the balloon
shown by the tray notification icon evert time media stream starts captureing
audio using a microphone">
On 2012/04/25 15:34:53, MAD wrote:
> Only audio mic? The title says cam/mic... Feels weird...

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:562:
BrowserProcessImpl::media_stream_capture_indicator() {
On 2012/04/25 15:34:53, MAD wrote:
> The coding style wants the name of the function on the same line as the return
> type, so I would prefer this format:
> MediaStreamCaptureIndicator*
BrowserProcessImpl::media_stream_capture_indicator(
>     ) {

Thanks Nico for addressing this, it looks like
returntype
  BrowserProcessImpl::method_name() {
is pretty common in the file, so I stick with the style.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:562:
BrowserProcessImpl::media_stream_capture_indicator() {
On 2012/04/25 15:52:51, Nico wrote:
> On 2012/04/25 15:34:53, MAD wrote:
> > The coding style wants the name of the function on the same line as the
return
> > type, so I would prefer this format:
> > MediaStreamCaptureIndicator*
> BrowserProcessImpl::media_stream_capture_indicator(
> >     ) {
> 
> line 584 (20 lines below) has
> 
> returntype
>   BrowserProcessImpl::method_name() {
> 
> so I'd be consistent with that. Just indent the method name 4.

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:105:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/04/25 15:34:53, MAD wrote:
> Nit: You already DCHECKed on that in the caller method... Just as you don't
need
> to check g_browser_process again, I don't think you need to DCHECK again...

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:111: LOG(WARNING) <<
"This platform doesn't support notification icons";
On 2012/04/25 15:34:53, MAD wrote:
> VLOG maybe? Or do you really want this LOG to always show up?

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:111: LOG(WARNING) <<
"This platform doesn't support notification icons";
On 2012/04/25 15:52:51, Nico wrote:
> On 2012/04/25 15:34:53, MAD wrote:
> > VLOG maybe? Or do you really want this LOG to always show up?
> 
> No log needed at all

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:136:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/04/25 15:34:53, MAD wrote:
> Nit: Again, not needed...

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:143: string16 message=
l10n_util::GetStringFUTF16(
On 2012/04/25 15:34:53, MAD wrote:
> message= -> message =

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:193: // The icon will
take the ownership of the passed context menu.
On 2012/04/25 15:34:53, MAD wrote:
> I would still use a scoped_ptr<> in case a return statement eventually gets
> added within this function... You can simply call .release() to transfer the
> ownership here.

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:200: dev !=
devices.end(); ++dev) {
On 2012/04/25 15:34:53, MAD wrote:
> Nit: consistency, above you align to 4 columns, and here to the (, 
> 
> One way I like to use to avoid for loops on multiple line is to declare the
> variable outside the loop. Like as follows:
> 
> content::MediaStreamDevices::const_iterator dev = devices.begin();
> for (; dev != devices.end(); ++dev) {
> 
> or
> 
> 
> content::MediaStreamDevices::const_iterator dev;
> for (dev = devices.begin(); dev != devices.end(); ++dev) {

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:231: if (!users_.empty())
On 2012/04/25 15:34:53, MAD wrote:
> Don't you want to clear the context menu when users_ is empty?

If users_ is empty, we will call Hide() to destroy the status stay, which will
clear the context menu at the same time. This is done outside
RemoveCaptureDeviceUser().

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.h (right):

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:18: 
On 2012/04/25 15:52:51, Nico wrote:
> class comment that explains why this exists / what it's used for

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:21: explicit
MediaStreamCaptureIndicator();
On 2012/04/25 15:34:53, MAD wrote:
> I think you only need explicit when there is a single argument.

Done.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:29: OVERRIDE;
On 2012/04/25 15:34:53, MAD wrote:
> OVERRIDE should be indented 4 columns... Or you could change the previous
lines
> to have this:
> 
> virtual bool GetAcceleratorForCommandId(
>     int command_id, ui::Accelerator* accelerator) OVERRIDE;

hope this is fine.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:44: const std::string
device,
On 2012/04/25 15:34:53, MAD wrote:
> Why not a & reference here?

a mistake, thanks.

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/status_icon...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

http://codereview.chromium.org/10168008/diff/21006/chrome/browser/status_icon...
chrome/browser/status_icons/desktop_notification_balloon.cc:22: // The browser
process may have gone away during shutting down, in this case
On 2012/04/25 15:52:51, Nico wrote:
> nit: "during shutdown"

Done.

[no longer working on chromium, 2012-04-30 10:08:07]

Added John to the reviewer list.

Hi John,

This CL is a follow-up to mflodman's CL 10161024. Since mflodman is on vocation
this week so I merged with this CL i order to get the feature in for M20. I have
already changed the code to use MediaObserver interface in favor of keeping
ContentBrowserClient small.

Could you please take a look at it?
Thanks,

BR,
/SX

[tommi (sloooow) - chröme, 2012-04-30 10:57:07]

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15757: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_ITEM" desc="URL to display in the MediaStrem
tray list when it is using the capture devices">
s/MediaStrem/MediaStream.

Are translators supposed to know what "MediaStream tray list" is?

Also, I don't think it makes sense to display a URL.
Instead I think this should be the title of the tab that is currently using the
capture device.

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15760: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_TOOLTIP" desc="Tool tip for the capture
notification status tray shown by the MediaStream">
again, I don't think "MediaStream" is known concept and not something
translators need to be aware of.  Same below.

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15763: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_TITLE" desc="Title of the capture
notification status tray shown by MediaStream">
what's the difference between this and the tool tip?
If this is in the menu, then I don't think that's a good idea since users expect
to be able to click on anything that's in a menu (menu items are always
interactive unless they're grayed out).

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:159: if (status_icon_)
DCHECK(...on UI thread ...)

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:200: if (!status_icon_)
missing thread dcheck since this method accesses variables that don't have locks
and the class is used by multiple threads.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:217:
l10n_util::GetStringUTF16(IDS_MEDIA_STREAM_STATUS_TRAY_TITLE));
I don't think a menu should have a title.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:220: for
(CaptureDeviceUserList::iterator iter = users_.begin();
missing thread dcheck for users_

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:231: int command_id =
IDC_MEDIA_CONTEXT_MEDIA_STREAM_CAPTURE_LIST;
nit: no need for these two temporaries

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:248:
content::MediaStreamDevices::const_iterator dev = devices.begin();
missing thread dcheck in this function

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:252:
users_.push_back(CaptureDeviceUser(render_process_id,
do we need to first check if we've already got this render view in the users_
list?

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:269: users_.begin(),
users_.end(), UserEquals(render_process_id,
missing thread dcheck for users_

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.h (right):

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:50: struct
CaptureDeviceUser {
Should this struct also include usage count?

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:62: std::string url;
do we really care about the URL (render_view_id should be enough)?  Should this
perhaps be title?

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:71: const std::string&
url, content::MediaStreamDeviceType type);
is it enough to compare only the render_view_id?

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:135: typedef
std::list<CaptureDeviceUser> CaptureDeviceUserList;
What about having the 'users' list a map between render views and usage info?
Also, I would like to not use the word "user" here because this doesn't have
anything to do with the user.

std::map<int, ActiveCaptureDevices>

http://codereview.chromium.org/10168008/diff/39003/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10168008/diff/39003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:613: ++it) {
fix indent

[no longer working on chromium, 2012-04-30 13:12:54]

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15757: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_ITEM" desc="URL to display in the MediaStrem
tray list when it is using the capture devices">
On 2012/04/30 10:57:07, tommi wrote:
> s/MediaStrem/MediaStream.
> 
> Are translators supposed to know what "MediaStream tray list" is?
> 
> Also, I don't think it makes sense to display a URL.
> Instead I think this should be the title of the tab that is currently using
the
> capture device.

It is just description of the variable, it should not be translated. And our
infobar also uses the word MediaStream.

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15760: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_TOOLTIP" desc="Tool tip for the capture
notification status tray shown by the MediaStream">
On 2012/04/30 10:57:07, tommi wrote:
> again, I don't think "MediaStream" is known concept and not something
> translators need to be aware of.  Same below.

See answers in my previous comment.

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15760: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_TOOLTIP" desc="Tool tip for the capture
notification status tray shown by the MediaStream">
On 2012/04/30 10:57:07, tommi wrote:
> again, I don't think "MediaStream" is known concept and not something
> translators need to be aware of.  Same below.

See answers in my previous comment.

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15763: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_TITLE" desc="Title of the capture
notification status tray shown by MediaStream">
On 2012/04/30 10:57:07, tommi wrote:
> what's the difference between this and the tool tip?
> If this is in the menu, then I don't think that's a good idea since users
expect
> to be able to click on anything that's in a menu (menu items are always
> interactive unless they're grayed out).

In my discussion with Serge, he seemed to be favor of showing this text, we can
double check with him on this.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:159: if (status_icon_)
On 2012/04/30 10:57:07, tommi wrote:
> DCHECK(...on UI thread ...)

From Mad: Nit: You already DCHECKed on that in the caller method... Just as you
don't need
to check g_browser_process again, I don't think you need to DCHECK again...

But I am open to any idea, let me know if you strongly think I should add back
the thread dcheck.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:200: if (!status_icon_)
On 2012/04/30 10:57:07, tommi wrote:
> missing thread dcheck since this method accesses variables that don't have
locks
> and the class is used by multiple threads.

See my answers above.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:217:
l10n_util::GetStringUTF16(IDS_MEDIA_STREAM_STATUS_TRAY_TITLE));
On 2012/04/30 10:57:07, tommi wrote:
> I don't think a menu should have a title.

Done, I changed it to IDS_MEDIA_STREAM_STATUS_TRAY_TEXT.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:220: for
(CaptureDeviceUserList::iterator iter = users_.begin();
On 2012/04/30 10:57:07, tommi wrote:
> missing thread dcheck for users_

See my answer in other comments.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:231: int command_id =
IDC_MEDIA_CONTEXT_MEDIA_STREAM_CAPTURE_LIST;
On 2012/04/30 10:57:07, tommi wrote:
> nit: no need for these two temporaries

Done.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:248:
content::MediaStreamDevices::const_iterator dev = devices.begin();
On 2012/04/30 10:57:07, tommi wrote:
> missing thread dcheck in this function

See my answer in other comments.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:252:
users_.push_back(CaptureDeviceUser(render_process_id,
On 2012/04/30 10:57:07, tommi wrote:
> do we need to first check if we've already got this render view in the users_
> list?

Not for the current solution, we save the information anyway, but we don't show
all of them when building up the menu.

The reason for me to do it is that how the UI looks is not completely decided
yet, we may have a second version soon after we have something up and running
(we may need to talk to the privacy team to make the final decision). So I chose
to store all the information now, and will refactor it in the follow-up version.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.cc:269: users_.begin(),
users_.end(), UserEquals(render_process_id,
On 2012/04/30 10:57:07, tommi wrote:
> missing thread dcheck for users_

See answer in other comments.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.h (right):

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:50: struct
CaptureDeviceUser {
On 2012/04/30 10:57:07, tommi wrote:
> Should this struct also include usage count?

The problem of using the usage count is that we need one audio usage count and
one video usage count. The readability gets worse if we need to maintain two
counters.

Right now we handle the counting in the list, for example:
if one media stream session is using both audio and video, we store two stucts
in the list:
1, render_process_id, render_view_id, url, audio
2, render_process_id, render_view_id, url, video
When building the menu, it only shows one item since both have the same
render_process_id, render_view_id and url.

I know it is a bit duplication on the information for the current solution, but
hope it is not a problem. And it seems to me that the how the UI looks is not
completely decided yet, we may do a follow-up refactoring on this class in the
future. By then we may use counters instead.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:62: std::string url;
On 2012/04/30 10:57:07, tommi wrote:
> do we really care about the URL (render_view_id should be enough)?  Should
this
> perhaps be title?

the URL is used as a friendly message telling the users which page is using the
devices. Can we get the url by the render_view_id?

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:71: const std::string&
url, content::MediaStreamDeviceType type);
On 2012/04/30 10:57:07, tommi wrote:
> is it enough to compare only the render_view_id?

They are not only for comparison, we need both render_process_id and
render_view_id to get the tab.

http://codereview.chromium.org/10168008/diff/39003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:135: typedef
std::list<CaptureDeviceUser> CaptureDeviceUserList;
On 2012/04/30 10:57:07, tommi wrote:
> What about having the 'users' list a map between render views and usage info?
> Also, I would like to not use the word "user" here because this doesn't have
> anything to do with the user.
> 
> std::map<int, ActiveCaptureDevices>

According to my understanding, it is a valid case to have one tab having
multiple media streams with different urls.
If this is true, we may not be able to use a map here since the first parameter
must be unique in the map.

hmm, the struct mainly tells the device usage info, not the device info. What
about changing the names to CaptureDevicesUageInfo add CaptureDevicesUagelist?

[tommi (sloooow) - chröme, 2012-04-30 13:42:45]

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15757: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_ITEM" desc="URL to display in the MediaStrem
tray list when it is using the capture devices">
On 2012/04/30 13:12:54, xians1 wrote:
> On 2012/04/30 10:57:07, tommi wrote:
> > s/MediaStrem/MediaStream.
> > 
> > Are translators supposed to know what "MediaStream tray list" is?
> > 
> > Also, I don't think it makes sense to display a URL.
> > Instead I think this should be the title of the tab that is currently using
> the
> > capture device.
> 
> It is just description of the variable, it should not be translated. And our
> infobar also uses the word MediaStream.

I think you may not have read my comment.  Please read the first sentence again.

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15760: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_TOOLTIP" desc="Tool tip for the capture
notification status tray shown by the MediaStream">
On 2012/04/30 13:12:54, xians1 wrote:
> On 2012/04/30 10:57:07, tommi wrote:
> > again, I don't think "MediaStream" is known concept and not something
> > translators need to be aware of.  Same below.
> 
> See answers in my previous comment.

Please read my comment.

* Translators are the people that do the translation.
* What does "MediaStream" mean to them and how does that help them translating
the string?

If I were translating this string, then this description would be less confusing
"Tool tip for the capture notification status tray shown by Chrome".

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15761: +      Google Chrome is using your
camera and microphone
use PRODUCT_NAME instead of "Google Chrome" as is done elsewhere.

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15763: +    <message
name="IDS_MEDIA_STREAM_STATUS_TRAY_TITLE" desc="Title of the capture
notification status tray shown by MediaStream">
On 2012/04/30 13:12:54, xians1 wrote:
> On 2012/04/30 10:57:07, tommi wrote:
> > what's the difference between this and the tool tip?
> > If this is in the menu, then I don't think that's a good idea since users
> expect
> > to be able to click on anything that's in a menu (menu items are always
> > interactive unless they're grayed out).
> 
> In my discussion with Serge, he seemed to be favor of showing this text, we
can
> double check with him on this.

I don't recall him commenting on this menu entry in particular so there may be
some misunderstanding there.  Menus do not have a non-actionable title in Chrome
or any of the operating systems we support.

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15764: +      Your camera and microphone is
used by Google Chrome
PRODUCT_NAME

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15767: +      Google Chrome is using your
camera and microphone.
PRODUCT_NAME

http://codereview.chromium.org/10168008/diff/39003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15770: +      <ph
name="HOST">$1<ex>html5rocks.com</ex></ph> is using Google Chrome for recording.
Look for the Google Chrome notification icon to know which tab is using the
devices.
PRODUCT_NAME

[jam, 2012-04-30 16:09:00]

content lgtm

[no longer working on chromium, 2012-05-02 11:18:11]

Could you please take another look?

Thanks,
BR,
/SX

[tommi (sloooow) - chröme, 2012-05-02 12:10:55]

http://codereview.chromium.org/10168008/diff/47003/chrome/app/chrome_command_...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/10168008/diff/47003/chrome/app/chrome_command_...
chrome/app/chrome_command_ids.h:308: #define
IDC_MEDIA_CONTEXT_MEDIA_STREAM_CAPTURE_LIST 51301
should this be IDC_MEDIA_STREAM_CAPTURE_LIST?

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
File chrome/browser/media/media_internals.cc (right):

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
chrome/browser/media/media_internals.cc:84:
DCHECK(media_stream_capture_indicator_.get());
this dcheck isn't very useful since you deref the pointer straight away anyway

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
chrome/browser/media/media_internals.cc:84:
DCHECK(media_stream_capture_indicator_.get());
no need for this dcheck

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.h (right):

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:48: struct
CaptureDeviceUser {
CaptureDeviceTab?
(the term "User" is a little unclear to me in this context)

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:63: class UserEquals {
TabEquals?

http://codereview.chromium.org/10168008/diff/47003/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10168008/diff/47003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:171: DeviceRequest
new_request = DeviceRequest(requester, options,
change to:

DeviceRequest new_request(...params here...);

And disallow Copy+Assign in DeviceRequest so that we can't do the other thing.

http://codereview.chromium.org/10168008/diff/47003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:252: DeviceRequest
new_request = DeviceRequest(requester, options,
same here and elsewhere we use DeviceRequest.

http://codereview.chromium.org/10168008/diff/47003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:614: ++it) {
fix indent

[no longer working on chromium, 2012-05-02 13:24:29]

http://codereview.chromium.org/10168008/diff/47003/chrome/app/chrome_command_...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/10168008/diff/47003/chrome/app/chrome_command_...
chrome/app/chrome_command_ids.h:308: #define
IDC_MEDIA_CONTEXT_MEDIA_STREAM_CAPTURE_LIST 51301
On 2012/05/02 12:10:55, tommi wrote:
> should this be IDC_MEDIA_STREAM_CAPTURE_LIST?

As discussed offline, we will change it to 
IDC_MEDIA_CONTEXT_MEDIA_STREAM_CAPTURE_LIST_FIRST
and IDC_MEDIA_CONTEXT_MEDIA_STREAM_CAPTURE_LIST_LAST

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
File chrome/browser/media/media_internals.cc (right):

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
chrome/browser/media/media_internals.cc:84:
DCHECK(media_stream_capture_indicator_.get());
On 2012/05/02 12:10:55, tommi wrote:
> this dcheck isn't very useful since you deref the pointer straight away anyway

Done.

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
chrome/browser/media/media_internals.cc:84:
DCHECK(media_stream_capture_indicator_.get());
On 2012/05/02 12:10:55, tommi wrote:
> no need for this dcheck

Done.

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.h (right):

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:48: struct
CaptureDeviceUser {
On 2012/05/02 12:10:55, tommi wrote:
> CaptureDeviceTab?
> (the term "User" is a little unclear to me in this context)

Done.

http://codereview.chromium.org/10168008/diff/47003/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:63: class UserEquals {
On 2012/05/02 12:10:55, tommi wrote:
> TabEquals?

Done.

http://codereview.chromium.org/10168008/diff/47003/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10168008/diff/47003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:171: DeviceRequest
new_request = DeviceRequest(requester, options,
On 2012/05/02 12:10:55, tommi wrote:
> change to:
> 
> DeviceRequest new_request(...params here...);
> 
> And disallow Copy+Assign in DeviceRequest so that we can't do the other thing.

Done with using the constructor, copy and assin is needed by line 318 for
std::make_pair, lets keep it for now.

http://codereview.chromium.org/10168008/diff/47003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:252: DeviceRequest
new_request = DeviceRequest(requester, options,
On 2012/05/02 12:10:55, tommi wrote:
> same here and elsewhere we use DeviceRequest.

Done.

http://codereview.chromium.org/10168008/diff/47003/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:614: ++it) {
On 2012/05/02 12:10:55, tommi wrote:
> fix indent

Done.

[Nico, 2012-05-02 16:29:56]

LGTM

A test would be nice.

http://codereview.chromium.org/10168008/diff/47006/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.h (right):

http://codereview.chromium.org/10168008/diff/47006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:20: // This indicator is
owned by MediaInternals and deleted when the MediaInternals
s/the//

http://codereview.chromium.org/10168008/diff/47006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:41: // Called on IO thread
when the MediaStream closes the opened devices.
s/the MediaStream/MediaStream/

[tommi (sloooow) - chröme, 2012-05-02 16:30:46]

lgtm

[no longer working on chromium, 2012-05-02 18:00:34]

To Nico,

A test is definitely great. I will try to add it as a follow-up.
Thanks.

BR,
/SX

http://codereview.chromium.org/10168008/diff/47006/chrome/browser/media/media...
File chrome/browser/media/media_stream_capture_indicator.h (right):

http://codereview.chromium.org/10168008/diff/47006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:20: // This indicator is
owned by MediaInternals and deleted when the MediaInternals
On 2012/05/02 16:29:56, Nico wrote:
> s/the//

Done.

http://codereview.chromium.org/10168008/diff/47006/chrome/browser/media/media...
chrome/browser/media/media_stream_capture_indicator.h:41: // Called on IO thread
when the MediaStream closes the opened devices.
On 2012/05/02 16:29:56, Nico wrote:
> s/the MediaStream/MediaStream/

Done.

[commit-bot: I haz the power, 2012-05-03 17:44:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/10168008/87003

[commit-bot: I haz the power, 2012-05-03 20:02:06]

Change committed as 135198