Indicate tab recording state in tab strip

Indicate tab recording state in tab strip

Adds MediaStreamCaptureIndicator::IsProcessCapturing() to retrieve recording state.


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

[stevenjb, 2012-10-22 23:03:08]

xians@ for media changes
sky@ for tab changes

I'm not especially familiar with either piece of code, so please don't hesitate
to suggest alternate approaches.

I apologize for converting the media code to use WeakPtr instead of ref counting
as part of this, but with other code now accessing MediaStreamCaptureIndicator,
a straight singleton + WeakPtr model seemed more appropriate. I can separate
that out (or revert it) if preferred.

[sky, 2012-10-23 00:34:23]

LGTM

[no longer working on chromium, 2012-10-23 08:56:53]

Awesome, great work.

two nits, lgtm if you address them.

http://codereview.chromium.org/11236052/diff/4001/chrome/browser/media/media_...
File chrome/browser/media/media_internals.cc (right):

http://codereview.chromium.org/11236052/diff/4001/chrome/browser/media/media_...
chrome/browser/media/media_internals.cc:73: render_view_id,
nit, please fix the indentation.

http://codereview.chromium.org/11236052/diff/4001/chrome/browser/media/media_...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/11236052/diff/4001/chrome/browser/media/media_...
chrome/browser/media/media_stream_capture_indicator.cc:193:
weak_ptr_factory_.GetWeakPtr(),
Do we know that if MediaInternals will always outlive UI thread?
When the browser is shutting down, we will get here and send
DoDevicesClosedOnUIThread tasks to UI thread, if UI thread is busy, there will
be delay on executing tasks.

[stevenjb, 2012-10-23 18:11:31]

http://codereview.chromium.org/11236052/diff/4001/chrome/browser/media/media_...
File chrome/browser/media/media_internals.cc (right):

http://codereview.chromium.org/11236052/diff/4001/chrome/browser/media/media_...
chrome/browser/media/media_internals.cc:73: render_view_id,
On 2012/10/23 08:56:53, xians1 wrote:
> nit, please fix the indentation.
Strange, this is correct locally, must be a codereview bug.

http://codereview.chromium.org/11236052/diff/4001/chrome/browser/media/media_...
File chrome/browser/media/media_stream_capture_indicator.cc (right):

http://codereview.chromium.org/11236052/diff/4001/chrome/browser/media/media_...
chrome/browser/media/media_stream_capture_indicator.cc:193:
weak_ptr_factory_.GetWeakPtr(),
On 2012/10/23 08:56:53, xians1 wrote:
> Do we know that if MediaInternals will always outlive UI thread?
> When the browser is shutting down, we will get here and send
> DoDevicesClosedOnUIThread tasks to UI thread, if UI thread is busy, there will
> be delay on executing tasks. 

As a Singleton, yes, it will outlive the UI message loop. However, Bind +
WeakPtr will also ensure that if MediaInternals were deleted before the callback
would be called, the result is a no-op instead of a crash. Since the callback
happens on the UI thread (where the Singleton is created/destroyed), this is
safe.

[commit-bot: I haz the power, 2012-10-23 18:17:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11236052/9001

[commit-bot: I haz the power, 2012-10-23 18:17:39]

Presubmit check for 11236052-9001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Image chrome/app/theme/default_200_percent/tab_recording.png has width 40,
expected to be 32

Image chrome/app/theme/default_200_percent/tab_recording.png has height 40,
expected to be 32

Presubmit checks took 5.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[stevenjb, 2012-10-24 21:49:50]

xians@ - can you PTAL at patch set 4 (3-4 delta actually, specifically
media_stream_capture_indicator.cc)?

I finally got to testing this on a ChromeOS device and it turns out that we
weren't actually updating the device list or notifying the tab strip. 

Thanks!

[miu, 2012-10-24 23:53:22]

stevenjb@,

It looks like I'm working on stuff related to this (approaching from the
renderer side of things).  You'll want to take a look at:
https://codereview.chromium.org/11166002/

-Yuri

[no longer working on chromium, 2012-10-25 08:42:33]

On 2012/10/24 21:49:50, stevenjb (chromium) wrote:
> xians@ - can you PTAL at patch set 4 (3-4 delta actually, specifically
> media_stream_capture_indicator.cc)?
> 
> I finally got to testing this on a ChromeOS device and it turns out that we
> weren't actually updating the device list or notifying the tab strip. 
> 
> Thanks!

Oh, true, we don't have system tray on chromeOS. And the system tray will be
deprecated after the recording omnibox kicks in. But we might keep the balloon.
I will take care of it in a separate CL.

lgtm++

[commit-bot: I haz the power, 2012-10-25 16:47:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11236052/16007

[commit-bot: I haz the power, 2012-10-25 16:47:21]

Presubmit check for 11236052-16007 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Image chrome/app/theme/default_200_percent/tab_recording.png has width 40,
expected to be 32

Image chrome/app/theme/default_200_percent/tab_recording.png has height 40,
expected to be 32

Presubmit checks took 1.4s to calculate.