mojo api for view based lockscreen

mojo api for view based lockscreen.

BUG=721524
Review-Url: https://codereview.chromium.org/2876673002
Cr-Commit-Position: refs/heads/master@{#472966}
Committed: https://chromium.googlesource.com/chromium/src/+/2bbdd107bcc11f6b41469249dfc51bac7b25eb01

[xiaoyinh(OOO Sep 11-29), 2017-05-11 00:06:14]

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

[commit-bot: I haz the power, 2017-05-11 00:07:31]

Dry run: CQ is trying da patch.

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

[xiaoyinh(OOO Sep 11-29), 2017-05-11 00:14:35]

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

[xiaoyinh(OOO Sep 11-29), 2017-05-11 00:14:35]

jdufault@, could you help to take a look? This has a very simple mojo interface
as the initial version, please let me know if the overall approach looks
reasonable.

[commit-bot: I haz the power, 2017-05-11 00:25:27]

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

[commit-bot: I haz the power, 2017-05-11 00:25:27]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdufault, 2017-05-11 01:14:51]

General approach seems good

https://codereview.chromium.org/2876673002/diff/1/ash/login/ui/lock_contents_...
File ash/login/ui/lock_contents_view.cc (right):

https://codereview.chromium.org/2876673002/diff/1/ash/login/ui/lock_contents_...
ash/login/ui/lock_contents_view.cc:40: const int user_index = 0;
nit: kUserIndex

https://codereview.chromium.org/2876673002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2876673002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/lock/screen_locker.cc:298:
LockScreenClientChromeOS::Get()->ShowErrorMessage();
Can you make another delegate_ implementation that calls the mojo methods? Then
if we are showing views lock screen, install the new delegate_ and hopefully we
will only need minimal code changes here.

[xiaoyinh(OOO Sep 11-29), 2017-05-11 16:47:05]

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

[commit-bot: I haz the power, 2017-05-11 16:47:58]

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-11 18:34:51]

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

[commit-bot: I haz the power, 2017-05-11 18:34:52]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2017-05-11 20:29:52]

https://codereview.chromium.org/2876673002/diff/1/ash/login/ui/lock_contents_...
File ash/login/ui/lock_contents_view.cc (right):

https://codereview.chromium.org/2876673002/diff/1/ash/login/ui/lock_contents_...
ash/login/ui/lock_contents_view.cc:40: const int user_index = 0;
On 2017/05/11 01:14:51, jdufault wrote:
> nit: kUserIndex

Done.

https://codereview.chromium.org/2876673002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2876673002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/lock/screen_locker.cc:298:
LockScreenClientChromeOS::Get()->ShowErrorMessage();
On 2017/05/11 01:14:51, jdufault wrote:
> Can you make another delegate_ implementation that calls the mojo methods?
Then
> if we are showing views lock screen, install the new delegate_ and hopefully
we
> will only need minimal code changes here.

How about reuse StubDelegate in the above?

[jdufault, 2017-05-11 20:34:06]

https://codereview.chromium.org/2876673002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2876673002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/lock/screen_locker.cc:298:
LockScreenClientChromeOS::Get()->ShowErrorMessage();
On 2017/05/11 20:29:52, xiaoyinh wrote:
> On 2017/05/11 01:14:51, jdufault wrote:
> > Can you make another delegate_ implementation that calls the mojo methods?
> Then
> > if we are showing views lock screen, install the new delegate_ and hopefully
> we
> > will only need minimal code changes here.
> 
> How about reuse StubDelegate in the above?

sg, please rename it to MojoDelegate or similar.

[xiaoyinh(OOO Sep 11-29), 2017-05-11 20:45:13]

Description was changed from

==========
mojo api for view based lockscreen.

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

to

==========
mojo api for view based lockscreen.

BUG=721524
==========

[xiaoyinh(OOO Sep 11-29), 2017-05-11 20:46:33]

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

[xiaoyinh(OOO Sep 11-29), 2017-05-11 20:46:35]

xiyuan@, jamescook@, could you take a look and let me know?

