ash: Merge LoginStatus update code path

ash: Merge LoginStatus update code path

- Add LoginStatusChanged to SessionStateObserver;
- Make SessionController fire LoginStatusChanged;
- Shell call UpdateAfterLoginStatusChange on LoginStatusChanged;
- Remove Shell::OnLoginStateChanged and notify ShellObserver in
  Shell::LoginStatusChanged;
- Remove UpdateAfterLoginStatusChange calls from SystemTrayDelegateChromeOS
  and TestSessionControllerClient;
- Remove Shell::OnLoginStateChanged calls from LoginLockStateNotifier
  since LoginStatusChanged should cover that;
- Fix ash_shell_with_content that SessionController is not bound
  to its client;

BUG=701193, 679450
Review-Url: https://codereview.chromium.org/2797743002
Cr-Commit-Position: refs/heads/master@{#462378}
Committed: https://chromium.googlesource.com/chromium/src/+/233f4e71bd49e79e477ef3ba814d53e8036e3c50

[xiyuan, 2017-04-04 16:24:26]

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

[commit-bot: I haz the power, 2017-04-04 16:24:54]

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-04-04 17:35:02]

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

[commit-bot: I haz the power, 2017-04-04 17:35:04]

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

[xiyuan, 2017-04-04 20:00:09]

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

[commit-bot: I haz the power, 2017-04-04 20:01:11]

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-04-04 20:37:16]

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

[commit-bot: I haz the power, 2017-04-04 20:37:17]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-04-04 22:09:36]

Description was changed from

==========
ash: Merge LoginStatus update code path

- Add LoginStatusChanged to SessionStateObserver;
- Make SessionController fire LoginStatusChanged;
- Shell call UpdateAfterLoginStatusChange on LoginStatusChanged;
- Remove Shell::OnLoginStateChanged and notify ShellObserver in
  LoginStatusChanged;
- Remove UpdateAfterLoginStatusChange from SystemTrayDelegateChromeOS
  and TestSessionControllerClient;
- Fix ash_shell_with_content that SessionController is not bound
  to its client;

BUG=701193,679450
==========

to

==========
ash: Merge LoginStatus update code path

- Add LoginStatusChanged to SessionStateObserver;
- Make SessionController fire LoginStatusChanged;
- Shell call UpdateAfterLoginStatusChange on LoginStatusChanged;
- Remove Shell::OnLoginStateChanged and notify ShellObserver in
  Shell::LoginStatusChanged;
- Remove UpdateAfterLoginStatusChange calls from SystemTrayDelegateChromeOS
  and TestSessionControllerClient;
- Remove Shell::OnLoginStateChanged calls from LoginLockStateNotifier
  since LoginStatusChanged should cover that;
- Fix ash_shell_with_content that SessionController is not bound
  to its client;

BUG=701193,679450
==========

[xiyuan, 2017-04-04 22:16:45]

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

[xiyuan, 2017-04-04 22:16:46]

Could you help to review this?

Removes Shell::UpdateAfterLoginStatusChange and Shell::OnLoginStateChanged calls
from chrome. SessionController maintains ash::LoginStatus and update its
observers to achieve the same functionality. This would make cash and mash using
the same code path. The timing might change a bit but there should be no
behavior change.

[msw, 2017-04-05 00:15:47]

Wow, it's a bit complex, and tough to say for sure that everything should behave
the same, but I don't really have any specific concerns. lgtm with minor nits.

https://codereview.chromium.org/2797743002/diff/20001/ash/shell/content/clien...
File ash/shell/content/client/shell_browser_main_parts.cc (left):

https://codereview.chromium.org/2797743002/diff/20001/ash/shell/content/clien...
ash/shell/content/client/shell_browser_main_parts.cc:143:
ash::Shell::Get()->CreateShelfView();
I guess this isn't needed because it's called by Shell::SessionStateChanged?
Maybe update the comment above (remove "before creating shelf")?

