Put Kiosk App parameters into device settings.

Put Kiosk App parameters into device settings.

Remove the existing Kiosk App loading logic that presisted data in
local_state and replace it by code that reads/writes from/to device
settings. We're now using the existing device-local accounts users list,
which is also one step towards handling kiosk apps within the standard
login/user framework.

BUG=chromium:216359
TEST=Configure Kiosk App in device settings, observe it showing up on the login screen.

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

[Mattias Nissler (ping if slow), 2013-04-22 18:26:17]

Bartosz, Nikita: This is the CL that rewires kiosk app persistence to use
device-local account device settings. I've kept the UserManager changes minimal
(just filtering) as my understanding is Bartosz is messing with it at the moment
:)

Zel, Xiyuan: FYI, this CL actually gets us to the point where you can configure
Kiosk Apps in DMServer. There are still some rough edges to resolve, but
basically we can start looking at live Kiosk App setup flows with this
committed. If you're curious, I can give you instructions on how to configure a
test server appropriately.

[xiyuan, 2013-04-22 20:01:04]

LGTM

We also need to wire OnKioskAppsListChanged in KioskAppsHandler, which was lost
when I refactor out KioskAppPrefs.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[xiyuan, 2013-04-23 06:36:56]

https://codereview.chromium.org/14306004/diff/1/chrome/browser/chromeos/app_m...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/1/chrome/browser/chromeos/app_m...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:87:
CrosSettings::Get()->SetInteger(kAccountsPrefDeviceLocalAccountAutoLoginId,
Is this kAccountsPrefDeviceLocalAccountAutoLoginDelay instead of AutoLoginId?

[Mattias Nissler (ping if slow), 2013-04-23 14:01:01]

+bartfab whom I missed originally.

https://codereview.chromium.org/14306004/diff/1/chrome/browser/chromeos/app_m...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/1/chrome/browser/chromeos/app_m...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:87:
CrosSettings::Get()->SetInteger(kAccountsPrefDeviceLocalAccountAutoLoginId,
On 2013/04/23 06:36:56, xiyuan wrote:
> Is this kAccountsPrefDeviceLocalAccountAutoLoginDelay instead of AutoLoginId?

Yes :)

[bartfab (slow), 2013-04-23 15:59:40]

LGTM with a few nits

https://codereview.chromium.org/14306004/diff/7001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:3608: If this policy is set to False,
auto-login (if configured) cannot be bypassed.'''
What if auto-login is configured with a timeout, say 5 minutes? If auto-login
"cannot be bypassed", does that mean you cannot log in as anyone else during
those 5 minutes when the login screen is being shown?

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/ap...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:21: #include
"content/public/browser/notification_details.h"
Nit: No longer used.

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager.cc (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager.cc:62: const std::string
suffix(std::string("@") + kKioskAppUserDomain);
Why not use gaia::ExtractDomainName()?

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:839: if (domain_name ==
UserManager::kLocallyManagedUserDomain) {
Nit: Why |domain_name| here and |domain| 50 lines down? It would be nice to
settle on one name.

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:895: if
(!(*user)->GetAsString(&user_id))
Nit: Add NOTREACHED()?

[Nikita (slow), 2013-04-23 17:04:49]

chromeos/login lgtm

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:889: base::ListValue
public_accounts;
nit: Could this be extracted to a separate method?

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:900: // standard user
framework.
nit: Please file a bug.

[kenlin, 2013-04-23 18:09:42]

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:246: optional
string id = 1;
Did I understand correctly that by using <appid>@kiosk-apps.localhost
convention, Chrome OS device will understand appId is the app to download and
run?  Does it assume CWS will host the app?  Can we any other websites hosting
the app?

[Mattias Nissler (ping if slow), 2013-04-23 18:28:10]

https://codereview.chromium.org/14306004/diff/7001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:3608: If this policy is set to False,
auto-login (if configured) cannot be bypassed.'''
On 2013/04/23 15:59:40, bartfab wrote:
> What if auto-login is configured with a timeout, say 5 minutes? If auto-login
> "cannot be bypassed", does that mean you cannot log in as anyone else during
> those 5 minutes when the login screen is being shown?

You're right. Fixed the sentence to take into account delayed auto-login.

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/ap...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/ap...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:21: #include
"content/public/browser/notification_details.h"
On 2013/04/23 15:59:40, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager.cc (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager.cc:62: const std::string
suffix(std::string("@") + kKioskAppUserDomain);
On 2013/04/23 15:59:40, bartfab wrote:
> Why not use gaia::ExtractDomainName()?

Because I need the part left to the @ without, and ExtractDomainName does
canonicalization first, so assuming no knowledge of the mangling rules I don't
know which substring to extract.

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:839: if (domain_name ==
UserManager::kLocallyManagedUserDomain) {
On 2013/04/23 15:59:40, bartfab wrote:
> Nit: Why |domain_name| here and |domain| 50 lines down? It would be nice to
> settle on one name.

Ha, this is an edit artifact actually :) Fixed.

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:889: base::ListValue
public_accounts;
On 2013/04/23 17:04:50, Nikita Kostylev wrote:
> nit: Could this be extracted to a separate method?

Done.

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:895: if
(!(*user)->GetAsString(&user_id))
On 2013/04/23 15:59:40, bartfab wrote:
> Nit: Add NOTREACHED()?

Done.

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:900: // standard user
framework.
On 2013/04/23 17:04:50, Nikita Kostylev wrote:
> nit: Please file a bug.

Done.

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/7001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:246: optional
string id = 1;
On 2013/04/23 18:09:42, kenlin wrote:
> Did I understand correctly that by using <mailto:appid>@kiosk-apps.localhost
> convention, Chrome OS device will understand appId is the app to download and
> run?  Does it assume CWS will host the app?  Can we any other websites hosting
> the app?

Good point. Maybe it's not such a bright idea to use the same field for kiosk
apps and device-local accounts after all? I'd like to reuse the auto-login part
though to avoid ambiguity. I'll think more about this and update the CL tomorrow
with a proposal.

[Mattias Nissler (ping if slow), 2013-04-24 21:00:59]

Here's a new approach with the app ID as a separate field. I think that's
better, the other version tied too much logic to the id field. What do people
think?

[xiyuan, 2013-04-24 22:20:36]

SLGTM

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/resources/...
File
chrome/browser/resources/options/chromeos/kiosk_app_disable_bailout_confirm.js
(right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/kiosk_app_disable_bailout_confirm.js:35:
'cros.accounts.deviceLocalAccountAutoLoginBailoutEnabled',
Why not use el.pref to keep the pref name in one place?

[bartfab (slow), 2013-04-25 11:17:28]

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:96:
scoped_ptr<base::DictionaryValue> new_accounts_dict(
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:99:
scoped_ptr<base::DictionaryValue> entry_dict(new base::DictionaryValue());
What is the advantage of a scoped_ptr here if you will unconditionally pass
ownership four lines later anyway?

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:203: NOTREACHED();
Nit: #include "base/logging.h"

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:208: if
(!entry_dict->GetStringWithoutPathExpansion(
The code should look at account->key() instead to determine whether this is a
kiosk app account. Trying to guess the account type based on the contents of
|entry_dict| is brittle and will break if two different account types happen to
introduce identically-named keys inside their entry dicts. The user ID is
supposed to identify the account type, so let's use that.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:8: #include "base/string_util.h"
Nit: Not used.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:65: user_id.substr(0,
user_id.size() - arraysize(kKioskAppUserDomain)));
Will that not leave a dangling "@" at the end of |candidate_app_id|?

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:66: if
(extensions::Extension::IdIsValid(candidate_app_id)) {
Since you are explicitly storing the app ID in a different place now, why does
the local part of the user ID still have to be a vaild app ID? Should this not
just be an opaque identifier?

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:77:
DCHECK(extensions::Extension::IdIsValid(app_id));
Same as above: Why does the local part need to be a valid app ID?

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager.h (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.h:87: // Parses a Kiosk App user ID.
Fills in |app_id| and returns true on success.
I think it is not clear from the description that this actually maps between two
different namespaces (app IDs and synthetic user IDs). Could you expand the
comment?

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:1432:
kAccountsPrefDeviceLocalAccounts, &device_local_accounts)) {
By switching from a ListValue to a DictionaryValue for the public accounts, we
are losing information about their ordering. I think this is a bad thing.
Imagine a device pre-configured with three kiosk apps. You would not want the
three pods to appear in random, unpredictable order.

FWIW, I ran into the same problem when moving all user information into a single
data structure in Local State. I ended up going with the obvious solution: A
ListValue that holds a DictionaryValue for each user which then contains a key
"user_id" that tells you what user it is for. You have to do a linear search to
find a particular user but given the expected sizes of these lists, I see no
issues with that.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:1441: if
(entry_dict->HasKey(kAccountsPrefDeviceLocalAccountsKeyKioskAppId)) {
As in KioskAppManager, this code should look at account->key() instead.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:124:
scoped_ptr<base::DictionaryValue> account_dict(
1/ Nit: #include "base/memory/scoped_ptr.h"

2/  Why use a scoped_ptr if you will pass ownership unconditionally anyway?

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:131:
scoped_ptr<base::DictionaryValue> entry_dict(
Why use a scoped_ptr if you will pass ownership unconditionally anyway?

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:259: // Whether
the keyboard shortcut to prevent zero-delay auto-login should be
Does this affect all device-local accounts or kiosk-app accounts only?

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/cros_settings_names.cc:7: #include
"base/basictypes.h"
Nit: No longer used.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:469: if
(entry->id().empty() || account_dict->HasKey(entry->id()))
This way, we silently skip duplicates. We should at least print out a warning.
Before, duplicates would end up in |account_list| and the UserManager would
produce warnings about duplicates when processing it.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:472:
scoped_ptr<base::DictionaryValue> entry_dict(new base::DictionaryValue());
As before: Why use a scoped_ptr if you are passing ownership unconditionally?

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc:16: #include
"content/public/browser/notification_details.h"
Nit: No longer used.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc:17: #include
"content/public/browser/notification_service.h"
Nit: No longer used.

[xiyuan, 2013-04-25 17:26:32]

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:65: user_id.substr(0,
user_id.size() - arraysize(kKioskAppUserDomain)));
On 2013/04/25 11:17:28, bartfab wrote:
> Will that not leave a dangling "@" at the end of |candidate_app_id|?

I had the same question when looking at this. Then I found out that it won't
because arraysize include the NULL terminator. But think we might want to
document it.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:66: if
(extensions::Extension::IdIsValid(candidate_app_id)) {
On 2013/04/25 11:17:28, bartfab wrote:
> Since you are explicitly storing the app ID in a different place now, why does
> the local part of the user ID still have to be a vaild app ID? Should this not
> just be an opaque identifier?

I don't see the the benefit of using an opaque identifier here. Right now the
kiosk app mechanism depends on this to be an app id. On crash restarting, the
login-user is the only thing we have and we derive the app id from that. This
might be wrong and we might want to change it later. But that's how it is works
now.

Are you concerning this will be inconsistent with the separately stored app id?

[Mattias Nissler (ping if slow), 2013-04-26 09:10:05]

Hey guys, new version is up for review. I hope I'm closing in on this now :)

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:96:
scoped_ptr<base::DictionaryValue> new_accounts_dict(
On 2013/04/25 11:17:28, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:99:
scoped_ptr<base::DictionaryValue> entry_dict(new base::DictionaryValue());
On 2013/04/25 11:17:28, bartfab wrote:
> What is the advantage of a scoped_ptr here if you will unconditionally pass
> ownership four lines later anyway?

I made that a habit, don't have to worry about memory leaks any longer if there
aren't any unowned pointers in the code I write.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:203: NOTREACHED();
On 2013/04/25 11:17:28, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:208: if
(!entry_dict->GetStringWithoutPathExpansion(
On 2013/04/25 11:17:28, bartfab wrote:
> The code should look at account->key() instead to determine whether this is a
> kiosk app account. Trying to guess the account type based on the contents of
> |entry_dict| is brittle and will break if two different account types happen
to
> introduce identically-named keys inside their entry dicts. The user ID is
> supposed to identify the account type, so let's use that.

Switched to the type enum.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:8: #include "base/string_util.h"
On 2013/04/25 11:17:28, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:65: user_id.substr(0,
user_id.size() - arraysize(kKioskAppUserDomain)));
On 2013/04/25 11:17:28, bartfab wrote:
> Will that not leave a dangling "@" at the end of |candidate_app_id|?

Obsolete

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:66: if
(extensions::Extension::IdIsValid(candidate_app_id)) {
On 2013/04/25 11:17:28, bartfab wrote:
> Since you are explicitly storing the app ID in a different place now, why does
> the local part of the user ID still have to be a vaild app ID? Should this not
> just be an opaque identifier?

Obsolete

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:66: if
(extensions::Extension::IdIsValid(candidate_app_id)) {
On 2013/04/25 17:26:32, xiyuan wrote:
> On 2013/04/25 11:17:28, bartfab wrote:
> > Since you are explicitly storing the app ID in a different place now, why
does
> > the local part of the user ID still have to be a vaild app ID? Should this
not
> > just be an opaque identifier?
> 
> I don't see the the benefit of using an opaque identifier here. Right now the
> kiosk app mechanism depends on this to be an app id. On crash restarting, the
> login-user is the only thing we have and we derive the app id from that. This
> might be wrong and we might want to change it later. But that's how it is
works
> now.
> 
> Are you concerning this will be inconsistent with the separately stored app
id?

Some background from an offline discussion I had with Bartosz: The concern here
is tying too many systems together which use the same ID for different purposes.
In particular:

- UserManager and the login code use user@domain ids for identifying users
- Device settings use the ID for distinguishing device-local accounts
- For public sessions, the same identifier is used to fetch policy
- For device policy, the identifier gets assigned by the server

So, keep all these uses reasonably separated (so we can iterate them), the plan
Bartosz and me are following now is this:

- UserManager and the login code continue to use the user@domain scheme
- These identifiers are not persisted directly to device settings. Instead, we
have some code that constructs the appropriate format used by UserManager when
reading device-local accounts from UserManager. That way, we can change the
naming scheme for local users in the login code without having to worry about
migrating data.
- For device policy, the server can use any identifier scheme in the
DeviceLocalAcccountInfoProto.id field, and that won't affect how these users
appear in Chrome. Instead, we put the appropriate user type in a separate field.

Hope this makes sense!
-

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.cc:77:
DCHECK(extensions::Extension::IdIsValid(app_id));
On 2013/04/25 11:17:28, bartfab wrote:
> Same as above: Why does the local part need to be a valid app ID?

Obsolete

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager.h (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.h:87: // Parses a Kiosk App user ID.
Fills in |app_id| and returns true on success.
On 2013/04/25 11:17:28, bartfab wrote:
> I think it is not clear from the description that this actually maps between
two
> different namespaces (app IDs and synthetic user IDs). Could you expand the
> comment?

Obsolete.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:1432:
kAccountsPrefDeviceLocalAccounts, &device_local_accounts)) {
On 2013/04/25 11:17:28, bartfab wrote:
> By switching from a ListValue to a DictionaryValue for the public accounts, we
> are losing information about their ordering. I think this is a bad thing.
> Imagine a device pre-configured with three kiosk apps. You would not want the
> three pods to appear in random, unpredictable order.
> 
> FWIW, I ran into the same problem when moving all user information into a
single
> data structure in Local State. I ended up going with the obvious solution: A
> ListValue that holds a DictionaryValue for each user which then contains a key
> "user_id" that tells you what user it is for. You have to do a linear search
to
> find a particular user but given the expected sizes of these lists, I see no
> issues with that.

Switched to the list.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:1441: if
(entry_dict->HasKey(kAccountsPrefDeviceLocalAccountsKeyKioskAppId)) {
On 2013/04/25 11:17:28, bartfab wrote:
> As in KioskAppManager, this code should look at account->key() instead.

Switched to checking the type key.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:124:
scoped_ptr<base::DictionaryValue> account_dict(
On 2013/04/25 11:17:28, bartfab wrote:
> 1/ Nit: #include "base/memory/scoped_ptr.h"
> 
> 2/  Why use a scoped_ptr if you will pass ownership unconditionally anyway?

1/ Done.
2/ Habit to avoid memory leaks.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:131:
scoped_ptr<base::DictionaryValue> entry_dict(
On 2013/04/25 11:17:28, bartfab wrote:
> Why use a scoped_ptr if you will pass ownership unconditionally anyway?

See above

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:259: // Whether
the keyboard shortcut to prevent zero-delay auto-login should be
On 2013/04/25 11:17:28, bartfab wrote:
> Does this affect all device-local accounts or kiosk-app accounts only?

It should eventually affect all device-local accounts, but we need to refactor
the kiosk code to use the standard login flow first.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/cros_settings_names.cc:7: #include
"base/basictypes.h"
On 2013/04/25 11:17:28, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:469: if
(entry->id().empty() || account_dict->HasKey(entry->id()))
On 2013/04/25 11:17:28, bartfab wrote:
> This way, we silently skip duplicates. We should at least print out a warning.
> Before, duplicates would end up in |account_list| and the UserManager would
> produce warnings about duplicates when processing it.

Obsolete, using a list.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:472:
scoped_ptr<base::DictionaryValue> entry_dict(new base::DictionaryValue());
On 2013/04/25 11:17:28, bartfab wrote:
> As before: Why use a scoped_ptr if you are passing ownership unconditionally?

same as before.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/resources/...
File
chrome/browser/resources/options/chromeos/kiosk_app_disable_bailout_confirm.js
(right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/kiosk_app_disable_bailout_confirm.js:35:
'cros.accounts.deviceLocalAccountAutoLoginBailoutEnabled',
On 2013/04/24 22:20:36, xiyuan wrote:
> Why not use el.pref to keep the pref name in one place?

Note that the pref is now inverted, and we're setting the pref to false now. I
wanted to be extra-verbose to make clear what's going on here, happy to switch
back if you're thinking the other way is easier to understand.

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc (right):

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc:16: #include
"content/public/browser/notification_details.h"
On 2013/04/25 11:17:28, bartfab wrote:
> Nit: No longer used.

Still used, we're sending a notification in HandleKioskAppsLoaded().

https://codereview.chromium.org/14306004/diff/25001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/kiosk_app_menu_handler.cc:17: #include
"content/public/browser/notification_service.h"
On 2013/04/25 11:17:28, bartfab wrote:
> Nit: No longer used.

Ditto.

[bartfab (slow), 2013-04-26 12:34:59]

lgtm

https://codereview.chromium.org/14306004/diff/53028/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/53028/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:254: std::string
auto_login_account_id_;
Nit: Remove trailing underscore.

https://codereview.chromium.org/14306004/diff/53028/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:259: if
(CrosSettings::Get()->GetList(kAccountsPrefDeviceLocalAccounts,
Nit: This is a bit inconsistent: In some places, e.g. RemoveApp(), you trust
CrosSettings::Get() to always yield a valid list, without checking its return
value. Here, you do check the return value.

[Mattias Nissler (ping if slow), 2013-04-26 13:44:46]

Here's a new version that fixes failing tests and addresses Bartosz' latest
comments.

https://codereview.chromium.org/14306004/diff/53028/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/53028/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:254: std::string
auto_login_account_id_;
On 2013/04/26 12:34:59, bartfab wrote:
> Nit: Remove trailing underscore.

Done.

https://codereview.chromium.org/14306004/diff/53028/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:259: if
(CrosSettings::Get()->GetList(kAccountsPrefDeviceLocalAccounts,
On 2013/04/26 12:34:59, bartfab wrote:
> Nit: This is a bit inconsistent: In some places, e.g. RemoveApp(), you trust
> CrosSettings::Get() to always yield a valid list, without checking its return
> value. Here, you do check the return value.

Fixed the other two call sites.

[xiyuan, 2013-04-26 17:15:13]

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:138: return;
nit: 1 more space indentation.

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_observer.h (right):

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager_observer.h:21: virtual void
OnKioskAppsListChanged() {}
nit: This also fires when auto launch app is changed. Maybe rename it to
something more generic, e.g. OnKioskAppSettingsChanged. Or update the comment to
make it clear for future reader.

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/stub_cros_settings_provider.cc (right):

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/stub_cros_settings_provider.cc:59:
values_.SetValue(kAccountsPrefDeviceLocalAccounts, new base::DictionaryValue);
This should be ListValue since we changed it back now, right?

[Mattias Nissler (ping if slow), 2013-04-29 18:25:17]

OK, addressed Xiyuan's comments. If there are no further complaints, I'll land
this now.

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:138: return;
On 2013/04/26 17:15:13, xiyuan wrote:
> nit: 1 more space indentation.

Done.

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_observer.h (right):

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager_observer.h:21: virtual void
OnKioskAppsListChanged() {}
On 2013/04/26 17:15:13, xiyuan wrote:
> nit: This also fires when auto launch app is changed. Maybe rename it to
> something more generic, e.g. OnKioskAppSettingsChanged. Or update the comment
to
> make it clear for future reader.

Done, good point.

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/stub_cros_settings_provider.cc (right):

https://codereview.chromium.org/14306004/diff/78001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/stub_cros_settings_provider.cc:59:
values_.SetValue(kAccountsPrefDeviceLocalAccounts, new base::DictionaryValue);
On 2013/04/26 17:15:13, xiyuan wrote:
> This should be ListValue since we changed it back now, right?

Done.

[xiyuan, 2013-04-29 19:03:27]

Launch it!

[Mattias Nissler (ping if slow), 2013-04-30 13:15:24]

Ha, I almost launched it :)

Turns out that there's an issue with forward-compatibility: If I change the
device settings format as originally planned, old clients will treat Kiosk App
device-local accounts as if they were Public Sessions. To avoid that, I've
adjusted the protobuf format once more, no changes to the upper layers.

Since Bartosz is out, adding Joao to give this a quick sanity check, in
particular the changes from patch set 10 to 11. Thanks!

[Mattias Nissler (ping if slow), 2013-04-30 13:28:35]

Adding Joao for realz.

[Joao da Silva, 2013-04-30 14:31:14]

lgtm. Bin should agree with the proto changes before landing.

https://codereview.chromium.org/14306004/diff/103001/chrome/app/policy/policy...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/14306004/diff/103001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3603: 'caption': '''Enable bailout
keyboard shortcut for auto-login.''',
nit: no '.' at the end of the caption (only 3 other policies deviate from this,
feel free to fix them in this CL too)

https://codereview.chromium.org/14306004/diff/103001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc
(right):

https://codereview.chromium.org/14306004/diff/103001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:62:
.WillRepeatedly(Return(static_cast<User*>(NULL)));
ReturnNull?

https://codereview.chromium.org/14306004/diff/103001/chrome/browser/policy/pr...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/103001/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:235: optional
string deprecated_public_session_id = 1;
IIUC, this proto will be used both for public accounts and kiosk apps; and old
clients that don't know about kiosk apps will see an empty field here, and will
ignore this proto. Is that right?

[Mattias Nissler (ping if slow), 2013-04-30 14:51:45]

Addressed Joao's nits.

Bin, here are the policy changes for defining Kiosk Apps, can you take a look
and let me know if you have any concerns?

https://codereview.chromium.org/14306004/diff/103001/chrome/app/policy/policy...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/14306004/diff/103001/chrome/app/policy/policy...
chrome/app/policy/policy_templates.json:3603: 'caption': '''Enable bailout
keyboard shortcut for auto-login.''',
On 2013/04/30 14:31:15, Joao da Silva wrote:
> nit: no '.' at the end of the caption (only 3 other policies deviate from
this,
> feel free to fix them in this CL too)

Done.

https://codereview.chromium.org/14306004/diff/103001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc
(right):

https://codereview.chromium.org/14306004/diff/103001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc:62:
.WillRepeatedly(Return(static_cast<User*>(NULL)));
On 2013/04/30 14:31:15, Joao da Silva wrote:
> ReturnNull?

Done.

https://codereview.chromium.org/14306004/diff/103001/chrome/browser/policy/pr...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/103001/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:235: optional
string deprecated_public_session_id = 1;
On 2013/04/30 14:31:15, Joao da Silva wrote:
> IIUC, this proto will be used both for public accounts and kiosk apps; and old
> clients that don't know about kiosk apps will see an empty field here, and
will
> ignore this proto. Is that right?

Correct.

[binzhao, 2013-04-30 22:01:10]

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:235: optional
string deprecated_public_session_id = 1;
My understanding is that the old client has no knowledge of account type, then
it only looks at the id field and it means a public session.

For new client, if account type is not set, then it assumes it is a public
session and read it from id field. If type is set, then it honors the type and
read id field.

Not sure why we need to deprecate this and have an new field then.

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:251: optional
AccountType type = 3;
we can specify the default to be ACCOUNT_TYPE_PUBLIC_SESSION then we don't need
to add a new field.

[Mattias Nissler (ping if slow), 2013-04-30 23:37:36]

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:235: optional
string deprecated_public_session_id = 1;
On 2013/04/30 22:01:10, binzhao wrote:
> My understanding is that the old client has no knowledge of account type, then
> it only looks at the id field and it means a public session.
> 
> For new client, if account type is not set, then it assumes it is a public
> session and read it from id field. If type is set, then it honors the type and
> read id field.
> 
> Not sure why we need to deprecate this and have an new field then.

The issue is forward-compatibility: IMHO, old clients should ignore new account
types instead of creating Public Sessions for accounts with types they can't
handle. If unknown types default to be Public Sessions on old clients, we run
into these problems:
- We'll add unintentional Public Sessions, which will confuse users. Most
likely, the policy configuration for these accounts will be missing/incomplete,
so the Public Sessions will have cryptic account names etc. which adds to the
confusion.
- Once an old client actually upgrades, it starts understanding the new account
type. But then it may already have accumulated state for an account that was
incorrectly defaulted to be a Public Session, so there now is inconsistent
state.
- If we default to Public Session, we'll also try to fetch Public Session
policy. Currently, there is no plan to support this for Kiosk Apps, so old
clients would make unnecessary requests to the server (they'd be otherwise
harmless).

The only option I see that avoids deprecating the existing field is to make a
rule that we can only switch on support for new account types once we're sure
all old clients have been updated to understand the new type. That's quite
inflexible however, as it wouldn't allow us to test/release new features before
the client-side implementation hits the stable channel, and even if that's the
case, there might be stray devices that may take longer to update and thus run
into the issues I mention above.

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:251: optional
AccountType type = 3;
On 2013/04/30 22:01:10, binzhao wrote:
> we can specify the default to be ACCOUNT_TYPE_PUBLIC_SESSION then we don't
need
> to add a new field.

See my comment above.

[binzhao, 2013-05-01 00:30:44]

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:235: optional
string deprecated_public_session_id = 1;
OK, I agree when admin configures KIOSK, then pre-28 devices should behave like
nothing is configured rather than going into public session mode. It also means
the cloud-side need to set both for both new and old public session to work.

But please see my comments on the protobuf enum below.



On 2013/04/30 23:37:37, Mattias Nissler wrote:
> On 2013/04/30 22:01:10, binzhao wrote:
> > My understanding is that the old client has no knowledge of account type,
then
> > it only looks at the id field and it means a public session.
> > 
> > For new client, if account type is not set, then it assumes it is a public
> > session and read it from id field. If type is set, then it honors the type
and
> > read id field.
> > 
> > Not sure why we need to deprecate this and have an new field then.
> 
> The issue is forward-compatibility: IMHO, old clients should ignore new
account
> types instead of creating Public Sessions for accounts with types they can't
> handle. If unknown types default to be Public Sessions on old clients, we run
> into these problems:
> - We'll add unintentional Public Sessions, which will confuse users. Most
> likely, the policy configuration for these accounts will be
missing/incomplete,
> so the Public Sessions will have cryptic account names etc. which adds to the
> confusion.
> - Once an old client actually upgrades, it starts understanding the new
account
> type. But then it may already have accumulated state for an account that was
> incorrectly defaulted to be a Public Session, so there now is inconsistent
> state.
> - If we default to Public Session, we'll also try to fetch Public Session
> policy. Currently, there is no plan to support this for Kiosk Apps, so old
> clients would make unnecessary requests to the server (they'd be otherwise
> harmless).
> 
> The only option I see that avoids deprecating the existing field is to make a
> rule that we can only switch on support for new account types once we're sure
> all old clients have been updated to understand the new type. That's quite
> inflexible however, as it wouldn't allow us to test/release new features
before
> the client-side implementation hits the stable channel, and even if that's the
> case, there might be stray devices that may take longer to update and thus run
> into the issues I mention above.

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:251: optional
AccountType type = 3;
Is Chromium using proto1 or proto2?

I am not very familiar with Protobuf c++. There are some known issues with
Protobuf enum in Java. If we add another enum which the old client does not
recognize, when the old client call getType(), it gets the default value. If not
default value is specified, then the first value in the enum list is returned.

As far as I remember, a good practice is defining UNKNOWN to act as a default
value.

These two threads have some details

https://groups.google.com/a/google.com/forum/?fromgroups=#!msg/protobuf-team/...

https://groups.google.com/a/google.com/forum/?fromgroups=#!searchin/protobuf-...

On 2013/04/30 23:37:37, Mattias Nissler wrote:
> On 2013/04/30 22:01:10, binzhao wrote:
> > we can specify the default to be ACCOUNT_TYPE_PUBLIC_SESSION then we don't
> need
> > to add a new field.
> 
> See my comment above.

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:256: optional
string kiosk_app_id = 4;
Do we want to define a wrapper message type - KioskAppInfo to encapsulate all
Kiosk app info?

[Mattias Nissler (ping if slow), 2013-05-01 10:10:55]

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:235: optional
string deprecated_public_session_id = 1;
On 2013/05/01 00:30:44, binzhao wrote:
> OK, I agree when admin configures KIOSK, then pre-28 devices should behave
like
> nothing is configured rather than going into public session mode. It also
means
> the cloud-side need to set both for both new and old public session to work.

Yes that'd be helpful. It's probably not strictly necessary, because we'll have
to continue supporting the deprecated field due to the data that has it which is
present in the systems. Or would you be willing to run a migration job so we can
get rid of |deprecated_public_session_id| quicker?

> 
> But please see my comments on the protobuf enum below.
> 
> 
> 
> On 2013/04/30 23:37:37, Mattias Nissler wrote:
> > On 2013/04/30 22:01:10, binzhao wrote:
> > > My understanding is that the old client has no knowledge of account type,
> then
> > > it only looks at the id field and it means a public session.
> > > 
> > > For new client, if account type is not set, then it assumes it is a public
> > > session and read it from id field. If type is set, then it honors the type
> and
> > > read id field.
> > > 
> > > Not sure why we need to deprecate this and have an new field then.
> > 
> > The issue is forward-compatibility: IMHO, old clients should ignore new
> account
> > types instead of creating Public Sessions for accounts with types they can't
> > handle. If unknown types default to be Public Sessions on old clients, we
run
> > into these problems:
> > - We'll add unintentional Public Sessions, which will confuse users. Most
> > likely, the policy configuration for these accounts will be
> missing/incomplete,
> > so the Public Sessions will have cryptic account names etc. which adds to
the
> > confusion.
> > - Once an old client actually upgrades, it starts understanding the new
> account
> > type. But then it may already have accumulated state for an account that was
> > incorrectly defaulted to be a Public Session, so there now is inconsistent
> > state.
> > - If we default to Public Session, we'll also try to fetch Public Session
> > policy. Currently, there is no plan to support this for Kiosk Apps, so old
> > clients would make unnecessary requests to the server (they'd be otherwise
> > harmless).
> > 
> > The only option I see that avoids deprecating the existing field is to make
a
> > rule that we can only switch on support for new account types once we're
sure
> > all old clients have been updated to understand the new type. That's quite
> > inflexible however, as it wouldn't allow us to test/release new features
> before
> > the client-side implementation hits the stable channel, and even if that's
the
> > case, there might be stray devices that may take longer to update and thus
run
> > into the issues I mention above.
>

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:251: optional
AccountType type = 3;
On 2013/05/01 00:30:44, binzhao wrote:
> Is Chromium using proto1 or proto2?
> 
> I am not very familiar with Protobuf c++. There are some known issues with
> Protobuf enum in Java. If we add another enum which the old client does not
> recognize, when the old client call getType(), it gets the default value. If
not
> default value is specified, then the first value in the enum list is returned.

If read the code and tested it against a proto containing an enum value the
client doesn't know. It all works out because the enum parsing code in protobuf
checks whether the value it gets from the binary message is a valid enum
constant. If not, it just skips the field and has_type() returns false.

> 
> As far as I remember, a good practice is defining UNKNOWN to act as a default
> value.

I actually had that in an earlier (unpublished) iteration and then removed it
because I didn't need it to get what I wanted :)

> 
> These two threads have some details
> 
>
https://groups.google.com/a/google.com/forum/?fromgroups=#%21msg/protobuf-tea...
> 
>
https://groups.google.com/a/google.com/forum/?fromgroups=#%21searchin/protobu...

Judging from these threads, has_type() is actually a supported way of covering
this case for proto2. I don't see any reason why we'd need to care about proto1,
do you?

> 
> On 2013/04/30 23:37:37, Mattias Nissler wrote:
> > On 2013/04/30 22:01:10, binzhao wrote:
> > > we can specify the default to be ACCOUNT_TYPE_PUBLIC_SESSION then we don't
> > need
> > > to add a new field.
> > 
> > See my comment above.
>

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:256: optional
string kiosk_app_id = 4;
On 2013/05/01 00:30:44, binzhao wrote:
> Do we want to define a wrapper message type - KioskAppInfo to encapsulate all
> Kiosk app info?

Fair, I'll do that.

[Mattias Nissler (ping if slow), 2013-05-01 16:38:23]

New version is up.

Bin: Good to go?

[binzhao, 2013-05-01 16:56:48]

lgtm

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
File chrome/browser/policy/proto/chromeos/chrome_device_policy.proto (right):

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:235: optional
string deprecated_public_session_id = 1;
Is there a good way to tell most of the devices have upgraded so it is safe not
set this deprecated field any more? We can have an offline discussion how to
migrate cases like this.

On 2013/05/01 10:10:55, Mattias Nissler wrote:
> On 2013/05/01 00:30:44, binzhao wrote:
> > OK, I agree when admin configures KIOSK, then pre-28 devices should behave
> like
> > nothing is configured rather than going into public session mode. It also
> means
> > the cloud-side need to set both for both new and old public session to work.
> 
> Yes that'd be helpful. It's probably not strictly necessary, because we'll
have
> to continue supporting the deprecated field due to the data that has it which
is
> present in the systems. Or would you be willing to run a migration job so we
can
> get rid of |deprecated_public_session_id| quicker?
> 
> > 
> > But please see my comments on the protobuf enum below.
> > 
> > 
> > 
> > On 2013/04/30 23:37:37, Mattias Nissler wrote:
> > > On 2013/04/30 22:01:10, binzhao wrote:
> > > > My understanding is that the old client has no knowledge of account
type,
> > then
> > > > it only looks at the id field and it means a public session.
> > > > 
> > > > For new client, if account type is not set, then it assumes it is a
public
> > > > session and read it from id field. If type is set, then it honors the
type
> > and
> > > > read id field.
> > > > 
> > > > Not sure why we need to deprecate this and have an new field then.
> > > 
> > > The issue is forward-compatibility: IMHO, old clients should ignore new
> > account
> > > types instead of creating Public Sessions for accounts with types they
can't
> > > handle. If unknown types default to be Public Sessions on old clients, we
> run
> > > into these problems:
> > > - We'll add unintentional Public Sessions, which will confuse users. Most
> > > likely, the policy configuration for these accounts will be
> > missing/incomplete,
> > > so the Public Sessions will have cryptic account names etc. which adds to
> the
> > > confusion.
> > > - Once an old client actually upgrades, it starts understanding the new
> > account
> > > type. But then it may already have accumulated state for an account that
was
> > > incorrectly defaulted to be a Public Session, so there now is inconsistent
> > > state.
> > > - If we default to Public Session, we'll also try to fetch Public Session
> > > policy. Currently, there is no plan to support this for Kiosk Apps, so old
> > > clients would make unnecessary requests to the server (they'd be otherwise
> > > harmless).
> > > 
> > > The only option I see that avoids deprecating the existing field is to
make
> a
> > > rule that we can only switch on support for new account types once we're
> sure
> > > all old clients have been updated to understand the new type. That's quite
> > > inflexible however, as it wouldn't allow us to test/release new features
> > before
> > > the client-side implementation hits the stable channel, and even if that's
> the
> > > case, there might be stray devices that may take longer to update and thus
> run
> > > into the issues I mention above.
> > 
>

https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/pr...
chrome/browser/policy/proto/chromeos/chrome_device_policy.proto:251: optional
AccountType type = 3;
Right, if you check hasXXX(), then it is fine. If we don't check hasXXX(), then
we can put a UNKNOWN.

On 2013/05/01 10:10:55, Mattias Nissler wrote:
> On 2013/05/01 00:30:44, binzhao wrote:
> > Is Chromium using proto1 or proto2?
> > 
> > I am not very familiar with Protobuf c++. There are some known issues with
> > Protobuf enum in Java. If we add another enum which the old client does not
> > recognize, when the old client call getType(), it gets the default value. If
> not
> > default value is specified, then the first value in the enum list is
returned.
> 
> If read the code and tested it against a proto containing an enum value the
> client doesn't know. It all works out because the enum parsing code in
protobuf
> checks whether the value it gets from the binary message is a valid enum
> constant. If not, it just skips the field and has_type() returns false.
> 
> > 
> > As far as I remember, a good practice is defining UNKNOWN to act as a
default
> > value.
> 
> I actually had that in an earlier (unpublished) iteration and then removed it
> because I didn't need it to get what I wanted :)
> 
> > 
> > These two threads have some details
> > 
> >
>
https://groups.google.com/a/google.com/forum/?fromgroups=#%2521msg/protobuf-t...
> > 
> >
>
https://groups.google.com/a/google.com/forum/?fromgroups=#%2521searchin/proto...
> 
> Judging from these threads, has_type() is actually a supported way of covering
> this case for proto2. I don't see any reason why we'd need to care about
proto1,
> do you?
> 
> > 
> > On 2013/04/30 23:37:37, Mattias Nissler wrote:
> > > On 2013/04/30 22:01:10, binzhao wrote:
> > > > we can specify the default to be ACCOUNT_TYPE_PUBLIC_SESSION then we
don't
> > > need
> > > > to add a new field.
> > > 
> > > See my comment above.
> > 
>

[binzhao, 2013-05-01 17:08:34]

btw, since you change the field name, if you just sync this proto over to
Google3, it will break our code in Cpanel. Once this is checked in, I can
merge it to Google3. Let me know when it is in.


On Wed, May 1, 2013 at 9:56 AM, <binzhao@google.com> wrote:

> lgtm
>
>
>
>
>
> https://codereview.chromium.**org/14306004/diff/102004/**
>
chrome/browser/policy/proto/**chromeos/chrome_device_policy.**proto<https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/proto/chromeos/chrome_device_policy.proto>
> File chrome/browser/policy/proto/**chromeos/chrome_device_policy.**proto
> (right):
>
> https://codereview.chromium.**org/14306004/diff/102004/**
> chrome/browser/policy/proto/**chromeos/chrome_device_policy.**
>
proto#newcode235<https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/proto/chromeos/chrome_device_policy.proto#newcode235>
> chrome/browser/policy/proto/**chromeos/chrome_device_policy.**proto:235:
> optional string deprecated_public_session_id = 1;
> Is there a good way to tell most of the devices have upgraded so it is
> safe not set this deprecated field any more? We can have an offline
> discussion how to migrate cases like this.
>
>
> On 2013/05/01 10:10:55, Mattias Nissler wrote:
>
>> On 2013/05/01 00:30:44, binzhao wrote:
>> > OK, I agree when admin configures KIOSK, then pre-28 devices should
>>
> behave
>
>> like
>> > nothing is configured rather than going into public session mode. It
>>
> also
>
>> means
>> > the cloud-side need to set both for both new and old public session
>>
> to work.
>
>  Yes that'd be helpful. It's probably not strictly necessary, because
>>
> we'll have
>
>> to continue supporting the deprecated field due to the data that has
>>
> it which is
>
>> present in the systems. Or would you be willing to run a migration job
>>
> so we can
>
>> get rid of |deprecated_public_session_id| quicker?
>>
>
>  >
>> > But please see my comments on the protobuf enum below.
>> >
>> >
>> >
>> > On 2013/04/30 23:37:37, Mattias Nissler wrote:
>> > > On 2013/04/30 22:01:10, binzhao wrote:
>> > > > My understanding is that the old client has no knowledge of
>>
> account type,
>
>> > then
>> > > > it only looks at the id field and it means a public session.
>> > > >
>> > > > For new client, if account type is not set, then it assumes it
>>
> is a public
>
>> > > > session and read it from id field. If type is set, then it
>>
> honors the type
>
>> > and
>> > > > read id field.
>> > > >
>> > > > Not sure why we need to deprecate this and have an new field
>>
> then.
>
>> > >
>> > > The issue is forward-compatibility: IMHO, old clients should
>>
> ignore new
>
>> > account
>> > > types instead of creating Public Sessions for accounts with types
>>
> they can't
>
>> > > handle. If unknown types default to be Public Sessions on old
>>
> clients, we
>
>> run
>> > > into these problems:
>> > > - We'll add unintentional Public Sessions, which will confuse
>>
> users. Most
>
>> > > likely, the policy configuration for these accounts will be
>> > missing/incomplete,
>> > > so the Public Sessions will have cryptic account names etc. which
>>
> adds to
>
>> the
>> > > confusion.
>> > > - Once an old client actually upgrades, it starts understanding
>>
> the new
>
>> > account
>> > > type. But then it may already have accumulated state for an
>>
> account that was
>
>> > > incorrectly defaulted to be a Public Session, so there now is
>>
> inconsistent
>
>> > > state.
>> > > - If we default to Public Session, we'll also try to fetch Public
>>
> Session
>
>> > > policy. Currently, there is no plan to support this for Kiosk
>>
> Apps, so old
>
>> > > clients would make unnecessary requests to the server (they'd be
>>
> otherwise
>
>> > > harmless).
>> > >
>> > > The only option I see that avoids deprecating the existing field
>>
> is to make
>
>> a
>> > > rule that we can only switch on support for new account types once
>>
> we're
>
>> sure
>> > > all old clients have been updated to understand the new type.
>>
> That's quite
>
>> > > inflexible however, as it wouldn't allow us to test/release new
>>
> features
>
>> > before
>> > > the client-side implementation hits the stable channel, and even
>>
> if that's
>
>> the
>> > > case, there might be stray devices that may take longer to update
>>
> and thus
>
>> run
>> > > into the issues I mention above.
>> >
>>
>
>
> https://codereview.chromium.**org/14306004/diff/102004/**
> chrome/browser/policy/proto/**chromeos/chrome_device_policy.**
>
proto#newcode251<https://codereview.chromium.org/14306004/diff/102004/chrome/browser/policy/proto/chromeos/chrome_device_policy.proto#newcode251>
> chrome/browser/policy/proto/**chromeos/chrome_device_policy.**proto:251:
> optional AccountType type = 3;
> Right, if you check hasXXX(), then it is fine. If we don't check
> hasXXX(), then we can put a UNKNOWN.
>
>
> On 2013/05/01 10:10:55, Mattias Nissler wrote:
>
>> On 2013/05/01 00:30:44, binzhao wrote:
>> > Is Chromium using proto1 or proto2?
>> >
>> > I am not very familiar with Protobuf c++. There are some known
>>
> issues with
>
>> > Protobuf enum in Java. If we add another enum which the old client
>>
> does not
>
>> > recognize, when the old client call getType(), it gets the default
>>
> value. If
>
>> not
>> > default value is specified, then the first value in the enum list is
>>
> returned.
>
>  If read the code and tested it against a proto containing an enum
>>
> value the
>
>> client doesn't know. It all works out because the enum parsing code in
>>
> protobuf
>
>> checks whether the value it gets from the binary message is a valid
>>
> enum
>
>> constant. If not, it just skips the field and has_type() returns
>>
> false.
>
>  >
>> > As far as I remember, a good practice is defining UNKNOWN to act as
>>
> a default
>
>> > value.
>>
>
>  I actually had that in an earlier (unpublished) iteration and then
>>
> removed it
>
>> because I didn't need it to get what I wanted :)
>>
>
>  >
>> > These two threads have some details
>> >
>> >
>>
>
> https://groups.google.com/a/**google.com/forum/?fromgroups=#**
>
%2521msg/protobuf-team/**jVTTjRBwX-4/8e-Vn-GBwuQJ<https://groups.google.com/a/google.com/forum/?fromgroups=#%2521msg/protobuf-team/jVTTjRBwX-4/8e-Vn-GBwuQJ>
>
>> >
>> >
>>
>
> https://groups.google.com/a/**google.com/forum/?fromgroups=#**
> %2521searchin/protobuf-**discuss/proto2%252420java%**
>
252420enums/protobuf-discuss/**w_qJ5cE9CaU/N0eWgELaRC4J<https://groups.google.com/a/google.com/forum/?fromgroups=#%2521searchin/protobuf-discuss/proto2%252420java%252420enums/protobuf-discuss/w_qJ5cE9CaU/N0eWgELaRC4J>
>
>
>  Judging from these threads, has_type() is actually a supported way of
>>
> covering
>
>> this case for proto2. I don't see any reason why we'd need to care
>>
> about proto1,
>
>> do you?
>>
>
>  >
>> > On 2013/04/30 23:37:37, Mattias Nissler wrote:
>> > > On 2013/04/30 22:01:10, binzhao wrote:
>> > > > we can specify the default to be ACCOUNT_TYPE_PUBLIC_SESSION
>>
> then we don't
>
>> > > need
>> > > > to add a new field.
>> > >
>> > > See my comment above.
>> >
>>
>
>
>
https://codereview.chromium.**org/14306004/<https://codereview.chromium.org/1...
>



-- 
regards,
Bin

[Mattias Nissler (ping if slow), 2013-05-02 08:10:38]

On 2013/05/01 17:08:34, binzhao wrote:
> btw, since you change the field name, if you just sync this proto over to
> Google3, it will break our code in Cpanel. Once this is checked in, I can
> merge it to Google3. Let me know when it is in.

I was planning to sync with the old name and then kindly ask you to update the
name :) Let's discuss on the CL once I've sent it out.

> 
> 
> On Wed, May 1, 2013 at 9:56 AM, <mailto:binzhao@google.com> wrote:
> 
> > lgtm
> >
> >
> >
> >
> >
> > https://codereview.chromium.**org/14306004/diff/102004/**
> >
> chrome/browser/policy/proto/**chromeos/chrome_device_policy.**proto<<crxref
style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">https://codereview.chromium.org/14306004/</crxref>diff/102004/chrome/browser/policy/proto/chromeos/chrome_device_policy.proto>
> > File chrome/browser/policy/proto/**chromeos/chrome_device_policy.**proto
> > (right):
> >
> > https://codereview.chromium.**org/14306004/diff/102004/**
> > chrome/browser/policy/proto/**chromeos/chrome_device_policy.**
> >
> proto#newcode235<<crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">https://codereview.chromium.org/14306004/</crxref>diff/102004/chrome/browser/policy/proto/chromeos/chrome_device_policy.proto#newcode235>
> > chrome/browser/policy/proto/**chromeos/chrome_device_policy.**proto:235:
> > optional string deprecated_public_session_id = 1;
> > Is there a good way to tell most of the devices have upgraded so it is
> > safe not set this deprecated field any more? We can have an offline
> > discussion how to migrate cases like this.
> >
> >
> > On 2013/05/01 10:10:55, Mattias Nissler wrote:
> >
> >> On 2013/05/01 00:30:44, binzhao wrote:
> >> > OK, I agree when admin configures KIOSK, then pre-28 devices should
> >>
> > behave
> >
> >> like
> >> > nothing is configured rather than going into public session mode. It
> >>
> > also
> >
> >> means
> >> > the cloud-side need to set both for both new and old public session
> >>
> > to work.
> >
> >  Yes that'd be helpful. It's probably not strictly necessary, because
> >>
> > we'll have
> >
> >> to continue supporting the deprecated field due to the data that has
> >>
> > it which is
> >
> >> present in the systems. Or would you be willing to run a migration job
> >>
> > so we can
> >
> >> get rid of |deprecated_public_session_id| quicker?
> >>
> >
> >  >
> >> > But please see my comments on the protobuf enum below.
> >> >
> >> >
> >> >
> >> > On 2013/04/30 23:37:37, Mattias Nissler wrote:
> >> > > On 2013/04/30 22:01:10, binzhao wrote:
> >> > > > My understanding is that the old client has no knowledge of
> >>
> > account type,
> >
> >> > then
> >> > > > it only looks at the id field and it means a public session.
> >> > > >
> >> > > > For new client, if account type is not set, then it assumes it
> >>
> > is a public
> >
> >> > > > session and read it from id field. If type is set, then it
> >>
> > honors the type
> >
> >> > and
> >> > > > read id field.
> >> > > >
> >> > > > Not sure why we need to deprecate this and have an new field
> >>
> > then.
> >
> >> > >
> >> > > The issue is forward-compatibility: IMHO, old clients should
> >>
> > ignore new
> >
> >> > account
> >> > > types instead of creating Public Sessions for accounts with types
> >>
> > they can't
> >
> >> > > handle. If unknown types default to be Public Sessions on old
> >>
> > clients, we
> >
> >> run
> >> > > into these problems:
> >> > > - We'll add unintentional Public Sessions, which will confuse
> >>
> > users. Most
> >
> >> > > likely, the policy configuration for these accounts will be
> >> > missing/incomplete,
> >> > > so the Public Sessions will have cryptic account names etc. which
> >>
> > adds to
> >
> >> the
> >> > > confusion.
> >> > > - Once an old client actually upgrades, it starts understanding
> >>
> > the new
> >
> >> > account
> >> > > type. But then it may already have accumulated state for an
> >>
> > account that was
> >
> >> > > incorrectly defaulted to be a Public Session, so there now is
> >>
> > inconsistent
> >
> >> > > state.
> >> > > - If we default to Public Session, we'll also try to fetch Public
> >>
> > Session
> >
> >> > > policy. Currently, there is no plan to support this for Kiosk
> >>
> > Apps, so old
> >
> >> > > clients would make unnecessary requests to the server (they'd be
> >>
> > otherwise
> >
> >> > > harmless).
> >> > >
> >> > > The only option I see that avoids deprecating the existing field
> >>
> > is to make
> >
> >> a
> >> > > rule that we can only switch on support for new account types once
> >>
> > we're
> >
> >> sure
> >> > > all old clients have been updated to understand the new type.
> >>
> > That's quite
> >
> >> > > inflexible however, as it wouldn't allow us to test/release new
> >>
> > features
> >
> >> > before
> >> > > the client-side implementation hits the stable channel, and even
> >>
> > if that's
> >
> >> the
> >> > > case, there might be stray devices that may take longer to update
> >>
> > and thus
> >
> >> run
> >> > > into the issues I mention above.
> >> >
> >>
> >
> >
> > https://codereview.chromium.**org/14306004/diff/102004/**
> > chrome/browser/policy/proto/**chromeos/chrome_device_policy.**
> >
> proto#newcode251<<crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">https://codereview.chromium.org/14306004/</crxref>diff/102004/chrome/browser/policy/proto/chromeos/chrome_device_policy.proto#newcode251>
> > chrome/browser/policy/proto/**chromeos/chrome_device_policy.**proto:251:
> > optional AccountType type = 3;
> > Right, if you check hasXXX(), then it is fine. If we don't check
> > hasXXX(), then we can put a UNKNOWN.
> >
> >
> > On 2013/05/01 10:10:55, Mattias Nissler wrote:
> >
> >> On 2013/05/01 00:30:44, binzhao wrote:
> >> > Is Chromium using proto1 or proto2?
> >> >
> >> > I am not very familiar with Protobuf c++. There are some known
> >>
> > issues with
> >
> >> > Protobuf enum in Java. If we add another enum which the old client
> >>
> > does not
> >
> >> > recognize, when the old client call getType(), it gets the default
> >>
> > value. If
> >
> >> not
> >> > default value is specified, then the first value in the enum list is
> >>
> > returned.
> >
> >  If read the code and tested it against a proto containing an enum
> >>
> > value the
> >
> >> client doesn't know. It all works out because the enum parsing code in
> >>
> > protobuf
> >
> >> checks whether the value it gets from the binary message is a valid
> >>
> > enum
> >
> >> constant. If not, it just skips the field and has_type() returns
> >>
> > false.
> >
> >  >
> >> > As far as I remember, a good practice is defining UNKNOWN to act as
> >>
> > a default
> >
> >> > value.
> >>
> >
> >  I actually had that in an earlier (unpublished) iteration and then
> >>
> > removed it
> >
> >> because I didn't need it to get what I wanted :)
> >>
> >
> >  >
> >> > These two threads have some details
> >> >
> >> >
> >>
> >
> > https://groups.google.com/a/**google.com/forum/?fromgroups=#**
> >
>
%2521msg/protobuf-team/**jVTTjRBwX-4/8e-Vn-GBwuQJ<https://groups.google.com/a/google.com/forum/?fromgroups=#%2521msg/protobuf-team/jVTTjRBwX-4/8e-Vn-GBwuQJ>
> >
> >> >
> >> >
> >>
> >
> > https://groups.google.com/a/**google.com/forum/?fromgroups=#**
> > %2521searchin/protobuf-**discuss/proto2%252420java%**
> >
>
252420enums/protobuf-discuss/**w_qJ5cE9CaU/N0eWgELaRC4J<https://groups.google.com/a/google.com/forum/?fromgroups=#%2521searchin/protobuf-discuss/proto2%252420java%252420enums/protobuf-discuss/w_qJ5cE9CaU/N0eWgELaRC4J>
> >
> >
> >  Judging from these threads, has_type() is actually a supported way of
> >>
> > covering
> >
> >> this case for proto2. I don't see any reason why we'd need to care
> >>
> > about proto1,
> >
> >> do you?
> >>
> >
> >  >
> >> > On 2013/04/30 23:37:37, Mattias Nissler wrote:
> >> > > On 2013/04/30 22:01:10, binzhao wrote:
> >> > > > we can specify the default to be ACCOUNT_TYPE_PUBLIC_SESSION
> >>
> > then we don't
> >
> >> > > need
> >> > > > to add a new field.
> >> > >
> >> > > See my comment above.
> >> >
> >>
> >
> >
> >
>
https://codereview.chromium.**org/14306004/%3Chttps://codereview.chromium.org...>
> >
> 
> 
> 
> -- 
> regards,
> Bin

[Mattias Nissler (ping if slow), 2013-05-02 11:09:00]

Committed patchset #17 manually as r197892 (presubmit successful).