cros: Use SessionController for lock starting code

cros: Use SessionController for lock starting code

- Add SessionController::StartLock to start locking on ash
  side and returns when post lock animation is finished and
  ash is fully locked, or aborted when ash exits before the
  lock code goes thread;
- Update ScreenLocker/WebUIScreenLocker to use StartLock instead
  of ash::LockStateController to start locking and observe
  lock animation;

BUG=678988
Review-Url: https://codereview.chromium.org/2830933002
Cr-Commit-Position: refs/heads/master@{#466159}
Committed: https://chromium.googlesource.com/chromium/src/+/7f80c700a7f188fc1aa30713c4a486138f957c69

[xiyuan, 2017-04-19 22:30:44]

Description was changed from

==========
cros: Update lock stating code to use SessionController

- Add SessionController::RunLockAnimation to start locking on
  ash side and returns when post lock animation finishes and
  ash is fully locked;
- Update ScreenLocker/WebUIScreenLocker to use RunLockAnimation
  instead of ash::LockStateController to start locking and observe
  lock animation;

BUG=678988
==========

to

==========
cros: Use SessionController for lock starting code

- Add SessionController::RunLockAnimation to start locking on
  ash side and returns when post lock animation finishes and
  ash is fully locked;
- Update ScreenLocker/WebUIScreenLocker to use RunLockAnimation
  instead of ash::LockStateController to start locking and observe
  lock animation;

BUG=678988
==========

[xiyuan, 2017-04-19 22:36:57]

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

[xiyuan, 2017-04-19 22:37:26]

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

[commit-bot: I haz the power, 2017-04-19 22:37:34]

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-19 23:13:12]

LGTM. Lemme know if you want me to look again after sorting out the test
failures.

https://codereview.chromium.org/2830933002/diff/20001/ash/session/session_con...
File ash/session/session_controller.cc (left):

https://codereview.chromium.org/2830933002/diff/20001/ash/session/session_con...
ash/session/session_controller.cc:217: // Shell could have no instance in tests.
Thanks for cleaning this up.

https://codereview.chromium.org/2830933002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/2830933002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:264:
->SetCapture(NULL);
nit: nullptr

https://codereview.chromium.org/2830933002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.h (right):

https://codereview.chromium.org/2830933002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.h:13: #include
"ash/wm/lock_state_observer.h"
Is this still needed?

[commit-bot: I haz the power, 2017-04-19 23:26:57]

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

[commit-bot: I haz the power, 2017-04-19 23:26:58]

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_...)

[Tom Sepez, 2017-04-20 01:25:00]

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

https://codereview.chromium.org/2830933002/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:147: // the post-lock animation
finishes and the system is fully locked. When
Can something go wrong and this fail to lock? Do we need status return?

[xiyuan, 2017-04-20 15:14:40]

The test failures are real. It could happen when lock fails (timeout somehow)
and we leave the mojo call un-returned. I would try adding a bool |locked| to
return and make sure we call it.

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

https://codereview.chromium.org/2830933002/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:147: // the post-lock animation
finishes and the system is fully locked. When
On 2017/04/20 01:25:00, Tom Sepez (OOO Wed - Thu) wrote:
> Can something go wrong and this fail to lock? Do we need status return?

LockStateController has a |lock_fail_timer_| to handle this case. The flow is
roughly like this:
1. LockStateController runs pre-lock animation and
   starts |lock_fail_timer_|
   (triggered by this call as ScreenLocker responses to the
    lock request);
2. Meanwhile, ScreenLocker loads the webui and sets LOCKED state
   when the webui loads and finishes initialization;
3. If LOCKED states comes before |lock_fail_timer_| fires,
   LockStateController runs post-lock animation and the callback
   will be called;
4. If |lock_fail_timer_| fires first, we bail (sign out the
   current user and go back to login screen);

This CL changes the lock starting part (Step 1) from a directly
LockStateController call to this mojo call. The rest of the flow should still
stand.

And, a status is necessary to handle the test failures and mojo DCHECK when
|lock_fail_timer_| fires. Thanks for the suggestion. Will add a bool |locked|.

[xiyuan, 2017-04-20 18:21:21]

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

[commit-bot: I haz the power, 2017-04-20 18:21:48]

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

[xiyuan, 2017-04-20 18:23:59]

