Fixed suggesting adding more users into multi-profile session when we don't have more

Fixed suggesting adding more users into multi-profile session when we don't have more

BUG=397105
Committed: https://crrev.com/23c20f9ed8bb21d9a14540dabbe986cdebc5e241
Cr-Commit-Position: refs/heads/master@{#296383}

[Roman Sorokin (ftl), 2014-09-10 15:03:14]

rsorokin@chromium.org changed reviewers:
+ dzhioev@chromium.org

[Roman Sorokin (ftl), 2014-09-10 15:03:35]

dzhioev@, please review

[dzhioev (left Google), 2014-09-18 07:14:23]

I'm sorry for a long delay. You need to add ash owners to this review.

https://codereview.chromium.org/560033002/diff/1/ash/session/session_state_de...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/560033002/diff/1/ash/session/session_state_de...
ash/session/session_state_delegate.h:86: virtual int
GetNumberOfAdmittedForMultiProfileUsers() const = 0;
I think "GetNumberOfUsersAdmittedForMultiProfile" sounds better.

https://codereview.chromium.org/560033002/diff/1/ash/system/user/user_view.cc
File ash/system/user/user_view.cc (right):

https://codereview.chromium.org/560033002/diff/1/ash/system/user/user_view.cc...
ash/system/user/user_view.cc:505: admitted_users_to_be_added <= 0 ||
Checking for "<= 0" doesn't make sense to me. Can we just check that it is not
0?

https://codereview.chromium.org/560033002/diff/1/chrome/browser/ui/ash/sessio...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/560033002/diff/1/chrome/browser/ui/ash/sessio...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:90: .size();
Such formatting looks strange. Can you please appply 'git cl format'.

[Roman Sorokin (ftl), 2014-09-22 08:36:05]

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

[Roman Sorokin (ftl), 2014-09-22 08:36:06]

+jamescook@

dzhioev@, jamescook@ please review

https://codereview.chromium.org/560033002/diff/1/ash/session/session_state_de...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/560033002/diff/1/ash/session/session_state_de...
ash/session/session_state_delegate.h:86: virtual int
GetNumberOfAdmittedForMultiProfileUsers() const = 0;
On 2014/09/18 07:14:22, dzhioev wrote:
> I think "GetNumberOfUsersAdmittedForMultiProfile" sounds better.

Done.

https://codereview.chromium.org/560033002/diff/1/ash/system/user/user_view.cc
File ash/system/user/user_view.cc (right):

https://codereview.chromium.org/560033002/diff/1/ash/system/user/user_view.cc...
ash/system/user/user_view.cc:505: admitted_users_to_be_added <= 0 ||
On 2014/09/18 07:14:22, dzhioev wrote:
> Checking for "<= 0" doesn't make sense to me. Can we just check that it is not
> 0?

Done.

https://codereview.chromium.org/560033002/diff/1/chrome/browser/ui/ash/sessio...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/560033002/diff/1/chrome/browser/ui/ash/sessio...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:90: .size();
'git cl format' have done that.
On 2014/09/18 07:14:22, dzhioev wrote:
> Such formatting looks strange. Can you please appply 'git cl format'.

[James Cook, 2014-09-22 19:52:23]

Please also add a test for the "can add user" logic.

https://codereview.chromium.org/560033002/diff/40001/ash/system/user/user_vie...
File ash/system/user/user_view.cc (right):

https://codereview.chromium.org/560033002/diff/40001/ash/system/user/user_vie...
ash/system/user/user_view.cc:505: add_user_disabled_ = !multi_profile_allowed ||
Please change this to add_user_enabled_ and invert the initialization and all
the other conditionals. I know you didn't add this member, but I strongly prefer
positive-sense booleans.

https://codereview.chromium.org/560033002/diff/40001/ash/system/user/user_vie...
ash/system/user/user_view.cc:508: delegate->GetMaximumNumberOfLoggedInUsers());
Given that all the data to decide whether or not a user should be added lives in
the delegate, can you replace GetNumberOfUsersAdmittedForMultiProfile() with
something like bool CanAddUserToMultiProfile(int* error_message_id)?

The decision whether adding is allowed or not is more of a model/controller
decision than a UI view decision.

Moving the logic will also make it easier for you to add a test, as there are
existing tests of SessionStateDelegate but none for this class.

https://codereview.chromium.org/560033002/diff/40001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/560033002/diff/40001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:89:
->GetUsersAdmittedForMultiProfile()
This should be named something like GetUsersAllowedForMultiProfile() or
GetUsersPermittedForMultiProfile().  "Admitted" sounds like they have already
logged in.

[Roman Sorokin (ftl), 2014-09-23 15:42:37]

Please review

https://codereview.chromium.org/560033002/diff/40001/ash/system/user/user_vie...
File ash/system/user/user_view.cc (right):

https://codereview.chromium.org/560033002/diff/40001/ash/system/user/user_vie...
ash/system/user/user_view.cc:505: add_user_disabled_ = !multi_profile_allowed ||
On 2014/09/22 19:52:23, James Cook wrote:
> Please change this to add_user_enabled_ and invert the initialization and all
> the other conditionals. I know you didn't add this member, but I strongly
prefer
> positive-sense booleans.

