cros: Make sure views-based lock screen is destroyed after it is dismissed.

cros: Make sure views-based lock screen is destroyed after it is dismissed.

Add a CHECK to verify we only ever display one lock screen.

BUG=719015
Review-Url: https://codereview.chromium.org/2896093003
Cr-Commit-Position: refs/heads/master@{#478117}
Committed: https://chromium.googlesource.com/chromium/src/+/eb4c9f1ea4721f5d35f19a89481bb16560330a76

[jdufault, 2017-05-22 23:54:00]

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

[commit-bot: I haz the power, 2017-05-22 23:54:43]

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-05-23 01:14:56]

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

[commit-bot: I haz the power, 2017-05-23 01:14:57]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-05-23 19:06:03]

Description was changed from

==========
cros: Destroy views-lock screen is destroyed when dismissed.

Add a CHECK to verify we only ever display one lock screen.

BUG=
==========

to

==========
cros: Destroy views-lock screen is destroyed when dismissed.

Add a CHECK to verify we only ever display one lock screen.

BUG=719015
==========

[jdufault, 2017-06-05 20:27:51]

Description was changed from

==========
cros: Destroy views-lock screen is destroyed when dismissed.

Add a CHECK to verify we only ever display one lock screen.

BUG=719015
==========

to

==========
cros: Make sure views-based lock screen is destroyed after it is dismissed.

Add a CHECK to verify we only ever display one lock screen.

BUG=719015
==========

[jdufault, 2017-06-05 20:27:52]

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

[jdufault, 2017-06-05 20:28:41]

xiyuan@ PTAL, thanks.

[xiyuan, 2017-06-05 20:39:33]

https://codereview.chromium.org/2896093003/diff/1/ash/login/ui/lock_screen.h
File ash/login/ui/lock_screen.h (right):

https://codereview.chromium.org/2896093003/diff/1/ash/login/ui/lock_screen.h#...
ash/login/ui/lock_screen.h:19: ASH_EXPORT void DestroyLockScreen();
I would prefer to expose ShowLockScreen and DestroyLockScreen in
mojom::LockScreen and add the impl of both to LockScreenController.

[jdufault, 2017-06-06 18:10:26]

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

[commit-bot: I haz the power, 2017-06-06 18:11:33]

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-06-06 18:44:16]

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

[commit-bot: I haz the power, 2017-06-06 18:44:20]

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

[jdufault, 2017-06-06 20:23:48]

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

[jdufault, 2017-06-06 20:23:53]

Patchset #2 (id:20001) has been deleted

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

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-06-06 21:03:03]

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

[commit-bot: I haz the power, 2017-06-06 21:03:04]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-06-06 21:48:43]

https://codereview.chromium.org/2896093003/diff/1/ash/login/ui/lock_screen.h
File ash/login/ui/lock_screen.h (right):

https://codereview.chromium.org/2896093003/diff/1/ash/login/ui/lock_screen.h#...
ash/login/ui/lock_screen.h:19: ASH_EXPORT void DestroyLockScreen();
On 2017/06/05 20:39:33, xiyuan wrote:
> I would prefer to expose ShowLockScreen and DestroyLockScreen in
> mojom::LockScreen and add the impl of both to LockScreenController.

I've changed the approach to go through mojo. I've added a callback to
AuthenticateUser since I believe it makes sense for ash to decide when to
dismiss the lock screen (ideally chrome has little control over lock screen
life-time).

[xiyuan, 2017-06-06 22:37:52]

Cool.

https://codereview.chromium.org/2896093003/diff/40001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller.cc (right):

https://codereview.chromium.org/2896093003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:35: account_id, password,
authenticated_by_pin, std::move(callback)));
nit: DCHECK_EQ(1u, pending_user_auths_.size()), since there should be one auth
request on fly. Or you think we would need to support more than one?

https://codereview.chromium.org/2896093003/diff/40001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller_unittest.cc (right):

https://codereview.chromium.org/2896093003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller_unittest.cc:37: base::BindOnce([](bool
auth_success) {}));
nit: use mojom::LockScreenClient::AuthenticateUserCallback() to create a null
callback instead.

https://codereview.chromium.org/2896093003/diff/40001/ash/login/mock_lock_scr...
File ash/login/mock_lock_screen_client.h (right):

