Do not reload account picker when device-local account policy changes

Do not reload account picker when device-local account policy changes

Previously, the entire account picker would reload and lose state
whenever a device-local account's policy changed. With this CL, only the
contents of the pod affected by the policy change is updated, no reload
is done and no state is lost.

BUG=398511
TEST=New browser test

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287426

[bartfab (slow), 2014-07-29 19:23:57]

Hi Nikita,

Could you review this CL? I would have sent it to Pavel but I know he is sick
and I need this for M38.

Hi Philipp,

Could you review device_local_account_browsertest.cc?

[Nikita (slow), 2014-07-30 09:27:58]

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/user_selection_screen.h:16: #include
"chrome/browser/chromeos/policy/device_local_account_policy_service.h"
Would it be possible to encapsulate chrome/browser/chromeos/policy dependency
somehow and move it out of this class as much as possible so that it would be
trivial to extract UserSelectionScreen out of src/chrome even if Policy code
stays in src/chrome for now?

This is needed for login code refactoring for Athena. End goal is to extract as
much as possible while extracted components are not allowed to depend on
src/chrome.

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/user_manager_impl.cc (left):

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/user_manager_impl.cc:885:
NotifyUserListChanged();
Please rebase with https://codereview.chromium.org/417623002/

[pneubeck (no reviews), 2014-07-31 12:25:43]

lgtm

[Nikita (slow), 2014-08-01 12:20:22]

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/user_manager_impl.cc (left):

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/user_manager_impl.cc:885:
NotifyUserListChanged();
On 2014/07/30 09:27:58, Nikita Kostylev wrote:
> Please rebase with https://codereview.chromium.org/417623002/

My CL got reverted, I've found few issues with it, fixed.
Most notably I've postponed RetrieveTrustedDevicePolicies() call from ctor.

I'm planning to put it into CQ today.

[bartfab (slow), 2014-08-04 13:46:49]

Hi Nikita,

Please take another look.

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/user_selection_screen.h:16: #include
"chrome/browser/chromeos/policy/device_local_account_policy_service.h"
On 2014/07/30 09:27:58, Nikita Kostylev wrote:
> Would it be possible to encapsulate chrome/browser/chromeos/policy dependency
> somehow and move it out of this class as much as possible so that it would be
> trivial to extract UserSelectionScreen out of src/chrome even if Policy code
> stays in src/chrome for now?
> 
> This is needed for login code refactoring for Athena. End goal is to extract
as
> much as possible while extracted components are not allowed to depend on
> src/chrome.

I moved all policy-related code to a ChromeUserSelectionScreen subclass. The
base UserSelectionScreen is not getting any new dependencies anymore.

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/user_manager_impl.cc (left):

https://codereview.chromium.org/427053002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/user_manager_impl.cc:885:
NotifyUserListChanged();
On 2014/08/01 12:20:22, Nikita Kostylev wrote:
> On 2014/07/30 09:27:58, Nikita Kostylev wrote:
> > Please rebase with https://codereview.chromium.org/417623002/
> 
> My CL got reverted, I've found few issues with it, fixed.
> Most notably I've postponed RetrieveTrustedDevicePolicies() call from ctor.
> 
> I'm planning to put it into CQ today.

Done.

[bartfab (slow), 2014-08-04 17:27:14]

Hi Pavel,

As discussed, could you take a look since Nikita is OOO?

[dzhioev (left Google), 2014-08-04 17:48:38]

On 2014/08/04 17:27:14, bartfab wrote:
> Hi Pavel,
> 
> As discussed, could you take a look since Nikita is OOO?

LGTM

[bartfab (slow), 2014-08-04 17:49:54]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-04 17:51:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bartfab@chromium.org/427053002/40001

[commit-bot: I haz the power, 2014-08-05 01:00:23]

Change committed as 287426

[Nikita (slow), 2014-08-05 12:09:24]

lgtm

https://codereview.chromium.org/427053002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/427053002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:38: for
(user_manager::UserList::const_iterator it = users_.begin();
nit: You may as well add GetUsers() getter.

[bartfab (slow), 2014-08-05 18:52:03]

https://codereview.chromium.org/427053002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/427053002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:38: for
(user_manager::UserList::const_iterator it = users_.begin();
On 2014/08/05 12:09:24, Nikita Kostylev wrote:
> nit: You may as well add GetUsers() getter.

Sorry, I do not understand. Where would you like me to add this getter? If you
mean UserManager - I think passing in dependencies rather than reaching out to
singletons like UserManager is actually better.

[Nikita (slow), 2014-08-06 08:14:40]

https://codereview.chromium.org/427053002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/427053002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:38: for
(user_manager::UserList::const_iterator it = users_.begin();
On 2014/08/05 18:52:02, bartfab wrote:
> On 2014/08/05 12:09:24, Nikita Kostylev wrote:
> > nit: You may as well add GetUsers() getter.
> 
> Sorry, I do not understand. Where would you like me to add this getter? If you
> mean UserManager - I think passing in dependencies rather than reaching out to
> singletons like UserManager is actually better.

I mean instead of having users_ as a protected field in UserSelectionScreen you
may instead just expose it through the getter.

[bartfab (slow), 2014-08-07 14:14:50]

https://codereview.chromium.org/427053002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/427053002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:38: for
(user_manager::UserList::const_iterator it = users_.begin();
On 2014/08/06 08:14:40, Nikita Kostylev wrote:
> On 2014/08/05 18:52:02, bartfab wrote:
> > On 2014/08/05 12:09:24, Nikita Kostylev wrote:
> > > nit: You may as well add GetUsers() getter.
> > 
> > Sorry, I do not understand. Where would you like me to add this getter? If
you
> > mean UserManager - I think passing in dependencies rather than reaching out
to
> > singletons like UserManager is actually better.
> 
> I mean instead of having users_ as a protected field in UserSelectionScreen
you
> may instead just expose it through the getter.

Thanks, somehow I did not realize that:
a) using a getter makes sense here,
b) the getter even exists already,
and finnally,
c) this method actually knows the |users| already and does need to read them
form anywhere.

Doh. I uploaded follow-up CL 446383002 for this.