ash: Add SessionController/Client mojo interfaces

ash: Add SessionController/Client mojo interfaces

SessionController serves as an interface to maintain a session info
cache in ash. SessionControllerClient runs on the other side where
session manager code runs (chrome) and update SessionController with
session info. They are targeted to replace SessionStateDelegate to
use in both mash and classic ash.

BUG=648964, 496761
Committed: https://crrev.com/468b45fb766ba4da0e7ef3aaaa2d5a05d5f34602
Cr-Commit-Position: refs/heads/master@{#437407}

[xiyuan, 2016-12-01 18:57:30]

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

[commit-bot: I haz the power, 2016-12-01 18:58:14]

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, 2016-12-01 19:02:57]

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

[commit-bot: I haz the power, 2016-12-01 19:02:59]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiyuan, 2016-12-01 19:12:33]

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

[commit-bot: I haz the power, 2016-12-01 19:12:52]

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, 2016-12-01 19:19:09]

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

[commit-bot: I haz the power, 2016-12-01 19:19:10]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xiyuan, 2016-12-01 19:25:50]

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

[commit-bot: I haz the power, 2016-12-01 19:26:32]

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

[xiyuan, 2016-12-01 20:39:49]

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

[commit-bot: I haz the power, 2016-12-01 20:40:31]

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

[xiyuan, 2016-12-01 20:52:28]

xiyuan@chromium.org changed reviewers:
+ achuith@chromium.org, jamescook@chromium.org, sky@chromium.org,
stevenjb@chromium.org

[xiyuan, 2016-12-01 20:52:28]

Sorry for the big CL. But a good portion of the code is mojo skeleton and
shuffling SessionStateDelegate code around. Let me know if I should chop it into
smaller pieces.

Suggested split:
jamescook:
  everything :p

