Media indicator for background recording task

Media indicator for background recording task

BUG=323766
TEST=MediaTrayItemTest.NotifyMediaCaptureChange

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

[oshima, 2014-04-30 13:40:44]

I'll add test in later patch, but the rest is ready for review.

[oshima, 2014-04-30 16:14:28]

Added MediaTrayItemTest.NotifyMediaCaptureChange test

PTAL

[Mr4D (OOO till 08-26), 2014-04-30 18:24:36]

Please see comments!

Thanks for your help on adding this for MultiProfile!

https://codereview.chromium.org/253183003/diff/160001/ash/ash_chromeos_string...
File ash/ash_chromeos_strings.grdp (right):

https://codereview.chromium.org/253183003/diff/160001/ash/ash_chromeos_string...
ash/ash_chromeos_strings.grdp:317: </message>
Before I forget: Screen sharing - wasn't that also part of this fix?

https://codereview.chromium.org/253183003/diff/160001/ash/media_delegate.h
File ash/media_delegate.h (right):

https://codereview.chromium.org/253183003/diff/160001/ash/media_delegate.h#ne...
ash/media_delegate.h:18: MEDIA_CAPTURE_AUDIO_VIDEO = MEDIA_CAPTURE_AUDIO |
MEDIA_CAPTURE_VIDEO,
I thought we also show tab/screen sharing? If so - shouldn't we at least add the
constant now?

https://codereview.chromium.org/253183003/diff/160001/ash/session/session_sta...
File ash/session/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/253183003/diff/160001/ash/session/session_sta...
ash/session/session_state_delegate_stub.cc:68: return 2;
This is highly confusing since the default should be 1. (Who knows who will use
this stub nor or later on).

As such: Please add a comment here that the test body of ash_shell uses 2 users
for visual verification, but the default should be 1.

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
File ash/system/tray/media_security/media_tray_item.cc (right):

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
ash/system/tray/media_security/media_tray_item.cc:42: virtual void
OnMediaCaptureChanged() OVERRIDE {
Please add the

 // MediaCaptureObserver overrides:

comment.

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
ash/system/tray/media_security/media_tray_item.cc:51:
session_state_delegate->GetBrowserContextByIndex(index);
What do we do with visiting windows? Shouldn't we check that the associated
window is visible on the current screen - or did we say that we show the icon no
matter on which desktop it is?

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
ash/system/tray/media_security/media_tray_item.cc:69: }
Since this indicator does only get shown for inactive users I would be tempted
to name it something like MultiUserMediaTrayItem?

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
File ash/system/tray/media_security/media_tray_item.h (right):

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
ash/system/tray/media_security/media_tray_item.h:23: // SystemTrayItem:
"SystemTrayItem overrides:"

should the comment be to my knowledge.

https://codereview.chromium.org/253183003/diff/160001/ash/system/user/user_ca...
File ash/system/user/user_card_view.cc (right):

https://codereview.chromium.org/253183003/diff/160001/ash/system/user/user_ca...
ash/system/user/user_card_view.cc:81: 
// MediaCaptureObserver overrides:

https://codereview.chromium.org/253183003/diff/160001/ash/system/user/user_ca...
ash/system/user/user_card_view.cc:88: context);
Same here: Don't we care about visiting windows?

https://codereview.chromium.org/253183003/diff/160001/ash/test/test_shell_del...
File ash/test/test_shell_delegate.cc (right):

https://codereview.chromium.org/253183003/diff/160001/ash/test/test_shell_del...
ash/test/test_shell_delegate.cc:53: 
.. overrides:

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.cc (right):

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:80: if (foreground_state ==
ash::MEDIA_CAPTURE_AUDIO_VIDEO)
Why is that? If both are capturing - which is possible - we should show the
indication that another user is (also) capturing.

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.h (right):

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.h:17: // ash::MediaDelegate:
.. overrides:

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.h:24: //
MediaCaptureDevicesDispatcher::Observer:
.. overrides:

[oshima, 2014-04-30 20:40:30]

https://codereview.chromium.org/253183003/diff/160001/ash/ash_chromeos_string...
File ash/ash_chromeos_strings.grdp (right):

https://codereview.chromium.org/253183003/diff/160001/ash/ash_chromeos_string...
ash/ash_chromeos_strings.grdp:317: </message>
On 2014/04/30 18:24:36, Mr4D wrote:
> Before I forget: Screen sharing - wasn't that also part of this fix?

It's not and the bug was wrong. We already have an indicator for that and it's
always visible (regardless of profile). 

Tab casting wasn't the part of the bug. I'll talk to kuscher about this.

https://codereview.chromium.org/253183003/diff/160001/ash/media_delegate.h
File ash/media_delegate.h (right):

