Fix MediaClient::RequestCaptureState().

Fix MediaClient::RequestCaptureState().

MediaClient::RequestCaptureState() was failing because chrome
does not have access to ash::SessionStateDelegate and ash::
ShellContentState.

1. Changed places that used ash::SessionStateDelegate to use
UserManager.

2. Changed places that used ash::ShellContentState to use
ChromeShellContentState. Right now the only places in ash that
use ash::ShellContentState are in tests so maybe we can move
ShellContentState to chrome completely in the future.

3. Changed MultiProfileMediaTrayItem and TrayUser in ash to 
use SessionController to get the number of logged in users.

BUG=676091
TEST=ash_unittests

Review-Url: https://codereview.chromium.org/2615663002
Cr-Commit-Position: refs/heads/master@{#442679}
Committed: https://chromium.googlesource.com/chromium/src/+/42968739363fbecc9515b3631d2e05ac7046fb02

[riajiang, 2017-01-06 00:09:33]

Patchset #2 (id:20001) has been deleted

[riajiang, 2017-01-06 22:21:55]

Description was changed from

==========
Fix MediaClient::RequestCaptureState().

BUG=676091
TEST=ash_unittests
==========

to

==========
Fix MediaClient::RequestCaptureState().

BUG=676091
TEST=ash_unittests
==========

[riajiang, 2017-01-09 19:20:15]

Description was changed from

==========
Fix MediaClient::RequestCaptureState().

BUG=676091
TEST=ash_unittests
==========

to

==========
Fix MediaClient::RequestCaptureState().

MediaClient::RequestCaptureState() was failing because chrome
does not have access to ash::SessionStateDelegate and ash::
ShellContentState.

1. Changed places that used ash::SessionStateDelegate to use
UserManager.
2. Changed places that used ash::ShellContentState to use
ChromeShellContentState. Right now the only places in ash that
use ash::ShellContentState are in tests so maybe we can move
ShellContentState to chrome completely in the future.
3. Changed MultiProfileMediaTrayItem and TrayUser in ash to 
use SessionController to get the number of logged in users.

BUG=676091
TEST=ash_unittests
==========

[riajiang, 2017-01-09 19:24:10]

riajiang@chromium.org changed reviewers:
+ jamescook@chromium.org

[riajiang, 2017-01-09 19:24:10]

Hi James, PTAL, thanks!

[riajiang, 2017-01-09 19:24:37]

Description was changed from

==========
Fix MediaClient::RequestCaptureState().

MediaClient::RequestCaptureState() was failing because chrome
does not have access to ash::SessionStateDelegate and ash::
ShellContentState.

1. Changed places that used ash::SessionStateDelegate to use
UserManager.
2. Changed places that used ash::ShellContentState to use
ChromeShellContentState. Right now the only places in ash that
use ash::ShellContentState are in tests so maybe we can move
ShellContentState to chrome completely in the future.
3. Changed MultiProfileMediaTrayItem and TrayUser in ash to 
use SessionController to get the number of logged in users.

BUG=676091
TEST=ash_unittests
==========

to

==========
Fix MediaClient::RequestCaptureState().

MediaClient::RequestCaptureState() was failing because chrome
does not have access to ash::SessionStateDelegate and ash::
ShellContentState.

1. Changed places that used ash::SessionStateDelegate to use
UserManager.

2. Changed places that used ash::ShellContentState to use
ChromeShellContentState. Right now the only places in ash that
use ash::ShellContentState are in tests so maybe we can move
ShellContentState to chrome completely in the future.

3. Changed MultiProfileMediaTrayItem and TrayUser in ash to 
use SessionController to get the number of logged in users.

BUG=676091
TEST=ash_unittests
==========

[James Cook, 2017-01-09 21:07:19]

xiyuan, one question on this.

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/chrom...
File ash/common/system/chromeos/media_security/multi_profile_media_tray_item.cc
(right):

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/chrom...
ash/common/system/chromeos/media_security/multi_profile_media_tray_item.cc:49:
index < WmShell::Get()->session_controller()->NumberOfLoggedInUsers();
xiyuan, this works, but is the long-term plan that session-related stuff in ash
goes through SessionController, or is the plan that ash has its own instance of
UserManager?

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:230:
WmShell::Get()->session_controller()->NumberOfLoggedInUsers()) {
nit: line wrapping might be better here and below if you cached a
SessionController* in a local variable.

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_content_state.cc (right):

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_content_state.cc:39: DestroyInstance();
optional: I like to do this:

DCHECK_EQ(this, g_instance);
g_instance = nullptr;

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_content_state.h (right):

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_content_state.h:16: static void
SetInstance(ChromeShellContentState* state);
Do you use these public methods outside this class?

If not, remove Set/DestroyInstance and just set |instance_| (or a g_instance in
an anonymous namespace in the .cc file) in the constructor and destructor

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_client.cc (right):

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_client.cc:174: static_cast<size_t>(i) <
nit: This might be easier to read if |i| was a uint32 and the cast was done
inside the loop

[xiyuan, 2017-01-09 21:21:46]

xiyuan@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xiyuan, 2017-01-09 21:21:47]

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/chrom...
File ash/common/system/chromeos/media_security/multi_profile_media_tray_item.cc
(right):

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/chrom...
ash/common/system/chromeos/media_security/multi_profile_media_tray_item.cc:49:
index < WmShell::Get()->session_controller()->NumberOfLoggedInUsers();
On 2017/01/09 21:07:19, James Cook wrote:
> xiyuan, this works, but is the long-term plan that session-related stuff in
ash
> goes through SessionController, or is the plan that ash has its own instance
of
> UserManager?

