cros: Initial structure for views-based lock.

cros: Initial structure for views-based lock.

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

[jdufault, 2017-05-05 01:09:15]

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

[commit-bot: I haz the power, 2017-05-05 01:09:41]

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-05 02:37:39]

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

[commit-bot: I haz the power, 2017-05-05 02:37:40]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-05-05 20:33:26]

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

[commit-bot: I haz the power, 2017-05-05 20:35:07]

Dry run: CQ is trying da patch.

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

[jdufault, 2017-05-05 20:50:06]

Description was changed from

==========
cros: Initial structure for views-based lock.

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

to

==========
cros: Initial structure for views-based lock.

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

[jdufault, 2017-05-05 20:51:11]

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

[jdufault, 2017-05-05 20:51:22]

Patchset #1 (id:1) has been deleted

[jdufault, 2017-05-05 20:51:36]

xiyuan@ PTAL

[commit-bot: I haz the power, 2017-05-05 22:27:33]

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

[commit-bot: I haz the power, 2017-05-05 22:27:34]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-08 17:11:50]

https://codereview.chromium.org/2859363003/diff/20001/ash/login/views/lock_sc...
File ash/login/views/lock_screen.cc (right):

https://codereview.chromium.org/2859363003/diff/20001/ash/login/views/lock_sc...
ash/login/views/lock_screen.cc:29:
session_controller->SetSessionInfo(std::move(info));
In current code, the session state code originates only from browser, managed by
session manager component. As the code is put under ash/, it should run in ash.
We would need a mojo call back to browser to update the session state source.

Is the reason why we need to put the code under ash/? If so, this does not look
like a good reason. The UI code might not need to talk to ash directly. Depends
on where the code runs, if we run the code in browser, it could talk to session
manager component, similar to what we have today.

[jdufault, 2017-05-08 18:42:01]

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

[commit-bot: I haz the power, 2017-05-08 18:42:49]

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-08 20:16:53]

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

[commit-bot: I haz the power, 2017-05-08 20:16:54]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-05-08 20:46:51]

https://codereview.chromium.org/2859363003/diff/20001/ash/login/views/lock_sc...
File ash/login/views/lock_screen.cc (right):

https://codereview.chromium.org/2859363003/diff/20001/ash/login/views/lock_sc...
ash/login/views/lock_screen.cc:29:
session_controller->SetSessionInfo(std::move(info));
On 2017/05/08 17:11:50, xiyuan wrote:
> In current code, the session state code originates only from browser, managed
by
> session manager component. As the code is put under ash/, it should run in
ash.
> We would need a mojo call back to browser to update the session state source.
> 
> Is the reason why we need to put the code under ash/? If so, this does not
look
> like a good reason. The UI code might not need to talk to ash directly.
Depends
> on where the code runs, if we run the code in browser, it could talk to
session
> manager component, similar to what we have today.

Done; discussed offline. login will stay in ash with high-level mojo API calls
to browser (ie, lockScreen, like what we have now with chrome.send). session
manager logic stays in browser.

[xiyuan, 2017-05-08 20:55:27]

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

[xiyuan, 2017-05-08 20:55:27]

lgtm

Let's see what jamescook@ thinks.

[James Cook, 2017-05-08 22:04:21]

Overall structure seems fine - some questions and nits

