Adding infrastructure for possibility of changing manager names for the supervised accounts.

Adding infrastructure for possibility of changing manager names for the supervised accounts.

CL https://chromereviews.googleplex.com/90457013/ for pre-checks.

BUG=419007
Committed: https://crrev.com/270ffc018fa0a0cfc0be8ccb2bf4323cab1c4924
Cr-Commit-Position: refs/heads/master@{#299296}

[merkulova, 2014-10-03 09:35:25]

merkulova@chromium.org changed reviewers:
+ derat@chromium.org, jennyz@chromium.org

[merkulova, 2014-10-03 09:36:45]

merkulova@chromium.org changed reviewers:
+ treib@chromium.org

[merkulova, 2014-10-03 09:39:01]

treib@ for supervised_user_service.cc and overall review
jennyz@ for ash/system files
derat@ for chrome/browser/ui/ash files

[Daniel Erat, 2014-10-03 16:02:10]

it's not your fault, but i wish that SystemTrayDelegate weren't such a dumping
ground for everything. is the code that you're going to add later to use this
actually limited to the system tray?

https://codereview.chromium.org/627593003/diff/1/ash/system/tray/default_syst...
File ash/system/tray/default_system_tray_delegate.h (right):

https://codereview.chromium.org/627593003/diff/1/ash/system/tray/default_syst...
ash/system/tray/default_system_tray_delegate.h:93: const base::Closure&
callback) OVERRIDE;
not having any way to remove the callback seems strange. would an observer
interface make more sense for this?

[jennyz, 2014-10-03 20:28:12]

On 2014/10/03 16:02:10, Daniel Erat wrote:
> it's not your fault, but i wish that SystemTrayDelegate weren't such a dumping
> ground for everything. is the code that you're going to add later to use this
> actually limited to the system tray?
> 
>
https://codereview.chromium.org/627593003/diff/1/ash/system/tray/default_syst...
> File ash/system/tray/default_system_tray_delegate.h (right):
> 
>
https://codereview.chromium.org/627593003/diff/1/ash/system/tray/default_syst...
> ash/system/tray/default_system_tray_delegate.h:93: const base::Closure&
> callback) OVERRIDE;
> not having any way to remove the callback seems strange. would an observer
> interface make more sense for this?

+1. Agree with derat@

[Marc Treib, 2014-10-06 07:40:19]

c/b/supervised_user lgtm with one nit below.

https://codereview.chromium.org/627593003/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/627593003/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_service.cc:308: return name =
name.empty() ? GetCustodianEmailAddress() : name;
nit: Remove "name = "

[merkulova, 2014-10-06 15:37:25]

"it's not your fault, but i wish that SystemTrayDelegate weren't such a dumping
ground for everything. is the code that you're going to add later to use this
actually limited to the system tray?" -- The code actually updates system tray
view on chrome OS.
The problem is that tray items know nothing about profile (and that's ok) while
their view should be updated when user status is changed to Supervised. Only
system_tray_delegate_chromeos may observe these changes. But it need get tray
items somehow to notify them. That's why these dependencies introduced.

https://codereview.chromium.org/627593003/diff/1/ash/system/tray/default_syst...
File ash/system/tray/default_system_tray_delegate.h (right):

https://codereview.chromium.org/627593003/diff/1/ash/system/tray/default_syst...
ash/system/tray/default_system_tray_delegate.h:93: const base::Closure&
callback) OVERRIDE;
On 2014/10/03 16:02:09, Daniel Erat wrote:
> not having any way to remove the callback seems strange. would an observer
> interface make more sense for this?

Switched to observer. Is it what you mean? 
No removal possibility as weak_ptrs used.

https://codereview.chromium.org/627593003/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/627593003/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_service.cc:308: return name =
name.empty() ? GetCustodianEmailAddress() : name;
On 2014/10/06 07:40:19, Marc Treib wrote:
> nit: Remove "name = "

Done.

[Daniel Erat, 2014-10-06 15:45:58]

https://codereview.chromium.org/627593003/diff/20001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/custodian_info_tray_observer.h (right):

