mash: Shell ref clean up for screen lock

mash: Shell ref clean up for screen lock

- Skip manipulating active window state to exit full screen
  http://crbug.com/714677 tracks work for mash;
- Update LockWindow creation code to run on mash;
- Add a SessionController::NotifyChromeLockAnimationsComplete
  to replace the direct ash::Shell PowerEventObserver call;

BUG=678988
Review-Url: https://codereview.chromium.org/2836163002
Cr-Commit-Position: refs/heads/master@{#466814}
Committed: https://chromium.googlesource.com/chromium/src/+/84e360a7f1dddd9ce125ad6f1de090b7d207c569

[xiyuan, 2017-04-24 20:37:49]

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

[commit-bot: I haz the power, 2017-04-24 20:39:03]

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-24 20:39:18]

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

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

More direct ash::Shell call clean up in screen locking code.

jamescook@: Please review everything.

tsepez@: For mojom change.

Thanks.

[James Cook, 2017-04-24 20:50:23]

LGTM.

Aside: It's unfortunate that Chrome and Ash both need to know so much about the
details of the animation sequence. I wonder if someday more of this code could
move into ash.

[Tom Sepez, 2017-04-24 20:54:03]

otherwise lgtm

https://codereview.chromium.org/2836163002/diff/1/ash/public/interfaces/sessi...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2836163002/diff/1/ash/public/interfaces/sessi...
ash/public/interfaces/session_controller.mojom:147: // |locked| == true means
that the post-lock animation is finished and ash is
Is this comment still true in light of the new method?

[xiyuan, 2017-04-24 21:08:23]

https://codereview.chromium.org/2836163002/diff/1/ash/public/interfaces/sessi...
File ash/public/interfaces/session_controller.mojom (right):

https://codereview.chromium.org/2836163002/diff/1/ash/public/interfaces/sessi...
ash/public/interfaces/session_controller.mojom:147: // |locked| == true means
that the post-lock animation is finished and ash is
On 2017/04/24 20:54:03, Tom Sepez wrote:
> Is this comment still true in light of the new method?

Added "ash" before "post-lock" animation to clarify. This is only mean for
animations run by ash. The newly added one is the animation done in the webui
following the ash animation. 

The full event chain is something like this:
1. ash: pre lock animation (lift user desktop);
2. chrome: show webui, when init finishes, set session state LOCKED;
3.- ash: post lock animation (raise lock container);
4. chrome: footer bar fade in
5. ash: PowerEventObserver notifies power_manager

It is interleaved with ash and chrome actions. :( The callback of StartLock is
after 3 and chrome uses it to kick start 4. The added code in this CL is to kick
start 5 after 4. 

I wish we could simplify this too...

[James Cook, 2017-04-24 21:09:34]

On 2017/04/24 21:08:23, xiyuan wrote:
>
https://codereview.chromium.org/2836163002/diff/1/ash/public/interfaces/sessi...
> File ash/public/interfaces/session_controller.mojom (right):
> 
>
https://codereview.chromium.org/2836163002/diff/1/ash/public/interfaces/sessi...
> ash/public/interfaces/session_controller.mojom:147: // |locked| == true means
> that the post-lock animation is finished and ash is
> On 2017/04/24 20:54:03, Tom Sepez wrote:
> > Is this comment still true in light of the new method?
> 
> Added "ash" before "post-lock" animation to clarify. This is only mean for
> animations run by ash. The newly added one is the animation done in the webui
> following the ash animation. 
> 
> The full event chain is something like this:
> 1. ash: pre lock animation (lift user desktop);
> 2. chrome: show webui, when init finishes, set session state LOCKED;
> 3.- ash: post lock animation (raise lock container);
> 4. chrome: footer bar fade in
> 5. ash: PowerEventObserver notifies power_manager
> 
> It is interleaved with ash and chrome actions. :( The callback of StartLock is
> after 3 and chrome uses it to kick start 4. The added code in this CL is to
kick
> start 5 after 4. 
> 
> I wish we could simplify this too...

FYI jdufault has a proposal to convert shelf to native Views UI, so maybe the
footer bar fade-in step will go away (or at least not be webui)

[xiyuan, 2017-04-24 21:22:41]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-04-24 21:22:41]

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/2836163002/#ps20001
(title: "update comment")

[commit-bot: I haz the power, 2017-04-24 21:23:30]

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-24 22:14:59]

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

[commit-bot: I haz the power, 2017-04-24 22:15:00]

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-24 22:30:24]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-04-24 22:30:24]

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/2836163002/#ps40001
(title: "rebase, fix compile")

[commit-bot: I haz the power, 2017-04-24 22:31:13]

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-24 23:11:53]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493073024115090,
"parent_rev": "830317fadd15e973b40109059fb1eaaaa3874c90", "commit_rev":
"84e360a7f1dddd9ce125ad6f1de090b7d207c569"}

[commit-bot: I haz the power, 2017-04-24 23:12:03]

Description was changed from

==========
mash: Shell ref clean up for screen lock

- Skip manipulating active window state to exit full screen
  http://crbug.com/714677 tracks work for mash;
- Update LockWindow creation code to run on mash;
- Add a SessionController::NotifyChromeLockAnimationsComplete
  to replace the direct ash::Shell PowerEventObserver call;

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

to

==========
mash: Shell ref clean up for screen lock

- Skip manipulating active window state to exit full screen
  http://crbug.com/714677 tracks work for mash;
- Update LockWindow creation code to run on mash;
- Add a SessionController::NotifyChromeLockAnimationsComplete
  to replace the direct ash::Shell PowerEventObserver call;

BUG=678988

Review-Url: https://codereview.chromium.org/2836163002
Cr-Commit-Position: refs/heads/master@{#466814}
Committed:
https://chromium.googlesource.com/chromium/src/+/84e360a7f1dddd9ce125ad6f1de0...
==========

[commit-bot: I haz the power, 2017-04-24 23:12:04]

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