https://codereview.chromium.org/2859363003/diff/40001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/BUILD.gn#newcode204
ash/BUILD.gn:204: "login/views/contents_view.cc",
Why the extra /views/ subdir? Do you anticipate that there will be code here
that isn't views?

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
File ash/login/views/contents_view.cc (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.cc:18: {
any particular reason you're introducing new scopes here and below?

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
File ash/login/views/contents_view.h (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.h:14: // ContentsView hosts the root view for the
lock screen.
Is there a single instance, or one per display?

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.h:15: class ContentsView : public views::View,
public views::ButtonListener {
I would prefer unique class names for as long as we can handle them.
LockScreenContentsView? LockContentsView?

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.h:20: // views::ButtonListener
super nit: end with :

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.h:29: }  // ash
"namespace ash"

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/lock_sc...
File ash/login/views/lock_screen.h (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/lock_sc...
ash/login/views/lock_screen.h:21: }  // ash
namespace ash

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

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:205:
ScreenLocker::Delegate::Delegate() {}
nit: = default where you can (and above)

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:251:
ash::ShowLockScreenInWidget(lock_window);
This is pretty weird (injecting ash-based views into a widget created by
chrome). Is this a temporary state during development?

Why not just create the ContentsView here and eliminate ShowLockScreenInWidget,
then move all the code into ash when you are ready?

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:261:
screenlock_icon_provider_.reset(new ScreenlockIconProvider);
nit: MakeUnique while you're here

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

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.h:66: virtual void
SetInputEnabled(bool enabled) = 0;
nit: SetPasswordEnabled() or SetPasswordInputEnabled()

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.h:102: };
nice method docs, by the way

[jdufault, 2017-05-08 22:39:56]

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

[commit-bot: I haz the power, 2017-05-08 22:44:32]

Dry run: CQ is trying da patch.

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

[jdufault, 2017-05-08 23:02:35]

https://codereview.chromium.org/2859363003/diff/40001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/BUILD.gn#newcode204
ash/BUILD.gn:204: "login/views/contents_view.cc",
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> Why the extra /views/ subdir? Do you anticipate that there will be code here
> that isn't views?

Yes - mojo implementations soonish for the existing webui chrome.send API calls.
Later on I'd like to migrate the login code in chrome, ie, talking to
cryptohome, here as well.

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
File ash/login/views/contents_view.cc (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.cc:18: {
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> any particular reason you're introducing new scopes here and below?

Removed (I originally had a few more labels for testing but simplified on
upload, forgot to remove the braces).

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
File ash/login/views/contents_view.h (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.h:14: // ContentsView hosts the root view for the
lock screen.
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> Is there a single instance, or one per display?

Done.

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.h:15: class ContentsView : public views::View,
public views::ButtonListener {
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> I would prefer unique class names for as long as we can handle them.
> LockScreenContentsView? LockContentsView?

Done.

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.h:20: // views::ButtonListener
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> super nit: end with :

Done.

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/content...
ash/login/views/contents_view.h:29: }  // ash
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> "namespace ash"

Done.

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/lock_sc...
File ash/login/views/lock_screen.h (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/login/views/lock_sc...
ash/login/views/lock_screen.h:21: }  // ash
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> namespace ash

Done.

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

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:205:
ScreenLocker::Delegate::Delegate() {}
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> nit: = default where you can (and above)

Done.

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:251:
ash::ShowLockScreenInWidget(lock_window);
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> This is pretty weird (injecting ash-based views into a widget created by
> chrome). Is this a temporary state during development?

LockWindow will move into ash/ soon. I want to get this CL landed ASAP so others
can start work.

> 
> Why not just create the ContentsView here and eliminate
ShowLockScreenInWidget,
> then move all the code into ash when you are ready?

Creating in ash/ first reduces overall churn and forces chrome dependencies to
go through mojo.

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:261:
screenlock_icon_provider_.reset(new ScreenlockIconProvider);
On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> nit: MakeUnique while you're here

Done.

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

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.h:66: virtual void
SetInputEnabled(bool enabled) = 0;
On 2017/05/08 22:04:21, James Cook (sheriff - slow) wrote:
> nit: SetPasswordEnabled() or SetPasswordInputEnabled()

Done.

https://codereview.chromium.org/2859363003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.h:102: };
On 2017/05/08 22:04:21, James Cook (sheriff - slow) wrote:
> nice method docs, by the way

FYI: This section is mostly copied from webui_screen_locker.cc (I did some minor
cleanup though).

[James Cook, 2017-05-08 23:24:10]

LGTM.

(Aside: Always creating the ShelfView at startup is turning out to be a fiddly
refactor. I think I can make it work, but I'm gardener this week, so I'm
probably not going to get to it until next week.)

https://codereview.chromium.org/2859363003/diff/40001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/BUILD.gn#newcode204
ash/BUILD.gn:204: "login/views/contents_view.cc",
On 2017/05/08 23:02:34, jdufault wrote:
> On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> > Why the extra /views/ subdir? Do you anticipate that there will be code here
> > that isn't views?
> 
> Yes - mojo implementations soonish for the existing webui chrome.send API
calls.
> Later on I'd like to migrate the login code in chrome, ie, talking to
> cryptohome, here as well.

Hrm. What do you think the other subdirectories are going to be?

If you don't think it'll be that many files, I would just throw them all under
ash/login to keep the directories from getting too deep.

I would keep ash/login/views if you think there will be an ash/login/webui
someday.

https://codereview.chromium.org/2859363003/diff/80001/ash/login/views/lock_co...
File ash/login/views/lock_contents_view.h (right):

https://codereview.chromium.org/2859363003/diff/80001/ash/login/views/lock_co...
ash/login/views/lock_contents_view.h:17: // is effectively only a single
instance.
nice comment

[jdufault, 2017-05-08 23:48:28]

https://codereview.chromium.org/2859363003/diff/40001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2859363003/diff/40001/ash/BUILD.gn#newcode204
ash/BUILD.gn:204: "login/views/contents_view.cc",
On 2017/05/08 23:24:09, James Cook (sheriff - slow) wrote:
> On 2017/05/08 23:02:34, jdufault wrote:
> > On 2017/05/08 22:04:20, James Cook (sheriff - slow) wrote:
> > > Why the extra /views/ subdir? Do you anticipate that there will be code
here
> > > that isn't views?
> > 
> > Yes - mojo implementations soonish for the existing webui chrome.send API
> calls.
> > Later on I'd like to migrate the login code in chrome, ie, talking to
> > cryptohome, here as well.
> 
> Hrm. What do you think the other subdirectories are going to be?
> 
> If you don't think it'll be that many files, I would just throw them all under
> ash/login to keep the directories from getting too deep.
> 
> I would keep ash/login/views if you think there will be an ash/login/webui
> someday.

There will be some webui code to interface with gaia, but I'm not sure it will
be enough to justify a separate directory.

I do think there will be significant chunks of non-views code though - take a
look at chrome/browser/chromeos/login/* for some of it (though that directory
also includes oobe). easy_unlock is a good candidate.

https://codereview.chromium.org/2859363003/diff/80001/ash/login/views/lock_co...
File ash/login/views/lock_contents_view.h (right):

https://codereview.chromium.org/2859363003/diff/80001/ash/login/views/lock_co...
ash/login/views/lock_contents_view.h:17: // is effectively only a single
instance.
On 2017/05/08 23:24:09, James Cook (sheriff - slow) wrote:
> nice comment

:)

[jdufault, 2017-05-08 23:48:32]

The CQ bit was unchecked by jdufault@chromium.org

[jdufault, 2017-05-08 23:48:34]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2017-05-08 23:48:35]

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

[commit-bot: I haz the power, 2017-05-09 00:06:53]

CQ is trying da patch.

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

[James Cook, 2017-05-09 02:39:05]

OK, seeing the pile of code in c/b/chromeos/login makes me think a subdir is OK.

I see you have this in the CQ, and this is a bikeshed, but I think you should
call the directory something other than "views". Seeing that name implies to me
that there is a non-views UI implementation (e.g. chrome/browser/ui/views vs.
c/b/ui/cocoa).

How about "ui" to match c/b/chromeos/login/ui?

[commit-bot: I haz the power, 2017-05-09 04:19:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494287314749340,
"parent_rev": "f7d6f760f8e0fd9fd4683b1d41628c69bebf95d5", "commit_rev":
"65bb182d82588d3c7e8723c4bd45c3219d121cb4"}

[commit-bot: I haz the power, 2017-05-09 04:19:37]

Description was changed from

==========
cros: Initial structure for views-based lock.

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

to

==========
cros: Initial structure for views-based lock.

BUG=719015

Review-Url: https://codereview.chromium.org/2859363003
Cr-Commit-Position: refs/heads/master@{#470196}
Committed:
https://chromium.googlesource.com/chromium/src/+/65bb182d82588d3c7e8723c4bd45...
==========

[commit-bot: I haz the power, 2017-05-09 04:19:39]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/65bb182d82588d3c7e8723c4bd45...

[jdufault, 2017-05-09 17:09:28]

On 2017/05/09 02:39:05, James Cook (sheriff - slow) wrote:
> OK, seeing the pile of code in c/b/chromeos/login makes me think a subdir is
OK.
> 
> I see you have this in the CQ, and this is a bikeshed, but I think you should
> call the directory something other than "views". Seeing that name implies to
me
> that there is a non-views UI implementation (e.g. chrome/browser/ui/views vs.
> c/b/ui/cocoa).
> 
> How about "ui" to match c/b/chromeos/login/ui?

I'll send a follow-up CL