https://codereview.chromium.org/627593003/diff/20001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/custodian_info_tray_observer.h:8: class
CustodianInfoTrayObserver {
this should be in the ash namespace, i think

https://codereview.chromium.org/627593003/diff/20001/ash/system/tray/system_t...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/627593003/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:305:
base::WeakPtr<CustodianInfoTrayObserver> observer) {
what's the lifetime of the observer relative to this class?

i was suggesting not using weak pointers at all here. the usual pattern is that
class A outlives its observer class B. B calls A::AddObserver(this) in its c'tor
and A::RemoveObserver(this) in its d'tor.

[merkulova, 2014-10-07 11:27:38]

https://codereview.chromium.org/627593003/diff/20001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/custodian_info_tray_observer.h (right):

https://codereview.chromium.org/627593003/diff/20001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/custodian_info_tray_observer.h:8: class
CustodianInfoTrayObserver {
On 2014/10/06 15:45:58, Daniel Erat wrote:
> this should be in the ash namespace, i think

Done.

[Daniel Erat, 2014-10-07 16:16:00]

thanks! the general structure of this looks fine to me now; i just have a few
comments and questions

https://codereview.chromium.org/627593003/diff/40001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/custodian_info_tray_observer.h (right):

https://codereview.chromium.org/627593003/diff/40001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/custodian_info_tray_observer.h:14: virtual void
OnCustodianInfoChanged() {}
make this be abstract instead of having an empty implementation (since it
doesn't make sense to implement this interface without overriding this method)

https://codereview.chromium.org/627593003/diff/40001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/tray_supervised_user.h (right):

https://codereview.chromium.org/627593003/diff/40001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/tray_supervised_user.h:38: void
OnCustodianInfoChanged();
add OVERRIDE

https://codereview.chromium.org/627593003/diff/40001/ash/system/tray/system_t...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/627593003/diff/40001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:303: virtual void
AddObserver(CustodianInfoTrayObserver* observer) {
any reason these aren't abstract like all the other methods in this interface?

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:300: #if
defined(OS_CHROMEOS)
do you actually need this? GetCustodianEmailAddress() above looks like it does
the same thing -- if you delete this block, won't it just get called instead? or
is there a case where the kSupervisedUserCustodianEmail pref differs from the
email address you'd get from UserManager?

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:302: name =
base::UTF16ToUTF8(chromeos::ChromeUserManager::Get()
why does this call base::UTF16ToUTF8() while GetCustodianEmailAddress() doesn't?
what's the actual encoding that's used for this pref's value?

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1318: void
SystemTrayDelegateChromeOS::AddObserver(
please move this and RemoveObserver so they match the order in the .h file

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:135: virtual void
AddObserver(ash::CustodianInfoTrayObserver* observer) OVERRIDE;
nit: rename these to AddCustodianInfoTrayObserver and
RemoveCustodianInfoTrayObserver

[Marc Treib, 2014-10-08 07:51:19]

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:300: #if
defined(OS_CHROMEOS)
On 2014/10/07 16:16:00, Daniel Erat wrote:
> do you actually need this? GetCustodianEmailAddress() above looks like it does
> the same thing -- if you delete this block, won't it just get called instead?
or
> is there a case where the kSupervisedUserCustodianEmail pref differs from the
> email address you'd get from UserManager?

Not quite: above we call GetManagerDisplayEmail, here it's GetManagerDisplayName

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:302: name =
base::UTF16ToUTF8(chromeos::ChromeUserManager::Get()
On 2014/10/07 16:16:00, Daniel Erat wrote:
> why does this call base::UTF16ToUTF8() while GetCustodianEmailAddress()
doesn't?
> what's the actual encoding that's used for this pref's value?

GetManagerDisplayName returns a base::string16, while GetManagerDisplayEmail
returns an std::string

[merkulova, 2014-10-08 09:19:37]

https://codereview.chromium.org/627593003/diff/40001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/custodian_info_tray_observer.h (right):

https://codereview.chromium.org/627593003/diff/40001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/custodian_info_tray_observer.h:14: virtual void
OnCustodianInfoChanged() {}
On 2014/10/07 16:16:00, Daniel Erat wrote:
> make this be abstract instead of having an empty implementation (since it
> doesn't make sense to implement this interface without overriding this method)

Done.

https://codereview.chromium.org/627593003/diff/40001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/tray_supervised_user.h (right):

https://codereview.chromium.org/627593003/diff/40001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/tray_supervised_user.h:38: void
OnCustodianInfoChanged();
On 2014/10/07 16:16:00, Daniel Erat wrote:
> add OVERRIDE

Done.

https://codereview.chromium.org/627593003/diff/40001/ash/system/tray/system_t...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/627593003/diff/40001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:303: virtual void
AddObserver(CustodianInfoTrayObserver* observer) {
On 2014/10/07 16:16:00, Daniel Erat wrote:
> any reason these aren't abstract like all the other methods in this interface?

I don't want to clutter default_system_tray_delegate and
system_tray_delegate_linux with cloned empty unused methods.

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1318: void
SystemTrayDelegateChromeOS::AddObserver(
On 2014/10/07 16:16:00, Daniel Erat wrote:
> please move this and RemoveObserver so they match the order in the .h file

Done.

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/627593003/diff/40001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:135: virtual void
AddObserver(ash::CustodianInfoTrayObserver* observer) OVERRIDE;
On 2014/10/07 16:16:00, Daniel Erat wrote:
> nit: rename these to AddCustodianInfoTrayObserver and
> RemoveCustodianInfoTrayObserver

Done.

[Daniel Erat, 2014-10-08 14:10:46]

lgtm after some more comments are addressed

https://codereview.chromium.org/627593003/diff/60001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/custodian_info_tray_observer.h (right):

https://codereview.chromium.org/627593003/diff/60001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/custodian_info_tray_observer.h:10: class
CustodianInfoTrayObserver {
add a comment before this line explaining how the interface is used:

  // Used to observe SystemTrayDelegate.

https://codereview.chromium.org/627593003/diff/60001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/tray_supervised_user.cc (right):

https://codereview.chromium.org/627593003/diff/60001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/tray_supervised_user.cc:115: kNotificationId))
this line should be indented four more spaces, i believe

https://codereview.chromium.org/627593003/diff/60001/ash/system/tray/system_t...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/627593003/diff/60001/ash/system/tray/system_t...
ash/system/tray/system_tray_delegate.h:321: };
every other method in this interface is abstract, though. please make these two
abstract as well to be consistent.

https://codereview.chromium.org/627593003/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/627593003/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:304:
->GetManagerDisplayName(
thanks, i missed this difference even while looking closely. :-(

https://codereview.chromium.org/627593003/diff/60001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/627593003/diff/60001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1303: // Overridden from
SupervisedUserServiceObserver.
don't need to fix it here, but chrome typically doesn't have method comments in
.cc files -- the documentation belongs in the header instead (and it's already
also present there, too)

https://codereview.chromium.org/627593003/diff/60001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/627593003/diff/60001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:247: virtual void
OnCustodianInfoChanged() OVERRIDE;
s/OVERRIDE/override/ to match the rest of this file

[jennyz, 2014-10-08 20:57:10]

lgtm with nits.

https://codereview.chromium.org/627593003/diff/60001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/tray_supervised_user.cc (right):

https://codereview.chromium.org/627593003/diff/60001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/tray_supervised_user.cc:14: #include
"base/bind.h"
DO you need this include?

[merkulova, 2014-10-09 09:14:47]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-10-09 09:15:32]

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

[commit-bot: I haz the power, 2014-10-09 10:31:49]

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

[commit-bot: I haz the power, 2014-10-09 10:31:51]

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

[merkulova, 2014-10-09 11:39:09]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-10-09 11:40:33]

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

[commit-bot: I haz the power, 2014-10-09 11:44:16]

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

[commit-bot: I haz the power, 2014-10-09 11:44:20]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[merkulova, 2014-10-09 11:58:35]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-10-09 11:59:20]

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

[commit-bot: I haz the power, 2014-10-09 15:32:00]

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

[commit-bot: I haz the power, 2014-10-09 15:32:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[merkulova, 2014-10-10 07:27:12]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-10-10 07:28:07]

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

[commit-bot: I haz the power, 2014-10-10 10:28:21]

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

[commit-bot: I haz the power, 2014-10-10 10:28:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[merkulova, 2014-10-10 12:01:46]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-10-10 12:02:15]

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

[commit-bot: I haz the power, 2014-10-10 12:54:47]

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

[commit-bot: I haz the power, 2014-10-10 12:54:49]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[merkulova, 2014-10-13 12:35:30]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-10-13 12:36:11]

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

[commit-bot: I haz the power, 2014-10-13 13:58:14]

Committed patchset #9 (id:450001)

[commit-bot: I haz the power, 2014-10-13 13:59:06]

Patchset 9 (id:??) landed as
https://crrev.com/270ffc018fa0a0cfc0be8ccb2bf4323cab1c4924
Cr-Commit-Position: refs/heads/master@{#299296}