Renamed RunLockAnimation to StartLock. Think it fits better with the added a
|locked| bool to return. Use this to fix the failed tests that exits without
waiting for the lock animation to finish.

PTAL. Thanks.

https://codereview.chromium.org/2830933002/diff/20001/ash/session/session_con...
File ash/session/session_controller.cc (left):

https://codereview.chromium.org/2830933002/diff/20001/ash/session/session_con...
ash/session/session_controller.cc:217: // Shell could have no instance in tests.
On 2017/04/19 23:13:12, James Cook wrote:
> Thanks for cleaning this up.

I have to put it back. SessionControllerTest derives directly from testing::Test
and has no Shell. :(

https://codereview.chromium.org/2830933002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/2830933002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:264:
->SetCapture(NULL);
On 2017/04/19 23:13:12, James Cook wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/2830933002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.h (right):

https://codereview.chromium.org/2830933002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.h:13: #include
"ash/wm/lock_state_observer.h"
On 2017/04/19 23:13:12, James Cook wrote:
> Is this still needed?

Nope. Removed.

[James Cook, 2017-04-20 19:25:21]

still lgtm

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

https://codereview.chromium.org/2830933002/diff/40001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:147: // |locked| == true means
that the post-lock animation finishes and ash is
super nit: finishes -> finished

[commit-bot: I haz the power, 2017-04-20 19:35:24]

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

[commit-bot: I haz the power, 2017-04-20 19:35:25]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-04-20 19:55:41]

lgtm

[xiyuan, 2017-04-20 21:00:26]

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

https://codereview.chromium.org/2830933002/diff/40001/ash/public/interfaces/s...
ash/public/interfaces/session_controller.mojom:147: // |locked| == true means
that the post-lock animation finishes and ash is
On 2017/04/20 19:25:21, James Cook wrote:
> super nit: finishes -> finished

Done.

[xiyuan, 2017-04-20 21:00:33]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-04-20 21:00:34]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2830933002/#ps60001
(title: "fix nit")

[xiyuan, 2017-04-20 21:00:42]

The CQ bit was unchecked by xiyuan@chromium.org

[xiyuan, 2017-04-20 21:02:19]

Description was changed from

==========
cros: Use SessionController for lock starting code

- Add SessionController::RunLockAnimation to start locking on
  ash side and returns when post lock animation finishes and
  ash is fully locked;
- Update ScreenLocker/WebUIScreenLocker to use RunLockAnimation
  instead of ash::LockStateController to start locking and observe
  lock animation;

BUG=678988
==========

to

==========
cros: Use SessionController for lock starting code

- Add SessionController::StartLock to start locking on ash
  side and returns when post lock animation is finished and
  ash is fully locked, or aborted when ash exits before the
  lock code goes thread;
- Update ScreenLocker/WebUIScreenLocker to use StartLock instead
  of ash::LockStateController to start locking and observe
  lock animation;

BUG=678988
==========

[xiyuan, 2017-04-20 21:02:49]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2017-04-20 21:04:00]

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-20 22:10:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492722169364840,
"parent_rev": "9ec28cc10b38e2692212f35cf5a4a3c84e749723", "commit_rev":
"7f80c700a7f188fc1aa30713c4a486138f957c69"}

[commit-bot: I haz the power, 2017-04-20 22:10:45]

Description was changed from

==========
cros: Use SessionController for lock starting code

- Add SessionController::StartLock to start locking on ash
  side and returns when post lock animation is finished and
  ash is fully locked, or aborted when ash exits before the
  lock code goes thread;
- Update ScreenLocker/WebUIScreenLocker to use StartLock instead
  of ash::LockStateController to start locking and observe
  lock animation;

BUG=678988
==========

to

==========
cros: Use SessionController for lock starting code

- Add SessionController::StartLock to start locking on ash
  side and returns when post lock animation is finished and
  ash is fully locked, or aborted when ash exits before the
  lock code goes thread;
- Update ScreenLocker/WebUIScreenLocker to use StartLock instead
  of ash::LockStateController to start locking and observe
  lock animation;

BUG=678988

Review-Url: https://codereview.chromium.org/2830933002
Cr-Commit-Position: refs/heads/master@{#466159}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f80c700a7f188fc1aa30713c4a4...
==========

[commit-bot: I haz the power, 2017-04-20 22:10:46]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7f80c700a7f188fc1aa30713c4a4...