yes, ash code should use SessionController. The browser side code uses
UserManager/SessionManager since those two are part of the browser process ATM.
UserManager/SessionManager might move to their own process in the future but I
don't think we want to move them into ash.

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:86: WmShell::Get()->session_controller();
I have an incomplete CL (replace SessionStateDelegate with SessionController) to
do similar things in this file. Think this would break tests. The test mocks the
user and user sessions via TestSessionStateDelegate and SessionController does
not know anything about the mock. We might need to update relevant tests (in
tray_user_unittest.cc) as well.

[riajiang, 2017-01-09 22:37:34]

The CQ bit was checked by riajiang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 22:38:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 23:35:41]

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

[commit-bot: I haz the power, 2017-01-09 23:35:42]

Dry run: This issue passed the CQ dry run.

[riajiang, 2017-01-09 23:46:51]

As discussed with xiyuan@ offline, removed changes in TrayUser and
MultiProfileMediaTrayItem since he is working on a CL that does the same thing
and is already looking into TestSessionStateDelegate for testing. For the
purpose of this CL, it would work if I just change
SessionStateDelegateStub::NumberOfLoggedInUsers() to return 1 which is the
correct number of logged in users instead of the default 2. Once xiyuan@'s CL is
landed we don't need to use this
SessionStateDelegateStub::NumberOfLoggedInUsers() anymore. 
Please take another look, thanks!

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2615663002/diff/80001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:230:
WmShell::Get()->session_controller()->NumberOfLoggedInUsers()) {
On 2017/01/09 21:07:19, James Cook wrote:
> nit: line wrapping might be better here and below if you cached a
> SessionController* in a local variable.

Done.

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_content_state.cc (right):

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_content_state.cc:39: DestroyInstance();
On 2017/01/09 21:07:19, James Cook wrote:
> optional: I like to do this:
> 
> DCHECK_EQ(this, g_instance);
> g_instance = nullptr;

Done.

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_content_state.h (right):

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_content_state.h:16: static void
SetInstance(ChromeShellContentState* state);
On 2017/01/09 21:07:19, James Cook wrote:
> Do you use these public methods outside this class?
> 
> If not, remove Set/DestroyInstance and just set |instance_| (or a g_instance
in
> an anonymous namespace in the .cc file) in the constructor and destructor

It's not used outside. Moved it to ctor and dtor.

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/media_client.cc (right):

https://codereview.chromium.org/2615663002/diff/80001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/media_client.cc:174: static_cast<size_t>(i) <
On 2017/01/09 21:07:19, James Cook wrote:
> nit: This might be easier to read if |i| was a uint32 and the cast was done
> inside the loop

Done.

[James Cook, 2017-01-09 23:57:01]

LGTM

[riajiang, 2017-01-10 02:32:46]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2017-01-10 02:32:56]

The CQ bit was unchecked by riajiang@chromium.org

[xiyuan, 2017-01-10 19:40:17]

lgtm

[riajiang, 2017-01-10 19:54:46]

Patchset #6 (id:120001) has been deleted

[riajiang, 2017-01-10 19:55:13]

The CQ bit was checked by riajiang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 19:55:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 20:25:43]

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

[commit-bot: I haz the power, 2017-01-10 20:25:44]

Dry run: This issue passed the CQ dry run.

[riajiang, 2017-01-10 20:37:11]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2017-01-10 20:37:11]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2615663002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-10 20:38:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 20:43:49]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484080631162400,
"parent_rev": "24ccd0688d195440d8c37f0f256d9a379a25db25", "commit_rev":
"42968739363fbecc9515b3631d2e05ac7046fb02"}

[commit-bot: I haz the power, 2017-01-10 20:44:28]

Description was changed from

==========
Fix MediaClient::RequestCaptureState().

MediaClient::RequestCaptureState() was failing because chrome
does not have access to ash::SessionStateDelegate and ash::
ShellContentState.

1. Changed places that used ash::SessionStateDelegate to use
UserManager.

2. Changed places that used ash::ShellContentState to use
ChromeShellContentState. Right now the only places in ash that
use ash::ShellContentState are in tests so maybe we can move
ShellContentState to chrome completely in the future.

3. Changed MultiProfileMediaTrayItem and TrayUser in ash to 
use SessionController to get the number of logged in users.

BUG=676091
TEST=ash_unittests
==========

to

==========
Fix MediaClient::RequestCaptureState().

MediaClient::RequestCaptureState() was failing because chrome
does not have access to ash::SessionStateDelegate and ash::
ShellContentState.

1. Changed places that used ash::SessionStateDelegate to use
UserManager.

2. Changed places that used ash::ShellContentState to use
ChromeShellContentState. Right now the only places in ash that
use ash::ShellContentState are in tests so maybe we can move
ShellContentState to chrome completely in the future.

3. Changed MultiProfileMediaTrayItem and TrayUser in ash to 
use SessionController to get the number of logged in users.

BUG=676091
TEST=ash_unittests

Review-Url: https://codereview.chromium.org/2615663002
Cr-Commit-Position: refs/heads/master@{#442679}
Committed:
https://chromium.googlesource.com/chromium/src/+/42968739363fbecc9515b3631d2e...
==========

[commit-bot: I haz the power, 2017-01-10 20:44:30]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/42968739363fbecc9515b3631d2e...