[cros] Display emails of users are stored in a separate dictionary in Local State.

[cros] Display emails of users are stored in a separate dictionary in Local State.


*) In Javascript, 'username' means canonical email and 'email' means displayed email.
*) In C++, both 'email' and 'username' refer to the canonical email (because it is
   used in all cases except passing to JS); 'display email' refers to the raw email
   as entered by user.

BUG=chromium-os:11328
TEST=Manual: after adding a user with dot in the email, dot should be preserved on the login screen, in Preferences, in the 'report bug' page, in the whitelist.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113207

[Ivan Korotkov, 2011-12-02 18:04:00]

PTAL

jhawkins: options/ Javascript
nkostylev/whywhat: UserManager-related stuff

[James Hawkins, 2011-12-02 18:12:01]

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_options.js (right):

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_options.js:6: 
Remove blank line while you're here.

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_options.js:43:
this.allowWhitelist_ = AccountsOptions.currentUserIsOwner();
"Forward declare" and document this variable.

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_options.js:115:
console.log('Chrome -> whitelist "' + e.user.name + '" ' + e.user.email);
Remove console spam.

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_user_list.js (right):

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_user_list.js:77: * Removes
given user from the tmodel and updates backend.
tmodel: is this a spelling mistake?

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:333: * Returns username
(canonical email) of the user logged in (ChromeOS only).
You keep specifying "canonical email" everywhere because it is indeed confusing.
Why not just s/username/canonicalEmail/?

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc
(right):

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc:89:
base::DictionaryValue* user_dict = new DictionaryValue;
This doesn't appear necessary to be on the object. Move to unnamed namespace?

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc:145: bool
AccountsOptionsHandler::WhitelistUser(const std::string& username) {
Same as above; consider moving to unnamed namespace.

[Ivan Korotkov, 2011-12-02 18:30:30]

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_options.js (right):

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_options.js:6: 
On 2011/12/02 18:12:01, James Hawkins wrote:
> Remove blank line while you're here.

Done.

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_options.js:43:
this.allowWhitelist_ = AccountsOptions.currentUserIsOwner();
On 2011/12/02 18:12:01, James Hawkins wrote:
> "Forward declare" and document this variable.

Done.

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_options.js:115:
console.log('Chrome -> whitelist "' + e.user.name + '" ' + e.user.email);
On 2011/12/02 18:12:01, James Hawkins wrote:
> Remove console spam.

Oops, done.

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_user_list.js (right):

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_user_list.js:77: * Removes
given user from the tmodel and updates backend.
On 2011/12/02 18:12:01, James Hawkins wrote:
> tmodel: is this a spelling mistake?

Yep, fixed.

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:333: * Returns username
(canonical email) of the user logged in (ChromeOS only).
On 2011/12/02 18:12:01, James Hawkins wrote:
> You keep specifying "canonical email" everywhere because it is indeed
confusing.
> Why not just s/username/canonicalEmail/?

IMO it would create a mess given that C++ extensively uses "username" and that
there has been no concept of "canonical email" before. I think I'd rather
document it clearly in some single place in JS.

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc
(right):

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc:89:
base::DictionaryValue* user_dict = new DictionaryValue;
On 2011/12/02 18:12:01, James Hawkins wrote:
> This doesn't appear necessary to be on the object. Move to unnamed namespace?

This is called from CoreChromeOSOptionsHandler

http://codereview.chromium.org/8773046/diff/1001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc:145: bool
AccountsOptionsHandler::WhitelistUser(const std::string& username) {
On 2011/12/02 18:12:01, James Hawkins wrote:
> Same as above; consider moving to unnamed namespace.

Done.

[James Hawkins, 2011-12-02 19:52:30]

resources and webui lgtm with nit.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc
(right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc:160: bool
AccountsOptionsHandler::IsLoggedInOwner(const std::string& username) {
This should be able to be moved to unnamed namespace.

[whywhat, 2011-12-05 12:14:49]

LGTM
Please, file a cleanup bug to use username/email in C++ code consistently

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils_browsertest.cc:256: username, "",
"password", credentials, false, true, false, this);
nit: You pass "" here and std::string() in other place.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.h:34: // Delegate to be used while
user removing.
nit: Move to a separate header

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_options.js (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_options.js:19:
this.allowWhitelist_ = false;
nit: rename to showWhitelist

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:528: std::string
username;
Rename this username to typed_email

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:62:
));
Unindent these by 4 spaces

[Nikita (slow), 2011-12-05 13:13:04]

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:542: // We did not log in (we crashed or
are debugging), so we need to
nit: Please update comment since we're not passing username directly now.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.h:207: std::string
next_login_display_email_;
nit: rename to display_email_ or login_user_display_email_

Next in this current is "current", right?

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:889:
user_profile->GetProfileSyncService(email)->SetPassphrase(password_, false);
Please double check how email is used in Sync service and that it's not passed
to Sync servers.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.h (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.h:56: // this value, shown in UI and
used for Sync.
nit: used for Sync > displayed in Sync status?
As far as I understand per our discussion it only is used to display Sync
status? Please double check that it won't affect Sync in other ways.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user.h (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.h:17: // A class representing information
about a previously logged in user.
nit: Update comment how where email_ and where display_email_ are supposed to be
used.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:370:
prefs->ScheduleSavePersistentPrefs();
nit: Cleanup, this call could be removed.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:420:
local_state->ScheduleSavePersistentPrefs();
nit: Cleanup, this call could be removed.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:471: DVLOG(1) << "Saving displayed
email in Local State";
nit: cleanup log.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:472:
local_state->ScheduleSavePersistentPrefs();
Please remove since prefs are automatically scheduled for save so it's a no-op
now.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:481: // TODO(ivankr): won't work
with users that are not registered but present
Is this only for whitelist? We may as well ignore whitelist use case for display
emails.

[Nikita (slow), 2011-12-05 13:14:15]

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.h:207: std::string
next_login_display_email_;
On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> nit: rename to display_email_ or login_user_display_email_
> 
> Next in this current is "current", right?

* In this context

[Ivan Korotkov, 2011-12-05 14:12:47]

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:542: // We did not log in (we crashed or
are debugging), so we need to
On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> nit: Please update comment since we're not passing username directly now.

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.h:207: std::string
next_login_display_email_;
On 2011/12/05 13:14:15, Nikita Kostylev wrote:
> On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> > nit: rename to display_email_ or login_user_display_email_
> > 
> > Next in this current is "current", right?
> 
> * In this context

Done. 'next' means the call to CompleteLogin() that will follow
SetDisplayEmail().

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:889:
user_profile->GetProfileSyncService(email)->SetPassphrase(password_, false);
On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> Please double check how email is used in Sync service and that it's not passed
> to Sync servers.

As discussed offline, removed from this CL.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils_browsertest.cc:256: username, "",
"password", credentials, false, true, false, this);
On 2011/12/05 12:14:49, whywhat wrote:
> nit: You pass "" here and std::string() in other place.

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user.h (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.h:17: // A class representing information
about a previously logged in user.
On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> nit: Update comment how where email_ and where display_email_ are supposed to
be
> used.

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:370:
prefs->ScheduleSavePersistentPrefs();
On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> nit: Cleanup, this call could be removed.

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:420:
local_state->ScheduleSavePersistentPrefs();
On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> nit: Cleanup, this call could be removed.

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:471: DVLOG(1) << "Saving displayed
email in Local State";
On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> nit: cleanup log.

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:472:
local_state->ScheduleSavePersistentPrefs();
On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> Please remove since prefs are automatically scheduled for save so it's a no-op
> now.

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:481: // TODO(ivankr): won't work
with users that are not registered but present
On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> Is this only for whitelist? We may as well ignore whitelist use case for
display
> emails.

Yes, it's for whitelist only. I'll leave a TODO for myself for now.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.h:34: // Delegate to be used while
user removing.
On 2011/12/05 12:14:49, whywhat wrote:
> nit: Move to a separate header

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_options.js (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_options.js:19:
this.allowWhitelist_ = false;
On 2011/12/05 12:14:49, whywhat wrote:
> nit: rename to showWhitelist

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:528: std::string
username;
On 2011/12/05 12:14:49, whywhat wrote:
> Rename this username to typed_email

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc
(right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc:160: bool
AccountsOptionsHandler::IsLoggedInOwner(const std::string& username) {
On 2011/12/02 19:52:30, James Hawkins wrote:
> This should be able to be moved to unnamed namespace.

Done.

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:62:
));
On 2011/12/05 12:14:49, whywhat wrote:
> Unindent these by 4 spaces

Done.

[Nikita (slow), 2011-12-05 17:55:34]

lgtm

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

http://codereview.chromium.org/8773046/diff/4002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.h:207: std::string
next_login_display_email_;
On 2011/12/05 14:12:48, Ivan Korotkov wrote:
> On 2011/12/05 13:14:15, Nikita Kostylev wrote:
> > On 2011/12/05 13:13:04, Nikita Kostylev wrote:
> > > nit: rename to display_email_ or login_user_display_email_
> > > 
> > > Next in this current is "current", right?
> > 
> > * In this context
> 
> Done. 'next' means the call to CompleteLogin() that will follow
> SetDisplayEmail().

Since it's get called from the sign in process already in action let's keep the
name simple.

[commit-bot: I haz the power, 2011-12-06 15:21:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/8773046/24002

[commit-bot: I haz the power, 2011-12-06 15:21:27]

Can't process patch for file
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-12-06 15:24:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/8773046/27002