https://codereview.chromium.org/2896093003/diff/40001/ash/login/mock_lock_scr...
ash/login/mock_lock_screen_client.h:22: bool authenticate_user_callback_result =
true;
move this to private and add a setter instead.

https://codereview.chromium.org/2896093003/diff/40001/ash/login/ui/lock_scree...
File ash/login/ui/lock_screen.cc (right):

https://codereview.chromium.org/2896093003/diff/40001/ash/login/ui/lock_scree...
ash/login/ui/lock_screen.cc:18: LockWindow* window_ = nullptr;
Just call it |window| or |g_window|. No trailing "_".

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

https://codereview.chromium.org/2896093003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:321: user_context,
base::BindOnce([](bool auth_success) {}));
nit: Use ScreenLocker::AuthenticateCallback() to create a null callback since we
don't care.

[jdufault, 2017-06-06 23:52:05]

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

[commit-bot: I haz the power, 2017-06-06 23:52:26]

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-06-07 00:38:34]

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

[commit-bot: I haz the power, 2017-06-07 00:38:35]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-06-07 17:47:24]

Patchset #4 (id:80001) has been deleted

[jdufault, 2017-06-07 17:49:00]

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

[commit-bot: I haz the power, 2017-06-07 17:49:19]

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-06-07 18:49:00]

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

[commit-bot: I haz the power, 2017-06-07 18:49:01]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-06-07 18:58:35]

https://codereview.chromium.org/2896093003/diff/40001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller.cc (right):

https://codereview.chromium.org/2896093003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:35: account_id, password,
authenticated_by_pin, std::move(callback)));
On 2017/06/06 22:37:51, xiyuan wrote:
> nit: DCHECK_EQ(1u, pending_user_auths_.size()), since there should be one auth
> request on fly. Or you think we would need to support more than one?

Removed support for >1 concurrent auth.

https://codereview.chromium.org/2896093003/diff/40001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller_unittest.cc (right):

https://codereview.chromium.org/2896093003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller_unittest.cc:37: base::BindOnce([](bool
auth_success) {}));
On 2017/06/06 22:37:51, xiyuan wrote:
> nit: use mojom::LockScreenClient::AuthenticateUserCallback() to create a null
> callback instead.

Done.

https://codereview.chromium.org/2896093003/diff/40001/ash/login/mock_lock_scr...
File ash/login/mock_lock_screen_client.h (right):

https://codereview.chromium.org/2896093003/diff/40001/ash/login/mock_lock_scr...
ash/login/mock_lock_screen_client.h:22: bool authenticate_user_callback_result =
true;
On 2017/06/06 22:37:51, xiyuan wrote:
> move this to private and add a setter instead.

Done.

https://codereview.chromium.org/2896093003/diff/40001/ash/login/ui/lock_scree...
File ash/login/ui/lock_screen.cc (right):

https://codereview.chromium.org/2896093003/diff/40001/ash/login/ui/lock_scree...
ash/login/ui/lock_screen.cc:18: LockWindow* window_ = nullptr;
On 2017/06/06 22:37:52, xiyuan wrote:
> Just call it |window| or |g_window|. No trailing "_".

Done.

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

https://codereview.chromium.org/2896093003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:321: user_context,
base::BindOnce([](bool auth_success) {}));
On 2017/06/06 22:37:52, xiyuan wrote:
> nit: Use ScreenLocker::AuthenticateCallback() to create a null callback since
we
> don't care.

Done.

[xiyuan, 2017-06-07 19:02:28]

lgtm

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
File ash/login/mock_lock_screen_client.h (right):

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
ash/login/mock_lock_screen_client.h:24: }
nit: move this the last of all methods. Prevailing style is to put accessors at
the end.

[jdufault, 2017-06-07 22:45:16]

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

[commit-bot: I haz the power, 2017-06-07 22:45:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2017-06-07 22:53:21]

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
File ash/login/mock_lock_screen_client.h (right):

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
ash/login/mock_lock_screen_client.h:24: }
On 2017/06/07 19:02:28, xiyuan wrote:
> nit: move this the last of all methods. Prevailing style is to put accessors
at
> the end.

I've done some reordering. Does the accessor go after overrides? I recall
usually seeing overrides last.