[xiyuan, 2017-05-11 21:17:33]

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:15: ShowErrorMessage();
Do we need the error message to be displayed?

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:21: // Request user authentication in
chrome.
nit: Document what happens when auth succeeds or fails.

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:23: string password,
Transfer plain password over mojo makes me nervous. We probably should do the
key hashing before sending it back.

[xiaoyinh(OOO Sep 11-29), 2017-05-13 00:19:50]

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

[commit-bot: I haz the power, 2017-05-13 00:20:32]

Dry run: CQ is trying da patch.

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

[xiaoyinh(OOO Sep 11-29), 2017-05-13 00:29:14]

On 2017/05/11 20:34:06, jdufault wrote:
>
https://codereview.chromium.org/2876673002/diff/1/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/lock/screen_locker.cc (right):
> 
>
https://codereview.chromium.org/2876673002/diff/1/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/lock/screen_locker.cc:298:
> LockScreenClientChromeOS::Get()->ShowErrorMessage();
> On 2017/05/11 20:29:52, xiaoyinh wrote:
> > On 2017/05/11 01:14:51, jdufault wrote:
> > > Can you make another delegate_ implementation that calls the mojo methods?
> > Then
> > > if we are showing views lock screen, install the new delegate_ and
hopefully
> > we
> > > will only need minimal code changes here.
> > 
> > How about reuse StubDelegate in the above?
> 
> sg, please rename it to MojoDelegate or similar.

Done. Thanks!

[xiaoyinh(OOO Sep 11-29), 2017-05-13 00:29:49]

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:15: ShowErrorMessage();
On 2017/05/11 21:17:33, xiyuan wrote:
> Do we need the error message to be displayed?

chrome will request to show error message when the unlock authentication is
failed. I think this will be needed as long as we are  authenticating in chrome.

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:21: // Request user authentication in
chrome.
On 2017/05/11 21:17:33, xiyuan wrote:
> nit: Document what happens when auth succeeds or fails.

Done.

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:23: string password,
On 2017/05/11 21:17:33, xiyuan wrote:
> Transfer plain password over mojo makes me nervous. We probably should do the
> key hashing before sending it back.

Thanks. The new patch hash the password in LockScreenController before sending
it to chrome. In order to get the system salt, I add +chromeos/cryptohome to
ash/DEPS. Please let me know if this is a bad idea.

[jdufault, 2017-05-13 00:57:40]

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

https://codereview.chromium.org/2876673002/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:26: // Hash password before sending through
mojo.
Instead of caching system_salt_ you can call through GetSystemSalt here so that
we always know system salt is available. Otherwise, it is possible this will run
before we have the system salt. This is unlikely on lock screen but we should
consider it for sign-in screen.

ie,

   chromeos::SystemSaltGetter::Get()->GetSystemSalt(
       base::Bind(&DoAuthenticateUser, account_id, password, ...));

https://codereview.chromium.org/2876673002/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:28: chromeos::Key key(password);
Unfortunately PIN is hashed differently. See [1]. Feel free to add a TODO.

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlo...

https://codereview.chromium.org/2876673002/diff/40001/ash/login/ui/lock_conte...
File ash/login/ui/lock_contents_view.cc (right):

https://codereview.chromium.org/2876673002/diff/40001/ash/login/ui/lock_conte...
ash/login/ui/lock_contents_view.cc:46: user_session->account_id, std::string(),
false);
nit: false /*authenticated_by_pin*/

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

https://codereview.chromium.org/2876673002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:243: // Create delegate that
will eventually call into the views-based lock
nit: Can you update the comment?

// Create delegate that calls into the views-based lock screen via mojo.

[commit-bot: I haz the power, 2017-05-13 01:33:46]

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

[commit-bot: I haz the power, 2017-05-13 01:33:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sadrul, 2017-05-13 05:16:47]

sadrul@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2017-05-13 05:16:47]

Any chance this new lockscreen could live out of ash? It could live in, for
example, //mash/login (or //mash/lockscreen, etc.). For now, it would just be a
library (with its dependencies on aura, views, etc.) that ash uses directly. But
it would make it much easier to turn this into a separate mus-app in the future.

[jdufault, 2017-05-15 17:35:54]

