Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12218078:
Implement a policy to autologin a public account. (Closed)
|
Created:
7 years, 10 months ago by dconnelly Modified:
7 years, 9 months ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement a policy to autologin a public account.
This introduces a policy to automatically login a public account on
Chrome OS after a specified period of time has elapsed at the login
screen. There are two new policy settings:
"DeviceLocalAccountAutoLoginUsername", which determines which public
account will be logged in, and "DeviceLocalAccountAutoLoginTimerMillis",
which specifies the amount of time that should elapse before autologin
takes place.
The autologin timer is started in ExistingUserController when either the
sign-in screen UI is finished loading or the aforementioned policy
settings are changed.
BUG=152933
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188331
Patch Set 1 #Patch Set 2 : small comment fixes #
Total comments: 18
Patch Set 3 : Review comments: naming, style, etc #Patch Set 4 : more names #Patch Set 5 : last upload failed #
Total comments: 31
Patch Set 6 : reset timer on user activity, review comments #
Total comments: 8
Patch Set 7 : rebase #
Total comments: 19
Patch Set 8 : nits #Patch Set 9 : add browsertests #Patch Set 10 : add another comment #Patch Set 11 : fix useractivitydetector observer loop after session logout #Patch Set 12 : split out FakeSessionManagerClient #
Total comments: 100
Patch Set 13 : bartfab review comments; split out NotificationWatcher #
Total comments: 2
Patch Set 14 : replace a NOTREACHED by no-op #Patch Set 15 : move user activity timer reset to WebUILoginDisplay #
Total comments: 26
Patch Set 16 : nits; fix lazy timer initialization #
Total comments: 2
Patch Set 17 : add test asserts for timer #
Total comments: 2
Patch Set 18 : split an assert #
Total comments: 24
Patch Set 19 : nits; split out unit tests; move NotificationWatcher #Patch Set 20 : rebase #Patch Set 21 : shorten timer delay in test; add two more stop/start timer cases #
Total comments: 8
Patch Set 22 : more nits #
Total comments: 132
Patch Set 23 : bartfab review #Patch Set 24 : rebase #Patch Set 25 : rebase #
Total comments: 3
Patch Set 26 : remove NotificationWatcher from content/ #
Total comments: 16
Patch Set 27 : remove some notifications from FakeSessionManagerClient #
Total comments: 4
Patch Set 28 : move some testing stuff, remove old proto pulled in by rebasing #Patch Set 29 : rebase #Patch Set 30 : rebase #Patch Set 31 : use scoped_refptr in tests for MessageLoopRunner #Patch Set 32 : rebase #Patch Set 33 : rebase #Patch Set 34 : REEEEEEEEEEEEBASE #Patch Set 35 : r3b@$3 #Messages
Total messages: 66 (0 generated)
No tests yet.
Looking quite good already. https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3508: 'desc': '''Specify a public account to be automatically logged in after a period of time. The public account must already be configured (see the "DeviceLocalAccounts" policy and the timeout is in milliseconds.''', The grouping is not really necessary for two policies. Also, any documentation of the policies should go with the policies directly, not into a group description. https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3514: 'caption': '''Public account for auto-login''', We have just renamed public accounts to public sessions... :). https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3516: 'supported_on': ['chrome_os:0.26-'], Nit: s/0.26/26/ https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3528: 'caption': '''Public account auto-login timer''', The order in which you arranged these items does not match the other policies. Could you reorder so it's name, type, schema, supported_on, ...? https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3530: 'supported_on': ['chrome_os:0.26-'], nit: s/0.26/26/ https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:172: kAccountsPrefDeviceLocalAccountAutoLoginTimerMillis, We tend to abbreviate milliseconds as "Ms, not "Millis" https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:941: timer = 0; multi-line if => style guide requires curly braces {} https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/se... File chrome/browser/chromeos/settings/cros_settings_names.cc (right): https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/se... chrome/browser/chromeos/settings/cros_settings_names.cc:24: const char kAccountsPrefDeviceLocalAccountAutoLoginTimerMillis[] = Here, too: s/Millis/Ms/ Or, even better: s/LoginTimerMillis/LoginDelay/ https://codereview.chromium.org/12218078/diff/2001/chrome/browser/policy/prot... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12218078/diff/2001/chrome/browser/policy/prot... chrome/browser/policy/proto/chrome_device_policy.proto:272: optional DeviceLocalAccountAutoLoginProto device_local_account_auto_login = 22; Either there is a semicolon on its own line here or the line is 81 chars long.
https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3508: 'desc': '''Specify a public account to be automatically logged in after a period of time. The public account must already be configured (see the "DeviceLocalAccounts" policy and the timeout is in milliseconds.''', On 2013/02/08 18:38:42, bartfab wrote: > The grouping is not really necessary for two policies. Also, any documentation > of the policies should go with the policies directly, not into a group > description. Done. https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3514: 'caption': '''Public account for auto-login''', On 2013/02/08 18:38:42, bartfab wrote: > We have just renamed public accounts to public sessions... :). Done. https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3516: 'supported_on': ['chrome_os:0.26-'], On 2013/02/08 18:38:42, bartfab wrote: > Nit: s/0.26/26/ Done. https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3528: 'caption': '''Public account auto-login timer''', On 2013/02/08 18:38:42, bartfab wrote: > The order in which you arranged these items does not match the other policies. > Could you reorder so it's name, type, schema, supported_on, ...? Done. https://codereview.chromium.org/12218078/diff/2001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3530: 'supported_on': ['chrome_os:0.26-'], On 2013/02/08 18:38:42, bartfab wrote: > nit: s/0.26/26/ Done. https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:172: kAccountsPrefDeviceLocalAccountAutoLoginTimerMillis, On 2013/02/08 18:38:42, bartfab wrote: > We tend to abbreviate milliseconds as "Ms, not "Millis" Done. https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:941: timer = 0; On 2013/02/08 18:38:42, bartfab wrote: > multi-line if => style guide requires curly braces {} Done. https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/se... File chrome/browser/chromeos/settings/cros_settings_names.cc (right): https://codereview.chromium.org/12218078/diff/2001/chrome/browser/chromeos/se... chrome/browser/chromeos/settings/cros_settings_names.cc:24: const char kAccountsPrefDeviceLocalAccountAutoLoginTimerMillis[] = On 2013/02/08 18:38:42, bartfab wrote: > Here, too: s/Millis/Ms/ > > Or, even better: s/LoginTimerMillis/LoginDelay/ Done. https://codereview.chromium.org/12218078/diff/2001/chrome/browser/policy/prot... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12218078/diff/2001/chrome/browser/policy/prot... chrome/browser/policy/proto/chrome_device_policy.proto:272: optional DeviceLocalAccountAutoLoginProto device_local_account_auto_login = 22; On 2013/02/08 18:38:42, bartfab wrote: > Either there is a semicolon on its own line here or the line is 81 chars long. Done.
https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/app/policy/p... File chrome/app/policy/policy_templates.json (right): https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/app/policy/p... chrome/app/policy/policy_templates.json:115: # For your editing convenience: highest ID currently used: 173 Nit: Rebase. You are ~15 policies out of date :). https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/app/policy/p... chrome/app/policy/policy_templates.json:3516: 'desc': '''A public session that should be automatically logged in after a period of time has elapsed at the login screen. The public session must already be configured (see "DeviceLocalAccounts").''', Nit: I don't think we have been using the ancient typewriter convention of two spaces after a full stop in this file. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/app/policy/p... chrome/app/policy/policy_templates.json:3530: 'desc': '''The amount of time (in milliseconds) that should elapse before automatically logging in a public session (see the "DeviceLocalAccountAutoLoginId" policy). If this policy isn't specified, 0 milliseconds will be used as the timeout.''', Nit: We normally tend to have this format for the description (no idea whether it's random custom or actually useful to stick to): '''1-line description If this policy is set, [...] If this policy is unset, [...] This policy is specified in milliseconds.''' https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:434: // Stop the autologin timer when attempting login. Nit: You used the term "autologin" in this file and the term "auto-login" in the header. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:466: StopAutoLoginTimer(); Just an idea: Instead of having to stop the timer all over the place, how about you leave it running and then check whether a login attempt is already in progress if/when it fires? This should be easy as |login_performer_| will evaluate to |true| when a login is going on and |false| otherwise. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:882: // Policy load failure stops login attempts--restart the timer. Nit: spaces around the dash. Also, -- for an em dash is... unusual. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:948: username)); Nit: Rather than indenting deeper and deeper, I think it would be easier to return when one of the conditions is not met. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.h:66: // Start the public account auto-login timer. Nit: s/public account/public session/ (here and elsewhere)
Code looking good mostly! https://codereview.chromium.org/12218078/diff/8002/chrome/browser/policy/prot... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12218078/diff/8002/chrome/browser/policy/prot... chrome/browser/policy/proto/chrome_device_policy.proto:240: message DeviceLocalAccountAutoLoginProto { The design doc originally proposed to put the fields below into DeviceLocalAccountsProto. That'd also be more in line with DeviceAutoUpdateSettingsProto above, any reason to have a separate message type?
https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:243: StartAutoLoginTimer(); nit: add {} https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:435: StopAutoLoginTimer(); I think that we want to stop this timer as soon as possible. Consider it placing in these methods instead: CompleteLogin(...) - entry point for any new user sign in final stage. That gets eventually into PerformLogin() but a bit later. However in this case I worry that user might start "Add new user" flow (if configuration supports that) and that gets aborted with auto login timer. You should probably stop time in that case too. Login(...) - entry point for existing user sign in. https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:514: StopAutoLoginTimer(); Why not place this call to the beginning of this method? https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:564: StopAutoLoginTimer(); Why not place this call to the beginning of this method? https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:721: StartAutoLoginTimer(); Are you sure this is what you want? This callback will be called for any other user sign in attempt failure. So current implementation stops auto-login timer in Login(...) (PerformLogin) but then starts that again when this user enters incorrect credentials? https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:883: StartAutoLoginTimer(); I thought that this error is kind of unrecoverable? https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.h:66: // Start the public account auto-login timer. nit: General comment about naming of these two methods. If you want to be specific that this time is only used for public sessions (at least for now) maybe it makes sense to rename these methods?
Sorry it took so long. I think I addressed all the comments. Also I added a hook to restart the timer on user activity after suggested by Mattias and Bartosz. I'm doing some final tests of everything right now but I wanted to go ahead and get the changes uploaded for review. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/app/policy/p... File chrome/app/policy/policy_templates.json (right): https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/app/policy/p... chrome/app/policy/policy_templates.json:3516: 'desc': '''A public session that should be automatically logged in after a period of time has elapsed at the login screen. The public session must already be configured (see "DeviceLocalAccounts").''', On 2013/02/11 09:46:01, bartfab wrote: > Nit: I don't think we have been using the ancient typewriter convention of two > spaces after a full stop in this file. Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/app/policy/p... chrome/app/policy/policy_templates.json:3530: 'desc': '''The amount of time (in milliseconds) that should elapse before automatically logging in a public session (see the "DeviceLocalAccountAutoLoginId" policy). If this policy isn't specified, 0 milliseconds will be used as the timeout.''', On 2013/02/11 09:46:01, bartfab wrote: > Nit: We normally tend to have this format for the description (no idea whether > it's random custom or actually useful to stick to): > > '''1-line description > > If this policy is set, [...] > > If this policy is unset, [...] > > This policy is specified in milliseconds.''' Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:243: StartAutoLoginTimer(); On 2013/02/11 09:56:58, Nikita Kostylev wrote: > nit: add {} Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:434: // Stop the autologin timer when attempting login. On 2013/02/11 09:46:01, bartfab wrote: > Nit: You used the term "autologin" in this file and the term "auto-login" in the > header. Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:435: StopAutoLoginTimer(); On 2013/02/11 09:56:58, Nikita Kostylev wrote: > I think that we want to stop this timer as soon as possible. Consider it placing > in these methods instead: > > CompleteLogin(...) - entry point for any new user sign in final stage. That gets > eventually into PerformLogin() but a bit later. > However in this case I worry that user might start "Add new user" flow (if > configuration supports that) and that gets aborted with auto login timer. You > should probably stop time in that case too. > > > Login(...) - entry point for existing user sign in. Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:514: StopAutoLoginTimer(); On 2013/02/11 09:56:58, Nikita Kostylev wrote: > Why not place this call to the beginning of this method? Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:564: StopAutoLoginTimer(); On 2013/02/11 09:56:58, Nikita Kostylev wrote: > Why not place this call to the beginning of this method? Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:721: StartAutoLoginTimer(); On 2013/02/11 09:56:58, Nikita Kostylev wrote: > Are you sure this is what you want? This callback will be called for any other > user sign in attempt failure. > > So current implementation stops auto-login timer in Login(...) (PerformLogin) > but then starts that again when this user enters incorrect credentials? Yes, this is the intended behavior. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:882: // Policy load failure stops login attempts--restart the timer. On 2013/02/11 09:46:01, bartfab wrote: > Nit: spaces around the dash. Also, -- for an em dash is... unusual. My bad, ---. You don't need the spaces, though, according to http://en.wikipedia.org/wiki/Dash https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:883: StartAutoLoginTimer(); On 2013/02/11 09:56:58, Nikita Kostylev wrote: > I thought that this error is kind of unrecoverable? I ran into a problem during debugging where I screwed up the policy loading on the backend, but I was able to SSH into the VM image and also fiddle with the policy server and fix the problem. So it's unrecoverable at the UI but perhaps not in the backend. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.cc:948: username)); On 2013/02/11 09:46:01, bartfab wrote: > Nit: Rather than indenting deeper and deeper, I think it would be easier to > return when one of the conditions is not met. Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.h:66: // Start the public account auto-login timer. On 2013/02/11 09:46:01, bartfab wrote: > Nit: s/public account/public session/ (here and elsewhere) Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.h:66: // Start the public account auto-login timer. On 2013/02/11 09:56:58, Nikita Kostylev wrote: > nit: General comment about naming of these two methods. > > If you want to be specific that this time is only used for public sessions (at > least for now) maybe it makes sense to rename these methods? Done. https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/poli... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://chromiumcodereview.appspot.com/12218078/diff/8002/chrome/browser/poli... chrome/browser/policy/proto/chrome_device_policy.proto:240: message DeviceLocalAccountAutoLoginProto { On 2013/02/11 09:46:02, Mattias Nissler wrote: > The design doc originally proposed to put the fields below into > DeviceLocalAccountsProto. That'd also be more in line with > DeviceAutoUpdateSettingsProto above, any reason to have a separate message type? Done.
https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/app/policy/... File chrome/app/policy/policy_templates.json (right): https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/app/policy/... chrome/app/policy/policy_templates.json:115: # For your editing convenience: highest ID currently used: 173 You still need to rebase. Better do it now and then test your changes. Things may have changed that break your tests.
oh god, okay On Mon, Feb 11, 2013 at 5:19 PM, <bartfab@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/app/policy/... > File chrome/app/policy/policy_templates.json (right): > > https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/app/policy/... > > chrome/app/policy/policy_templates.json:115: # For your editing > convenience: highest ID currently used: 173 > You still need to rebase. Better do it now and then test your changes. > Things may have changed that break your tests. > > https://chromiumcodereview.appspot.com/12218078/
https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/browser/chr... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller.cc:895: // Policy load failure stops login attempts---restart the timer. Hmm. The request to add space around the odd -- yielded a --- with no spaces. I give up :). It was just a nit anyway. https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/browser/chr... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller.h:74: // UserActivityObserver::Observer overrides: nit: s/overrides/implementation/ or s/overrides// https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller.h:216: int public_session_auto_login_timer_; nit: Calling this "timer" is not really accurate. It should be "timeout" or, even better, matching the policy, "delay". https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller.h:219: std::string public_session_auto_login_username_; I think in the case of device local accounts (aka public accounts, aka public sessions), Mattias called this "account id", not "username". But it's the same thing, so treat this comment as a non-actionable FYI.
https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/app/policy/... File chrome/app/policy/policy_templates.json (right): https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/app/policy/... chrome/app/policy/policy_templates.json:115: # For your editing convenience: highest ID currently used: 173 On 2013/02/11 16:19:17, bartfab wrote: > You still need to rebase. Better do it now and then test your changes. Things > may have changed that break your tests. Done. https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/browser/chr... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller.h:74: // UserActivityObserver::Observer overrides: On 2013/02/11 17:11:21, bartfab wrote: > nit: > > s/overrides/implementation/ > or > s/overrides// Done. https://chromiumcodereview.appspot.com/12218078/diff/12001/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller.h:216: int public_session_auto_login_timer_; On 2013/02/11 17:11:21, bartfab wrote: > nit: Calling this "timer" is not really accurate. It should be "timeout" or, > even better, matching the policy, "delay". Done.
lgtm https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/chro... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.h:74: // UserActivityObserver::Observer implementation Nit: Colon at the end of the line.
https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/8002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:721: StartAutoLoginTimer(); On 2013/02/11 16:17:56, dconnelly wrote: > On 2013/02/11 09:56:58, Nikita Kostylev wrote: > > Are you sure this is what you want? This callback will be called for any other > > user sign in attempt failure. > > > > So current implementation stops auto-login timer in Login(...) (PerformLogin) > > but then starts that again when this user enters incorrect credentials? > > Yes, this is the intended behavior. I think that has some potential issues when one user has started sign in and is in the process of migrating cryptohome from one password to another. https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:947: ash::Shell::GetInstance()->user_activity_detector()->AddObserver(this); Remove observer in dtor? https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:950: ash::Shell::GetInstance()->user_activity_detector()->RemoveObserver(this); On a normal boot path (i.e. no public accounts configured) you'll be removing observer that you hadn't in fact added. https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:958: StartPublicSessionAutoLoginTimer(); What's the point of restarting timer?
chrome/browser/policy and chrome/browser/chromeos/settings LGTM with nits. https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/app/policy/p... File chrome/app/policy/policy_templates.json (right): https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/app/policy/p... chrome/app/policy/policy_templates.json:3538: If this policy is set, it determines the amount of time that should elapse before automatically logging into the public session specified by the "DeviceLocalAccountAutoLoginId" policy. nit: ...the amount of time _without user activity_ that should elapse... https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/poli... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/poli... chrome/browser/policy/proto/chrome_device_policy.proto:240: // should be logged in automatically. nit: mention that this is the same ID as in DeviceLocalAccountInfoProto.id https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/poli... chrome/browser/policy/proto/chrome_device_policy.proto:244: // automatically logging in. nit: mention that this is in milliseconds.
https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:947: ash::Shell::GetInstance()->user_activity_detector()->AddObserver(this); On 2013/02/11 17:30:29, Nikita Kostylev wrote: > Remove observer in dtor? The observer could be unconditionally added and removed. No harm in having it always there, except a small amount of overhead. https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:950: ash::Shell::GetInstance()->user_activity_detector()->RemoveObserver(this); On 2013/02/11 17:30:29, Nikita Kostylev wrote: > On a normal boot path (i.e. no public accounts configured) you'll be removing > observer that you hadn't in fact added. It's actually benign to remove an observer that does not exists. ObserverListBase just ignores it. https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:958: StartPublicSessionAutoLoginTimer(); On 2013/02/11 17:30:29, Nikita Kostylev wrote: > What's the point of restarting timer? The idea is that the timer performs an auto-login X minutes after the last user activity on the login screen. * If the device boots and nobody touches it -> auto login after X minutes. * If the device boots and someone logs in within X minutes -> no auto login. * If the device boots, someone tries to log in but changes their mind / fails -> auto-login X minutes after the last user activity. This requires that the timer be restarted whenever there is user activity or when a login fails.
https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:947: ash::Shell::GetInstance()->user_activity_detector()->AddObserver(this); On 2013/02/11 17:37:31, bartfab wrote: > On 2013/02/11 17:30:29, Nikita Kostylev wrote: > > Remove observer in dtor? > > The observer could be unconditionally added and removed. No harm in having it > always there, except a small amount of overhead. Ok then. https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:950: ash::Shell::GetInstance()->user_activity_detector()->RemoveObserver(this); On 2013/02/11 17:37:31, bartfab wrote: > On 2013/02/11 17:30:29, Nikita Kostylev wrote: > > On a normal boot path (i.e. no public accounts configured) you'll be removing > > observer that you hadn't in fact added. > > It's actually benign to remove an observer that does not exists. > ObserverListBase just ignores it. I thought that previously it had a such check so it was either removed or I'm confusing that with a different check.
https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:958: StartPublicSessionAutoLoginTimer(); On 2013/02/11 17:37:31, bartfab wrote: > On 2013/02/11 17:30:29, Nikita Kostylev wrote: > > What's the point of restarting timer? > > The idea is that the timer performs an auto-login X minutes after the last user > activity on the login screen. > > * If the device boots and nobody touches it -> auto login after X minutes. Take into account that device will power off after a period of inactivity on login screen. I see that user_activity_detector() observer is not removed when login is successful. Consider the situation that user has started (new) session and has some UI that is rendered by the login screen. So ExistingUserController instance does still exist but auto-login timer doesn't make sense at this point. > * If the device boots and someone logs in within X minutes -> no auto login. > * If the device boots, someone tries to log in but changes their mind / fails -> > auto-login X minutes after the last user activity. > > This requires that the timer be restarted whenever there is user activity or > when a login fails.
Nits fixed; still working on the other comments. Now that we're already past the branch point I'm going to write tests to include with this CL so that I can better understand/address the discussed concerns. https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/app/policy/p... File chrome/app/policy/policy_templates.json (right): https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/app/policy/p... chrome/app/policy/policy_templates.json:3538: If this policy is set, it determines the amount of time that should elapse before automatically logging into the public session specified by the "DeviceLocalAccountAutoLoginId" policy. On 2013/02/11 17:33:29, Mattias Nissler wrote: > nit: ...the amount of time _without user activity_ that should elapse... Done. https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/chro... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/chro... chrome/browser/chromeos/login/existing_user_controller.h:74: // UserActivityObserver::Observer implementation On 2013/02/11 17:30:27, bartfab wrote: > Nit: Colon at the end of the line. Done. https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/poli... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/poli... chrome/browser/policy/proto/chrome_device_policy.proto:240: // should be logged in automatically. On 2013/02/11 17:33:29, Mattias Nissler wrote: > nit: mention that this is the same ID as in DeviceLocalAccountInfoProto.id Done. https://chromiumcodereview.appspot.com/12218078/diff/8004/chrome/browser/poli... chrome/browser/policy/proto/chrome_device_policy.proto:244: // automatically logging in. On 2013/02/11 17:33:29, Mattias Nissler wrote: > nit: mention that this is in milliseconds. Done.
On 2013/02/12 12:19:41, dconnelly wrote: > Nits fixed; still working on the other comments. Now that we're already past > the branch point I'm going to write tests to include with this CL so that I can > better understand/address the discussed concerns. This implementation is now part of M27, right?
Yes, they punted it On Tue, Feb 12, 2013 at 3:22 PM, <nkostylev@chromium.org> wrote: > On 2013/02/12 12:19:41, dconnelly wrote: >> >> Nits fixed; still working on the other comments. Now that we're already >> past >> the branch point I'm going to write tests to include with this CL so that >> I > > can >> >> better understand/address the discussed concerns. > > > This implementation is now part of M27, right? > > > https://codereview.chromium.org/12218078/
Okay. I've added some browsertests. Sorry for taking so long. I had them done on Monday but I spent the last two days refactoring and now it doesn't even look like very much. Anyway I've been staring at this for too long and I've developed tunnel vision and I need some feedback before going any further. This uses some of mnissler's mocks/etc. from crrev.com/12212096 that will be split out at the appropriate time. https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:947: ash::Shell::GetInstance()->user_activity_detector()->AddObserver(this); On 2013/02/11 17:30:29, Nikita Kostylev wrote: > Remove observer in dtor? Done. https://codereview.chromium.org/12218078/diff/8004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/existing_user_controller.cc:950: ash::Shell::GetInstance()->user_activity_detector()->RemoveObserver(this); On 2013/02/11 17:51:31, Nikita Kostylev wrote: > On 2013/02/11 17:37:31, bartfab wrote: > > On 2013/02/11 17:30:29, Nikita Kostylev wrote: > > > On a normal boot path (i.e. no public accounts configured) you'll be > removing > > > observer that you hadn't in fact added. > > > > It's actually benign to remove an observer that does not exists. > > ObserverListBase just ignores it. > > I thought that previously it had a such check so it was either removed or I'm > confusing that with a different check. You're not confused--there was a check before. I was replicating some other code that performs a similar task, but then I looked at the header for ObserverList and found what Bartosz just said, so I removed the check.
PTAL I split out FakeSessionManagerClient since mnissler's CL already landed. I also fixed a dumb problem where I was modifying an observer list during a notification, hanging the login screen after a previous logout. (I modified the tests as well.)
https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3538: 'example_value': "bob@example.com", Nit: I would go with an example value that explicitly shows this is a *public* account, not one intended for a single person (Bob) to use. https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3543: If this policy is set, the specified session will be automatically logged in after a period of time has elapsed at the login screen. The public session must already be configured (see "DeviceLocalAccounts"). Nit: Add "without user interaction" after "has elapsed at the login screen". https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3556: 'example_value': 300, If this policy realy is in milliseconds, 300 is a bad example value (0.3s is a bit too short to be really useful :). https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3561: If the DeviceLocalAccountAutoLoginId policy is unset, this policy has no effect. Otherwise: Nit: Put the policy name in quotes for consistency. Or, even better, use bars around the policy names instead of quotes. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:242: std::string setting = *content::Details<const std::string>(details).ptr(); Nit: I would make this a "const std::string" https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:510: StartPublicSessionAutoLoginTimer(); Notice that we end up here if the policy subsystem is not quite ready yet and we need to wait for a callback before continuing with the login. In such a case, the login attempt is still pending and the timer should not be restarted. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:570: StartPublicSessionAutoLoginTimer(); Notice that we end up here if the policy subsystem is not quite ready yet and we need to wait for a callback before continuing with the login. In such a case, the login attempt is still pending and the timer should not be restarted. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:896: // Policy load failure stops login attempts---restart the timer. Nit: I am still bothered by these triple dashes and the lack of spaces around them :). https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:927: public_session_auto_login_username_ = ""; You can simplify this to: if (!cros_settings_->GetString( kAccountsPrefDeviceLocalAccountAutoLoginId, &public_session_auto_login_username_)) { public_session_auto_login_username_ = ""; } https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:936: ? ash::Shell::GetInstance()->user_activity_detector() : NULL; I think it is allowed to put the |?| at the start of the line but it is very uncommon. Better put it at the end of the previous line. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:949: if (auto_login_timer_.IsRunning()) { It is pretty obvious what this function is doing but I think that a comment would still be useful here: Document the fact that this restarts the timer but only if it is currently running. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:969: is_login_in_progress_) { Nit: The conditions in lines 956-958 and 967-969 are the exact logical inverses of each other but they are arranged in different order. Could you arrange them in the same order for consistency? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:72: // UserActivityObserver::Observer implementation. The overrides should follow the order in which the base classes are listed in lines 41-45 above. Since UserActivityObserver is the last base class, overrides for ExistingUserController should be declared last as well. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:213: base::OneShotTimer<ExistingUserController> auto_login_timer_; When I was adding the session length limiter, there was a lot of pushback against always instantiating a timer for this purpose, even if the policy that requires it is not set. The same could be said about the auto-login timer. It will only actually be used on a tiny fraction of Chrome OS devices. Maybe better wrap this in a scoped_ptr to keep memory usage minimal when auto-login is disabled? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:298: FRIEND_TEST_ALL_PREFIXES(ExistingUserControllerPublicSessionTest, Since you already added the base classes of your tests as friends, you can proxy any accesses to the ExistingUserController through these classes - no need to make the tests themselves friends as well. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:5: #include "ash/shell.h" I trust you checked that the list of includes matches what your current code uses? It is pretty hard to verify this as part of the review :). https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:156: // Observes user list change notifications and quits the message loop once a Is this not a straight copy & paste from Mattias' test as well? Code duplication makes me :...( https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:318: // CrosInProcessBrowserTest::CleanUpOnMainThread or it sometimes "or it sometimes crashes" sounds like something is going on that we do not understand. If this is a crash we can reproduce and understand, better state it here (e.g. "because it is an observer of singleton X"). If this is an unexplicable crash, it would be good to get a backtrace and understand why it happens only sometimes. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:488: return expected == actual; Why not do the dereferencing and comparison all on one line? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:507: browser_policy_connector()->GetDeviceLocalAccountPolicyService()-> Indent 4 spaces. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:508: GetBrokerForAccount(kAutoLoginUsername)->core()->store(); Indent 4 spaces. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:520: .WillRepeatedly(Return(&session_manager_client_)); Indent 4 spaces. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:577: const std::string& username, const std::string& password, The arguments should either *all* fit on one line or *all* be placed on individual lines, not a combination of both. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:665: // Cleanup. To match the other comments, this should be a verb in the imperative form -> "Clean up." instead of "Cleanup." (which is a noun) https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:689: ExistingUserController* euc = existing_user_controller(); The style guide discourages abbreviations. |euc| is hard to read and remember. Better use verbose variable names. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:696: ASSERT_FALSE(auto_login_timer()->IsRunning()); Here and in general: You should be using ASSERT_* only when it is critical to abort the test if the condition is not met (e.g. if the timer is not running, the test would deadlock, so abort). Otherwise, you should be using EXPECT_*. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:717: content::RunAllPendingInMessageLoop(); Could you document what tasks this drains from the message loop's queue and why that is needed? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:757: set_auto_login_delay(kAutoLoginShortDelay); If I understand correctly, you detect that the timer has restarted by the fact that it picked up the modified login delay. Could you document this in a comment? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:773: // Turn off settings notifications to EUC. Just as in variable names, you should not be using abbreviations in comments. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:866: content::RunAllPendingInMessageLoop(); Nit: You have an emtpy line before the content::RunAllPendingInMessageLoop() at the end in some tests and not in others. It would be nice to be consistent here. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:874: // Timer should fire immediately. Here and in the other tests: Can you document that PrepareLogin() sets up an expectation that verifies the login happened? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:889: base::TimeDelta::FromMilliseconds(2 * kAutoLoginShortDelay), Boo, waiting 200ms :(. Could you inspect the time the timer is set to and then fast-forward the message loop by 100ms? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:919: RetailModeLoginStopsAutoLogin) { This is not necessary. Public sessions and retail mode cannot be mixed. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/mock_user_manager.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_user_manager.h:92: // Creates a new public account user. Nit: Since we are switching over to the term "public session", IMHO, we should be using "public session" in all new code. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/webui_screen_locker.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/webui_screen_locker.cc:222: void WebUIScreenLocker::OnSigninScreenReady() { The order of these method overrides should match the order of their declarations in LoginDisplay::Delegate. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/webui_screen_locker.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/webui_screen_locker.h:76: virtual void OnSigninScreenReady() OVERRIDE; The order of these method overrides should match the order of their declarations in LoginDisplay::Delegate. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/cros_settings_names.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/cros_settings_names.cc:22: const char kAccountsPrefDeviceLocalAccountAutoLoginId[] = Nit: No need for a blank line before these four lines IMHO. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/cros_settings_names.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/cros_settings_names.h:19: extern const char kAccountsPrefDeviceLocalAccountAutoLoginId[]; Nit: No need for a blank line before these two lines IMHO. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/device_settings_provider.cc:46: kAllowRedeemChromeOsRegistrationOffers, The list seems to have been ordered alphabetically. Any particular reason for changing this? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/device_settings_provider.cc:249: int timer; This should be called |delay|, not |timer|. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/owner_key_util.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/owner_key_util.h:60: Why the blank line? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/dev... File chrome/browser/policy/device_local_account_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:46: #include "chromeos/dbus/fake_session_manager_client.h" You should update the list of includes, removing those that are used by |fake_session_manager_client.h| now but no longer but this file (e.g. session_manager_client.h). https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/pro... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/pro... chrome/browser/policy/proto/chrome_device_policy.proto:244: // The amount of time, in milliseconds, that should elapse at the signin Nit: ... "without user interaction" https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Since this is a new file, it should be (c) 2013. https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:17: void FakeSessionManagerClient::AddObserver(Observer* observer) OVERRIDE { You should only be specifying OVERRIDE in declarations, not in definitions. https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:20: void FakeSessionManagerClient::RemoveObserver(Observer* observer) OVERRIDE { Since these methods moved to a proper source file now, you should expand the very condensed formatting a bit IMHO: * Add empty lines between all definitions. * Put the closing braces on separate lines. https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:41: MessageLoop::current()->PostTask(FROM_HERE, #include "base/location.h" for this https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... File chromeos/dbus/fake_session_manager_client.h (right): https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.h:24: virtual void AddObserver(Observer* observer) OVERRIDE; #include "base/compiler_specific.h" for this https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.h:71: DISALLOW_COPY_AND_ASSIGN(FakeSessionManagerClient); #include "base/basictypes.h" for this
I'll add another browsertest tomorrow to check that the timer isn't starting when the lock screen comes up. (I've already tested it manually.) https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3538: 'example_value': "bob@example.com", On 2013/02/25 16:51:23, bartfab wrote: > Nit: I would go with an example value that explicitly shows this is a *public* > account, not one intended for a single person (Bob) to use. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3543: If this policy is set, the specified session will be automatically logged in after a period of time has elapsed at the login screen. The public session must already be configured (see "DeviceLocalAccounts"). On 2013/02/25 16:51:23, bartfab wrote: > Nit: Add "without user interaction" after "has elapsed at the login screen". Done. https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3556: 'example_value': 300, On 2013/02/25 16:51:23, bartfab wrote: > If this policy realy is in milliseconds, 300 is a bad example value (0.3s is a > bit too short to be really useful :). Done. https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3561: If the DeviceLocalAccountAutoLoginId policy is unset, this policy has no effect. Otherwise: On 2013/02/25 16:51:23, bartfab wrote: > Nit: Put the policy name in quotes for consistency. Or, even better, use bars > around the policy names instead of quotes. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:242: std::string setting = *content::Details<const std::string>(details).ptr(); On 2013/02/25 16:51:23, bartfab wrote: > Nit: I would make this a "const std::string" Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:510: StartPublicSessionAutoLoginTimer(); On 2013/02/25 16:51:23, bartfab wrote: > Notice that we end up here if the policy subsystem is not quite ready yet and we > need to wait for a callback before continuing with the login. In such a case, > the login attempt is still pending and the timer should not be restarted. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:570: StartPublicSessionAutoLoginTimer(); On 2013/02/25 16:51:23, bartfab wrote: > Notice that we end up here if the policy subsystem is not quite ready yet and we > need to wait for a callback before continuing with the login. In such a case, > the login attempt is still pending and the timer should not be restarted. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:896: // Policy load failure stops login attempts---restart the timer. On 2013/02/25 16:51:23, bartfab wrote: > Nit: I am still bothered by these triple dashes and the lack of spaces around > them :). Done. But I'm going to make sure that every email I send you has three dashes and no spaces around them. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:927: public_session_auto_login_username_ = ""; On 2013/02/25 16:51:23, bartfab wrote: > You can simplify this to: > > if (!cros_settings_->GetString( > kAccountsPrefDeviceLocalAccountAutoLoginId, > &public_session_auto_login_username_)) { > public_session_auto_login_username_ = ""; > } Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:936: ? ash::Shell::GetInstance()->user_activity_detector() : NULL; On 2013/02/25 16:51:23, bartfab wrote: > I think it is allowed to put the |?| at the start of the line but it is very > uncommon. Better put it at the end of the previous line. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:949: if (auto_login_timer_.IsRunning()) { On 2013/02/25 16:51:23, bartfab wrote: > It is pretty obvious what this function is doing but I think that a comment > would still be useful here: Document the fact that this restarts the timer but > only if it is currently running. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:969: is_login_in_progress_) { On 2013/02/25 16:51:23, bartfab wrote: > Nit: The conditions in lines 956-958 and 967-969 are the exact logical inverses > of each other but they are arranged in different order. Could you arrange them > in the same order for consistency? Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:72: // UserActivityObserver::Observer implementation. On 2013/02/25 16:51:23, bartfab wrote: > The overrides should follow the order in which the base classes are listed in > lines 41-45 above. Since UserActivityObserver is the last base class, overrides > for ExistingUserController should be declared last as well. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:213: base::OneShotTimer<ExistingUserController> auto_login_timer_; On 2013/02/25 16:51:23, bartfab wrote: > When I was adding the session length limiter, there was a lot of pushback > against always instantiating a timer for this purpose, even if the policy that > requires it is not set. The same could be said about the auto-login timer. It > will only actually be used on a tiny fraction of Chrome OS devices. Maybe better > wrap this in a scoped_ptr to keep memory usage minimal when auto-login is > disabled? Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:298: FRIEND_TEST_ALL_PREFIXES(ExistingUserControllerPublicSessionTest, On 2013/02/25 16:51:23, bartfab wrote: > Since you already added the base classes of your tests as friends, you can proxy > any accesses to the ExistingUserController through these classes - no need to > make the tests themselves friends as well. I tried that, it doesn't work. From https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members: "Note that even though your fixture is a friend to your production class, your tests are not automatically friends to it, as they are technically defined in sub-classes of the fixture." https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:5: #include "ash/shell.h" On 2013/02/25 16:51:23, bartfab wrote: > I trust you checked that the list of includes matches what your current code > uses? It is pretty hard to verify this as part of the review :). Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:156: // Observes user list change notifications and quits the message loop once a On 2013/02/25 16:51:23, bartfab wrote: > Is this not a straight copy & paste from Mattias' test as well? Code duplication > makes me :...( He refactored it into a different thing and I missed it when I looked at his submitted CL. I've now refactored that out into its own file. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:318: // CrosInProcessBrowserTest::CleanUpOnMainThread or it sometimes On 2013/02/25 16:51:23, bartfab wrote: > "or it sometimes crashes" sounds like something is going on that we do not > understand. If this is a crash we can reproduce and understand, better state it > here (e.g. "because it is an observer of singleton X"). If this is an > unexplicable crash, it would be good to get a backtrace and understand why it > happens only sometimes. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:488: return expected == actual; On 2013/02/25 16:51:23, bartfab wrote: > Why not do the dereferencing and comparison all on one line? Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:507: browser_policy_connector()->GetDeviceLocalAccountPolicyService()-> On 2013/02/25 16:51:23, bartfab wrote: > Indent 4 spaces. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:508: GetBrokerForAccount(kAutoLoginUsername)->core()->store(); On 2013/02/25 16:51:23, bartfab wrote: > Indent 4 spaces. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:520: .WillRepeatedly(Return(&session_manager_client_)); On 2013/02/25 16:51:23, bartfab wrote: > Indent 4 spaces. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:577: const std::string& username, const std::string& password, On 2013/02/25 16:51:23, bartfab wrote: > The arguments should either *all* fit on one line or *all* be placed on > individual lines, not a combination of both. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:665: // Cleanup. On 2013/02/25 16:51:23, bartfab wrote: > To match the other comments, this should be a verb in the imperative form -> > "Clean up." instead of "Cleanup." (which is a noun) Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:689: ExistingUserController* euc = existing_user_controller(); On 2013/02/25 16:51:23, bartfab wrote: > The style guide discourages abbreviations. |euc| is hard to read and remember. > Better use verbose variable names. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:696: ASSERT_FALSE(auto_login_timer()->IsRunning()); On 2013/02/25 16:51:23, bartfab wrote: > Here and in general: You should be using ASSERT_* only when it is critical to > abort the test if the condition is not met (e.g. if the timer is not running, > the test would deadlock, so abort). Otherwise, you should be using EXPECT_*. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:717: content::RunAllPendingInMessageLoop(); On 2013/02/25 16:51:23, bartfab wrote: > Could you document what tasks this drains from the message loop's queue and why > that is needed? Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:757: set_auto_login_delay(kAutoLoginShortDelay); On 2013/02/25 16:51:23, bartfab wrote: > If I understand correctly, you detect that the timer has restarted by the fact > that it picked up the modified login delay. Could you document this in a > comment? Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:773: // Turn off settings notifications to EUC. On 2013/02/25 16:51:23, bartfab wrote: > Just as in variable names, you should not be using abbreviations in comments. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:866: content::RunAllPendingInMessageLoop(); On 2013/02/25 16:51:23, bartfab wrote: > Nit: You have an emtpy line before the content::RunAllPendingInMessageLoop() at > the end in some tests and not in others. It would be nice to be consistent here. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:874: // Timer should fire immediately. On 2013/02/25 16:51:23, bartfab wrote: > Here and in the other tests: Can you document that PrepareLogin() sets up an > expectation that verifies the login happened? Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:889: base::TimeDelta::FromMilliseconds(2 * kAutoLoginShortDelay), On 2013/02/25 16:51:23, bartfab wrote: > Boo, waiting 200ms :(. Could you inspect the time the timer is set to and then > fast-forward the message loop by 100ms? There's no real point to this test, then. There's already a test that verifies that updating the policy updates the timer and a test when the timer is zero. Should I just delete this one? https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:919: RetailModeLoginStopsAutoLogin) { On 2013/02/25 16:51:23, bartfab wrote: > This is not necessary. Public sessions and retail mode cannot be mixed. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/mock_user_manager.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_user_manager.h:92: // Creates a new public account user. On 2013/02/25 16:51:23, bartfab wrote: > Nit: Since we are switching over to the term "public session", IMHO, we should > be using "public session" in all new code. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/webui_screen_locker.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/webui_screen_locker.cc:222: void WebUIScreenLocker::OnSigninScreenReady() { On 2013/02/25 16:51:23, bartfab wrote: > The order of these method overrides should match the order of their declarations > in LoginDisplay::Delegate. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/webui_screen_locker.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/webui_screen_locker.h:76: virtual void OnSigninScreenReady() OVERRIDE; On 2013/02/25 16:51:23, bartfab wrote: > The order of these method overrides should match the order of their declarations > in LoginDisplay::Delegate. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/cros_settings_names.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/cros_settings_names.cc:22: const char kAccountsPrefDeviceLocalAccountAutoLoginId[] = On 2013/02/25 16:51:23, bartfab wrote: > Nit: No need for a blank line before these four lines IMHO. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/cros_settings_names.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/cros_settings_names.h:19: extern const char kAccountsPrefDeviceLocalAccountAutoLoginId[]; On 2013/02/25 16:51:23, bartfab wrote: > Nit: No need for a blank line before these two lines IMHO. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/device_settings_provider.cc:46: kAllowRedeemChromeOsRegistrationOffers, On 2013/02/25 16:51:23, bartfab wrote: > The list seems to have been ordered alphabetically. Any particular reason for > changing this? Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/device_settings_provider.cc:249: int timer; On 2013/02/25 16:51:23, bartfab wrote: > This should be called |delay|, not |timer|. Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/owner_key_util.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/owner_key_util.h:60: On 2013/02/25 16:51:23, bartfab wrote: > Why the blank line? Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/dev... File chrome/browser/policy/device_local_account_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:46: #include "chromeos/dbus/fake_session_manager_client.h" On 2013/02/25 16:51:23, bartfab wrote: > You should update the list of includes, removing those that are used by > |fake_session_manager_client.h| now but no longer but this file (e.g. > session_manager_client.h). Done. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/pro... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/pro... chrome/browser/policy/proto/chrome_device_policy.proto:244: // The amount of time, in milliseconds, that should elapse at the signin On 2013/02/25 16:51:23, bartfab wrote: > Nit: ... "without user interaction" Done. https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/25 16:51:23, bartfab wrote: > Since this is a new file, it should be (c) 2013. Done. https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:17: void FakeSessionManagerClient::AddObserver(Observer* observer) OVERRIDE { On 2013/02/25 16:51:23, bartfab wrote: > You should only be specifying OVERRIDE in declarations, not in definitions. Done. https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:20: void FakeSessionManagerClient::RemoveObserver(Observer* observer) OVERRIDE { On 2013/02/25 16:51:23, bartfab wrote: > Since these methods moved to a proper source file now, you should expand the > very condensed formatting a bit IMHO: > > * Add empty lines between all definitions. > * Put the closing braces on separate lines. Done. https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:41: MessageLoop::current()->PostTask(FROM_HERE, On 2013/02/25 16:51:23, bartfab wrote: > #include "base/location.h" for this Done. https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... File chromeos/dbus/fake_session_manager_client.h (right): https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.h:24: virtual void AddObserver(Observer* observer) OVERRIDE; On 2013/02/25 16:51:23, bartfab wrote: > #include "base/compiler_specific.h" for this Done. https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.h:71: DISALLOW_COPY_AND_ASSIGN(FakeSessionManagerClient); On 2013/02/25 16:51:23, bartfab wrote: > #include "base/basictypes.h" for this Done.
PTAL Never mind about the additional test. ExistingUserController isn't used by the lock screen, so auto-login won't ever happen there. On Tue, Feb 26, 2013 at 7:04 PM, <dconnelly@chromium.org> wrote: > I'll add another browsertest tomorrow to check that the timer isn't starting > when the lock screen comes up. (I've already tested it manually.) > > > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > File chrome/app/policy/policy_templates.json (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > chrome/app/policy/policy_templates.json:3538: 'example_value': > "bob@example.com", > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: I would go with an example value that explicitly shows this is a > > *public* >> >> account, not one intended for a single person (Bob) to use. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > chrome/app/policy/policy_templates.json:3543: If this policy is set, the > specified session will be automatically logged in after a period of time > has elapsed at the login screen. The public session must already be > configured (see "DeviceLocalAccounts"). > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: Add "without user interaction" after "has elapsed at the login > > screen". > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > chrome/app/policy/policy_templates.json:3556: 'example_value': 300, > On 2013/02/25 16:51:23, bartfab wrote: >> >> If this policy realy is in milliseconds, 300 is a bad example value > > (0.3s is a >> >> bit too short to be really useful :). > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > chrome/app/policy/policy_templates.json:3561: If the > DeviceLocalAccountAutoLoginId policy is unset, this policy has no > effect. Otherwise: > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: Put the policy name in quotes for consistency. Or, even better, > > use bars >> >> around the policy names instead of quotes. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/existing_user_controller.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:242: > std::string setting = *content::Details<const > std::string>(details).ptr(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: I would make this a "const std::string" > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:510: > StartPublicSessionAutoLoginTimer(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Notice that we end up here if the policy subsystem is not quite ready > > yet and we >> >> need to wait for a callback before continuing with the login. In such > > a case, >> >> the login attempt is still pending and the timer should not be > > restarted. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:570: > StartPublicSessionAutoLoginTimer(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Notice that we end up here if the policy subsystem is not quite ready > > yet and we >> >> need to wait for a callback before continuing with the login. In such > > a case, >> >> the login attempt is still pending and the timer should not be > > restarted. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:896: // Policy > load failure stops login attempts---restart the timer. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: I am still bothered by these triple dashes and the lack of spaces > > around >> >> them :). > > > Done. But I'm going to make sure that every email I send you has three > dashes and no spaces around them. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:927: > public_session_auto_login_username_ = ""; > On 2013/02/25 16:51:23, bartfab wrote: >> >> You can simplify this to: > > >> if (!cros_settings_->GetString( >> kAccountsPrefDeviceLocalAccountAutoLoginId, >> &public_session_auto_login_username_)) { >> public_session_auto_login_username_ = ""; >> } > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:936: ? > ash::Shell::GetInstance()->user_activity_detector() : NULL; > On 2013/02/25 16:51:23, bartfab wrote: >> >> I think it is allowed to put the |?| at the start of the line but it > > is very >> >> uncommon. Better put it at the end of the previous line. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:949: if > (auto_login_timer_.IsRunning()) { > On 2013/02/25 16:51:23, bartfab wrote: >> >> It is pretty obvious what this function is doing but I think that a > > comment >> >> would still be useful here: Document the fact that this restarts the > > timer but >> >> only if it is currently running. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:969: > is_login_in_progress_) { > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: The conditions in lines 956-958 and 967-969 are the exact logical > > inverses >> >> of each other but they are arranged in different order. Could you > > arrange them >> >> in the same order for consistency? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/existing_user_controller.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.h:72: // > UserActivityObserver::Observer implementation. > On 2013/02/25 16:51:23, bartfab wrote: >> >> The overrides should follow the order in which the base classes are > > listed in >> >> lines 41-45 above. Since UserActivityObserver is the last base class, > > overrides >> >> for ExistingUserController should be declared last as well. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.h:213: > base::OneShotTimer<ExistingUserController> auto_login_timer_; > On 2013/02/25 16:51:23, bartfab wrote: >> >> When I was adding the session length limiter, there was a lot of > > pushback >> >> against always instantiating a timer for this purpose, even if the > > policy that >> >> requires it is not set. The same could be said about the auto-login > > timer. It >> >> will only actually be used on a tiny fraction of Chrome OS devices. > > Maybe better >> >> wrap this in a scoped_ptr to keep memory usage minimal when auto-login > > is >> >> disabled? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.h:298: > FRIEND_TEST_ALL_PREFIXES(ExistingUserControllerPublicSessionTest, > On 2013/02/25 16:51:23, bartfab wrote: >> >> Since you already added the base classes of your tests as friends, you > > can proxy >> >> any accesses to the ExistingUserController through these classes - no > > need to >> >> make the tests themselves friends as well. > > > I tried that, it doesn't work. From > https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members: > "Note that even though your fixture is a friend to your production > class, your tests are not automatically friends to it, as they are > technically defined in sub-classes of the fixture." > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc > (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:5: > #include "ash/shell.h" > On 2013/02/25 16:51:23, bartfab wrote: >> >> I trust you checked that the list of includes matches what your > > current code >> >> uses? It is pretty hard to verify this as part of the review :). > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:156: > // Observes user list change notifications and quits the message loop > once a > On 2013/02/25 16:51:23, bartfab wrote: >> >> Is this not a straight copy & paste from Mattias' test as well? Code > > duplication >> >> makes me :...( > > > He refactored it into a different thing and I missed it when I looked at > his submitted CL. I've now refactored that out into its own file. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:318: > // CrosInProcessBrowserTest::CleanUpOnMainThread or it sometimes > On 2013/02/25 16:51:23, bartfab wrote: >> >> "or it sometimes crashes" sounds like something is going on that we do > > not >> >> understand. If this is a crash we can reproduce and understand, better > > state it >> >> here (e.g. "because it is an observer of singleton X"). If this is an >> unexplicable crash, it would be good to get a backtrace and understand > > why it >> >> happens only sometimes. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:488: > return expected == actual; > On 2013/02/25 16:51:23, bartfab wrote: >> >> Why not do the dereferencing and comparison all on one line? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:507: > browser_policy_connector()->GetDeviceLocalAccountPolicyService()-> > On 2013/02/25 16:51:23, bartfab wrote: >> >> Indent 4 spaces. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:508: > GetBrokerForAccount(kAutoLoginUsername)->core()->store(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Indent 4 spaces. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:520: > .WillRepeatedly(Return(&session_manager_client_)); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Indent 4 spaces. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:577: > const std::string& username, const std::string& password, > On 2013/02/25 16:51:23, bartfab wrote: >> >> The arguments should either *all* fit on one line or *all* be placed > > on >> >> individual lines, not a combination of both. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:665: > // Cleanup. > On 2013/02/25 16:51:23, bartfab wrote: >> >> To match the other comments, this should be a verb in the imperative > > form -> >> >> "Clean up." instead of "Cleanup." (which is a noun) > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:689: > ExistingUserController* euc = existing_user_controller(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> The style guide discourages abbreviations. |euc| is hard to read and > > remember. >> >> Better use verbose variable names. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:696: > ASSERT_FALSE(auto_login_timer()->IsRunning()); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Here and in general: You should be using ASSERT_* only when it is > > critical to >> >> abort the test if the condition is not met (e.g. if the timer is not > > running, >> >> the test would deadlock, so abort). Otherwise, you should be using > > EXPECT_*. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:717: > content::RunAllPendingInMessageLoop(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Could you document what tasks this drains from the message loop's > > queue and why >> >> that is needed? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:757: > set_auto_login_delay(kAutoLoginShortDelay); > On 2013/02/25 16:51:23, bartfab wrote: >> >> If I understand correctly, you detect that the timer has restarted by > > the fact >> >> that it picked up the modified login delay. Could you document this in > > a >> >> comment? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:773: > // Turn off settings notifications to EUC. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Just as in variable names, you should not be using abbreviations in > > comments. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:866: > content::RunAllPendingInMessageLoop(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: You have an emtpy line before the > > content::RunAllPendingInMessageLoop() at >> >> the end in some tests and not in others. It would be nice to be > > consistent here. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:874: > // Timer should fire immediately. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Here and in the other tests: Can you document that PrepareLogin() sets > > up an >> >> expectation that verifies the login happened? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:889: > base::TimeDelta::FromMilliseconds(2 * kAutoLoginShortDelay), > On 2013/02/25 16:51:23, bartfab wrote: >> >> Boo, waiting 200ms :(. Could you inspect the time the timer is set to > > and then >> >> fast-forward the message loop by 100ms? > > > There's no real point to this test, then. There's already a test that > verifies that updating the policy updates the timer and a test when the > timer is zero. Should I just delete this one? > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:919: > RetailModeLoginStopsAutoLogin) { > On 2013/02/25 16:51:23, bartfab wrote: >> >> This is not necessary. Public sessions and retail mode cannot be > > mixed. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/mock_user_manager.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/mock_user_manager.h:92: // Creates a new > public account user. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: Since we are switching over to the term "public session", IMHO, > > we should >> >> be using "public session" in all new code. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/webui_screen_locker.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/webui_screen_locker.cc:222: void > WebUIScreenLocker::OnSigninScreenReady() { > On 2013/02/25 16:51:23, bartfab wrote: >> >> The order of these method overrides should match the order of their > > declarations >> >> in LoginDisplay::Delegate. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/webui_screen_locker.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/webui_screen_locker.h:76: virtual void > OnSigninScreenReady() OVERRIDE; > On 2013/02/25 16:51:23, bartfab wrote: >> >> The order of these method overrides should match the order of their > > declarations >> >> in LoginDisplay::Delegate. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > File chrome/browser/chromeos/settings/cros_settings_names.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/cros_settings_names.cc:22: const char > kAccountsPrefDeviceLocalAccountAutoLoginId[] = > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: No need for a blank line before these four lines IMHO. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > File chrome/browser/chromeos/settings/cros_settings_names.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/cros_settings_names.h:19: extern const > char kAccountsPrefDeviceLocalAccountAutoLoginId[]; > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: No need for a blank line before these two lines IMHO. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > File chrome/browser/chromeos/settings/device_settings_provider.cc > (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/device_settings_provider.cc:46: > kAllowRedeemChromeOsRegistrationOffers, > On 2013/02/25 16:51:23, bartfab wrote: >> >> The list seems to have been ordered alphabetically. Any particular > > reason for >> >> changing this? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/device_settings_provider.cc:249: int > timer; > On 2013/02/25 16:51:23, bartfab wrote: >> >> This should be called |delay|, not |timer|. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > File chrome/browser/chromeos/settings/owner_key_util.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/owner_key_util.h:60: > On 2013/02/25 16:51:23, bartfab wrote: >> >> Why the blank line? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/dev... > File chrome/browser/policy/device_local_account_browsertest.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/dev... > chrome/browser/policy/device_local_account_browsertest.cc:46: #include > "chromeos/dbus/fake_session_manager_client.h" > On 2013/02/25 16:51:23, bartfab wrote: >> >> You should update the list of includes, removing those that are used > > by >> >> |fake_session_manager_client.h| now but no longer but this file (e.g. >> session_manager_client.h). > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/pro... > File chrome/browser/policy/proto/chrome_device_policy.proto (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/pro... > chrome/browser/policy/proto/chrome_device_policy.proto:244: // The > amount of time, in milliseconds, that should elapse at the signin > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: ... "without user interaction" > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > File chromeos/dbus/fake_session_manager_client.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.cc:1: // Copyright (c) 2012 > The Chromium Authors. All rights reserved. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Since this is a new file, it should be (c) 2013. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.cc:17: void > FakeSessionManagerClient::AddObserver(Observer* observer) OVERRIDE { > On 2013/02/25 16:51:23, bartfab wrote: >> >> You should only be specifying OVERRIDE in declarations, not in > > definitions. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.cc:20: void > FakeSessionManagerClient::RemoveObserver(Observer* observer) OVERRIDE { > On 2013/02/25 16:51:23, bartfab wrote: >> >> Since these methods moved to a proper source file now, you should > > expand the >> >> very condensed formatting a bit IMHO: > > >> * Add empty lines between all definitions. >> * Put the closing braces on separate lines. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.cc:41: > MessageLoop::current()->PostTask(FROM_HERE, > On 2013/02/25 16:51:23, bartfab wrote: >> >> #include "base/location.h" for this > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > File chromeos/dbus/fake_session_manager_client.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.h:24: virtual void > AddObserver(Observer* observer) OVERRIDE; > On 2013/02/25 16:51:23, bartfab wrote: >> >> #include "base/compiler_specific.h" for this > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.h:71: > DISALLOW_COPY_AND_ASSIGN(FakeSessionManagerClient); > On 2013/02/25 16:51:23, bartfab wrote: >> >> #include "base/basictypes.h" for this > > > Done. > > https://codereview.chromium.org/12218078/ On Tue, Feb 26, 2013 at 7:04 PM, <dconnelly@chromium.org> wrote: > I'll add another browsertest tomorrow to check that the timer isn't starting > when the lock screen comes up. (I've already tested it manually.) > > > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > File chrome/app/policy/policy_templates.json (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > chrome/app/policy/policy_templates.json:3538: 'example_value': > "bob@example.com", > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: I would go with an example value that explicitly shows this is a > > *public* >> >> account, not one intended for a single person (Bob) to use. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > chrome/app/policy/policy_templates.json:3543: If this policy is set, the > specified session will be automatically logged in after a period of time > has elapsed at the login screen. The public session must already be > configured (see "DeviceLocalAccounts"). > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: Add "without user interaction" after "has elapsed at the login > > screen". > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > chrome/app/policy/policy_templates.json:3556: 'example_value': 300, > On 2013/02/25 16:51:23, bartfab wrote: >> >> If this policy realy is in milliseconds, 300 is a bad example value > > (0.3s is a >> >> bit too short to be really useful :). > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/app/policy/policy_... > chrome/app/policy/policy_templates.json:3561: If the > DeviceLocalAccountAutoLoginId policy is unset, this policy has no > effect. Otherwise: > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: Put the policy name in quotes for consistency. Or, even better, > > use bars >> >> around the policy names instead of quotes. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/existing_user_controller.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:242: > std::string setting = *content::Details<const > std::string>(details).ptr(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: I would make this a "const std::string" > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:510: > StartPublicSessionAutoLoginTimer(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Notice that we end up here if the policy subsystem is not quite ready > > yet and we >> >> need to wait for a callback before continuing with the login. In such > > a case, >> >> the login attempt is still pending and the timer should not be > > restarted. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:570: > StartPublicSessionAutoLoginTimer(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Notice that we end up here if the policy subsystem is not quite ready > > yet and we >> >> need to wait for a callback before continuing with the login. In such > > a case, >> >> the login attempt is still pending and the timer should not be > > restarted. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:896: // Policy > load failure stops login attempts---restart the timer. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: I am still bothered by these triple dashes and the lack of spaces > > around >> >> them :). > > > Done. But I'm going to make sure that every email I send you has three > dashes and no spaces around them. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:927: > public_session_auto_login_username_ = ""; > On 2013/02/25 16:51:23, bartfab wrote: >> >> You can simplify this to: > > >> if (!cros_settings_->GetString( >> kAccountsPrefDeviceLocalAccountAutoLoginId, >> &public_session_auto_login_username_)) { >> public_session_auto_login_username_ = ""; >> } > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:936: ? > ash::Shell::GetInstance()->user_activity_detector() : NULL; > On 2013/02/25 16:51:23, bartfab wrote: >> >> I think it is allowed to put the |?| at the start of the line but it > > is very >> >> uncommon. Better put it at the end of the previous line. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:949: if > (auto_login_timer_.IsRunning()) { > On 2013/02/25 16:51:23, bartfab wrote: >> >> It is pretty obvious what this function is doing but I think that a > > comment >> >> would still be useful here: Document the fact that this restarts the > > timer but >> >> only if it is currently running. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:969: > is_login_in_progress_) { > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: The conditions in lines 956-958 and 967-969 are the exact logical > > inverses >> >> of each other but they are arranged in different order. Could you > > arrange them >> >> in the same order for consistency? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/existing_user_controller.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.h:72: // > UserActivityObserver::Observer implementation. > On 2013/02/25 16:51:23, bartfab wrote: >> >> The overrides should follow the order in which the base classes are > > listed in >> >> lines 41-45 above. Since UserActivityObserver is the last base class, > > overrides >> >> for ExistingUserController should be declared last as well. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.h:213: > base::OneShotTimer<ExistingUserController> auto_login_timer_; > On 2013/02/25 16:51:23, bartfab wrote: >> >> When I was adding the session length limiter, there was a lot of > > pushback >> >> against always instantiating a timer for this purpose, even if the > > policy that >> >> requires it is not set. The same could be said about the auto-login > > timer. It >> >> will only actually be used on a tiny fraction of Chrome OS devices. > > Maybe better >> >> wrap this in a scoped_ptr to keep memory usage minimal when auto-login > > is >> >> disabled? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.h:298: > FRIEND_TEST_ALL_PREFIXES(ExistingUserControllerPublicSessionTest, > On 2013/02/25 16:51:23, bartfab wrote: >> >> Since you already added the base classes of your tests as friends, you > > can proxy >> >> any accesses to the ExistingUserController through these classes - no > > need to >> >> make the tests themselves friends as well. > > > I tried that, it doesn't work. From > https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members: > "Note that even though your fixture is a friend to your production > class, your tests are not automatically friends to it, as they are > technically defined in sub-classes of the fixture." > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc > (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:5: > #include "ash/shell.h" > On 2013/02/25 16:51:23, bartfab wrote: >> >> I trust you checked that the list of includes matches what your > > current code >> >> uses? It is pretty hard to verify this as part of the review :). > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:156: > // Observes user list change notifications and quits the message loop > once a > On 2013/02/25 16:51:23, bartfab wrote: >> >> Is this not a straight copy & paste from Mattias' test as well? Code > > duplication >> >> makes me :...( > > > He refactored it into a different thing and I missed it when I looked at > his submitted CL. I've now refactored that out into its own file. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:318: > // CrosInProcessBrowserTest::CleanUpOnMainThread or it sometimes > On 2013/02/25 16:51:23, bartfab wrote: >> >> "or it sometimes crashes" sounds like something is going on that we do > > not >> >> understand. If this is a crash we can reproduce and understand, better > > state it >> >> here (e.g. "because it is an observer of singleton X"). If this is an >> unexplicable crash, it would be good to get a backtrace and understand > > why it >> >> happens only sometimes. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:488: > return expected == actual; > On 2013/02/25 16:51:23, bartfab wrote: >> >> Why not do the dereferencing and comparison all on one line? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:507: > browser_policy_connector()->GetDeviceLocalAccountPolicyService()-> > On 2013/02/25 16:51:23, bartfab wrote: >> >> Indent 4 spaces. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:508: > GetBrokerForAccount(kAutoLoginUsername)->core()->store(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Indent 4 spaces. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:520: > .WillRepeatedly(Return(&session_manager_client_)); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Indent 4 spaces. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:577: > const std::string& username, const std::string& password, > On 2013/02/25 16:51:23, bartfab wrote: >> >> The arguments should either *all* fit on one line or *all* be placed > > on >> >> individual lines, not a combination of both. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:665: > // Cleanup. > On 2013/02/25 16:51:23, bartfab wrote: >> >> To match the other comments, this should be a verb in the imperative > > form -> >> >> "Clean up." instead of "Cleanup." (which is a noun) > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:689: > ExistingUserController* euc = existing_user_controller(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> The style guide discourages abbreviations. |euc| is hard to read and > > remember. >> >> Better use verbose variable names. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:696: > ASSERT_FALSE(auto_login_timer()->IsRunning()); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Here and in general: You should be using ASSERT_* only when it is > > critical to >> >> abort the test if the condition is not met (e.g. if the timer is not > > running, >> >> the test would deadlock, so abort). Otherwise, you should be using > > EXPECT_*. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:717: > content::RunAllPendingInMessageLoop(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Could you document what tasks this drains from the message loop's > > queue and why >> >> that is needed? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:757: > set_auto_login_delay(kAutoLoginShortDelay); > On 2013/02/25 16:51:23, bartfab wrote: >> >> If I understand correctly, you detect that the timer has restarted by > > the fact >> >> that it picked up the modified login delay. Could you document this in > > a >> >> comment? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:773: > // Turn off settings notifications to EUC. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Just as in variable names, you should not be using abbreviations in > > comments. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:866: > content::RunAllPendingInMessageLoop(); > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: You have an emtpy line before the > > content::RunAllPendingInMessageLoop() at >> >> the end in some tests and not in others. It would be nice to be > > consistent here. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:874: > // Timer should fire immediately. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Here and in the other tests: Can you document that PrepareLogin() sets > > up an >> >> expectation that verifies the login happened? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:889: > base::TimeDelta::FromMilliseconds(2 * kAutoLoginShortDelay), > On 2013/02/25 16:51:23, bartfab wrote: >> >> Boo, waiting 200ms :(. Could you inspect the time the timer is set to > > and then >> >> fast-forward the message loop by 100ms? > > > There's no real point to this test, then. There's already a test that > verifies that updating the policy updates the timer and a test when the > timer is zero. Should I just delete this one? > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:919: > RetailModeLoginStopsAutoLogin) { > On 2013/02/25 16:51:23, bartfab wrote: >> >> This is not necessary. Public sessions and retail mode cannot be > > mixed. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/mock_user_manager.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/mock_user_manager.h:92: // Creates a new > public account user. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: Since we are switching over to the term "public session", IMHO, > > we should >> >> be using "public session" in all new code. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/webui_screen_locker.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/webui_screen_locker.cc:222: void > WebUIScreenLocker::OnSigninScreenReady() { > On 2013/02/25 16:51:23, bartfab wrote: >> >> The order of these method overrides should match the order of their > > declarations >> >> in LoginDisplay::Delegate. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/webui_screen_locker.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/webui_screen_locker.h:76: virtual void > OnSigninScreenReady() OVERRIDE; > On 2013/02/25 16:51:23, bartfab wrote: >> >> The order of these method overrides should match the order of their > > declarations >> >> in LoginDisplay::Delegate. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > File chrome/browser/chromeos/settings/cros_settings_names.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/cros_settings_names.cc:22: const char > kAccountsPrefDeviceLocalAccountAutoLoginId[] = > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: No need for a blank line before these four lines IMHO. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > File chrome/browser/chromeos/settings/cros_settings_names.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/cros_settings_names.h:19: extern const > char kAccountsPrefDeviceLocalAccountAutoLoginId[]; > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: No need for a blank line before these two lines IMHO. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > File chrome/browser/chromeos/settings/device_settings_provider.cc > (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/device_settings_provider.cc:46: > kAllowRedeemChromeOsRegistrationOffers, > On 2013/02/25 16:51:23, bartfab wrote: >> >> The list seems to have been ordered alphabetically. Any particular > > reason for >> >> changing this? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/device_settings_provider.cc:249: int > timer; > On 2013/02/25 16:51:23, bartfab wrote: >> >> This should be called |delay|, not |timer|. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > File chrome/browser/chromeos/settings/owner_key_util.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/s... > chrome/browser/chromeos/settings/owner_key_util.h:60: > On 2013/02/25 16:51:23, bartfab wrote: >> >> Why the blank line? > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/dev... > File chrome/browser/policy/device_local_account_browsertest.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/dev... > chrome/browser/policy/device_local_account_browsertest.cc:46: #include > "chromeos/dbus/fake_session_manager_client.h" > On 2013/02/25 16:51:23, bartfab wrote: >> >> You should update the list of includes, removing those that are used > > by >> >> |fake_session_manager_client.h| now but no longer but this file (e.g. >> session_manager_client.h). > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/pro... > File chrome/browser/policy/proto/chrome_device_policy.proto (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/policy/pro... > chrome/browser/policy/proto/chrome_device_policy.proto:244: // The > amount of time, in milliseconds, that should elapse at the signin > On 2013/02/25 16:51:23, bartfab wrote: >> >> Nit: ... "without user interaction" > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > File chromeos/dbus/fake_session_manager_client.cc (right): > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.cc:1: // Copyright (c) 2012 > The Chromium Authors. All rights reserved. > On 2013/02/25 16:51:23, bartfab wrote: >> >> Since this is a new file, it should be (c) 2013. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.cc:17: void > FakeSessionManagerClient::AddObserver(Observer* observer) OVERRIDE { > On 2013/02/25 16:51:23, bartfab wrote: >> >> You should only be specifying OVERRIDE in declarations, not in > > definitions. > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.cc:20: void > FakeSessionManagerClient::RemoveObserver(Observer* observer) OVERRIDE { > On 2013/02/25 16:51:23, bartfab wrote: >> >> Since these methods moved to a proper source file now, you should > > expand the >> >> very condensed formatting a bit IMHO: > > >> * Add empty lines between all definitions. >> * Put the closing braces on separate lines. > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.cc:41: > MessageLoop::current()->PostTask(FROM_HERE, > On 2013/02/25 16:51:23, bartfab wrote: >> >> #include "base/location.h" for this > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > File chromeos/dbus/fake_session_manager_client.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.h:24: virtual void > AddObserver(Observer* observer) OVERRIDE; > On 2013/02/25 16:51:23, bartfab wrote: >> >> #include "base/compiler_specific.h" for this > > > Done. > > > https://codereview.chromium.org/12218078/diff/27005/chromeos/dbus/fake_sessio... > chromeos/dbus/fake_session_manager_client.h:71: > DISALLOW_COPY_AND_ASSIGN(FakeSessionManagerClient); > On 2013/02/25 16:51:23, bartfab wrote: >> >> #include "base/basictypes.h" for this > > > Done. > > https://codereview.chromium.org/12218078/
https://codereview.chromium.org/12218078/diff/35002/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/35002/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:944: detector->AddObserver(this); Please see another CL that recently has landed: https://codereview.chromium.org/12321136/diff/3002/chrome/browser/chromeos/lo... We now have WebUILoginDisplay always subscribed as activity observer. What do you think about using that single observer and pass calls here to ExistingUserController? That would simplify the logic a bit and you won't need to add/remove EUC as the observer but only react to those events when needed.
https://codereview.chromium.org/12218078/diff/35002/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/35002/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:944: detector->AddObserver(this); On 2013/02/27 19:17:03, Nikita Kostylev wrote: > Please see another CL that recently has landed: > > https://codereview.chromium.org/12321136/diff/3002/chrome/browser/chromeos/lo... > > We now have WebUILoginDisplay always subscribed as activity observer. > > What do you think about using that single observer and pass calls here to > ExistingUserController? That would simplify the logic a bit and you won't need > to add/remove EUC as the observer but only react to those events when needed. Cool. Done.
https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:298: FRIEND_TEST_ALL_PREFIXES(ExistingUserControllerPublicSessionTest, On 2013/02/26 18:04:15, dconnelly wrote: > On 2013/02/25 16:51:23, bartfab wrote: > > Since you already added the base classes of your tests as friends, you can > proxy > > any accesses to the ExistingUserController through these classes - no need to > > make the tests themselves friends as well. > > I tried that, it doesn't work. From > https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members: > "Note that even though your fixture is a friend to your production class, your > tests are not automatically friends to it, as they are technically defined in > sub-classes of the fixture." Yes. You need to implement the accesses to the ExistingUserController in helper methods provided by your fixture, then call these from within the tests. https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:889: base::TimeDelta::FromMilliseconds(2 * kAutoLoginShortDelay), On 2013/02/26 18:04:15, dconnelly wrote: > On 2013/02/25 16:51:23, bartfab wrote: > > Boo, waiting 200ms :(. Could you inspect the time the timer is set to and then > > fast-forward the message loop by 100ms? > > There's no real point to this test, then. There's already a test that verifies > that updating the policy updates the timer and a test when the timer is zero. > Should I just delete this one? No, I think there is value in it - I would keep the test. In the light of all our slow and flaky tests, 200ms is not going to hurt anyone :). What I meant was to cheat the timer and make it think that 200ms have passed already. But this is quite tricky to do. https://codereview.chromium.org/12218078/diff/36001/chrome/app/policy/policy_... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12218078/diff/36001/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3538: 'example_value': "bob@localhost", It seems you misunderstood my comment: The host part (example.com) was fine. We will be using actual host names in reality. What I suggested you could change is the local part, from bob@ to say public@. So I would suggest an example value of public@example.com. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:9: #include "ash/shell.h" This is no longer used. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:10: #include "ash/wm/user_activity_detector.h" This is no longer used. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:954: if (auto_login_timer().IsRunning()) { This and the many calls to StopPublicSessionTimer() all cause the timer to be unconditionally instantiated, defeating the mechanism you put in to ensure that the timer is only constructed if needed. To fix this, you should make auto_login_timer() return a pointer to the timer, treating NULL and (!NULL && IsRunning()) as "no timer running". https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:10: #include "ash/wm/user_activity_observer.h" This is no longer used. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:215: base::OneShotTimer<ExistingUserController>& auto_login_timer(); All-lowercase should be used for the simplest of getters and setters only. Since this getter can trigger a lazy construction, CamelCaps would be more appropriate. But it is a judgment call, so I leave it up to you. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:288: // if there is an outstanding login attempt when ExistingUserController is Nit: This is the start of a proper sentence. Capitalize. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:476: base::Unretained(chromeos::UserManager::Get()), Include "base/bind_helper.h" for this. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... File chrome/browser/policy/device_local_account_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:10: #include "base/callback.h" This is no longer used. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:48: #include "content/public/browser/notification_observer.h" This is no longer used. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:49: #include "content/public/browser/notification_registrar.h" This is no longer used. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:50: #include "content/public/browser/notification_service.h" This is no longer used. https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... File content/public/browser/notification_watcher.h (right): https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... content/public/browser/notification_watcher.h:24: typedef base::Callback<bool(void)> ConditionTestCallback; Include "base/callback.h" for this. https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... content/public/browser/notification_watcher.h:34: const content::NotificationDetails& details) OVERRIDE; Include "base/compiler_specific.h" for this. https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... content/public/browser/notification_watcher.h:41: DISALLOW_COPY_AND_ASSIGN(NotificationWatcher); Include "base/basictypes.h" for this.
https://codereview.chromium.org/12218078/diff/36001/chrome/app/policy/policy_... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12218078/diff/36001/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:3538: 'example_value': "bob@localhost", On 2013/02/28 10:21:33, bartfab wrote: > It seems you misunderstood my comment: The host part (http://example.com) was fine. We > will be using actual host names in reality. What I suggested you could change is > the local part, from bob@ to say public@. So I would suggest an example value of > mailto:public@example.com. Done. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:9: #include "ash/shell.h" On 2013/02/28 10:21:33, bartfab wrote: > This is no longer used. Done. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:10: #include "ash/wm/user_activity_detector.h" On 2013/02/28 10:21:33, bartfab wrote: > This is no longer used. Done. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:954: if (auto_login_timer().IsRunning()) { On 2013/02/28 10:21:33, bartfab wrote: > This and the many calls to StopPublicSessionTimer() all cause the timer to be > unconditionally instantiated, defeating the mechanism you put in to ensure that > the timer is only constructed if needed. > > To fix this, you should make auto_login_timer() return a pointer to the timer, > treating NULL and (!NULL && IsRunning()) as "no timer running". Done. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:10: #include "ash/wm/user_activity_observer.h" On 2013/02/28 10:21:33, bartfab wrote: > This is no longer used. Done. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:215: base::OneShotTimer<ExistingUserController>& auto_login_timer(); On 2013/02/28 10:21:33, bartfab wrote: > All-lowercase should be used for the simplest of getters and setters only. Since > this getter can trigger a lazy construction, CamelCaps would be more > appropriate. But it is a judgment call, so I leave it up to you. Done. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:288: // if there is an outstanding login attempt when ExistingUserController is On 2013/02/28 10:21:33, bartfab wrote: > Nit: This is the start of a proper sentence. Capitalize. Done. https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:476: base::Unretained(chromeos::UserManager::Get()), On 2013/02/28 10:21:33, bartfab wrote: > Include "base/bind_helper.h" for this. Done. https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... File content/public/browser/notification_watcher.h (right): https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... content/public/browser/notification_watcher.h:24: typedef base::Callback<bool(void)> ConditionTestCallback; On 2013/02/28 10:21:33, bartfab wrote: > Include "base/callback.h" for this. Done. https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... content/public/browser/notification_watcher.h:34: const content::NotificationDetails& details) OVERRIDE; On 2013/02/28 10:21:33, bartfab wrote: > Include "base/compiler_specific.h" for this. Done. https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... content/public/browser/notification_watcher.h:41: DISALLOW_COPY_AND_ASSIGN(NotificationWatcher); On 2013/02/28 10:21:33, bartfab wrote: > Include "base/basictypes.h" for this. Done.
PTAL On Thu, Feb 28, 2013 at 11:21 AM, <bartfab@chromium.org> wrote: > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/existing_user_controller.h (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.h:298: > FRIEND_TEST_ALL_PREFIXES(ExistingUserControllerPublicSessionTest, > On 2013/02/26 18:04:15, dconnelly wrote: >> >> On 2013/02/25 16:51:23, bartfab wrote: >> > Since you already added the base classes of your tests as friends, > > you can >> >> proxy >> > any accesses to the ExistingUserController through these classes - > > no need to >> >> > make the tests themselves friends as well. > > >> I tried that, it doesn't work. From > > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members: >> >> "Note that even though your fixture is a friend to your production > > class, your >> >> tests are not automatically friends to it, as they are technically > > defined in >> >> sub-classes of the fixture." > > > Yes. You need to implement the accesses to the ExistingUserController in > helper methods provided by your fixture, then call these from within the > tests. > Got it. Done. > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > File > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc > (right): > > https://codereview.chromium.org/12218078/diff/27005/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:889: > base::TimeDelta::FromMilliseconds(2 * kAutoLoginShortDelay), > On 2013/02/26 18:04:15, dconnelly wrote: >> >> On 2013/02/25 16:51:23, bartfab wrote: >> > Boo, waiting 200ms :(. Could you inspect the time the timer is set > > to and then >> >> > fast-forward the message loop by 100ms? > > >> There's no real point to this test, then. There's already a test that > > verifies >> >> that updating the policy updates the timer and a test when the timer > > is zero. >> >> Should I just delete this one? > > > No, I think there is value in it - I would keep the test. In the light > of all our slow and flaky tests, 200ms is not going to hurt anyone :). > > What I meant was to cheat the timer and make it think that 200ms have > passed already. But this is quite tricky to do. > > https://codereview.chromium.org/12218078/diff/36001/chrome/app/policy/policy_... > File chrome/app/policy/policy_templates.json (right): > > https://codereview.chromium.org/12218078/diff/36001/chrome/app/policy/policy_... > chrome/app/policy/policy_templates.json:3538: 'example_value': > "bob@localhost", > It seems you misunderstood my comment: The host part (example.com) was > fine. We will be using actual host names in reality. What I suggested > you could change is the local part, from bob@ to say public@. So I would > suggest an example value of public@example.com. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/existing_user_controller.cc (right): > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:9: #include > "ash/shell.h" > This is no longer used. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:10: #include > "ash/wm/user_activity_detector.h" > This is no longer used. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:954: if > (auto_login_timer().IsRunning()) { > This and the many calls to StopPublicSessionTimer() all cause the timer > to be unconditionally instantiated, defeating the mechanism you put in > to ensure that the timer is only constructed if needed. > > To fix this, you should make auto_login_timer() return a pointer to the > timer, treating NULL and (!NULL && IsRunning()) as "no timer running". > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/existing_user_controller.h (right): > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.h:10: #include > "ash/wm/user_activity_observer.h" > This is no longer used. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.h:215: > base::OneShotTimer<ExistingUserController>& auto_login_timer(); > All-lowercase should be used for the simplest of getters and setters > only. Since this getter can trigger a lazy construction, CamelCaps would > be more appropriate. But it is a judgment call, so I leave it up to you. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > File > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc > (right): > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:288: > // if there is an outstanding login attempt when ExistingUserController > is > Nit: This is the start of a proper sentence. Capitalize. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:476: > base::Unretained(chromeos::UserManager::Get()), > Include "base/bind_helper.h" for this. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... > File chrome/browser/policy/device_local_account_browsertest.cc (right): > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... > chrome/browser/policy/device_local_account_browsertest.cc:10: #include > "base/callback.h" > This is no longer used. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... > chrome/browser/policy/device_local_account_browsertest.cc:48: #include > "content/public/browser/notification_observer.h" > This is no longer used. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... > chrome/browser/policy/device_local_account_browsertest.cc:49: #include > "content/public/browser/notification_registrar.h" > This is no longer used. > > https://codereview.chromium.org/12218078/diff/36001/chrome/browser/policy/dev... > chrome/browser/policy/device_local_account_browsertest.cc:50: #include > "content/public/browser/notification_service.h" > This is no longer used. > > https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... > File content/public/browser/notification_watcher.h (right): > > https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... > content/public/browser/notification_watcher.h:24: typedef > base::Callback<bool(void)> ConditionTestCallback; > Include "base/callback.h" for this. > > https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... > content/public/browser/notification_watcher.h:34: const > content::NotificationDetails& details) OVERRIDE; > Include "base/compiler_specific.h" for this. > > https://codereview.chromium.org/12218078/diff/36001/content/public/browser/no... > content/public/browser/notification_watcher.h:41: > DISALLOW_COPY_AND_ASSIGN(NotificationWatcher); > Include "base/basictypes.h" for this. > > https://codereview.chromium.org/12218078/
Just one more comment to go :). https://chromiumcodereview.appspot.com/12218078/diff/50003/chrome/browser/chr... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://chromiumcodereview.appspot.com/12218078/diff/50003/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:809: EXPECT_FALSE(auto_login_timer()->IsRunning()); Before accessing auto_login_timer()->, you will want to do an ASSERT_TRUE(auto_login_timer()) (here and elsewhere)
https://chromiumcodereview.appspot.com/12218078/diff/50003/chrome/browser/chr... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://chromiumcodereview.appspot.com/12218078/diff/50003/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:809: EXPECT_FALSE(auto_login_timer()->IsRunning()); On 2013/02/28 13:08:14, bartfab wrote: > Before accessing auto_login_timer()->, you will want to do an > ASSERT_TRUE(auto_login_timer()) (here and elsewhere) Done.
LGTM, ship it! (with one nit :) https://chromiumcodereview.appspot.com/12218078/diff/42007/chrome/browser/chr... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://chromiumcodereview.appspot.com/12218078/diff/42007/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:857: ASSERT_TRUE(auto_login_timer() && auto_login_timer()->IsRunning()); Nit: If you break this into two ASSERTs, it will be easier to see which of the two conditions was not met when tests fails.
mnissler, nkostylev: PTAL when you have a chance. https://chromiumcodereview.appspot.com/12218078/diff/42007/chrome/browser/chr... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://chromiumcodereview.appspot.com/12218078/diff/42007/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:857: ASSERT_TRUE(auto_login_timer() && auto_login_timer()->IsRunning()); On 2013/02/28 13:29:14, bartfab wrote: > Nit: If you break this into two ASSERTs, it will be easier to see which of the > two conditions was not met when tests fails. Done.
One thing that I find a bit odd with the test is that they're actually mixing unit testing (i.e. testing behavior of ExistingUserController in isolation where you control all inputs and outputs) with integration testing (where you test entire Chrome avoiding to reach into its internals as much as possible). Anyhow, not sure whether ExistingUserController allows testing in any of these two ways, just mentioning because that seems to be the two basic approaches to testing we take elsewhere. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:952: DCHECK(signin_screen_ready_ && Make this a check maybe? https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:129: class MockCloudPolicyStoreObserver : public policy::CloudPolicyStore::Observer { There's a copy of this in mock_cloud_policy_store_observer.h already. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:151: scoped_refptr<Authenticator> CreateAuthenticatorPublicSession( nit: Maybe rather CreateAuthenticatorForPublicSession? https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:463: : public ExistingUserControllerTest { nit: 4 space indentation https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:544: void DisableAutoLoginPolicyNotifications() { Yikes, why is this necessary? https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:598: // Clear the key so the device can't write owner settings. Can you elaborate? Personally, I know what's going on, but this is very subtle. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:634: RefreshDevicePolicy(); How does this ensure that ExistingUserController actually reads the new settings back from the system? https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:659: // Enables public sessions in tests. Not entirely accurate. Actually it mocks out session manager, i.e. policy load/stores from/to the device. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:860: // Wait for the timer to fire. It looks like you're actually waiting. That'll just make the test take longer for no good reason, which is a big no-no. Can you mock out the timer? https://codereview.chromium.org/12218078/diff/36038/content/public/browser/no... File content/public/browser/notification_watcher.h (right): https://codereview.chromium.org/12218078/diff/36038/content/public/browser/no... content/public/browser/notification_watcher.h:22: class CONTENT_EXPORT NotificationWatcher There's already WindowedNotificationObserver in content/public/test/test_utils.h, which does essentially the same thing. I wasn't aware of that when I wrote device_local_account_browsertest.cc, but I think it makes sense to reconcile these instead of having the same thing twice now that you need this code for your test as well. You probably want to check with the author of WindowedNotificationObserver.
lgtm https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:972: auto_login_timer_.reset(new base::OneShotTimer<ExistingUserController>); nit: Insert one empty line after this one to improve readability. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:860: // Wait for the timer to fire. On 2013/03/01 10:37:21, Mattias Nissler wrote: > It looks like you're actually waiting. That'll just make the test take longer > for no good reason, which is a big no-no. Can you mock out the timer? Take a look at this issue https://code.google.com/p/chromium/issues/detail?id=166469
Okay, I've split out some of the tests as existing_user_controller_unittest.cc. About half of the tests are still browsertests. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:952: DCHECK(signin_screen_ready_ && On 2013/03/01 10:37:21, Mattias Nissler wrote: > Make this a check maybe? Done. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:972: auto_login_timer_.reset(new base::OneShotTimer<ExistingUserController>); On 2013/03/04 15:52:08, Nikita Kostylev wrote: > nit: Insert one empty line after this one to improve readability. Done. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:129: class MockCloudPolicyStoreObserver : public policy::CloudPolicyStore::Observer { On 2013/03/01 10:37:21, Mattias Nissler wrote: > There's a copy of this in mock_cloud_policy_store_observer.h already. Done. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:151: scoped_refptr<Authenticator> CreateAuthenticatorPublicSession( On 2013/03/01 10:37:21, Mattias Nissler wrote: > nit: Maybe rather CreateAuthenticatorForPublicSession? Done. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:463: : public ExistingUserControllerTest { On 2013/03/01 10:37:21, Mattias Nissler wrote: > nit: 4 space indentation Done. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:544: void DisableAutoLoginPolicyNotifications() { On 2013/03/01 10:37:21, Mattias Nissler wrote: > Yikes, why is this necessary? So that the auto-login timer delay and username can be changed without starting the timer. The notifications trigger ConfigurePublicSessionAutoLoginTimer, which will start/restart/stop the timer based on the delivered policy. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:634: RefreshDevicePolicy(); On 2013/03/01 10:37:21, Mattias Nissler wrote: > How does this ensure that ExistingUserController actually reads the new settings > back from the system? That's the next line few lines (Run()). I've clarified the comment. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:659: // Enables public sessions in tests. On 2013/03/01 10:37:21, Mattias Nissler wrote: > Not entirely accurate. Actually it mocks out session manager, i.e. policy > load/stores from/to the device. Done. https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:860: // Wait for the timer to fire. On 2013/03/04 15:52:08, Nikita Kostylev wrote: > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > It looks like you're actually waiting. That'll just make the test take longer > > for no good reason, which is a big no-no. Can you mock out the timer? > > Take a look at this issue > https://code.google.com/p/chromium/issues/detail?id=166469 Okay, so I've dug into how I can avoid waiting. There are two options: 1. Create a mock timer, keep the existing test, and use the mock timer to move forward the fire delay to 0 after it's been set. But this won't test anything that the existing AutoLoginNoDelay and ConfigureAutoLogin tests don't already test: when the delay is changed the timer is updated, and the timer fires and does the right stuff when the delay reaches zero. 2. Refactor the existing timer to allow accelerating the time. Right now it uses a task runner that's configured to be a singleton per thread, can't be replaced, and which is instantiated by the message loop. To mock this I would need to do one of the following: - refactor MessageLoop to allow an injected task runner, which will affect tasks already in the loop - refactor ThreadTaskRunnerHandle to allow replacing the singleton task runner instance, which will also affect already-scheduled tasks - refactor Timer to store a pointer to a task runner, but Timer is used in a million places and this will make every instance larger - refactor everything to somehow use the new Clock interface that Nikita pointed me at. After discussing this with Bartosz we think a reasonable idea is to set the delay to something small, like 5 millisecond (instead of 100) and just block for 10 milliseconds. https://codereview.chromium.org/12218078/diff/36038/content/public/browser/no... File content/public/browser/notification_watcher.h (right): https://codereview.chromium.org/12218078/diff/36038/content/public/browser/no... content/public/browser/notification_watcher.h:22: class CONTENT_EXPORT NotificationWatcher On 2013/03/01 10:37:21, Mattias Nissler wrote: > There's already WindowedNotificationObserver in > content/public/test/test_utils.h, which does essentially the same thing. I > wasn't aware of that when I wrote device_local_account_browsertest.cc, but I > think it makes sense to reconcile these instead of having the same thing twice > now that you need this code for your test as well. > > You probably want to check with the author of WindowedNotificationObserver. Done.
bartfab, nkostylev: PTAL. mnissler is out of town all week but this might should go in earlier since it's blocking server-side work. On 2013/03/06 10:02:59, dconnelly wrote: > Okay, I've split out some of the tests as existing_user_controller_unittest.cc. > About half of the tests are still browsertests. > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/existing_user_controller.cc (right): > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:952: > DCHECK(signin_screen_ready_ && > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > Make this a check maybe? > > Done. > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller.cc:972: > auto_login_timer_.reset(new base::OneShotTimer<ExistingUserController>); > On 2013/03/04 15:52:08, Nikita Kostylev wrote: > > nit: Insert one empty line after this one to improve readability. > > Done. > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc > (right): > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:129: class > MockCloudPolicyStoreObserver : public policy::CloudPolicyStore::Observer { > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > There's a copy of this in mock_cloud_policy_store_observer.h already. > > Done. > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:151: > scoped_refptr<Authenticator> CreateAuthenticatorPublicSession( > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > nit: Maybe rather CreateAuthenticatorForPublicSession? > > Done. > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:463: : > public ExistingUserControllerTest { > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > nit: 4 space indentation > > Done. > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:544: void > DisableAutoLoginPolicyNotifications() { > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > Yikes, why is this necessary? > > So that the auto-login timer delay and username can be changed without starting > the timer. The notifications trigger ConfigurePublicSessionAutoLoginTimer, > which will start/restart/stop the timer based on the delivered policy. > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:634: > RefreshDevicePolicy(); > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > How does this ensure that ExistingUserController actually reads the new > settings > > back from the system? > > That's the next line few lines (Run()). I've clarified the comment. > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:659: // > Enables public sessions in tests. > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > Not entirely accurate. Actually it mocks out session manager, i.e. policy > > load/stores from/to the device. > > Done. > > https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:860: // > Wait for the timer to fire. > On 2013/03/04 15:52:08, Nikita Kostylev wrote: > > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > > It looks like you're actually waiting. That'll just make the test take > longer > > > for no good reason, which is a big no-no. Can you mock out the timer? > > > > Take a look at this issue > > https://code.google.com/p/chromium/issues/detail?id=166469 > > Okay, so I've dug into how I can avoid waiting. There are two options: > > 1. Create a mock timer, keep the existing test, and use the mock timer to move > forward the fire delay to 0 after it's been set. But this won't test anything > that the existing AutoLoginNoDelay and ConfigureAutoLogin tests don't already > test: when the delay is changed the timer is updated, and the timer fires and > does the right stuff when the delay reaches zero. > 2. Refactor the existing timer to allow accelerating the time. Right now it > uses a task runner that's configured to be a singleton per thread, can't be > replaced, and which is instantiated by the message loop. To mock this I would > need to do one of the following: > - refactor MessageLoop to allow an injected task runner, which will affect tasks > already in the loop > - refactor ThreadTaskRunnerHandle to allow replacing the singleton task runner > instance, which will also affect already-scheduled tasks > - refactor Timer to store a pointer to a task runner, but Timer is used in a > million places and this will make every instance larger > - refactor everything to somehow use the new Clock interface that Nikita pointed > me at. > > After discussing this with Bartosz we think a reasonable idea is to set the > delay to something small, like 5 millisecond (instead of 100) and just block for > 10 milliseconds. > > https://codereview.chromium.org/12218078/diff/36038/content/public/browser/no... > File content/public/browser/notification_watcher.h (right): > > https://codereview.chromium.org/12218078/diff/36038/content/public/browser/no... > content/public/browser/notification_watcher.h:22: class CONTENT_EXPORT > NotificationWatcher > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > There's already WindowedNotificationObserver in > > content/public/test/test_utils.h, which does essentially the same thing. I > > wasn't aware of that when I wrote device_local_account_browsertest.cc, but I > > think it makes sense to reconcile these instead of having the same thing twice > > now that you need this code for your test as well. > > > > You probably want to check with the author of WindowedNotificationObserver. > > Done.
lgtm Please don't forget to run this trybots: cros_* (3 of them, CQ would not execute them). linux_chromeos (CQ would skip some of tests) linux_chromeos_asan https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:598: // Clear the key so the device can't write owner settings. On 2013/03/01 10:37:21, Mattias Nissler wrote: > Can you elaborate? Personally, I know what's going on, but this is very subtle. nit: This comment was not addressed. https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_unittest.cc (right): https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_unittest.cc:36: const int kAutoLoginShortDelay = 100; Could this be set to 0? Or at 1? https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_unittest.cc:41: class ExistingUserControllerAutoLoginTest : public ::testing::Test { nit: Class name should be consistent with filename which is existing_user_controller_unittest. Rename file? https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_unittest.cc:114: MockLoginUtils* mock_login_utils_; nit: // Owned by LoginUtilsWrapper. https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_unittest.cc:115: MockLoginDisplay* mock_login_display_; nit: Missing comment: // |mock_login_display_| is owned by the ExistingUserController, which calls // CreateLoginDisplay() on the |mock_login_display_host_| to get it.
https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/36038/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:598: // Clear the key so the device can't write owner settings. On 2013/03/06 17:07:16, Nikita Kostylev wrote: > On 2013/03/01 10:37:21, Mattias Nissler wrote: > > Can you elaborate? Personally, I know what's going on, but this is very > subtle. > > nit: This comment was not addressed. Thank you. Done. https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_unittest.cc (right): https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_unittest.cc:36: const int kAutoLoginShortDelay = 100; On 2013/03/06 17:07:16, Nikita Kostylev wrote: > Could this be set to 0? Or at 1? Done. https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_unittest.cc:41: class ExistingUserControllerAutoLoginTest : public ::testing::Test { On 2013/03/06 17:07:16, Nikita Kostylev wrote: > nit: Class name should be consistent with filename which is > existing_user_controller_unittest. Rename file? Done. https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_unittest.cc:114: MockLoginUtils* mock_login_utils_; On 2013/03/06 17:07:16, Nikita Kostylev wrote: > nit: // Owned by LoginUtilsWrapper. Done. https://codereview.chromium.org/12218078/diff/60001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_unittest.cc:115: MockLoginDisplay* mock_login_display_; On 2013/03/06 17:07:16, Nikita Kostylev wrote: > nit: Missing comment: > > // |mock_login_display_| is owned by the ExistingUserController, which calls > // CreateLoginDisplay() on the |mock_login_display_host_| to get it. Done.
https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:168: kAccountsPrefDeviceLocalAccountAutoLoginId, #include "chrome/browser/chromeos/settings/cros_settings_names.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:9: Could you #include "base/basictypes.h" here? It is missing for some reason. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:5: #include "base/memory/scoped_ptr.h" This is included by "existing_user_controller.h" already. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:8: #include "chrome/browser/chromeos/login/login_display.h" This is not used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:9: #include "chrome/browser/chromeos/login/login_display_host.h" This is not used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:10: #include "chrome/browser/chromeos/login/login_utils.h" This is included by "existing_user_controller.h" already. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:22: using testing::AnyNumber; This is not used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:23: using testing::AtLeast; This is not used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:24: using testing::Mock; This is not used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:26: using testing::ReturnNull; This is not used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:27: using testing::SaveArg; This is not used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:35: const int kAutoLoginNoDelay = 0; This is not used anywhere. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:36: const int kAutoLoginShortDelay = 1; Nit: Add an "Ms" suffix to make it clear that this is in milliseconds. I don't think using the extremely short timeout of 1ms makes sense here. The unit test never actually lets the timer run, so it makes no practicial difference whether the delay is "long" or "short" and how long or short it actually is. How about just calling the delays |kAutoLoginDelay1| and |kAutoLoginDelay2| and setting them to values that would be reasonable in practice, making it clear that the test does not rely on specific, very short values (as opposted to the browser test, which does)? https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:37: const int kAutoLoginLongDelay = 10000; Nit: Add an "Ms" suffix to make it clear that this is in milliseconds. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:62: CrosSettings::Get()->RemoveSettingsObserver( Why do you not want the ExistingUserController to pick up policy changes automagically in this test? Could you document the reasoning? https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:63: kAccountsPrefDeviceLocalAccountAutoLoginId, #include "chrome/browser/chromeos/settings/cros_settings_names.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:88: std::string auto_login_username() { Nit: const Nit: Return const std::string& https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:95: int auto_login_delay() { Nit: const https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:102: bool is_login_in_progress() { Nit: const https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:114: // Owned by LoginUtilsWrapper Nit: Period at the end of the line. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:203: // Timer shouldn't start when the policy is disabled. You are not really disabling the policy here, you are setting it to blanks. Would you not have actually unset policy if you left out the SetAutoLoginSettings() call? https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:204: SetAutoLoginSettings("", 0); Nit: Use kAutoLoginNoDelay https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:207: EXPECT_EQ(auto_login_delay(), 0); Nit: Use kAutoLoginNoDelay https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:8: #include "base/compiler_specific.h" This is actually included by "existing_user_controller.h" and can be removed here (not your fault but a nice clean-up). https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:9: #include "base/memory/scoped_ptr.h" This is actually included by "existing_user_controller.h" and can be removed here (not your fault but a nice clean-up). https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:17: #include "chrome/browser/chromeos/login/login_display.h" No longer needed. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:18: #include "chrome/browser/chromeos/login/login_display_host.h" No longer needed. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:19: #include "chrome/browser/chromeos/login/login_performer.h" This is actually included by "existing_user_controller.h" and can be removed here (not your fault but a nice clean-up). https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:21: #include "chrome/browser/chromeos/login/login_utils.h" This is actually included by "existing_user_controller.h" and can be removed here (not your fault but a nice clean-up). https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:61: using ::testing::ReturnRef; This is not used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:196: // |mock_login_display_| is owned by the ExistingUserController, which calls Move this comment to the place where |mock_login_display_| is being declared (MockLoginDisplay* mock_login_display_;). https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:247: std::string auto_login_username() { Nit: const Nit: Return const std::string& https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:254: int auto_login_delay() { Nit: const https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:261: bool is_login_in_progress() { Nit: const https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:399: return expected == *content::Details<const std::string>(arg).ptr(); #include "content/public/browser/notification_details.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:412: if (!chromeos::UserManager::Get()->IsKnownUser(kAutoLoginUsername)) { #include "chrome/browser/chromeos/login/user_manager.h" fot this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:422: content::MessageLoopRunner* runner = new content::MessageLoopRunner; #include "content/public/test/test_utils.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:425: GetBrokerForAccount(kAutoLoginUsername)->core()->store(); #include "chrome/browser/policy/cloud_policy_core.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:441: base::FilePath owner_key_file = temp_dir_.path().AppendASCII("owner.key"); #include "base/file_path.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:442: std::vector<uint8> owner_key_bits; #include <vector> for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:443: ASSERT_TRUE(device_policy_.signing_key()->ExportPublicKey(&owner_key_bits)); #include "crypto/rsa_private_key.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:445: file_util::WriteFile( #include "base/file_util.h" https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:471: virtual void SetUpLoginDisplay() { OVERRIDE (clang should have complained) https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:484: void DisableAutoLoginPolicyNotifications() { This does nto seem to be used anywhere. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:486: kAccountsPrefDeviceLocalAccountAutoLoginId, #include "chrome/browser/chromeos/settings/cros_settings_names.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:495: scoped_refptr<Authenticator> create_authenticator( #include "base/memory/ref_counted.h" and "chrome/browser/chromeos/login/authenticator" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:506: &base::Callback<void(void)>::Run)); #include "base/callback.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:605: base::ScopedTempDir temp_dir_; #include "base/files/scoped_temp_dir.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:636: IN_PROC_BROWSER_TEST_F(ExistingUserControllerPublicSessionTest, I wonder if this test provides any additional coverage over AutoLoginNoDelay. The latter seems to be a strict superset of this test. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:674: // Wait for the timer to fire. So this test will *always* incur a 10ms wait, even if the timer works (and fires after half the time). We can do better than that: * Wait for kAutoLoginShortDelay + a small offset * Reduce kAutoLoginShortDelay further. Would 1ms work? https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:677: timer.Start(FROM_HERE, #include "base/location.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:693: set_auto_login_username(kAutoLoginUsername); Could you go through policy here instead? https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:720: ScopedMockUserManagerEnabler mock_user_manager; Why is this required for guest mode login? https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:725: set_auto_login_username(kAutoLoginUsername); Could you go through policy here instead? https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:753: set_auto_login_username(kAutoLoginUsername); Could you go through policy here instead? https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/mock_login_display.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.cc:9: MockLoginDisplay::MockLoginDisplay() : LoginDisplay(NULL, gfx::Rect()) { #include "ui/gfx/rect.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/mock_login_display.h (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.h:8: #include "chrome/browser/chromeos/login/help_app_launcher.h" What is this needed for? If it is required to make the class fully defined and compilable, it should be included by the .cc file IMHO. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.h:10: #include "chrome/browser/chromeos/login/user.h" What is this needed for? If it is required to make the class fully defined and compilable, it should be included by the .cc file IMHO. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.h:11: #include "chrome/test/base/ui_test_utils.h" What is this needed for? If it is required to make the class fully defined and compilable, it should be included by the .cc file IMHO. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.h:37: DISALLOW_COPY_AND_ASSIGN(MockLoginDisplay); #include "base/basictypes.h" for this https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/mock_login_display_host.h (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display_host.h:10: #include "chrome/test/base/ui_test_utils.h" What is this needed for? If it is required to make the class fully defined and compilable, it should be included by the .cc file IMHO. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... File chrome/browser/policy/device_local_account_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:10: #include "base/callback.h" You seem to have missed my comment: This is no longer used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:48: #include "content/public/browser/notification_observer.h" You seem to have missed my comment: This is no longer used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:49: #include "content/public/browser/notification_registrar.h" You seem to have missed my comment: This is no longer used. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:50: #include "content/public/browser/notification_service.h" You seem to have missed my comment: This is no longer used. https://codereview.chromium.org/12218078/diff/77001/content/public/test/notif... File content/public/test/notification_watcher.h (right): https://codereview.chromium.org/12218078/diff/77001/content/public/test/notif... content/public/test/notification_watcher.h:5: #ifndef CONTENT_PUBLIC_BROWSER_NOTIFICATION_WATCHER_H_ This no longer matches the file path.
https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:168: kAccountsPrefDeviceLocalAccountAutoLoginId, On 2013/03/07 12:17:23, bartfab wrote: > #include "chrome/browser/chromeos/settings/cros_settings_names.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.h:9: On 2013/03/07 12:17:23, bartfab wrote: > Could you #include "base/basictypes.h" here? It is missing for some reason. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:5: #include "base/memory/scoped_ptr.h" On 2013/03/07 12:17:23, bartfab wrote: > This is included by "existing_user_controller.h" already. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:8: #include "chrome/browser/chromeos/login/login_display.h" On 2013/03/07 12:17:23, bartfab wrote: > This is not used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:9: #include "chrome/browser/chromeos/login/login_display_host.h" On 2013/03/07 12:17:23, bartfab wrote: > This is not used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:10: #include "chrome/browser/chromeos/login/login_utils.h" On 2013/03/07 12:17:23, bartfab wrote: > This is included by "existing_user_controller.h" already. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:22: using testing::AnyNumber; On 2013/03/07 12:17:23, bartfab wrote: > This is not used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:23: using testing::AtLeast; On 2013/03/07 12:17:23, bartfab wrote: > This is not used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:24: using testing::Mock; On 2013/03/07 12:17:23, bartfab wrote: > This is not used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:26: using testing::ReturnNull; On 2013/03/07 12:17:23, bartfab wrote: > This is not used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:27: using testing::SaveArg; On 2013/03/07 12:17:23, bartfab wrote: > This is not used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:35: const int kAutoLoginNoDelay = 0; On 2013/03/07 12:17:23, bartfab wrote: > This is not used anywhere. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:36: const int kAutoLoginShortDelay = 1; On 2013/03/07 12:17:23, bartfab wrote: > Nit: Add an "Ms" suffix to make it clear that this is in milliseconds. > > I don't think using the extremely short timeout of 1ms makes sense here. The > unit test never actually lets the timer run, so it makes no practicial > difference whether the delay is "long" or "short" and how long or short it > actually is. How about just calling the delays |kAutoLoginDelay1| and > |kAutoLoginDelay2| and setting them to values that would be reasonable in > practice, making it clear that the test does not rely on specific, very short > values (as opposted to the browser test, which does)? Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:37: const int kAutoLoginLongDelay = 10000; On 2013/03/07 12:17:23, bartfab wrote: > Nit: Add an "Ms" suffix to make it clear that this is in milliseconds. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:62: CrosSettings::Get()->RemoveSettingsObserver( On 2013/03/07 12:17:23, bartfab wrote: > Why do you not want the ExistingUserController to pick up policy changes > automagically in this test? Could you document the reasoning? Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:63: kAccountsPrefDeviceLocalAccountAutoLoginId, On 2013/03/07 12:17:23, bartfab wrote: > #include "chrome/browser/chromeos/settings/cros_settings_names.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:88: std::string auto_login_username() { On 2013/03/07 12:17:23, bartfab wrote: > Nit: const > Nit: Return const std::string& Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:95: int auto_login_delay() { On 2013/03/07 12:17:23, bartfab wrote: > Nit: const Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:102: bool is_login_in_progress() { On 2013/03/07 12:17:23, bartfab wrote: > Nit: const Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:114: // Owned by LoginUtilsWrapper On 2013/03/07 12:17:23, bartfab wrote: > Nit: Period at the end of the line. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:203: // Timer shouldn't start when the policy is disabled. On 2013/03/07 12:17:23, bartfab wrote: > You are not really disabling the policy here, you are setting it to blanks. > Would you not have actually unset policy if you left out the > SetAutoLoginSettings() call? Yes. This is redundant. It seemed more clear to do it explicitly. Removed. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:204: SetAutoLoginSettings("", 0); On 2013/03/07 12:17:23, bartfab wrote: > Nit: Use kAutoLoginNoDelay Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:207: EXPECT_EQ(auto_login_delay(), 0); On 2013/03/07 12:17:23, bartfab wrote: > Nit: Use kAutoLoginNoDelay Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:8: #include "base/compiler_specific.h" On 2013/03/07 12:17:23, bartfab wrote: > This is actually included by "existing_user_controller.h" and can be removed > here (not your fault but a nice clean-up). Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:9: #include "base/memory/scoped_ptr.h" On 2013/03/07 12:17:23, bartfab wrote: > This is actually included by "existing_user_controller.h" and can be removed > here (not your fault but a nice clean-up). Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:17: #include "chrome/browser/chromeos/login/login_display.h" On 2013/03/07 12:17:23, bartfab wrote: > No longer needed. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:18: #include "chrome/browser/chromeos/login/login_display_host.h" On 2013/03/07 12:17:23, bartfab wrote: > No longer needed. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:19: #include "chrome/browser/chromeos/login/login_performer.h" On 2013/03/07 12:17:23, bartfab wrote: > This is actually included by "existing_user_controller.h" and can be removed > here (not your fault but a nice clean-up). Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:21: #include "chrome/browser/chromeos/login/login_utils.h" On 2013/03/07 12:17:23, bartfab wrote: > This is actually included by "existing_user_controller.h" and can be removed > here (not your fault but a nice clean-up). Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:61: using ::testing::ReturnRef; On 2013/03/07 12:17:23, bartfab wrote: > This is not used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:196: // |mock_login_display_| is owned by the ExistingUserController, which calls On 2013/03/07 12:17:23, bartfab wrote: > Move this comment to the place where |mock_login_display_| is being declared > (MockLoginDisplay* mock_login_display_;). Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:247: std::string auto_login_username() { On 2013/03/07 12:17:23, bartfab wrote: > Nit: const > Nit: Return const std::string& Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:254: int auto_login_delay() { On 2013/03/07 12:17:23, bartfab wrote: > Nit: const Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:261: bool is_login_in_progress() { On 2013/03/07 12:17:23, bartfab wrote: > Nit: const Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:399: return expected == *content::Details<const std::string>(arg).ptr(); On 2013/03/07 12:17:23, bartfab wrote: > #include "content/public/browser/notification_details.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:412: if (!chromeos::UserManager::Get()->IsKnownUser(kAutoLoginUsername)) { On 2013/03/07 12:17:23, bartfab wrote: > #include "chrome/browser/chromeos/login/user_manager.h" fot this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:422: content::MessageLoopRunner* runner = new content::MessageLoopRunner; On 2013/03/07 12:17:23, bartfab wrote: > #include "content/public/test/test_utils.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:425: GetBrokerForAccount(kAutoLoginUsername)->core()->store(); On 2013/03/07 12:17:23, bartfab wrote: > #include "chrome/browser/policy/cloud_policy_core.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:441: base::FilePath owner_key_file = temp_dir_.path().AppendASCII("owner.key"); On 2013/03/07 12:17:23, bartfab wrote: > #include "base/file_path.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:442: std::vector<uint8> owner_key_bits; On 2013/03/07 12:17:23, bartfab wrote: > #include <vector> for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:443: ASSERT_TRUE(device_policy_.signing_key()->ExportPublicKey(&owner_key_bits)); On 2013/03/07 12:17:23, bartfab wrote: > #include "crypto/rsa_private_key.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:445: file_util::WriteFile( On 2013/03/07 12:17:23, bartfab wrote: > #include "base/file_util.h" Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:471: virtual void SetUpLoginDisplay() { On 2013/03/07 12:17:23, bartfab wrote: > OVERRIDE (clang should have complained) Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:484: void DisableAutoLoginPolicyNotifications() { On 2013/03/07 12:17:23, bartfab wrote: > This does nto seem to be used anywhere. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:486: kAccountsPrefDeviceLocalAccountAutoLoginId, On 2013/03/07 12:17:23, bartfab wrote: > #include "chrome/browser/chromeos/settings/cros_settings_names.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:495: scoped_refptr<Authenticator> create_authenticator( On 2013/03/07 12:17:23, bartfab wrote: > #include "base/memory/ref_counted.h" and > "chrome/browser/chromeos/login/authenticator" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:506: &base::Callback<void(void)>::Run)); On 2013/03/07 12:17:23, bartfab wrote: > #include "base/callback.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:605: base::ScopedTempDir temp_dir_; On 2013/03/07 12:17:23, bartfab wrote: > #include "base/files/scoped_temp_dir.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:636: IN_PROC_BROWSER_TEST_F(ExistingUserControllerPublicSessionTest, On 2013/03/07 12:17:23, bartfab wrote: > I wonder if this test provides any additional coverage over AutoLoginNoDelay. > The latter seems to be a strict superset of this test. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:674: // Wait for the timer to fire. On 2013/03/07 12:17:23, bartfab wrote: > So this test will *always* incur a 10ms wait, even if the timer works (and fires > after half the time). We can do better than that: > > * Wait for kAutoLoginShortDelay + a small offset > * Reduce kAutoLoginShortDelay further. Would 1ms work? Both done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:677: timer.Start(FROM_HERE, On 2013/03/07 12:17:23, bartfab wrote: > #include "base/location.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:693: set_auto_login_username(kAutoLoginUsername); On 2013/03/07 12:17:23, bartfab wrote: > Could you go through policy here instead? Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:720: ScopedMockUserManagerEnabler mock_user_manager; On 2013/03/07 12:17:23, bartfab wrote: > Why is this required for guest mode login? It was old. Removed. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:725: set_auto_login_username(kAutoLoginUsername); On 2013/03/07 12:17:23, bartfab wrote: > Could you go through policy here instead? Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:753: set_auto_login_username(kAutoLoginUsername); On 2013/03/07 12:17:23, bartfab wrote: > Could you go through policy here instead? Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/mock_login_display.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.cc:9: MockLoginDisplay::MockLoginDisplay() : LoginDisplay(NULL, gfx::Rect()) { On 2013/03/07 12:17:23, bartfab wrote: > #include "ui/gfx/rect.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/mock_login_display.h (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.h:8: #include "chrome/browser/chromeos/login/help_app_launcher.h" On 2013/03/07 12:17:23, bartfab wrote: > What is this needed for? > > If it is required to make the class fully defined and compilable, it should be > included by the .cc file IMHO. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.h:10: #include "chrome/browser/chromeos/login/user.h" On 2013/03/07 12:17:23, bartfab wrote: > What is this needed for? > > If it is required to make the class fully defined and compilable, it should be > included by the .cc file IMHO. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.h:11: #include "chrome/test/base/ui_test_utils.h" On 2013/03/07 12:17:23, bartfab wrote: > What is this needed for? > > If it is required to make the class fully defined and compilable, it should be > included by the .cc file IMHO. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display.h:37: DISALLOW_COPY_AND_ASSIGN(MockLoginDisplay); On 2013/03/07 12:17:23, bartfab wrote: > #include "base/basictypes.h" for this Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/mock_login_display_host.h (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/mock_login_display_host.h:10: #include "chrome/test/base/ui_test_utils.h" On 2013/03/07 12:17:23, bartfab wrote: > What is this needed for? > > If it is required to make the class fully defined and compilable, it should be > included by the .cc file IMHO. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... File chrome/browser/policy/device_local_account_browsertest.cc (right): https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:10: #include "base/callback.h" On 2013/03/07 12:17:23, bartfab wrote: > You seem to have missed my comment: This is no longer used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:48: #include "content/public/browser/notification_observer.h" On 2013/03/07 12:17:23, bartfab wrote: > You seem to have missed my comment: This is no longer used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:49: #include "content/public/browser/notification_registrar.h" On 2013/03/07 12:17:23, bartfab wrote: > You seem to have missed my comment: This is no longer used. Done. https://codereview.chromium.org/12218078/diff/77001/chrome/browser/policy/dev... chrome/browser/policy/device_local_account_browsertest.cc:50: #include "content/public/browser/notification_service.h" On 2013/03/07 12:17:23, bartfab wrote: > You seem to have missed my comment: This is no longer used. Done. https://codereview.chromium.org/12218078/diff/77001/content/public/test/notif... File content/public/test/notification_watcher.h (right): https://codereview.chromium.org/12218078/diff/77001/content/public/test/notif... content/public/test/notification_watcher.h:5: #ifndef CONTENT_PUBLIC_BROWSER_NOTIFICATION_WATCHER_H_ On 2013/03/07 12:17:23, bartfab wrote: > This no longer matches the file path. Done.
Hi sky, jam, satorux: I need OWNERS approval for this CL in a few places. Would you guys mind checking out: sky: content/public/test/notification_watcher.* jam: content/content_tests.gypi satorux: chromeos/chromeos.gyp, chromeos/dbus/fake_session_manager_client.* Or suggest someone else :) Thanks!
https://codereview.chromium.org/12218078/diff/104001/content/public/test/noti... File content/public/test/notification_watcher.cc (right): https://codereview.chromium.org/12218078/diff/104001/content/public/test/noti... content/public/test/notification_watcher.cc:23: void NotificationWatcher::Run() { This class is effectively the same thing as WindowedNotificationObserver. If you need additional logic it should added there rather than a duplicate class.
Funny you mention that. I emailed jam (also on this thread) about that and he suggested leaving it separate. On Thursday, March 7, 2013, wrote: > > https://codereview.chromium.**org/12218078/diff/104001/** > content/public/test/**notification_watcher.cc<https://codereview.chromium.org/12218078/diff/104001/content/public/test/notification_watcher.cc> > File content/public/test/**notification_watcher.cc (right): > > https://codereview.chromium.**org/12218078/diff/104001/** > content/public/test/**notification_watcher.cc#**newcode23<https://codereview.chromium.org/12218078/diff/104001/content/public/test/notification_watcher.cc#newcode23> > content/public/test/**notification_watcher.cc:23: void > NotificationWatcher::Run() { > This class is effectively the same thing as > WindowedNotificationObserver. If you need additional logic it should > added there rather than a duplicate class. > > https://codereview.chromium.**org/12218078/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12218078/diff/104001/content/public/test/noti... File content/public/test/notification_watcher.h (right): https://codereview.chromium.org/12218078/diff/104001/content/public/test/noti... content/public/test/notification_watcher.h:32: void Run(); This needs better documentation. I would never expect that if the callback returns true it doesn't wait for the notification. In fact this class doesn't seem particularly general and I would be inclined to keep it in your test. Are there many users of it? I also think the bool return value is ambiguous and an enum would be much clearer. https://codereview.chromium.org/12218078/diff/104001/content/public/test/noti... content/public/test/notification_watcher.h:40: int type_; const
On 2013/03/07 22:39:41, dconnelly_google.com wrote: > Funny you mention that. I emailed jam (also on this thread) about that and > he suggested leaving it separate. I think there was miscommunication: I meant keep it within the test class that needs to use this, since it seems specific and not general. definitely don't put it in content/public/test. I try to keep that minimal so that it's easy to find the common stuff. this seems like such a small class that i'd personally rather duplicate it in the two places than add it to chrome/test
+1 On Thu, Mar 7, 2013 at 6:47 PM, <jam@chromium.org> wrote: > On 2013/03/07 22:39:41, dconnelly_google.com wrote: > >> Funny you mention that. I emailed jam (also on this thread) about that >> and >> he suggested leaving it separate. >> > > I think there was miscommunication: I meant keep it within the test class > that > needs to use this, since it seems specific and not general. definitely > don't put > it in content/public/test. I try to keep that minimal so that it's easy to > find > the common stuff. > > this seems like such a small class that i'd personally rather duplicate it > in > the two places than add it to chrome/test > > https://codereview.chromium.**org/12218078/<https://codereview.chromium.org/1... >
Okay, got it. Sorry about that. Fixing. On Friday, March 8, 2013, Scott Violet wrote: > +1 > > > On Thu, Mar 7, 2013 at 6:47 PM, <jam@chromium.org> wrote: > >> On 2013/03/07 22:39:41, dconnelly_google.com wrote: >> >>> Funny you mention that. I emailed jam (also on this thread) about that >>> and >>> he suggested leaving it separate. >>> >> >> I think there was miscommunication: I meant keep it within the test class >> that >> needs to use this, since it seems specific and not general. definitely >> don't put >> it in content/public/test. I try to keep that minimal so that it's easy >> to find >> the common stuff. >> >> this seems like such a small class that i'd personally rather duplicate >> it in >> the two places than add it to chrome/test >> >> https://codereview.chromium.**org/12218078/<https://codereview.chromium.org/1... >> > >
Okay sky, jam: NotificationWatcher is removed from content/ and just replicated in the tests. Sorry for the trouble. I'll remove you guys from the reviewers line.
On 2013/03/08 17:57:33, dconnelly wrote: > Okay sky, jam: NotificationWatcher is removed from content/ and just replicated > in the tests. Sorry for the trouble. I'll remove you guys from the reviewers > line. no trouble at all :)
Hi satorux, I need OWNERS approval for this CL (chromeos/). Can you take a look when you have a chance? Thanks!
chromeos/... LGTM
Mostly good, a couple remaining comments. https://codereview.chromium.org/12218078/diff/88002/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/88002/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:929: public_session_auto_login_username_ = ""; nit: could also do public_session_auto_login_username_.clear() if you find that's clearer (no pun intended ;)) https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:87: const StorePolicyCallback& callback) { nit: indentation https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:94: const StorePolicyCallback& callback) { nit: indentation https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:97: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); This shouldn't send PropertyChangeComplete, that signal is only generated for device policy. https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:106: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); ditto https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:116: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); Not sure this is a good idea for existing tests, in which we might rely on this is not triggered immediately? https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:125: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); only for device policy again https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:140: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); ditto
https://codereview.chromium.org/12218078/diff/88002/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/12218078/diff/88002/chrome/browser/chromeos/l... chrome/browser/chromeos/login/existing_user_controller.cc:929: public_session_auto_login_username_ = ""; On 2013/03/11 16:26:59, Mattias Nissler wrote: > nit: could also do public_session_auto_login_username_.clear() if you find > that's clearer (no pun intended ;)) Done. https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:87: const StorePolicyCallback& callback) { On 2013/03/11 16:26:59, Mattias Nissler wrote: > nit: indentation Done. https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:94: const StorePolicyCallback& callback) { On 2013/03/11 16:26:59, Mattias Nissler wrote: > nit: indentation Done. https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:97: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); On 2013/03/11 16:26:59, Mattias Nissler wrote: > This shouldn't send PropertyChangeComplete, that signal is only generated for > device policy. Done. https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:106: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); On 2013/03/11 16:26:59, Mattias Nissler wrote: > ditto Done. https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:116: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); On 2013/03/11 16:26:59, Mattias Nissler wrote: > Not sure this is a good idea for existing tests, in which we might rely on this > is not triggered immediately? Done. https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:125: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); On 2013/03/11 16:26:59, Mattias Nissler wrote: > only for device policy again Done. https://codereview.chromium.org/12218078/diff/88002/chromeos/dbus/fake_sessio... chromeos/dbus/fake_session_manager_client.cc:140: FOR_EACH_OBSERVER(Observer, observers_, PropertyChangeComplete(true)); On 2013/03/11 16:26:59, Mattias Nissler wrote: > ditto Done.
LGTM with nits and pending trybot happiness. https://codereview.chromium.org/12218078/diff/115001/chrome/browser/policy/pr... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12218078/diff/115001/chrome/browser/policy/pr... chrome/browser/policy/proto/chrome_device_policy.proto:263: message DeviceLocalAccountAutoLoginProto { This no longer used, please remove. https://codereview.chromium.org/12218078/diff/115001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/12218078/diff/115001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:1317: 'browser/policy/mock_cloud_policy_store.h', Stuff that's needed both in unit_tests and browser_tests should be in the test_support_common target in chrome_tests_unit.gypi.
https://codereview.chromium.org/12218078/diff/115001/chrome/browser/policy/pr... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12218078/diff/115001/chrome/browser/policy/pr... chrome/browser/policy/proto/chrome_device_policy.proto:263: message DeviceLocalAccountAutoLoginProto { On 2013/03/12 10:08:01, Mattias Nissler wrote: > This no longer used, please remove. Ugh, came back in during rebase :( Done. https://codereview.chromium.org/12218078/diff/115001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/12218078/diff/115001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:1317: 'browser/policy/mock_cloud_policy_store.h', On 2013/03/12 10:08:01, Mattias Nissler wrote: > Stuff that's needed both in unit_tests and browser_tests should be in the > test_support_common target in chrome_tests_unit.gypi. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12218078/119001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12218078/134001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12218078/156001
Failed to apply patch for chrome/app/policy/policy_templates.json:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/app/policy/policy_templates.json
Hunk #1 FAILED at 112.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej
Patch: chrome/app/policy/policy_templates.json
Index: chrome/app/policy/policy_templates.json
diff --git a/chrome/app/policy/policy_templates.json
b/chrome/app/policy/policy_templates.json
index
a38c3a734d1b96144a7e0a8348baa1c1b8399657..187d7848e7e1c6200a7316f0bf35f22077233526
100644
--- a/chrome/app/policy/policy_templates.json
+++ b/chrome/app/policy/policy_templates.json
@@ -112,7 +112,7 @@
# persistent IDs for all fields (but not for groups!) are needed. These are
# specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs,
# because doing so would break the deployed wire format!
-# For your editing convenience: highest ID currently used: 190
+# For your editing convenience: highest ID currently used: 192
#
# Placeholders:
# The following placeholder strings are automatically substituted:
@@ -3543,6 +3543,46 @@
Every list entry specifies an identifier, which is used internally to
tell the different device-local accounts apart.''',
},
{
+ 'name': 'DeviceLocalAccountAutoLoginId',
+ 'type': 'string',
+ 'schema': { 'type': 'string' },
+ 'supported_on': ['chrome_os:26-'],
+ 'device_only': True,
+ 'features': {
+ 'dynamic_refresh': True,
+ },
+ 'example_value': "public@example.com",
+ 'id': 191,
+ 'caption': '''Public session for auto-login''',
+ 'desc': '''A public session to auto-login after a delay.
+
+ If this policy is set, the specified session will be automatically logged
in after a period of time has elapsed at the login screen without user
interaction. The public session must already be configured (see
|DeviceLocalAccounts|).
+
+ If this policy is unset, there will be no auto-login.''',
+ },
+ {
+ 'name': 'DeviceLocalAccountAutoLoginDelay',
+ 'type': 'int',
+ 'schema': { 'type': 'integer' },
+ 'supported_on': ['chrome_os:26-'],
+ 'device_only': True,
+ 'features': {
+ 'dynamic_refresh': True,
+ },
+ 'example_value': 180000,
+ 'id': 192,
+ 'caption': '''Public session auto-login timer''',
+ 'desc': '''The public session auto-login delay.
+
+ If the |DeviceLocalAccountAutoLoginId| policy is unset, this policy has
no effect. Otherwise:
+
+ If this policy is set, it determines the amount of time without user
activity that should elapse before automatically logging into the public session
specified by the |DeviceLocalAccountAutoLoginId| policy.
+
+ If this policy is unset, 0 milliseconds will be used as the timeout.
+
+ This policy is specified in milliseconds.'''
+ },
+ {
'name': 'BackgroundModeEnabled',
'type': 'main',
'schema': { 'type': 'boolean' },
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12218078/162001
Retried try job too often on linux_chromeos for step(s) ash_unittests, aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, check_deps, chromeos_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, dbus_unittests, device_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, net_unittests, ppapi_unittests, printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12218078/164005
Message was sent while issue was closed.
Change committed as 188331 |