[xiyuan, 2017-06-07 23:03:21]

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
File ash/login/mock_lock_screen_client.h (right):

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
ash/login/mock_lock_screen_client.h:24: }
On 2017/06/07 22:53:21, jdufault wrote:
> On 2017/06/07 19:02:28, xiyuan wrote:
> > nit: move this the last of all methods. Prevailing style is to put accessors
> at
> > the end.
> 
> I've done some reordering. Does the accessor go after overrides? I recall
> usually seeing overrides last.

I personally prefer to put accessors to the very last. But style guide does not
specify this. So I guess it is up to each person's taste.

[commit-bot: I haz the power, 2017-06-07 23:18:26]

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

[commit-bot: I haz the power, 2017-06-07 23:18:27]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-06-08 17:33:08]

jdufault@chromium.org changed reviewers:
+ rsesek@chromium.org

[jdufault, 2017-06-08 17:33:38]

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

[jdufault, 2017-06-08 17:36:06]

rsesek@ PTAL at *lock_screen.mojom
jamescook@ PTAL at c/b/ui/ash/*

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
File ash/login/mock_lock_screen_client.h (right):

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
ash/login/mock_lock_screen_client.h:24: }
On 2017/06/07 23:03:21, xiyuan wrote:
> On 2017/06/07 22:53:21, jdufault wrote:
> > On 2017/06/07 19:02:28, xiyuan wrote:
> > > nit: move this the last of all methods. Prevailing style is to put
accessors
> > at
> > > the end.
> > 
> > I've done some reordering. Does the accessor go after overrides? I recall
> > usually seeing overrides last.
> 
> I personally prefer to put accessors to the very last. But style guide does
not
> specify this. So I guess it is up to each person's taste. 

I'd prefer to leave as is - I can reorder if you feel strongly though.

[xiyuan, 2017-06-08 17:49:06]

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
File ash/login/mock_lock_screen_client.h (right):

https://codereview.chromium.org/2896093003/diff/100001/ash/login/mock_lock_sc...
ash/login/mock_lock_screen_client.h:24: }
On 2017/06/08 17:36:06, jdufault wrote:
> On 2017/06/07 23:03:21, xiyuan wrote:
> > On 2017/06/07 22:53:21, jdufault wrote:
> > > On 2017/06/07 19:02:28, xiyuan wrote:
> > > > nit: move this the last of all methods. Prevailing style is to put
> accessors
> > > at
> > > > the end.
> > > 
> > > I've done some reordering. Does the accessor go after overrides? I recall
> > > usually seeing overrides last.
> > 
> > I personally prefer to put accessors to the very last. But style guide does
> not
> > specify this. So I guess it is up to each person's taste. 
> 
> I'd prefer to leave as is - I can reorder if you feel strongly though.

Acknowledged.

[James Cook, 2017-06-08 19:47:13]

c/b/ui/ash lgtm, but I have some suggestions for the rest of the code

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
File ash/login/lock_screen_controller.cc (right):

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller.cc:26: ::ash::ShowLockScreen();
Aside: I wonder if this could become ash::LockScreen::Show() at some point.

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller.cc:71: // expects a base::Callback instance,
but |callback| is a base::OnceCallback.
It looks like GetSystemSalt could be changed to take OnceCallback. Don't do that
for this CL, but it might be worth fixing.

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
File ash/login/lock_screen_controller_unittest.cc (right):

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:18: }  // namespace
nit: This whole file should go in the anonymous namespace

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:40: }
I think this test (or another test) should verify that the callback is actually
called.

https://codereview.chromium.org/2896093003/diff/140001/ash/login/mock_lock_sc...
File ash/login/mock_lock_screen_client.h (right):

https://codereview.chromium.org/2896093003/diff/140001/ash/login/mock_lock_sc...
ash/login/mock_lock_screen_client.h:21: MOCK_METHOD4(AuthenticateUser_,
Is this "MethodName_" thing a gmock convention? I have never seen it before.

I am not an ash/login OWNER but it feels to me like either the object should use
MOCK_METHOD for the entire LockScreenClient interface, or it shouldn't use gmock
at all.

https://codereview.chromium.org/2896093003/diff/140001/ash/login/ui/lock_scre...
File ash/login/ui/lock_screen.h (right):

https://codereview.chromium.org/2896093003/diff/140001/ash/login/ui/lock_scre...
ash/login/ui/lock_screen.h:16: ASH_EXPORT bool ShowLockScreen();
optional: I think this would be more readable if it was in a namespace or
utility class, so it could be ash::lock_screen::Show[LockScreen] or
ash::LockScreen::Show()

[Robert Sesek, 2017-06-08 21:34:48]

mojom lgtm

[jdufault, 2017-06-08 22:22:57]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2017-06-08 22:22:57]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rsesek@chromium.org, jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2896093003/#ps160001
(title: "Address comments")

[jdufault, 2017-06-08 22:23:14]

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
File ash/login/lock_screen_controller.cc (right):

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller.cc:26: ::ash::ShowLockScreen();
On 2017/06/08 19:47:12, James Cook wrote:
> Aside: I wonder if this could become ash::LockScreen::Show() at some point.

Added a TODO

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller.cc:71: // expects a base::Callback instance,
but |callback| is a base::OnceCallback.
On 2017/06/08 19:47:12, James Cook wrote:
> It looks like GetSystemSalt could be changed to take OnceCallback. Don't do
that
> for this CL, but it might be worth fixing.

Yes, I investigated that. It requires changing dbus to also use OnceCallback, at
which point I decided it wasn't worth the effort.

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
File ash/login/lock_screen_controller_unittest.cc (right):

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:18: }  // namespace
On 2017/06/08 19:47:12, James Cook wrote:
> nit: This whole file should go in the anonymous namespace

Done, there doesn't seem to be a strong consensus here though.

https://codereview.chromium.org/2896093003/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:40: }
On 2017/06/08 19:47:12, James Cook wrote:
> I think this test (or another test) should verify that the callback is
actually
> called.

Done.

https://codereview.chromium.org/2896093003/diff/140001/ash/login/mock_lock_sc...
File ash/login/mock_lock_screen_client.h (right):

https://codereview.chromium.org/2896093003/diff/140001/ash/login/mock_lock_sc...
ash/login/mock_lock_screen_client.h:21: MOCK_METHOD4(AuthenticateUser_,
On 2017/06/08 19:47:12, James Cook wrote:
> Is this "MethodName_" thing a gmock convention? I have never seen it before.
> 
> I am not an ash/login OWNER but it feels to me like either the object should
use
> MOCK_METHOD for the entire LockScreenClient interface, or it shouldn't use
gmock
> at all.

It's a bit of a mess, but gmock doesn't support move-only types (ie,
AuthenticateUserCallback). See [1] for some context.

When writing tests you have to use AuthenticateUser_ instead of
AuthenticateUser. So the net result is that you still have gmock for the entire
interface.

It's a bit ugly, but since this interface has a decently large surface area
(there are more methods coming) in my eyes gmock is still a net win.

1: https://github.com/google/googletest/issues/395#issuecomment-125645943

https://codereview.chromium.org/2896093003/diff/140001/ash/login/ui/lock_scre...
File ash/login/ui/lock_screen.h (right):

https://codereview.chromium.org/2896093003/diff/140001/ash/login/ui/lock_scre...
ash/login/ui/lock_screen.h:16: ASH_EXPORT bool ShowLockScreen();
On 2017/06/08 19:47:13, James Cook wrote:
> optional: I think this would be more readable if it was in a namespace or
> utility class, so it could be ash::lock_screen::Show[LockScreen] or
> ash::LockScreen::Show()

Added TODO

[commit-bot: I haz the power, 2017-06-08 22:23:38]

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-06-08 23:08:33]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496960577597180,
"parent_rev": "aa0a64440d8045e9ee0962ed1a1d7386a24d5921", "commit_rev":
"eb4c9f1ea4721f5d35f19a89481bb16560330a76"}

[commit-bot: I haz the power, 2017-06-08 23:08:44]

Description was changed from

==========
cros: Make sure views-based lock screen is destroyed after it is dismissed.

Add a CHECK to verify we only ever display one lock screen.

BUG=719015
==========

to

==========
cros: Make sure views-based lock screen is destroyed after it is dismissed.

Add a CHECK to verify we only ever display one lock screen.

BUG=719015

Review-Url: https://codereview.chromium.org/2896093003
Cr-Commit-Position: refs/heads/master@{#478117}
Committed:
https://chromium.googlesource.com/chromium/src/+/eb4c9f1ea4721f5d35f19a89481b...
==========

[commit-bot: I haz the power, 2017-06-08 23:08:46]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/eb4c9f1ea4721f5d35f19a89481b...