https://codereview.chromium.org/253183003/diff/160001/ash/media_delegate.h#ne...
ash/media_delegate.h:18: MEDIA_CAPTURE_AUDIO_VIDEO = MEDIA_CAPTURE_AUDIO |
MEDIA_CAPTURE_VIDEO,
On 2014/04/30 18:24:36, Mr4D wrote:
> I thought we also show tab/screen sharing? If so - shouldn't we at least add
the
> constant now?

see my comment in grd.

https://codereview.chromium.org/253183003/diff/160001/ash/session/session_sta...
File ash/session/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/253183003/diff/160001/ash/session/session_sta...
ash/session/session_state_delegate_stub.cc:68: return 2;
On 2014/04/30 18:24:36, Mr4D wrote:
> This is highly confusing since the default should be 1. (Who knows who will
use
> this stub nor or later on).
> 
> As such: Please add a comment here that the test body of ash_shell uses 2
users
> for visual verification, but the default should be 1.

This is for ash_shell as we discussed, and can be arbitrary within valid number
(1-3).
I moved this class to ash/shell/shell_delegate_impl.cc to make it clear.

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
File ash/system/tray/media_security/media_tray_item.cc (right):

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
ash/system/tray/media_security/media_tray_item.cc:42: virtual void
OnMediaCaptureChanged() OVERRIDE {
On 2014/04/30 18:24:36, Mr4D wrote:
> Please add the
> 
>  // MediaCaptureObserver overrides:
> 
> comment.

Done.

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
ash/system/tray/media_security/media_tray_item.cc:51:
session_state_delegate->GetBrowserContextByIndex(index);
On 2014/04/30 18:24:36, Mr4D wrote:
> What do we do with visiting windows? Shouldn't we check that the associated
> window is visible on the current screen - or did we say that we show the icon
no
> matter on which desktop it is?

That is handled in GetBackgroundMediaCaptureState.

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
ash/system/tray/media_security/media_tray_item.cc:69: }
On 2014/04/30 18:24:36, Mr4D wrote:
> Since this indicator does only get shown for inactive users I would be tempted
> to name it something like MultiUserMediaTrayItem?

There is no "multi user" keyword in ash so far (except kAshEnableMultiUserKey,
which seems to be obsolete),
so I'd like to sway away from "MultiUser" in ash if you're ok.

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
File ash/system/tray/media_security/media_tray_item.h (right):

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
ash/system/tray/media_security/media_tray_item.h:23: // SystemTrayItem:
On 2014/04/30 18:24:36, Mr4D wrote:
> "SystemTrayItem overrides:"
> 
> should the comment be to my knowledge.

this is new style. no overrides/implementation anymore.

https://codereview.chromium.org/253183003/diff/160001/ash/system/user/user_ca...
File ash/system/user/user_card_view.cc (right):

https://codereview.chromium.org/253183003/diff/160001/ash/system/user/user_ca...
ash/system/user/user_card_view.cc:88: context);
On 2014/04/30 18:24:36, Mr4D wrote:
> Same here: Don't we care about visiting windows?

MediaDelegate implementation takes care of it.

https://codereview.chromium.org/253183003/diff/160001/ash/test/test_shell_del...
File ash/test/test_shell_delegate.cc (right):

https://codereview.chromium.org/253183003/diff/160001/ash/test/test_shell_del...
ash/test/test_shell_delegate.cc:53: 
On 2014/04/30 18:24:36, Mr4D wrote:
> .. overrides:

fixed styles etc.

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.cc (right):

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:80: if (foreground_state ==
ash::MEDIA_CAPTURE_AUDIO_VIDEO)
On 2014/04/30 18:24:36, Mr4D wrote:
> Why is that? If both are capturing - which is possible - we should show the
> indication that another user is (also) capturing.

I confirmed this behavior with kuscher.
I'm happy to discuss with you and him if you want.

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.h (right):

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.h:17: // ash::MediaDelegate:
On 2014/04/30 18:24:36, Mr4D wrote:
> .. overrides:

ditto.

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.h:24: //
MediaCaptureDevicesDispatcher::Observer:
On 2014/04/30 18:24:36, Mr4D wrote:
> .. overrides:

ditto

[Mr4D (OOO till 08-26), 2014-05-01 14:36:56]

Don't forget to address the apps/extensions.
lgtm

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
File ash/system/tray/media_security/media_tray_item.cc (right):

https://codereview.chromium.org/253183003/diff/160001/ash/system/tray/media_s...
ash/system/tray/media_security/media_tray_item.cc:69: }
Only because it appears not not to be used (I think there were some usages in
the past but got recently removed since UX has changed) does not mean that it
shouldn't be used. Since this indicator is only used for multi user use cases it
feels rather misleading to not add it.

If you think that we might show the indicator for the current user in the future
you can leave it as it is.

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.cc (right):