stevenjb:
  c/b/ui/ash/*

achuith:
  components/session_manager/BUILD.gn
  components/user_manager/BUILD.gn

rickyz:
  ash/public/interfaces/session_controller.mojom
  c/b/chrome_content_browser_manifest_overlay.json

sky:
  c/b/ui/views/ash/chrome_browser_main_extra_parts_ash.*

Thanks

[achuithb, 2016-12-01 21:03:21]

On 2016/12/01 20:52:28, xiyuan wrote:
> Sorry for the big CL. But a good portion of the code is mojo skeleton and
> shuffling SessionStateDelegate code around. Let me know if I should chop it
into
> smaller pieces.
> 
> Suggested split:
> jamescook:
>   everything :p
> 
> stevenjb:
>   c/b/ui/ash/*
> 
> achuith:
>   components/session_manager/BUILD.gn
>   components/user_manager/BUILD.gn

lgtm for BUILD.gn files.

> rickyz:
>   ash/public/interfaces/session_controller.mojom
>   c/b/chrome_content_browser_manifest_overlay.json
> 
> sky:
>   c/b/ui/views/ash/chrome_browser_main_extra_parts_ash.*
> 
> Thanks

[commit-bot: I haz the power, 2016-12-01 21:36:34]

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

[commit-bot: I haz the power, 2016-12-01 21:36:35]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-12-01 23:01:44]

Wow, it's great to see this coming!

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
File ash/common/session/session_controller.cc (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:67: client_->RequestLockScreen();
Check if (client_) since it's possible someday we'll run without chrome.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:124: for (auto& user_session :
user_sessions_)
const auto&, if possible, here and below

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:129: for (auto& user_session :
user_sessions) {
user_sessions_? isn't |user_sessions| empty now that you have moved it?

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:130: if
(existing_users.find(user_session->account_id) == existing_users.end()) {
optional: if ( ... != end) continue; which would reduce indenting

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:139: }
nit: this function could use some comments explaining what each block is doing

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:150: }
Can you add a simple unit test for this class? Just enough to illustrate that
you can instantiate it by itself, and maybe exercise the SetUserSessions
function?

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
File ash/common/session/session_controller.h (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:20: class SessionController : public
mojom::SessionController {
Class comment please

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:28: // Replacement of
SessionStateDelegate interface.
Move or duplicate the SessionStateDelegate function comments here, if this is
going to replace them.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:40: void LockScreen();
nit: Comment that these mirror or are wrappers around
mojom::SessionControllerClient?

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:62: // Client interface to Chrome. Only
bound on Chrome OS.
nit: remove "Only bound on Chrome OS". I'm in the process of making ash
chromeos-only.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:66: uint32_t max_users_ = 0u;
#include <stdint.h>

or just use int, I'm not picky about uint/size_t for counts of things

https://codereview.chromium.org/2545723003/diff/60001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:509: std::unique_ptr<SessionController>
session_controller_;
super nit: alphabetize, unless construction order matters (accessor function
too)

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:9: // Matches
session_manager::SessionState
super nit: end with . (and below) for consistency

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:12: UNKNOWN,
nit: Please copy the comments for these enum values (this and below) into the
mojom file. Mojoms should be long-lived interfaces and have the best possible
documentation.

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:69: SetSessionState(SessionState
session_state);
If all of the above functions are going to be called each time the session state
changes, should SetSessionState() take a list of args with all the data from the
functions above? Right now I think it does 6 IPCs whenever any one thing
changes.

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:73:
SetUserSessions(array<UserSession> user_sessions);
Is this going to be re-sent every time the order of the list changes? If so it's
going to ship serialized bitmaps across IPC each time. Maybe the avatar bitmaps
should be taken out of UserSession and sent separately?

EDIT: I see http://crbug.com/670422 - maybe reference that here and/or in struct
UserSession above? But my comment might be a little different -- in addition to
watching for changes to the image it would be nice not to re-send the bitmap
when the list is reordered. You might want to add that to the bug. I'm OK with
fixing this in a follow-up CL.

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:82: // (if the user has already
signed in).
nit: wrap with line above

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
File ash/public/interfaces/session_controller.typemap (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.typemap:12: deps = [
I think this should be public_deps, since anyone who depends on the enums in the
mojom also depends on the headers in the deps here.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:35: SessionControllerClient*
instance = nullptr;
nit: I like g_instance for these.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:146:
user_manager::UserManager* const user_manager =
optional: Feel free to add "using user_manager::UserManager" if it makes the
wrapping better.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:244: // Tolerate ash crashing
and coming back up.
Aside: This is fine, but for version 1 (and maybe forever) we're going to
restart the browser if ash crashes. Otherwise ash might not get some of the
initial state that the browser sends when it starts up.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:267:
session_controller_->SetUserSessions(GetUserSessions());
Hmm, that's a lot of IPCs for a single state change. See mojom comment.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.h (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.h:18: // Updates session state
etc to ash via SessionControoler interface and handles
nit: SessionController

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.h:29: static
SessionControllerClient* Get();
Is this used?

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:78: return
SessionControllerClient::GetAddUserSessionPolicy();
I like how you migrated this code directly into static functions in
SessionControllerClient. That's nicer than what I did with my old
SystemTrayCommon class.

(You could also use the SessionControllerClient::Get() method here, up to you.)

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:50:
session_controller_client_ = base::MakeUnique<SessionControllerClient>();
Do you want to create this only when IsRunningInMash()? Right now it looks like
you have both a SessionControllerDelegate and a SessionControllerClient in
classic ash. Is that what you want?

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:87:
session_controller_client_.reset();

nit: either initialize before system_tray_client or teardown before it, so
teardown unrolls in the opposite order as construction

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h:36:
std::unique_ptr<SessionControllerClient> session_controller_client_;
nit: alphabetize unless order matters

[stevenjb, 2016-12-02 20:13:51]

I think James is a c/b/ui/ash owner now, but I scanned through it anyway (I'm
trying to keep somewhat up to date with the cros mojo changes) and didn't have
anything to add that James didn't already mention, so RS owner lgtm in case this
lands before James' owner change :)

[sky, 2016-12-02 20:42:03]

LGTM

[xiyuan, 2016-12-06 00:46:34]

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
File ash/common/session/session_controller.cc (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:67: client_->RequestLockScreen();
On 2016/12/01 23:01:43, James Cook wrote:
> Check if (client_) since it's possible someday we'll run without chrome.

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:124: for (auto& user_session :
user_sessions_)
On 2016/12/01 23:01:43, James Cook wrote:
> const auto&, if possible, here and below

Code removed.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:129: for (auto& user_session :
user_sessions) {
On 2016/12/01 23:01:43, James Cook wrote:
> user_sessions_? isn't |user_sessions| empty now that you have moved it?

Right. The code is wrong. Removed in new patch since we get rid of
SetUserSessions.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:130: if
(existing_users.find(user_session->account_id) == existing_users.end()) {
On 2016/12/01 23:01:43, James Cook wrote:
> optional: if ( ... != end) continue; which would reduce indenting

Code removed.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:139: }
On 2016/12/01 23:01:43, James Cook wrote:
> nit: this function could use some comments explaining what each block is doing

Acknowledged.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.cc:150: }
On 2016/12/01 23:01:43, James Cook wrote:
> Can you add a simple unit test for this class? Just enough to illustrate that
> you can instantiate it by itself, and maybe exercise the SetUserSessions
> function?

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
File ash/common/session/session_controller.h (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:20: class SessionController : public
mojom::SessionController {
On 2016/12/01 23:01:43, James Cook wrote:
> Class comment please

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:28: // Replacement of
SessionStateDelegate interface.
On 2016/12/01 23:01:43, James Cook wrote:
> Move or duplicate the SessionStateDelegate function comments here, if this is
> going to replace them.

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:40: void LockScreen();
On 2016/12/01 23:01:43, James Cook wrote:
> nit: Comment that these mirror or are wrappers around
> mojom::SessionControllerClient?

Copied SessionStateDelegate comments to them.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:62: // Client interface to Chrome. Only
bound on Chrome OS.
On 2016/12/01 23:01:43, James Cook wrote:
> nit: remove "Only bound on Chrome OS". I'm in the process of making ash
> chromeos-only.

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/session/sess...
ash/common/session/session_controller.h:66: uint32_t max_users_ = 0u;
On 2016/12/01 23:01:43, James Cook wrote:
> #include <stdint.h>
> 
> or just use int, I'm not picky about uint/size_t for counts of things

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:509: std::unique_ptr<SessionController>
session_controller_;
On 2016/12/01 23:01:43, James Cook wrote:
> super nit: alphabetize, unless construction order matters (accessor function
> too)

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:9: // Matches
session_manager::SessionState
On 2016/12/01 23:01:43, James Cook wrote:
> super nit: end with . (and below) for consistency

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:12: UNKNOWN,
On 2016/12/01 23:01:43, James Cook wrote:
> nit: Please copy the comments for these enum values (this and below) into the
> mojom file. Mojoms should be long-lived interfaces and have the best possible
> documentation.

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:69: SetSessionState(SessionState
session_state);
On 2016/12/01 23:01:43, James Cook wrote:
> If all of the above functions are going to be called each time the session
state
> changes, should SetSessionState() take a list of args with all the data from
the
> functions above? Right now I think it does 6 IPCs whenever any one thing
> changes.

Makes sense. I was thinking keeping them separate so that we could update them
individually. But the impl currently updates all of them when user signs in so
does not make sense to make separate calls. Combined them into one call.

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:73:
SetUserSessions(array<UserSession> user_sessions);
On 2016/12/01 23:01:43, James Cook wrote:
> Is this going to be re-sent every time the order of the list changes? If so
it's
> going to ship serialized bitmaps across IPC each time. Maybe the avatar
bitmaps
> should be taken out of UserSession and sent separately?
> 
> EDIT: I see http://crbug.com/670422 - maybe reference that here and/or in
struct
> UserSession above? But my comment might be a little different -- in addition
to
> watching for changes to the image it would be nice not to re-send the bitmap
> when the list is reordered. You might want to add that to the bug. I'm OK with
> fixing this in a follow-up CL.

Good point. I overlooked this.

Split SetUserSessions into two calls: UpdateUserSession and SetUserSessionOrder.
The first is called when a user session is added and would used later to update
user meta changes such as avatar change. The 2nd one sends the a list of session
ids to order the user sessions in ash.

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:82: // (if the user has already
signed in).
On 2016/12/01 23:01:43, James Cook wrote:
> nit: wrap with line above

Done.

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
File ash/public/interfaces/session_controller.typemap (right):

https://codereview.chromium.org/2545723003/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.typemap:12: deps = [
On 2016/12/01 23:01:44, James Cook wrote:
> I think this should be public_deps, since anyone who depends on the enums in
the
> mojom also depends on the headers in the deps here.

Done.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:35: SessionControllerClient*
instance = nullptr;
On 2016/12/01 23:01:44, James Cook wrote:
> nit: I like g_instance for these.

No longer needed and removed.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:146:
user_manager::UserManager* const user_manager =
On 2016/12/01 23:01:44, James Cook wrote:
> optional: Feel free to add "using user_manager::UserManager" if it makes the
> wrapping better.

Done. And using user_manager::UserList and User as well. :)

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:244: // Tolerate ash crashing
and coming back up.
On 2016/12/01 23:01:44, James Cook wrote:
> Aside: This is fine, but for version 1 (and maybe forever) we're going to
> restart the browser if ash crashes. Otherwise ash might not get some of the
> initial state that the browser sends when it starts up.

Removed.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:267:
session_controller_->SetUserSessions(GetUserSessions());
On 2016/12/01 23:01:44, James Cook wrote:
> Hmm, that's a lot of IPCs for a single state change. See mojom comment.

Changed.
- Combined simple info into a SessionInfo struct and only sent it when they are
changed.
- UserSession info is now sent in two mojo calls:
  - UpdateUserSession to send meta data
  - SetUserSessionOrder to handle active user session change.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.h (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.h:18: // Updates session state
etc to ash via SessionControoler interface and handles
On 2016/12/01 23:01:44, James Cook wrote:
> nit: SessionController

Done.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.h:29: static
SessionControllerClient* Get();
On 2016/12/01 23:01:44, James Cook wrote:
> Is this used?

Right. No longer needed and removed.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:78: return
SessionControllerClient::GetAddUserSessionPolicy();
On 2016/12/01 23:01:44, James Cook wrote:
> I like how you migrated this code directly into static functions in
> SessionControllerClient. That's nicer than what I did with my old
> SystemTrayCommon class.
> 
> (You could also use the SessionControllerClient::Get() method here, up to
you.)

I am lucky that those helpers do not have state in SessionStateDelgate or
SessionControllerClient. :)

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:50:
session_controller_client_ = base::MakeUnique<SessionControllerClient>();
On 2016/12/01 23:01:44, James Cook wrote:
> Do you want to create this only when IsRunningInMash()? Right now it looks
like
> you have both a SessionControllerDelegate and a SessionControllerClient in
> classic ash. Is that what you want?

Put this under IsRunningInMash() since it is not used yet in this CL. In the
future, I would like to use SessionControllerClient in all cases (mash and
classic ash).

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:87:
session_controller_client_.reset();
On 2016/12/01 23:01:44, James Cook wrote:
> 
> nit: either initialize before system_tray_client or teardown before it, so
> teardown unrolls in the opposite order as construction

Done.

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h (right):

https://codereview.chromium.org/2545723003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h:36:
std::unique_ptr<SessionControllerClient> session_controller_client_;
On 2016/12/01 23:01:44, James Cook wrote:
> nit: alphabetize unless order matters

Done.

[xiyuan, 2016-12-06 00:46:41]

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

[commit-bot: I haz the power, 2016-12-06 00:47:09]

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, 2016-12-06 00:49:58]

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

[commit-bot: I haz the power, 2016-12-06 00:49:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xiyuan, 2016-12-06 17:15:52]

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

[commit-bot: I haz the power, 2016-12-06 17:16:33]

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, 2016-12-06 17:22:28]

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

[commit-bot: I haz the power, 2016-12-06 17:22:29]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-12-06 18:08:25]

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
File ash/common/session/session_controller.cc (right):

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller.cc:125: for (auto session_id :
user_session_order) {
auto& or const auto& or use the real type in range-based-for-loops

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
File ash/common/session/session_controller.h (right):

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller.h:71: // Gets the user sessions.
nit: Document the order (arbitrary or current user first?)

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller.h:117: uint32_t active_session_id_ = 0u;
nit: document, since we have lots of uint32s and lots of types of ids. :-)

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
File ash/common/session/session_controller_unittest.cc (right):

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:21: namespace test {
nit: Remove. I only use "namespace test" for test utility code, not for tests
themselves.

optional: If possible put the entire test in the anonymous namespace so the
linker has less work to do.

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:121:
DISALLOW_COPY_AND_ASSIGN(SessionControllerTest);
nit: blank line above

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:124:
TEST_F(SessionControllerTest, SimpleSessionInfo) {
nit: Document what the test is testing (and below)

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:156: for (auto policy :
kTestCases) {
auto& or use the underlying type

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:236: }  // namespace ash
Nice tests. Good coverage and easy to read.

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:10: [Extensible]
Per offline conversation I don't think these should be [extensible], at least
not yet.

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:68: uint32 session_id;
Is this a general id used throughout chrome, or just for the ash/chrome
communication? Can you document what it means?

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:119: // its meta data (e.g. name,
avatar) is changed.
nit: Comment that there is no method to remove a user because ash/chrome doesn't
support that -- all users are logged out at once.

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:124:
SetUserSessionOrder(array<uint32> user_session_order);
I like this approach.

nit: maybe make the parameter user_session_ids (or similar) to emphasize that
the ints are ids

https://codereview.chromium.org/2545723003/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2545723003/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_controller_client.cc:40: const UserList
logged_in_users = UserManager::Get()->GetLoggedInUsers();
Just to double-check: This order in this list doesn't change, so the IDs you
return are stable?

https://codereview.chromium.org/2545723003/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_controller_client.cc:276:
ConnectToSessionControllerAndSetClient();
Do you need this? You're not calling it in the other Send* methods.

[xiyuan, 2016-12-06 19:23:24]

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

[commit-bot: I haz the power, 2016-12-06 19:24:11]

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

[xiyuan, 2016-12-06 19:24:21]

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
File ash/common/session/session_controller.cc (right):

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller.cc:125: for (auto session_id :
user_session_order) {
On 2016/12/06 18:08:24, James Cook wrote:
> auto& or const auto& or use the real type in range-based-for-loops

Done.

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
File ash/common/session/session_controller.h (right):

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller.h:71: // Gets the user sessions.
On 2016/12/06 18:08:24, James Cook wrote:
> nit: Document the order (arbitrary or current user first?)

Done.

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller.h:117: uint32_t active_session_id_ = 0u;
On 2016/12/06 18:08:25, James Cook wrote:
> nit: document, since we have lots of uint32s and lots of types of ids. :-)

Done.

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
File ash/common/session/session_controller_unittest.cc (right):

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:21: namespace test {
On 2016/12/06 18:08:25, James Cook wrote:
> nit: Remove. I only use "namespace test" for test utility code, not for tests
> themselves.
> 
> optional: If possible put the entire test in the anonymous namespace so the
> linker has less work to do.

Done.

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:121:
DISALLOW_COPY_AND_ASSIGN(SessionControllerTest);
On 2016/12/06 18:08:25, James Cook wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:124:
TEST_F(SessionControllerTest, SimpleSessionInfo) {
On 2016/12/06 18:08:25, James Cook wrote:
> nit: Document what the test is testing (and below)

Done.

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:156: for (auto policy :
kTestCases) {
On 2016/12/06 18:08:25, James Cook wrote:
> auto& or use the underlying type

Done.

https://codereview.chromium.org/2545723003/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:236: }  // namespace ash
On 2016/12/06 18:08:25, James Cook wrote:
> Nice tests. Good coverage and easy to read.

:)

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:10: [Extensible]
On 2016/12/06 18:08:25, James Cook wrote:
> Per offline conversation I don't think these should be [extensible], at least
> not yet.

Done.

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:68: uint32 session_id;
On 2016/12/06 18:08:25, James Cook wrote:
> Is this a general id used throughout chrome, or just for the ash/chrome
> communication? Can you document what it means?

The concept of the user session id does not exist yet. Currently, the
UserManager tracks the user sessions in two list: GetLoggedInUsers and
GetLRULoggedInUsers. GetLoggedInUsers returns a list that corresponding the
order of user logins. The order of the list never changes. I will be moving
tracking code from UserManager to SessionManager. The SessionManager will
generate a session id when a user session is created during login. The session
id would be same as the 1 + index of the list returned from GetLoggedInUsers.
The session id would be used throughout chrome to reference user sessions after
the change.

Added documentation.

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:119: // its meta data (e.g. name,
avatar) is changed.
On 2016/12/06 18:08:25, James Cook wrote:
> nit: Comment that there is no method to remove a user because ash/chrome
doesn't
> support that -- all users are logged out at once.

Done.

https://codereview.chromium.org/2545723003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:124:
SetUserSessionOrder(array<uint32> user_session_order);
On 2016/12/06 18:08:25, James Cook wrote:
> I like this approach.
> 
> nit: maybe make the parameter user_session_ids (or similar) to emphasize that
> the ints are ids

Done.

https://codereview.chromium.org/2545723003/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2545723003/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_controller_client.cc:40: const UserList
logged_in_users = UserManager::Get()->GetLoggedInUsers();
On 2016/12/06 18:08:25, James Cook wrote:
> Just to double-check: This order in this list doesn't change, so the IDs you
> return are stable?

Correct.

GetLoggedInUsers() reflects the order of user logins. UserManager should only
append to the list. It would be a bug if UserManager change the list order. I
will be moving the user session tracking code from UserManager to
SessionManager. After that, the user session id would be explicitly generated.

https://codereview.chromium.org/2545723003/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_controller_client.cc:276:
ConnectToSessionControllerAndSetClient();
On 2016/12/06 18:08:25, James Cook wrote:
> Do you need this? You're not calling it in the other Send* methods.

Nope. Left overs from refactoring. Removed now.

[James Cook, 2016-12-06 19:27:00]

LGTM! Thanks for making this happen.

[xiyuan, 2016-12-06 19:30:16]

On 2016/12/06 19:27:00, James Cook wrote:
> LGTM! Thanks for making this happen.

Thanks for reviewing and educational comments. :)

[xiyuan, 2016-12-06 19:32:44]

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

[xiyuan, 2016-12-06 19:32:45]

dcheng@, could you review the mojo files?

    ash/public/interfaces/session_controller.mojom
    c/b/chrome_content_browser_manifest_overlay.json

Thanks.

[commit-bot: I haz the power, 2016-12-06 19:49:23]

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

[commit-bot: I haz the power, 2016-12-06 19:49:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiyuan, 2016-12-06 19:51:54]

Patchset #7 (id:120001) has been deleted

[xiyuan, 2016-12-06 19:52:38]

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

[commit-bot: I haz the power, 2016-12-06 19:53:01]

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, 2016-12-06 20:56:01]

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

[commit-bot: I haz the power, 2016-12-06 20:56:02]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-12-08 18:59:55]

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

[xiyuan, 2016-12-08 18:59:56]

+tsepez

tsepez@, could you help to review the mojo files since dcheng@ is busy?

    ash/public/interfaces/session_controller.mojom
    c/b/chrome_content_browser_manifest_overlay.json

Thanks.

[dcheng, 2016-12-08 19:03:33]

(Sorry, I was at an offsite yesterday, and this is not a very small CL. I'm
currently already reviewing)

[Tom Sepez, 2016-12-08 19:05:23]

https://codereview.chromium.org/2545723003/diff/140001/ash/public/interfaces/...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2545723003/diff/140001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:70: uint32 session_id;
Do we hand this out to less trustworthy clients than the browser?  If so, what
happens if they lie about this obviously guessable ID?

https://codereview.chromium.org/2545723003/diff/140001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:73: string serialized_account_id;
 // TODO(xiyuan): Use typemapping for AccountId.
same question here.

[dcheng, 2016-12-08 19:07:18]

On 2016/12/08 19:05:23, Tom Sepez wrote:
>
https://codereview.chromium.org/2545723003/diff/140001/ash/public/interfaces/...
> File ash/public/interfaces/session_controller.mojom (right):
> 
>
https://codereview.chromium.org/2545723003/diff/140001/ash/public/interfaces/...
> ash/public/interfaces/session_controller.mojom:70: uint32 session_id;
> Do we hand this out to less trustworthy clients than the browser?  If so, what
> happens if they lie about this obviously guessable ID?
> 
>
https://codereview.chromium.org/2545723003/diff/140001/ash/public/interfaces/...
> ash/public/interfaces/session_controller.mojom:73: string
serialized_account_id;
>  // TODO(xiyuan): Use typemapping for AccountId.
> same question here.

OK, since tsepez is also looking now, I'll defer to him.

But please ping on chat first next time; large CLs usually take me longer to
review, and if multiple reviewers start reviewing the same thing, it's not a
good use of reviewer's time. Thanks.

[xiyuan, 2016-12-08 19:26:20]

https://codereview.chromium.org/2545723003/diff/140001/ash/public/interfaces/...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2545723003/diff/140001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:70: uint32 session_id;
On 2016/12/08 19:05:23, Tom Sepez wrote:
> Do we hand this out to less trustworthy clients than the browser?  If so, what
> happens if they lie about this obviously guessable ID?

The info is passed from session manager code (chrome browser) to ash. Session
manager code does not take this from external sources as input. It generates the
it. Currently session manager code is not fully refactored and the id is
actually an index. But this can be changed later if needed.

Ash uses the id so that it can re-order the user session list when the active
user session is changed. Having a wrong id would cause that user session to be
skipped during the re-ordering, i.e. user will not be visible in user tray.

Did I answer your question?

https://codereview.chromium.org/2545723003/diff/140001/ash/public/interfaces/...
ash/public/interfaces/session_controller.mojom:73: string serialized_account_id;
 // TODO(xiyuan): Use typemapping for AccountId.
On 2016/12/08 19:05:23, Tom Sepez wrote:
> same question here.

This is part of the user info inside a user session. It is passed from chrome to
ash. Ash uses it to switch to a user session (we probably can refactor to keep
only the session id.). Lying about it would cause ash unable to switch user
because the fake account id would not be recognized on session manager (chrome)
end, i.e. the user with fake account id will not be switched to.

[Tom Sepez, 2016-12-08 21:47:49]

LGTM.

[xiyuan, 2016-12-08 22:33:58]

Thanks for reviewing.

Rebased and updated to use ash_util::GetAshServiceName to connect to ash. Will
submit...

[xiyuan, 2016-12-08 22:34:45]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-12-08 22:34:46]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
achuith@chromium.org, stevenjb@chromium.org, jamescook@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2545723003/#ps160001
(title: "rebase, use ash_util::GetAshServiceName")

[commit-bot: I haz the power, 2016-12-08 22:35:16]

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, 2016-12-09 01:22:45]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481236485785430,
"parent_rev": "e844f364e85b3c3f787b18d0d1bdc8ffe970e4d2", "commit_rev":
"ab7f562f704a44ef580c3e423e00659d5a6bd738"}

[commit-bot: I haz the power, 2016-12-09 01:23:16]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-12-09 01:26:12]

Description was changed from

==========
ash: Add SessionController/Client mojo interfaces

SessionController serves as an interface to maintain a session info
cache in ash. SessionControllerClient runs on the other side where
session manager code runs (chrome) and update SessionController with
session info. They are targeted to replace SessionStateDelegate to
use in both mash and classic ash.

BUG=648964,496761
==========

to

==========
ash: Add SessionController/Client mojo interfaces

SessionController serves as an interface to maintain a session info
cache in ash. SessionControllerClient runs on the other side where
session manager code runs (chrome) and update SessionController with
session info. They are targeted to replace SessionStateDelegate to
use in both mash and classic ash.

BUG=648964,496761

Committed: https://crrev.com/468b45fb766ba4da0e7ef3aaaa2d5a05d5f34602
Cr-Commit-Position: refs/heads/master@{#437407}
==========

[commit-bot: I haz the power, 2016-12-09 01:26:13]

Patchset 8 (id:??) landed as
https://crrev.com/468b45fb766ba4da0e7ef3aaaa2d5a05d5f34602
Cr-Commit-Position: refs/heads/master@{#437407}