https://codereview.chromium.org/2797743002/diff/20001/ash/shell/example_sessi...
File ash/shell/example_session_controller_client.cc (right):

https://codereview.chromium.org/2797743002/diff/20001/ash/shell/example_sessi...
ash/shell/example_session_controller_client.cc:39: InitializeAndBind();
nit: add a comment?

[James Cook, 2017-04-05 00:16:23]

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

[James Cook, 2017-04-05 00:16:24]

Drive-by comments. Code looks good. I presume screen lock works OK manually.

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

https://codereview.chromium.org/2797743002/diff/20001/ash/common/session/sess...
ash/common/session/session_controller.h:99: LoginStatus GetLoginStatus() const;
optional: Maybe change to inline login_status() getter?

https://codereview.chromium.org/2797743002/diff/20001/ash/common/session/sess...
File ash/common/session/session_state_observer.h (right):

https://codereview.chromium.org/2797743002/diff/20001/ash/common/session/sess...
ash/common/session/session_state_observer.h:32: // Called when login status is
changed.
mention that |login_status| is the new status

https://codereview.chromium.org/2797743002/diff/20001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller.cc (left):

https://codereview.chromium.org/2797743002/diff/20001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller.cc:194: void
LockStateController::OnLoginStateChanged(LoginStatus status) {
Just to double check... This is safe to remove because OnLockStateChanged() is
always called?

Was this needed in the old code?

https://codereview.chromium.org/2797743002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right):

https://codereview.chromium.org/2797743002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:152:
SessionControllerClient::FlushForTesting();
Can this be done just when the state is changed? It's a little weird that it's
flushed on every read.

https://codereview.chromium.org/2797743002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:1426:
base::RunLoop().RunUntilIdle();  // Flush session state to ash.
optional: This would be better as SessionControllerClient::FlushForTesting().

[xiyuan, 2017-04-05 22:28:58]

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

[xiyuan, 2017-04-05 22:29:07]

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

https://codereview.chromium.org/2797743002/diff/20001/ash/common/session/sess...
ash/common/session/session_controller.h:99: LoginStatus GetLoginStatus() const;
On 2017/04/05 00:16:24, James Cook wrote:
> optional: Maybe change to inline login_status() getter?

Done.

https://codereview.chromium.org/2797743002/diff/20001/ash/common/session/sess...
File ash/common/session/session_state_observer.h (right):

https://codereview.chromium.org/2797743002/diff/20001/ash/common/session/sess...
ash/common/session/session_state_observer.h:32: // Called when login status is
changed.
On 2017/04/05 00:16:24, James Cook wrote:
> mention that |login_status| is the new status

Done.

https://codereview.chromium.org/2797743002/diff/20001/ash/shell/content/clien...
File ash/shell/content/client/shell_browser_main_parts.cc (left):

https://codereview.chromium.org/2797743002/diff/20001/ash/shell/content/clien...
ash/shell/content/client/shell_browser_main_parts.cc:143:
ash::Shell::Get()->CreateShelfView();
On 2017/04/05 00:15:47, msw wrote:
> I guess this isn't needed because it's called by Shell::SessionStateChanged?
> Maybe update the comment above (remove "before creating shelf")?

Yep. We no longer need to special handling this.

Comments updated.

https://codereview.chromium.org/2797743002/diff/20001/ash/shell/example_sessi...
File ash/shell/example_session_controller_client.cc (right):

https://codereview.chromium.org/2797743002/diff/20001/ash/shell/example_sessi...
ash/shell/example_session_controller_client.cc:39: InitializeAndBind();
On 2017/04/05 00:15:47, msw wrote:
> nit: add a comment?

Done.

https://codereview.chromium.org/2797743002/diff/20001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller.cc (left):

https://codereview.chromium.org/2797743002/diff/20001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller.cc:194: void
LockStateController::OnLoginStateChanged(LoginStatus status) {
On 2017/04/05 00:16:24, James Cook wrote:
> Just to double check... This is safe to remove because OnLockStateChanged() is
> always called?
> 
> Was this needed in the old code?

IIUC, this function is not actually needed. |login_status_| is only set here and
no one uses it. |system_is_locked_| is updated in OnLockStateChanged(). The only
way this can be useful is to set |system_is_locked_| to final state early to
skip the work in OnLockStateChanged(). But it would be disaster when it happens
because it skips post(un)lock animations and leaves the UI in a bad state.

OnLockStateChanged() already ensures the animations run once when
|system_is_locked_| changes. We don't need this function to mess with the state
so skip that.

The old code works because OnLoginStateChanged is only called when a user signs
in and not called when login status changes. So we don't see the two fight with
each other. This fight happens after I chain OnLoginStateChanged from login
status change.

https://codereview.chromium.org/2797743002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right):

https://codereview.chromium.org/2797743002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:152:
SessionControllerClient::FlushForTesting();
On 2017/04/05 00:16:24, James Cook wrote:
> Can this be done just when the state is changed? It's a little weird that it's
> flushed on every read.

Moved this to where we call SessionStarted(). Done.

https://codereview.chromium.org/2797743002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:1426:
base::RunLoop().RunUntilIdle();  // Flush session state to ash.
On 2017/04/05 00:16:24, James Cook wrote:
> optional: This would be better as SessionControllerClient::FlushForTesting().

Done.

[commit-bot: I haz the power, 2017-04-05 22:29:47]

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

[James Cook, 2017-04-05 23:26:44]

LGTM!

[commit-bot: I haz the power, 2017-04-05 23:48:57]

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

[commit-bot: I haz the power, 2017-04-05 23:48:57]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-04-06 06:19:15]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-04-06 06:19:16]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2797743002/#ps40001
(title: "for #2 comments")

