Add tray warning for Locally managed users.

Add tray warning for Locally managed users.

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

[Denis Kuznetsov (DE-MUC), 2013-04-29 13:21:54]

Dmitriy for all chromeos/ changes, Daniel for ash/ changes.

[Daniel Erat, 2013-04-29 13:32:57]

https://codereview.chromium.org/14507003/diff/1/ash/system/chromeos/managed/t...
File ash/system/chromeos/managed/tray_locally_managed_user.cc (right):

https://codereview.chromium.org/14507003/diff/1/ash/system/chromeos/managed/t...
ash/system/chromeos/managed/tray_locally_managed_user.cc:22: class
LocallyManagedUserDefaultView : public views::View {
this class is almost exactly the same as EnterpriseDefaultView.  can you
refactor EnterpriseDefaultView to support this use case instead of
copying-and-pasting?

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

https://codereview.chromium.org/14507003/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:565: }
nit: you can maybe make this a bit shorter by rewriting it as follows (assuming
that both clauses return the same type; ignore this suggestion if they don't):

additional->SetText(login == ash::user::LOGGED_IN_LOCALLY_MANAGED ?
    bundle.GetLocalizedString(IDS_ASH_STATUS_TRAY_LOCALLY_MANAGED_LABEL) :
    UTF8ToUTF16(delegate->GetUserEmail()));

https://codereview.chromium.org/14507003/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/14507003/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:485: if
(ash::user::LOGGED_IN_LOCALLY_MANAGED != GetUserLoginStatus())
nit: if (GetUserLoginStatus() != ash::user::...)

https://codereview.chromium.org/14507003/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:491: if
(ash::user::LOGGED_IN_LOCALLY_MANAGED != GetUserLoginStatus())
nit: if (GetUserLoginStatus() != ash::user::...)

[Denis Kuznetsov (DE-MUC), 2013-04-29 14:14:58]

https://codereview.chromium.org/14507003/diff/1/ash/system/chromeos/managed/t...
File ash/system/chromeos/managed/tray_locally_managed_user.cc (right):

https://codereview.chromium.org/14507003/diff/1/ash/system/chromeos/managed/t...
ash/system/chromeos/managed/tray_locally_managed_user.cc:22: class
LocallyManagedUserDefaultView : public views::View {
On 2013/04/29 13:32:57, Daniel Erat wrote:
> this class is almost exactly the same as EnterpriseDefaultView.  can you
> refactor EnterpriseDefaultView to support this use case instead of
> copying-and-pasting?

Done.

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

https://codereview.chromium.org/14507003/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:565: }
On 2013/04/29 13:32:57, Daniel Erat wrote:
> nit: you can maybe make this a bit shorter by rewriting it as follows
(assuming
> that both clauses return the same type; ignore this suggestion if they don't):
> 
> additional->SetText(login == ash::user::LOGGED_IN_LOCALLY_MANAGED ?
>     bundle.GetLocalizedString(IDS_ASH_STATUS_TRAY_LOCALLY_MANAGED_LABEL) :
>     UTF8ToUTF16(delegate->GetUserEmail()));

Done.

https://codereview.chromium.org/14507003/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/14507003/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:485: if
(ash::user::LOGGED_IN_LOCALLY_MANAGED != GetUserLoginStatus())
On 2013/04/29 13:32:57, Daniel Erat wrote:
> nit: if (GetUserLoginStatus() != ash::user::...)

Done.

https://codereview.chromium.org/14507003/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:491: if
(ash::user::LOGGED_IN_LOCALLY_MANAGED != GetUserLoginStatus())
On 2013/04/29 13:32:57, Daniel Erat wrote:
> nit: if (GetUserLoginStatus() != ash::user::...)

Done.

[Daniel Erat, 2013-04-29 14:34:27]

LGTM for ash

https://codereview.chromium.org/14507003/diff/5001/ash/ash_chromeos_strings.grdp
File ash/ash_chromeos_strings.grdp (right):

https://codereview.chromium.org/14507003/diff/5001/ash/ash_chromeos_strings.g...
ash/ash_chromeos_strings.grdp:237: <message
name="IDS_ASH_STATUS_TRAY_LOCALLY_MANAGED_LABEL" desc="label shown instead of
email for locally managed users">
nit: add a blank line above this one so it doesn't look like it's included in
the "Status Tray ScreenCapture strings" section

https://codereview.chromium.org/14507003/diff/5001/ash/system/chromeos/label_...
File ash/system/chromeos/label_tray_view.cc (right):

https://codereview.chromium.org/14507003/diff/5001/ash/system/chromeos/label_...
ash/system/chromeos/label_tray_view.cc:48:
rb.GetImageSkiaNamed(icon_resource_id_);
nit: i don't think this line needs to be wrapped

https://codereview.chromium.org/14507003/diff/5001/ash/system/chromeos/label_...
File ash/system/chromeos/label_tray_view.h (right):

https://codereview.chromium.org/14507003/diff/5001/ash/system/chromeos/label_...
ash/system/chromeos/label_tray_view.h:18: 
nit: delete extra blank line

https://codereview.chromium.org/14507003/diff/5001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/14507003/diff/5001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:561:
bundle.GetLocalizedString(IDS_ASH_STATUS_TRAY_LOCALLY_MANAGED_LABEL):
nit: add a space before the ':'

[Denis Kuznetsov (DE-MUC), 2013-04-30 10:29:28]

https://chromiumcodereview.appspot.com/14507003/diff/5001/ash/ash_chromeos_st...
File ash/ash_chromeos_strings.grdp (right):

https://chromiumcodereview.appspot.com/14507003/diff/5001/ash/ash_chromeos_st...
ash/ash_chromeos_strings.grdp:237: <message
name="IDS_ASH_STATUS_TRAY_LOCALLY_MANAGED_LABEL" desc="label shown instead of
email for locally managed users">
On 2013/04/29 14:34:27, Daniel Erat wrote:
> nit: add a blank line above this one so it doesn't look like it's included in
> the "Status Tray ScreenCapture strings" section

Done.

https://chromiumcodereview.appspot.com/14507003/diff/5001/ash/system/chromeos...
File ash/system/chromeos/label_tray_view.cc (right):

https://chromiumcodereview.appspot.com/14507003/diff/5001/ash/system/chromeos...
ash/system/chromeos/label_tray_view.cc:48:
rb.GetImageSkiaNamed(icon_resource_id_);
On 2013/04/29 14:34:27, Daniel Erat wrote:
> nit: i don't think this line needs to be wrapped

Done.

https://chromiumcodereview.appspot.com/14507003/diff/5001/ash/system/chromeos...
File ash/system/chromeos/label_tray_view.h (right):

https://chromiumcodereview.appspot.com/14507003/diff/5001/ash/system/chromeos...
ash/system/chromeos/label_tray_view.h:18: 
On 2013/04/29 14:34:27, Daniel Erat wrote:
> nit: delete extra blank line

Done.

https://chromiumcodereview.appspot.com/14507003/diff/5001/ash/system/user/tra...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/14507003/diff/5001/ash/system/user/tra...
ash/system/user/tray_user.cc:561:
bundle.GetLocalizedString(IDS_ASH_STATUS_TRAY_LOCALLY_MANAGED_LABEL):
On 2013/04/29 14:34:27, Daniel Erat wrote:
> nit: add a space before the ':'

Done.

[Dmitry Polukhin, 2013-04-30 11:04:41]

LGTM

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/ash_chromeos_s...
File ash/ash_chromeos_strings.grdp (right):

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/ash_chromeos_s...
ash/ash_chromeos_strings.grdp:239: Locally managed user
In mock I see that the string is "Limited user". Have you confirmed the string
with Min?

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/system/chromeo...
File ash/system/chromeos/enterprise/tray_enterprise.h (right):

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/system/chromeo...
ash/system/chromeos/enterprise/tray_enterprise.h:43: LabelTrayView*
default_view_;
It is not default_view_ anymore, please call it something like "label_view_".

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/system/chromeo...
File ash/system/chromeos/managed/tray_locally_managed_user.h (right):

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/system/chromeo...
ash/system/chromeos/managed/tray_locally_managed_user.h:38: LabelTrayView*
default_view_;
Ditto.

https://chromiumcodereview.appspot.com/14507003/diff/13001/chrome/browser/chr...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/14507003/diff/13001/chrome/browser/chr...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:570: // TODO(antrim):
find out what should we show in this case.
Please file bug for this TODO and link it here.

[Denis Kuznetsov (DE-MUC), 2013-04-30 11:32:52]

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/system/chromeo...
File ash/system/chromeos/enterprise/tray_enterprise.h (right):

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/system/chromeo...
ash/system/chromeos/enterprise/tray_enterprise.h:43: LabelTrayView*
default_view_;
On 2013/04/30 11:04:41, Dmitry Polukhin wrote:
> It is not default_view_ anymore, please call it something like "label_view_".

Done.

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/system/chromeo...
File ash/system/chromeos/managed/tray_locally_managed_user.h (right):

https://chromiumcodereview.appspot.com/14507003/diff/13001/ash/system/chromeo...
ash/system/chromeos/managed/tray_locally_managed_user.h:38: LabelTrayView*
default_view_;
On 2013/04/30 11:04:41, Dmitry Polukhin wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/14507003/diff/13001/chrome/browser/chr...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/14507003/diff/13001/chrome/browser/chr...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:570: // TODO(antrim):
find out what should we show in this case.
On 2013/04/30 11:04:41, Dmitry Polukhin wrote:
> Please file bug for this TODO and link it here.

Done.

[commit-bot: I haz the power, 2013-04-30 11:33:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/14507003/14020

[commit-bot: I haz the power, 2013-04-30 12:48:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/14507003/22003

[commit-bot: I haz the power, 2013-04-30 13:38:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-30 13:51:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/14507003/22003

[commit-bot: I haz the power, 2013-04-30 15:20:32]

Change committed as 197372