cros: Fix unlock crash with multi-display

cros: Fix unlock crash with multi-display

SessionController::RunUnlockAnimation expects the callback to be
invoked only once. SessionStateAnimatorImpl uses the callback per
animation per container. Mulit-display would have multiple containers
involved hence multiple animation and callbacks. The multiple
callback would cause a mojo CHECK failure. SessionStateAnimator
should only run the callback once after all animations finish.
The CL use a BarrierClosure to combine the animation callbacks.

BUG=712422
TEST=SessionStateAnimatiorImplContainersTest.AnimationCallbackOnMultiDisplay

Review-Url: https://codereview.chromium.org/2823343006
Cr-Commit-Position: refs/heads/master@{#465670}
Committed: https://chromium.googlesource.com/chromium/src/+/52558bf8b2796a8e97c73a2351e23a1f8c45e423

[xiyuan, 2017-04-19 08:24:32]

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

[commit-bot: I haz the power, 2017-04-19 08:24:58]

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-19 08:25:48]

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

[commit-bot: I haz the power, 2017-04-19 08:55:37]

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

[commit-bot: I haz the power, 2017-04-19 08:55:38]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-04-19 16:04:42]

I confirm this fix works. LGTM.

[James Cook, 2017-04-19 16:41:45]

LGTM. Nice analysis afakhry!

https://codereview.chromium.org/2823343006/diff/20001/ash/wm/session_state_an...
File ash/wm/session_state_animator.h (right):

https://codereview.chromium.org/2823343006/diff/20001/ash/wm/session_state_an...
ash/wm/session_state_animator.h:200: base::Closure callback) = 0;
Darn. It would be nice if this was a base::OnceCallback<> but it doesn't look
like BarrierClosure supports that.

https://codereview.chromium.org/2823343006/diff/20001/ash/wm/session_state_an...
File ash/wm/session_state_animator_impl_unittest.cc (right):

https://codereview.chromium.org/2823343006/diff/20001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl_unittest.cc:86:
TEST_F(SessionStateAnimatiorImplContainersTest,
nit: Reference the bug or explain what this is testing.

Thanks for adding a test!

[xiyuan, 2017-04-19 17:30:19]

https://codereview.chromium.org/2823343006/diff/20001/ash/wm/session_state_an...
File ash/wm/session_state_animator.h (right):

https://codereview.chromium.org/2823343006/diff/20001/ash/wm/session_state_an...
ash/wm/session_state_animator.h:200: base::Closure callback) = 0;
On 2017/04/19 16:41:45, James Cook wrote:
> Darn. It would be nice if this was a base::OnceCallback<> but it doesn't look
> like BarrierClosure supports that.

I was wondering about that too. But the problem is whether BarrierClosure can
assume |done_closure| to be a OnceCallback. That may or may not be true.

https://codereview.chromium.org/2823343006/diff/20001/ash/wm/session_state_an...
File ash/wm/session_state_animator_impl_unittest.cc (right):

https://codereview.chromium.org/2823343006/diff/20001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl_unittest.cc:86:
TEST_F(SessionStateAnimatiorImplContainersTest,
On 2017/04/19 16:41:45, James Cook wrote:
> nit: Reference the bug or explain what this is testing.
> 
> Thanks for adding a test!

Done.

[xiyuan, 2017-04-19 17:30:24]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-04-19 17:30:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
afakhry@chromium.org, jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2823343006/#ps40001
(title: "add comment for test")

[commit-bot: I haz the power, 2017-04-19 17:30:49]

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-19 18:09:22]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492623024843680,
"parent_rev": "79d9b86732316df528fbfe72409465e9bf2668b1", "commit_rev":
"52558bf8b2796a8e97c73a2351e23a1f8c45e423"}

[commit-bot: I haz the power, 2017-04-19 18:09:35]

Description was changed from

==========
cros: Fix unlock crash with multi-display

SessionController::RunUnlockAnimation expects the callback to be
invoked only once. SessionStateAnimatorImpl uses the callback per
animation per container. Mulit-display would have multiple containers
involved hence multiple animation and callbacks. The multiple
callback would cause a mojo CHECK failure. SessionStateAnimator
should only run the callback once after all animations finish.
The CL use a BarrierClosure to combine the animation callbacks.

BUG=712422
TEST=SessionStateAnimatiorImplContainersTest.AnimationCallbackOnMultiDisplay
==========

to

==========
cros: Fix unlock crash with multi-display

SessionController::RunUnlockAnimation expects the callback to be
invoked only once. SessionStateAnimatorImpl uses the callback per
animation per container. Mulit-display would have multiple containers
involved hence multiple animation and callbacks. The multiple
callback would cause a mojo CHECK failure. SessionStateAnimator
should only run the callback once after all animations finish.
The CL use a BarrierClosure to combine the animation callbacks.

BUG=712422
TEST=SessionStateAnimatiorImplContainersTest.AnimationCallbackOnMultiDisplay

Review-Url: https://codereview.chromium.org/2823343006
Cr-Commit-Position: refs/heads/master@{#465670}
Committed:
https://chromium.googlesource.com/chromium/src/+/52558bf8b2796a8e97c73a2351e2...
==========

[commit-bot: I haz the power, 2017-04-19 18:09:36]

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