https://codereview.chromium.org/253183003/diff/160001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:80: if (foreground_state ==
ash::MEDIA_CAPTURE_AUDIO_VIDEO)
Yes, please let's discuss with kuscher since this is a valid case which can
happen and it feels rather wrong to have this.

[oshima, 2014-05-01 18:47:17]

Update the patch per offline discussion. PTAL.

notable change:
* MediaDelegate::GetBackgroundMediaCaptureState ->
MediaDelegate::GetMediaCaptureState
* renamed MediaTrayItem -> MultiProfileMediaTrayItem
* Implementation of MediaDelegateChromeoS::GetMediaCaptureState has been changed
 so that it returns the recording status of all window but not just one on
background desktop.
* It now post a task to notify. This is necessary because the state of
MediaStreamCaptureIndicator gets updated using after this.

[oshima, 2014-05-02 17:26:33]

Stefan seems to be ooo today. james, can you review
changes in chrome/browser/ui/ash ?

[Mr4D (OOO till 08-26), 2014-05-02 18:07:29]

Please have a look!

(Sorry worked on another patch myself for some time ignoring my emails)

https://codereview.chromium.org/253183003/diff/260001/ash/system/user/user_ca...
File ash/system/user/user_card_view.cc (right):

https://codereview.chromium.org/253183003/diff/260001/ash/system/user/user_ca...
ash/system/user/user_card_view.cc:88:
Shell::GetInstance()->media_delegate()->GetMediaCaptureState(context);
According to GetMediaCaptureState you only check the state of the context's for
the passed (current) context. However - don't we want to show the state of the
*other* users and *not* the ones of the current user?

https://codereview.chromium.org/253183003/diff/260001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.cc (right):

https://codereview.chromium.org/253183003/diff/260001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:51: if
(web_contents->GetBrowserContext() != context)
Moment - didn't we want to find the state for the *other* users, so excluding
context and not only looking for context (which seems the opposite)?

https://codereview.chromium.org/253183003/diff/260001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:113: if (media_state !=
ash::MEDIA_CAPTURE_AUDIO_VIDEO)
You might want to use brackets here to cover this and the next statement.

[James Cook, 2014-05-02 18:17:23]

Let me know if you still need me to review this.

[oshima, 2014-05-02 18:59:04]

https://codereview.chromium.org/253183003/diff/260001/ash/system/user/user_ca...
File ash/system/user/user_card_view.cc (right):

https://codereview.chromium.org/253183003/diff/260001/ash/system/user/user_ca...
ash/system/user/user_card_view.cc:88:
Shell::GetInstance()->media_delegate()->GetMediaCaptureState(context);
On 2014/05/02 18:07:29, Mr4D wrote:
> According to GetMediaCaptureState you only check the state of the context's
for
> the passed (current) context. However - don't we want to show the state of the
> *other* users and *not* the ones of the current user?

This indicator is created for each of other users (index_ is for the other
users), and should contain the status of this user's media state only.

https://codereview.chromium.org/253183003/diff/260001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.cc (right):

https://codereview.chromium.org/253183003/diff/260001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:51: if
(web_contents->GetBrowserContext() != context)
On 2014/05/02 18:07:29, Mr4D wrote:
> Moment - didn't we want to find the state for the *other* users, so excluding
> context and not only looking for context (which seems the opposite)?

The context passed by caller is other user's context, not the one for desktop
user.

https://codereview.chromium.org/253183003/diff/260001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:113: if (media_state !=
ash::MEDIA_CAPTURE_AUDIO_VIDEO)
On 2014/05/02 18:07:29, Mr4D wrote:
> You might want to use brackets here to cover this and the next statement.

I changed to early exit instead. I think it's easier to read.

[Mr4D (OOO till 08-26), 2014-05-02 19:27:26]

Thanks for adding in the extensions!

lgtm

https://codereview.chromium.org/253183003/diff/260001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_delegate_chromeos.cc (right):

https://codereview.chromium.org/253183003/diff/260001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_delegate_chromeos.cc:51: if
(web_contents->GetBrowserContext() != context)
Right. But I was looking at the code for performance reasons (since n users with
n apps/extensions/browsers) could reach quickly an expensive performance
overhead if called for each user (several times). I thought about having some
more state keeping object, but after discussion I realized that this code gets
called highly infrequently - so it should not a problem (unless the state
constantly changes - which would be a bug).

So good as it is until proven otherwise.

[oshima, 2014-05-02 19:28:11]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-05-02 19:28:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/253183003/300001

[commit-bot: I haz the power, 2014-05-02 21:32:51]

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

[commit-bot: I haz the power, 2014-05-02 21:32:51]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[oshima, 2014-05-03 16:13:23]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-05-03 16:13:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/253183003/340001

[commit-bot: I haz the power, 2014-05-03 21:38:39]

Change committed as 268077