On 2017/05/13 05:16:47, sadrul wrote:
> Any chance this new lockscreen could live out of ash? It could live in, for
> example, //mash/login (or //mash/lockscreen, etc.). For now, it would just be
a
> library (with its dependencies on aura, views, etc.) that ash uses directly.
But
> it would make it much easier to turn this into a separate mus-app in the
future.

I can investigate this; I initially put the lock screen code in
//ui/chromeos/login but there were issues with using session_manager and the
like.

It'd be good not to block this CL on moving the code though.

[sadrul, 2017-05-15 18:22:11]

On 2017/05/15 17:35:54, jdufault wrote:
> On 2017/05/13 05:16:47, sadrul wrote:
> > Any chance this new lockscreen could live out of ash? It could live in, for
> > example, //mash/login (or //mash/lockscreen, etc.). For now, it would just
be
> a
> > library (with its dependencies on aura, views, etc.) that ash uses directly.
> But
> > it would make it much easier to turn this into a separate mus-app in the
> future.
> 
> I can investigate this; I initially put the lock screen code in
> //ui/chromeos/login but there were issues with using session_manager and the
> like.
> 
> It'd be good not to block this CL on moving the code though.

I actually think it will be easier (and there will be less churn) if we do this
from the start. (but I am no owner, so you can disagree/disregard)
SessionController already has a mojom
(//ash/public/interfaces/session_controller.mojom), so it should be possible for
the lock-screen code to talk to ash through that.

[xiaoyinh(OOO Sep 11-29), 2017-05-15 18:40:31]

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

[commit-bot: I haz the power, 2017-05-15 18:41:20]

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-05-15 19:30:30]

On 2017/05/15 18:22:11, sadrul wrote:
> On 2017/05/15 17:35:54, jdufault wrote:
> > On 2017/05/13 05:16:47, sadrul wrote:
> > > Any chance this new lockscreen could live out of ash? It could live in,
for
> > > example, //mash/login (or //mash/lockscreen, etc.). For now, it would just
> be
> > a
> > > library (with its dependencies on aura, views, etc.) that ash uses
directly.
> > But
> > > it would make it much easier to turn this into a separate mus-app in the
> > future.
> > 
> > I can investigate this; I initially put the lock screen code in
> > //ui/chromeos/login but there were issues with using session_manager and the
> > like.
> > 
> > It'd be good not to block this CL on moving the code though.
> 
> I actually think it will be easier (and there will be less churn) if we do
this
> from the start. (but I am no owner, so you can disagree/disregard)
> SessionController already has a mojom
> (//ash/public/interfaces/session_controller.mojom), so it should be possible
for
> the lock-screen code to talk to ash through that.

I'm OK with lock screen living in ash.

[xiaoyinh(OOO Sep 11-29), 2017-05-15 20:11:40]

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

[commit-bot: I haz the power, 2017-05-15 20:12:26]

Dry run: CQ is trying da patch.

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

[xiaoyinh(OOO Sep 11-29), 2017-05-15 20:13:05]

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

https://codereview.chromium.org/2876673002/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:26: // Hash password before sending through
mojo.
On 2017/05/13 00:57:40, jdufault wrote:
> Instead of caching system_salt_ you can call through GetSystemSalt here so
that
> we always know system salt is available. Otherwise, it is possible this will
run
> before we have the system salt. This is unlikely on lock screen but we should
> consider it for sign-in screen.
> 
> ie,
> 
>    chromeos::SystemSaltGetter::Get()->GetSystemSalt(
>        base::Bind(&DoAuthenticateUser, account_id, password, ...));
> 

Good point, thanks! I keep the caching part with the hope to save some time of
getting the system salt for each authentication attempt.

https://codereview.chromium.org/2876673002/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:28: chromeos::Key key(password);
On 2017/05/13 00:57:39, jdufault wrote:
> Unfortunately PIN is hashed differently. See [1]. Feel free to add a TODO.
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlo...

Added a TODO. Thanks for the information.

https://codereview.chromium.org/2876673002/diff/40001/ash/login/ui/lock_conte...
File ash/login/ui/lock_contents_view.cc (right):

https://codereview.chromium.org/2876673002/diff/40001/ash/login/ui/lock_conte...
ash/login/ui/lock_contents_view.cc:46: user_session->account_id, std::string(),
false);
On 2017/05/13 00:57:40, jdufault wrote:
> nit: false /*authenticated_by_pin*/