Done.

https://codereview.chromium.org/560033002/diff/40001/ash/system/user/user_vie...
ash/system/user/user_view.cc:508: delegate->GetMaximumNumberOfLoggedInUsers());
On 2014/09/22 19:52:23, James Cook wrote:
> Given that all the data to decide whether or not a user should be added lives
in
> the delegate, can you replace GetNumberOfUsersAdmittedForMultiProfile() with
> something like bool CanAddUserToMultiProfile(int* error_message_id)?
> 
> The decision whether adding is allowed or not is more of a model/controller
> decision than a UI view decision.
> 
> Moving the logic will also make it easier for you to add a test, as there are
> existing tests of SessionStateDelegate but none for this class.

Done.

https://codereview.chromium.org/560033002/diff/40001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/560033002/diff/40001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:89:
->GetUsersAdmittedForMultiProfile()
On 2014/09/22 19:52:23, James Cook wrote:
> This should be named something like GetUsersAllowedForMultiProfile() or
> GetUsersPermittedForMultiProfile().  "Admitted" sounds like they have already
> logged in.

You mean renaming UserManager::GetUsersAdmittedForMultiProfile?
This function is from different CL.

[James Cook, 2014-09-23 16:02:13]

LGTM with nits.  I still want the rename done.

https://codereview.chromium.org/560033002/diff/60001/ash/session/session_stat...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/560033002/diff/60001/ash/session/session_stat...
ash/session/session_state_delegate.h:52: enum MultiProfileAddUserError {
This is fine, but I would also be OK with a shorter name, like AddUserError and
ADD_USER_ERROR_*

https://codereview.chromium.org/560033002/diff/60001/ash/session/session_stat...
ash/session/session_state_delegate.h:55:
MULTIPROFILE_ADD_USER_ERROR_MAXIMUM_OF_USERS_REACHED,
nit: I would say _MAXIMUM_USERS_REACHED

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:89:
->GetUsersAdmittedForMultiProfile()
I know this was not added in this CL, but the name is confusing and it needs to
be renamed. If you want to do it in another CL that's OK, but please include me
on the review.

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc (right):

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc:254: // Make
sure adding users to multiprofiles disabled by logging all possible
nit: "logging" -> "logging in"

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc:291: }
Nice test suite!

[Roman Sorokin (ftl), 2014-09-24 08:30:47]

https://codereview.chromium.org/560033002/diff/60001/ash/session/session_stat...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/560033002/diff/60001/ash/session/session_stat...
ash/session/session_state_delegate.h:52: enum MultiProfileAddUserError {
On 2014/09/23 16:02:12, James Cook OOO until 9-29 wrote:
> This is fine, but I would also be OK with a shorter name, like AddUserError
and
> ADD_USER_ERROR_*

Done.

https://codereview.chromium.org/560033002/diff/60001/ash/session/session_stat...
ash/session/session_state_delegate.h:55:
MULTIPROFILE_ADD_USER_ERROR_MAXIMUM_OF_USERS_REACHED,
On 2014/09/23 16:02:12, James Cook OOO until 9-29 wrote:
> nit: I would say _MAXIMUM_USERS_REACHED

Done.

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:89:
->GetUsersAdmittedForMultiProfile()
Ok, I'll do it in another CL.
On 2014/09/23 16:02:12, James Cook OOO until 9-29 wrote:
> I know this was not added in this CL, but the name is confusing and it needs
to
> be renamed. If you want to do it in another CL that's OK, but please include
me
> on the review.

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc (right):

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc:254: // Make
sure adding users to multiprofiles disabled by logging all possible
On 2014/09/23 16:02:12, James Cook OOO until 9-29 wrote:
> nit: "logging" -> "logging in"

Done.

https://codereview.chromium.org/560033002/diff/60001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc:291: }
On 2014/09/23 16:02:12, James Cook OOO until 9-29 wrote:
> Nice test suite!

Acknowledged.

[Roman Sorokin (ftl), 2014-09-24 08:30:53]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-09-24 08:30:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/560033002/80001

[commit-bot: I haz the power, 2014-09-24 08:59:07]

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

[commit-bot: I haz the power, 2014-09-24 08:59:08]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Roman Sorokin (ftl), 2014-09-24 10:00:43]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-09-24 10:01:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/560033002/100001

[commit-bot: I haz the power, 2014-09-24 10:37:35]

Committed patchset #6 (id:100001) as cdce39e3d121fdf8a37cb1c8a01ad8162266accd

[commit-bot: I haz the power, 2014-09-24 10:38:19]

Patchset 6 (id:??) landed as
https://crrev.com/23c20f9ed8bb21d9a14540dabbe986cdebc5e241
Cr-Commit-Position: refs/heads/master@{#296383}

[Roman Sorokin (ftl), 2014-09-24 11:13:21]

Pavel, please take a look