mash: Run pre-unlock animation via SessionController

mash: Run pre-unlock animation via SessionController

- Replace LockStateController::OnLockScreenHide with mojo call
  SessionController.RunUnlockAnimation so that it works on mash;
- Implement lock state change detection in SessionController
  and remove relevant part in LoginLockStateNotifier that needs
  to talk directly to Shell;

BUG=679450
Review-Url: https://codereview.chromium.org/2801333002
Cr-Commit-Position: refs/heads/master@{#463295}
Committed: https://chromium.googlesource.com/chromium/src/+/7ebbf7f4312a35d83beb00e2bb66e57bf229d5f9

[xiyuan, 2017-04-07 17:15:38]

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

[commit-bot: I haz the power, 2017-04-07 17:16: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, 2017-04-07 17:40:55]

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

[commit-bot: I haz the power, 2017-04-07 17:40:56]

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, 2017-04-07 18:02:54]

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

[commit-bot: I haz the power, 2017-04-07 18:03:28]

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-07 18:33:07]

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

[commit-bot: I haz the power, 2017-04-07 18:33:08]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-04-07 19:20:24]

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

[James Cook, 2017-04-07 20:26:55]

A couple minor things and some questions.

Also, WDYT of writing a short (like 1-2 page) design doc on how we want session
state / screen lock to work in the long run? For now it's pretty clear we need
chrome to control screen unlock, but it would be nice in the long term if ash
could do more of it.

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

https://codereview.chromium.org/2801333002/diff/20001/ash/common/session/sess...
ash/common/session/session_controller.cc:84: // time, IsUserSessionBlocked needs
to return unblocked so that user windows
nit: "At that time" -> which time? Maybe "During the unlock animation"?

https://codereview.chromium.org/2801333002/diff/20001/ash/common/session/sess...
File ash/common/session/session_controller_unittest.cc (right):

https://codereview.chromium.org/2801333002/diff/20001/ash/common/session/sess...
ash/common/session/session_controller_unittest.cc:351: }
Thanks for adding a test.

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

https://codereview.chromium.org/2801333002/diff/20001/ash/common/session/sess...
ash/common/session/session_state_observer.h:36: virtual void
OnLockStateChanged(bool locked) {}
nit: Can this be LockStateChanged() to match the others? Or maybe the others
should be renamed to On*Changed (in a separate CL)?

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

https://codereview.chromium.org/2801333002/diff/20001/ash/shell.cc#newcode1242
ash/shell.cc:1242: observer.OnLockStateChanged(locked);
Aside: It's unfortunate that we have Shell observers for LockStateChanged and
also SessionStateObservers. Maybe not for this CL, but WDYT of converting
ShellObservers for OnLockStateChanged() to SessionStateObservers?

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

https://codereview.chromium.org/2801333002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:520:
session_manager::SessionManager::Get()->SetSessionState(
Just to double-check: If you click "sign out" from the lock screen, what
happens? Does that destroy the screen locker?

https://codereview.chromium.org/2801333002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/power/login_lock_state_notifier.cc (right):

https://codereview.chromium.org/2801333002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/power/login_lock_state_notifier.cc:18:
LoginLockStateNotifier::LoginLockStateNotifier() {
This isn't really a LockStateNotifier any more. It should probably be renamed
(or maybe folded into some other class that observes notifications).

[xiyuan, 2017-04-07 21:38:51]

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

[commit-bot: I haz the power, 2017-04-07 21:39:27]

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-07 21:50:01]

I will try to wrap up in a doc. Bad at writing so might take me some time. :p

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

https://codereview.chromium.org/2801333002/diff/20001/ash/common/session/sess...
ash/common/session/session_controller.cc:84: // time, IsUserSessionBlocked needs
to return unblocked so that user windows
On 2017/04/07 20:26:55, James Cook wrote:
> nit: "At that time" -> which time? Maybe "During the unlock animation"?

Done.

https://codereview.chromium.org/2801333002/diff/20001/ash/common/session/sess...
File ash/common/session/session_controller_unittest.cc (right):

https://codereview.chromium.org/2801333002/diff/20001/ash/common/session/sess...
ash/common/session/session_controller_unittest.cc:351: }
On 2017/04/07 20:26:55, James Cook wrote:
> Thanks for adding a test.

Because I know you will ask for it. :)

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

https://codereview.chromium.org/2801333002/diff/20001/ash/common/session/sess...
ash/common/session/session_state_observer.h:36: virtual void
OnLockStateChanged(bool locked) {}
On 2017/04/07 20:26:55, James Cook wrote:
> nit: Can this be LockStateChanged() to match the others? Or maybe the others
> should be renamed to On*Changed (in a separate CL)?

Changed to LockStateChanged().

I will do the rename to On*Changed in a separate CL.

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

https://codereview.chromium.org/2801333002/diff/20001/ash/shell.cc#newcode1242
ash/shell.cc:1242: observer.OnLockStateChanged(locked);
On 2017/04/07 20:26:55, James Cook wrote:
> Aside: It's unfortunate that we have Shell observers for LockStateChanged and
> also SessionStateObservers. Maybe not for this CL, but WDYT of converting
> ShellObservers for OnLockStateChanged() to SessionStateObservers?

SGTM. Added a TODO. I have thought about doing that. A follow up CL would be
better than in this CL.

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