Done.

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

https://codereview.chromium.org/2876673002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:243: // Create delegate that
will eventually call into the views-based lock
On 2017/05/13 00:57:40, jdufault wrote:
> nit: Can you update the comment?
> 
> // Create delegate that calls into the views-based lock screen via mojo.

Done.

[xiyuan, 2017-05-15 20:39:41]

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2876673002/diff/20001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:15: ShowErrorMessage();
On 2017/05/13 00:29:49, xiaoyinh wrote:
> On 2017/05/11 21:17:33, xiyuan wrote:
> > Do we need the error message to be displayed?
> 
> chrome will request to show error message when the unlock authentication is
> failed. I think this will be needed as long as we are  authenticating in
chrome.

Sorry, I should be more clear. I meant how would LockScreen know what error
message to be displayed?

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

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:13:
chromeos::SystemSaltGetter::Get()->GetSystemSalt(base::Bind(
SystemSaltGetter::Get() could return nullptr on tests. It probably not needed to
pre-fetch and cache the salt. SystemSaltGetter already does that.

It would be nice to make this code work for mash as well. Ash would run in a
separate process in this config. We need to initialize/shutdown SystemSaltGetter
for ash in WindowManagerApplication::InitializeComponents
[1]/ShutdownComponents.

[1]:
https://cs.chromium.org/chromium/src/ash/mus/window_manager_application.cc?rc...

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:58: chromeos::UserContext
user_context(account_id);
|user_context| not used?

[James Cook, 2017-05-15 21:15:01]

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

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:69: }
Please write a test for this class. It can be something small/stupid, like
verifying AuthenticateUser() on the class invokes the mojo method. But that will
give us a framework for future tests.

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller.h (right):

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:15: class ASH_EXPORT LockScreenController
Class documentation please.

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:29: void AuthenticateUser(const AccountId&
account_id,
Document what this does. For example, how does it deal with errors?

https://codereview.chromium.org/2876673002/diff/80001/ash/login/ui/lock_conte...
File ash/login/ui/lock_contents_view.cc (right):

https://codereview.chromium.org/2876673002/diff/80001/ash/login/ui/lock_conte...
ash/login/ui/lock_contents_view.cc:41: const int kUserIndex = 0;
super nit: When we're using a const local just to provide a nice name for a
variable, I prefer |user_index|.

https://codereview.chromium.org/2876673002/diff/80001/ash/login/ui/lock_conte...
ash/login/ui/lock_contents_view.cc:44:
session_controller->GetUserSession(kUserIndex);
nit: Just inline the call to Shell::Get()->session_controller() here since you
only use the pointer once.

https://codereview.chromium.org/2876673002/diff/80001/ash/login/ui/lock_conte...
ash/login/ui/lock_contents_view.cc:47: false /* authenticated_by_pin */);
Thanks for documenting what false means here! It makes it easy for me to read.

https://codereview.chromium.org/2876673002/diff/80001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2876673002/diff/80001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:15: ShowErrorMessage();
What message?

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/lock_screen_client_chromeos.cc (right):

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client_chromeos.cc:41: // TODO(xiaoyinh@):
Complete the implementation below.
super-nit: No "@" in todos. :-)

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/lock_screen_client_chromeos.h (right):

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client_chromeos.h:14: class
LockScreenClientChromeOS : public ash::mojom::LockScreenClient {
This class/file don't need to have "chromeos" in the name. c/b/ui/ash only
compiles on Chrome OS.

tip: tools/git/move_source_file.py will rename the file for you.

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:134:
lock_screen_client_.reset();
move before media_client_.reset() so that you tear down the objects in the
opposite order of constructing them.

If you can't do that, leave a comment why the destruction order has to be this
way.

[commit-bot: I haz the power, 2017-05-15 21:54:29]

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

[commit-bot: I haz the power, 2017-05-15 21:54:30]

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

[xiaoyinh(OOO Sep 11-29), 2017-05-16 17:30:08]

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

[commit-bot: I haz the power, 2017-05-16 17:31:05]

Dry run: CQ is trying da patch.

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

[xiaoyinh(OOO Sep 11-29), 2017-05-16 17:32:50]

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

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:13:
chromeos::SystemSaltGetter::Get()->GetSystemSalt(base::Bind(
On 2017/05/15 20:39:41, xiyuan wrote:
> SystemSaltGetter::Get() could return nullptr on tests. It probably not needed
to
> pre-fetch and cache the salt. SystemSaltGetter already does that.
> 
> It would be nice to make this code work for mash as well. Ash would run in a
> separate process in this config. We need to initialize/shutdown
SystemSaltGetter
> for ash in WindowManagerApplication::InitializeComponents
> [1]/ShutdownComponents.
> 
> [1]:
>
https://cs.chromium.org/chromium/src/ash/mus/window_manager_application.cc?rc...

Done. Added "+chromeos/cryptohome" in ash/mus/DEPS also.

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:58: chromeos::UserContext
user_context(account_id);
On 2017/05/15 20:39:41, xiyuan wrote:
> |user_context| not used?

Removed, thanks!

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.cc:69: }
On 2017/05/15 21:15:00, James Cook wrote:
> Please write a test for this class. It can be something small/stupid, like
> verifying AuthenticateUser() on the class invokes the mojo method. But that
will
> give us a framework for future tests.

Thanks!
I wrote a very simple test to verify that LockScreenClient receives
AuthenticateUser request. I will add more once we have more implementation
details.

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller.h (right):

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:15: class ASH_EXPORT LockScreenController
On 2017/05/15 21:15:00, James Cook wrote:
> Class documentation please.

Done.

https://codereview.chromium.org/2876673002/diff/80001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:29: void AuthenticateUser(const AccountId&
account_id,
On 2017/05/15 21:15:00, James Cook wrote:
> Document what this does. For example, how does it deal with errors?

Done.

https://codereview.chromium.org/2876673002/diff/80001/ash/login/ui/lock_conte...
File ash/login/ui/lock_contents_view.cc (right):

https://codereview.chromium.org/2876673002/diff/80001/ash/login/ui/lock_conte...
ash/login/ui/lock_contents_view.cc:41: const int kUserIndex = 0;
On 2017/05/15 21:15:00, James Cook wrote:
> super nit: When we're using a const local just to provide a nice name for a
> variable, I prefer |user_index|.

Done.

https://codereview.chromium.org/2876673002/diff/80001/ash/login/ui/lock_conte...
ash/login/ui/lock_contents_view.cc:44:
session_controller->GetUserSession(kUserIndex);
On 2017/05/15 21:15:00, James Cook wrote:
> nit: Just inline the call to Shell::Get()->session_controller() here since you
> only use the pointer once.

Done.

https://codereview.chromium.org/2876673002/diff/80001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2876673002/diff/80001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:15: ShowErrorMessage();
On 2017/05/15 21:15:00, James Cook wrote:
> What message?

Updated. Thanks!

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/lock_screen_client_chromeos.cc (right):

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client_chromeos.cc:41: // TODO(xiaoyinh@):
Complete the implementation below.
On 2017/05/15 21:15:01, James Cook wrote:
> super-nit: No "@" in todos. :-)

Done.

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/lock_screen_client_chromeos.h (right):

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client_chromeos.h:14: class
LockScreenClientChromeOS : public ash::mojom::LockScreenClient {
On 2017/05/15 21:15:01, James Cook wrote:
> This class/file don't need to have "chromeos" in the name. c/b/ui/ash only
> compiles on Chrome OS.
> 
> tip: tools/git/move_source_file.py will rename the file for you.

Done. Thanks for the information!

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/2876673002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:134:
lock_screen_client_.reset();
On 2017/05/15 21:15:01, James Cook wrote:
> move before media_client_.reset() so that you tear down the objects in the
> opposite order of constructing them.
> 
> If you can't do that, leave a comment why the destruction order has to be this
> way.

Done.

[xiyuan, 2017-05-16 18:22:25]

Mostly good.

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
File ash/login/lock_screen_controller.h (right):

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller.h:40: // error messages in the lock screen if
auth fails.
nit: also document what happens if auth succeeds.

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

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:29:
autentication_requests_count_++;
nit: pre-inc, i.e. ++autentication_requests_count_;

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:47: void SetUp() override {
nit: // test::AshTestBase:

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:49:
chromeos::SystemSaltGetter::Initialize();
This probably should go into AshTestHelper::SetUp[1] since we don't need this
when running under mus/mash. Think you can try the config by
- running ash_unittests --mus for mus config
- mash_unittests for mash (not sure whether your test would be included there
tho).

[1]:
https://cs.chromium.org/chromium/src/ash/test/ash_test_helper.cc?rcl=89689737...

[commit-bot: I haz the power, 2017-05-16 18:30:22]

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

[commit-bot: I haz the power, 2017-05-16 18:30:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[James Cook, 2017-05-16 20:05:02]

LGTM once xiyuan is happy with it.

Nice patch!

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

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller.cc:12:
LockScreenController::LockScreenController() {}
nit: = default here and below (if possible)

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

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:45: LockScreenControllerTest() {}
nit: = default

Also, you need ~LockScreenControllerTest() override

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:71: }
Hooray for a simple, clean test!

[xiaoyinh(OOO Sep 11-29), 2017-05-16 21:10:00]

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

[xiaoyinh(OOO Sep 11-29), 2017-05-16 21:11:31]

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

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller.cc:12:
LockScreenController::LockScreenController() {}
On 2017/05/16 20:05:02, James Cook wrote:
> nit: = default here and below (if possible)

Done.

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
File ash/login/lock_screen_controller.h (right):

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller.h:40: // error messages in the lock screen if
auth fails.
On 2017/05/16 18:22:25, xiyuan wrote:
> nit: also document what happens if auth succeeds.

Done.

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

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:29:
autentication_requests_count_++;
On 2017/05/16 18:22:25, xiyuan wrote:
> nit: pre-inc, i.e. ++autentication_requests_count_;

Done.

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:45: LockScreenControllerTest() {}
On 2017/05/16 20:05:02, James Cook wrote:
> nit: = default
> 
> Also, you need ~LockScreenControllerTest() override

This has been simplified as 
using LockScreenControllerTest = test::AshTestBase;
Since we don't need to initialize SystemSaltGetter in the test.

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:47: void SetUp() override {
On 2017/05/16 18:22:25, xiyuan wrote:
> nit: // test::AshTestBase:
This has been simplified as 
using LockScreenControllerTest = test::AshTestBase;
Since we don't need to initialize SystemSaltGetter in the test.

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:49:
chromeos::SystemSaltGetter::Initialize();
On 2017/05/16 18:22:25, xiyuan wrote:
> This probably should go into AshTestHelper::SetUp[1] since we don't need this
> when running under mus/mash. Think you can try the config by
> - running ash_unittests --mus for mus config
> - mash_unittests for mash (not sure whether your test would be included there
> tho).
> 
> [1]:
>
https://cs.chromium.org/chromium/src/ash/test/ash_test_helper.cc?rcl=89689737...

Done. Thanks for the information!

https://codereview.chromium.org/2876673002/diff/100001/ash/login/lock_screen_...
ash/login/lock_screen_controller_unittest.cc:71: }
On 2017/05/16 20:05:02, James Cook wrote:
> Hooray for a simple, clean test!

Thanks!

[commit-bot: I haz the power, 2017-05-16 21:11:38]

Dry run: CQ is trying da patch.

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

[xiyuan, 2017-05-16 21:27:31]

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

[xiyuan, 2017-05-16 21:27:32]

lgtm

+tsepez@ for lock_screen.mojom changes.

[xiaoyinh(OOO Sep 11-29), 2017-05-16 21:27:39]

xiaoyinh@chromium.org changed reviewers:
+ dkrahn@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-05-16 21:27:40]

dkrahn@, could you help to review the following? 
ash/login/DEPS
ash/mus/DEPS
ash/test/DEPS
where I add chromeos/cryptohome dependency.

tsepez@, could you help to review lock_screen.mojom?

[Tom Sepez, 2017-05-16 21:50:15]

https://codereview.chromium.org/2876673002/diff/120001/ash/public/interfaces/...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2876673002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/lock_screen.mojom:39:
AuthenticateUser(signin.mojom.AccountId account_id,
The code under this is still checking for number of attempts, right, not relying
on the other end of the connection to do so?

[xiaoyinh(OOO Sep 11-29), 2017-05-16 22:25:38]

https://codereview.chromium.org/2876673002/diff/120001/ash/public/interfaces/...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2876673002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/lock_screen.mojom:39:
AuthenticateUser(signin.mojom.AccountId account_id,
On 2017/05/16 21:50:15, Tom Sepez wrote:
> The code under this is still checking for number of attempts, right, not
relying
> on the other end of the connection to do so?

Correct, we should count the number of attempts in ash lock screen views. The
'login_attempts' in ShowErrorMessage is probably not needed
I will add a TODO to update the parameters when I work on the real
implementation.

[commit-bot: I haz the power, 2017-05-16 22:49:43]

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

[commit-bot: I haz the power, 2017-05-16 22:49:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Tom Sepez, 2017-05-17 18:58:49]

lgtm

[dkrahn, 2017-05-17 20:20:37]

On 2017/05/16 21:27:40, xiaoyinh wrote:
> dkrahn@, could you help to review the following? 
> ash/login/DEPS
> ash/mus/DEPS
> ash/test/DEPS
> where I add chromeos/cryptohome dependency.
> 
> tsepez@, could you help to review lock_screen.mojom?

lgtm

[jdufault, 2017-05-17 20:38:22]

lgtm with 2 nits

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

https://codereview.chromium.org/2876673002/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller.cc:52: if (lock_screen_client_) {
nit: invert and do an early return if no lock_screen_client_

https://codereview.chromium.org/2876673002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2876673002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/lock/screen_locker.cc:169: // Stubbed delegate
that will eventually be replaced by mojo calls.
nit: update comment

[xiaoyinh(OOO Sep 11-29), 2017-05-18 17:28:27]

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

[commit-bot: I haz the power, 2017-05-18 17:29:26]

Dry run: CQ is trying da patch.

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

[xiaoyinh(OOO Sep 11-29), 2017-05-18 17:33:50]

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

https://codereview.chromium.org/2876673002/diff/140001/ash/login/lock_screen_...
ash/login/lock_screen_controller.cc:52: if (lock_screen_client_) {
On 2017/05/17 20:38:22, jdufault wrote:
> nit: invert and do an early return if no lock_screen_client_

Done.

https://codereview.chromium.org/2876673002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2876673002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/lock/screen_locker.cc:169: // Stubbed delegate
that will eventually be replaced by mojo calls.
On 2017/05/17 20:38:22, jdufault wrote:
> nit: update comment

Done.

[xiaoyinh(OOO Sep 11-29), 2017-05-18 22:53:32]

The CQ bit was unchecked by xiaoyinh@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-05-18 22:53:32]

The CQ bit was checked by xiaoyinh@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-05-18 22:53:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, xiyuan@chromium.org, dkrahn@google.com,
tsepez@chromium.org, jdufault@chromium.org
Link to the patchset: https://codereview.chromium.org/2876673002/#ps160001
(title: "Incorporate comments from patch set 8 and rebase")

[commit-bot: I haz the power, 2017-05-18 22:54:39]

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-18 23:29:50]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495148012826220,
"parent_rev": "2b366b970abadb9d9f05ef314da12973fb831f24", "commit_rev":
"2bbdd107bcc11f6b41469249dfc51bac7b25eb01"}

[commit-bot: I haz the power, 2017-05-18 23:30:04]

Description was changed from

==========
mojo api for view based lockscreen.

BUG=721524
==========

to

==========
mojo api for view based lockscreen.

BUG=721524

Review-Url: https://codereview.chromium.org/2876673002
Cr-Commit-Position: refs/heads/master@{#472966}
Committed:
https://chromium.googlesource.com/chromium/src/+/2bbdd107bcc11f6b41469249dfc5...
==========

[commit-bot: I haz the power, 2017-05-18 23:30:06]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/2bbdd107bcc11f6b41469249dfc5...