[commit-bot: I haz the power, 2017-04-06 06:29:45]

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-04-06 07:00:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491459555476670,
"parent_rev": "25809e1270b9eb4103f1efcc089c781df8465e39", "commit_rev":
"233f4e71bd49e79e477ef3ba814d53e8036e3c50"}

[commit-bot: I haz the power, 2017-04-06 07:00:51]

Description was changed from

==========
ash: Merge LoginStatus update code path

- Add LoginStatusChanged to SessionStateObserver;
- Make SessionController fire LoginStatusChanged;
- Shell call UpdateAfterLoginStatusChange on LoginStatusChanged;
- Remove Shell::OnLoginStateChanged and notify ShellObserver in
  Shell::LoginStatusChanged;
- Remove UpdateAfterLoginStatusChange calls from SystemTrayDelegateChromeOS
  and TestSessionControllerClient;
- Remove Shell::OnLoginStateChanged calls from LoginLockStateNotifier
  since LoginStatusChanged should cover that;
- Fix ash_shell_with_content that SessionController is not bound
  to its client;

BUG=701193,679450
==========

to

==========
ash: Merge LoginStatus update code path

- Add LoginStatusChanged to SessionStateObserver;
- Make SessionController fire LoginStatusChanged;
- Shell call UpdateAfterLoginStatusChange on LoginStatusChanged;
- Remove Shell::OnLoginStateChanged and notify ShellObserver in
  Shell::LoginStatusChanged;
- Remove UpdateAfterLoginStatusChange calls from SystemTrayDelegateChromeOS
  and TestSessionControllerClient;
- Remove Shell::OnLoginStateChanged calls from LoginLockStateNotifier
  since LoginStatusChanged should cover that;
- Fix ash_shell_with_content that SessionController is not bound
  to its client;

BUG=701193,679450

Review-Url: https://codereview.chromium.org/2797743002
Cr-Commit-Position: refs/heads/master@{#462378}
Committed:
https://chromium.googlesource.com/chromium/src/+/233f4e71bd49e79e477ef3ba814d...
==========

[commit-bot: I haz the power, 2017-04-06 07:00:53]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/233f4e71bd49e79e477ef3ba814d...