https://codereview.chromium.org/2801333002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:520:
session_manager::SessionManager::Get()->SetSessionState(
On 2017/04/07 20:26:55, James Cook wrote:
> Just to double-check: If you click "sign out" from the lock screen, what
> happens? Does that destroy the screen locker?

ScreenLocker::Signout would be called to do a chrome::AttemptUserExit().
ScreenLocker would be visible during the shutdown. Think it is leaked in this
case.

This session state -> ACTIVE statement was here before I switch to use
SessionController. It is move to ScreenLocker::Hide to restore the active window
properly. This CL makes SessionController aware of the unlock animation starts
and included that in IsUserSessionBlocked. So this can be moved back to where it
was. :)

https://codereview.chromium.org/2801333002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/power/login_lock_state_notifier.cc (right):

https://codereview.chromium.org/2801333002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/power/login_lock_state_notifier.cc:18:
LoginLockStateNotifier::LoginLockStateNotifier() {
On 2017/04/07 20:26:55, James Cook wrote:
> This isn't really a LockStateNotifier any more. It should probably be renamed
> (or maybe folded into some other class that observes notifications).

Yes, my plan is to retire this class completely. I would probably move
NOTIFICATION_APP_TERMINATING into SessionControllerClient and add a mojo call to
forward it to ash.

[commit-bot: I haz the power, 2017-04-07 22:18:33]

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

[commit-bot: I haz the power, 2017-04-07 22:18:34]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-07 22:52:04]

LGTM

[xiyuan, 2017-04-07 23:20:33]

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

[xiyuan, 2017-04-07 23:20:34]

tsepez@, could you review session_controller.mojom change?

Thanks.

[Tom Sepez, 2017-04-07 23:21:55]

mojom LGTM

[xiyuan, 2017-04-07 23:24:30]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2017-04-07 23:25:21]

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-07 23:34:16]

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

[commit-bot: I haz the power, 2017-04-07 23:34:17]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xiyuan, 2017-04-10 15:46:45]

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

[xiyuan, 2017-04-10 15:46:46]

stevenjb@, could you do an owner review on
  c/b/notifications/login_state_notification_blocker_chromeos_unittest.cc

Thanks.

[stevenjb, 2017-04-10 15:52:45]

c/b/chromeos lgtm w/ question.

https://codereview.chromium.org/2801333002/diff/40001/chrome/browser/notifica...
File
chrome/browser/notifications/login_state_notification_blocker_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2801333002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/login_state_notification_blocker_chromeos_unittest.cc:70:
static_cast<ash::SessionStateObserver*>(ash::Shell::Get())
Why do we need the static_cast here?

[xiyuan, 2017-04-10 16:09:53]

https://codereview.chromium.org/2801333002/diff/40001/chrome/browser/notifica...
File
chrome/browser/notifications/login_state_notification_blocker_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2801333002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/login_state_notification_blocker_chromeos_unittest.cc:70:
static_cast<ash::SessionStateObserver*>(ash::Shell::Get())
On 2017/04/10 15:52:44, stevenjb wrote:
> Why do we need the static_cast here?

It is because LockStateChanged is now part of SessionStateObserver interface.
And Shell declared the interface in its private section. I moved
LockStateChanged to there. Hence, needed the cast. It is temporary. I will
update the test to not depend on ash.

[stevenjb, 2017-04-10 16:10:38]

Ah. I missed that the interface was private. Understood, still lgtm.


On Mon, Apr 10, 2017 at 10:09 AM, <xiyuan@chromium.org> wrote:

>
> https://codereview.chromium.org/2801333002/diff/40001/
> chrome/browser/notifications/login_state_notification_
> blocker_chromeos_unittest.cc
> File
> chrome/browser/notifications/login_state_notification_
> blocker_chromeos_unittest.cc
> (right):
>
> https://codereview.chromium.org/2801333002/diff/40001/
> chrome/browser/notifications/login_state_notification_
> blocker_chromeos_unittest.cc#newcode70
> chrome/browser/notifications/login_state_notification_
> blocker_chromeos_unittest.cc:70:
> static_cast<ash::SessionStateObserver*>(ash::Shell::Get())
> On 2017/04/10 15:52:44, stevenjb wrote:
> > Why do we need the static_cast here?
>
> It is because LockStateChanged is now part of SessionStateObserver
> interface. And Shell declared the interface in its private section. I
> moved LockStateChanged to there. Hence, needed the cast. It is
> temporary. I will update the test to not depend on ash.
>
> https://codereview.chromium.org/2801333002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xiyuan, 2017-04-10 16:12:01]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-04-10 16:12:01]

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

[commit-bot: I haz the power, 2017-04-10 16:12:33]

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-10 16:49:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491840721290180,
"parent_rev": "7d68ce72674bc9189bd8bcf305823d75c044489d", "commit_rev":
"7ebbf7f4312a35d83beb00e2bb66e57bf229d5f9"}

[commit-bot: I haz the power, 2017-04-10 16:50:05]

Description was changed from

==========
mash: Run pre-unlock animation via SessionController

- Replace LockStateController::OnLockScreenHide with mojo call
  SessionController.RunUnlockAnimation so that it works on mash;
- Implement lock state change detection in SessionController
  and remove relevant part in LoginLockStateNotifier that needs
  to talk directly to Shell;

BUG=679450
==========

to

==========
mash: Run pre-unlock animation via SessionController

- Replace LockStateController::OnLockScreenHide with mojo call
  SessionController.RunUnlockAnimation so that it works on mash;
- Implement lock state change detection in SessionController
  and remove relevant part in LoginLockStateNotifier that needs
  to talk directly to Shell;

BUG=679450

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

[commit-bot: I haz the power, 2017-04-10 16:50:07]

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