Remove retail mode.

Remove retail mode.

Now that we are using public accounts for our retail experience, we should get
rid of all the retail mode code which is no longer used.

Owner reviews requested,
nkostylev@ - login
pneubeck@ - enterprise
derat@ - ash
asargent@ - extensions
sky@ - c/b/browser_resources.grd and c/b/ui/startup

R=bartfab@chromium.org, derat@chromium.org, nkostylev@chromium.org
BUG=254840
Committed: https://crrev.com/b526cd6b9e2a79f8b7f1b8e2ceeb10e5f46cf787
Cr-Commit-Position: refs/heads/master@{#309022}

[Daniel Erat, 2014-09-29 20:49:24]

nice! lgtm for ash

[rkc, 2014-09-29 21:55:36]

rkc@chromium.org changed reviewers:
+ asargent@chromium.org, pneubeck@chromium.org, sky@chromium.org

[rkc, 2014-09-29 21:55:37]

Looks like bartfab@ is OOO. Adding pneubeck@ instead for the enterprise owners
review.

Also adding asargent@ for c/b/e and sky@ for c/b/browser_resources.grd and
c/b/ui/startup.

[asargent_no_longer_on_chrome, 2014-09-29 22:26:32]

c/b/e lgtm

[sky, 2014-09-29 23:43:22]

LGTM

[rkc, 2014-10-01 06:49:01]

Still need reviews from Nikita and Phillipp. Ping :)

[Nikita (slow), 2014-10-01 07:12:49]

On 2014/10/01 06:49:01, Rahul Chaturvedi wrote:
> Still need reviews from Nikita and Phillipp. Ping :)

Sorry, I'm OOO these 2 days.

lgtm with nits

[Nikita (slow), 2014-10-01 07:13:01]

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/existing_user_controller.cc (left):

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:1145: // Skip the
default first-run behavior for public accounts.
nit: Please leave public session comment.

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/lock/screen_locker.cc:384: // For a demo user, we
should never show the lock screen (crosbug.com/27647).
nit: updated comment

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/lock/screen_locker.cc:423: // For a guest/demo
user, screen_locker_ would have never been initialized.
nit: update comment

https://codereview.chromium.org/608283003/diff/40001/chromeos/login/user_name...
File chromeos/login/user_names.cc (left):

https://codereview.chromium.org/608283003/diff/40001/chromeos/login/user_name...
chromeos/login/user_names.cc:21: const char* kRetailModeUserName = "demouser@";
Please don't forget to cleanup on session_manager side.

https://codereview.chromium.org/608283003/diff/40001/components/user_manager/...
File components/user_manager/user_type.h (right):

https://codereview.chromium.org/608283003/diff/40001/components/user_manager/...
components/user_manager/user_type.h:18: USER_TYPE_PUBLIC_ACCOUNT = 2,
Please leave constant values as is. See above "Used in a histogram; do not
modify existing types."

Something like
/* USER_TYPE_RETAIL_MODE = 2, // deprecated */

[Nikita (slow), 2014-10-01 07:13:22]

thanks for this cleanup!

[rkc, 2014-10-01 17:29:51]

rkc@chromium.org changed reviewers:
+ atwilson@chromium.org

[rkc, 2014-10-01 17:29:52]

Adding atwilson@ for Enterprise review.

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/existing_user_controller.cc (left):

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:1145: // Skip the
default first-run behavior for public accounts.
On 2014/10/01 07:13:01, Nikita Kostylev wrote:
> nit: Please leave public session comment.

Done.

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/lock/screen_locker.cc:384: // For a demo user, we
should never show the lock screen (crosbug.com/27647).
On 2014/10/01 07:13:01, Nikita Kostylev wrote:
> nit: updated comment

Done.

https://codereview.chromium.org/608283003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/lock/screen_locker.cc:423: // For a guest/demo
user, screen_locker_ would have never been initialized.
On 2014/10/01 07:13:01, Nikita Kostylev wrote:
> nit: update comment

Done.

https://codereview.chromium.org/608283003/diff/40001/chromeos/login/user_name...
File chromeos/login/user_names.cc (left):

https://codereview.chromium.org/608283003/diff/40001/chromeos/login/user_name...
chromeos/login/user_names.cc:21: const char* kRetailModeUserName = "demouser@";
On 2014/10/01 07:13:01, Nikita Kostylev wrote:
> Please don't forget to cleanup on session_manager side.

Yep. That's my very next TODO after this CL lands.

https://codereview.chromium.org/608283003/diff/40001/components/user_manager/...
File components/user_manager/user_type.h (right):

https://codereview.chromium.org/608283003/diff/40001/components/user_manager/...
components/user_manager/user_type.h:18: USER_TYPE_PUBLIC_ACCOUNT = 2,
On 2014/10/01 07:13:01, Nikita Kostylev wrote:
> Please leave constant values as is. See above "Used in a histogram; do not
> modify existing types."
> 
> Something like
> /* USER_TYPE_RETAIL_MODE = 2, // deprecated */

Done.

[Andrew T Wilson (Slow), 2014-10-02 09:54:54]

If we're removing retail mode, should we be removing the retail-mode policies
from policy_templates.json (or at least marking them as deprecated/obsolete)?

[Mattias Nissler (ping if slow), 2014-10-02 09:56:46]

mnissler@chromium.org changed reviewers:
+ mnissler@chromium.org

[Mattias Nissler (ping if slow), 2014-10-02 09:56:47]

Drive-by review.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.h
(left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.h:81: class
PinnedLauncherAppsPolicyHandler
Please keep this, it's also used for regular sessions.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enterprise_install_attributes.cc:352:
NOTREACHED() << "Unknown device mode string: " << mode;
We'll now hit the NOTREACHED() here for devices already enrolled into retail
mode. The code should handle that in a more graceful way.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:600: optional
ScreenSaverProto login_screen_saver = 16;
Please deprecate these fields properly (to avoid accidental reuse later), see
OBSOLETE_online_only above for an example.

[bartfab (slow), 2014-10-15 09:52:22]

https://codereview.chromium.org/608283003/diff/60001/ash/shell/window_type_la...
File ash/shell/window_type_launcher.cc (right):

https://codereview.chromium.org/608283003/diff/60001/ash/shell/window_type_la...
ash/shell/window_type_launcher.cc:19: #include "base/bind.h"
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/ash/shell/window_type_la...
ash/shell/window_type_launcher.cc:21: #include "base/time/time.h"
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/ash/shell/window_type_la...
ash/shell/window_type_launcher.cc:22: #include
"content/public/browser/browser_thread.h"
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:559:
retail_mode_power_save_blocker_ = content::PowerSaveBlocker::Create(
Remove the |retail_mode_power_save_blocker_| member defined in the header file
as well.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:112: #include
"content/public/browser/power_save_blocker.h"
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/auth/login_performer.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/auth/login_performer.cc:91:
UserMetricsAction("Login_DemoUserLoginSuccess"));
Should this action be removed from |tools/metrics/actions/actions.xml| as well
now?

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:26: #include
"chromeos/dbus/session_manager_client.h"
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/existing_user_controller.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:491:
l10n_util::GetStringUTF8(IDS_CHROMEOS_ACC_LOGIN_SIGNIN_DEMOUSER));
Remove IDS_CHROMEOS_ACC_LOGIN_SIGNIN_DEMOUSER from chromeos_strings.grdp as
well.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:56: #include
"chromeos/settings/cros_settings_names.h"
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:16: #include
"chrome/browser/chromeos/policy/enterprise_install_attributes.h"
Nit: No longer needed.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:278:
EnterpriseInstallAttributes* install_attributes) {
Nit: No longer needed.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:697:
EnterpriseInstallAttributes* install_attributes) {
Nit: No longer needed.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.cc
(right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.cc:8:
#include "chrome/browser/lifetime/application_lifetime.h"
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:54: kAppPack,
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:58:
kIdleLogoutTimeout,
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:59:
kIdleLogoutWarningDuration,
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:69:
kScreenSaverExtensionId,
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:70:
kScreenSaverTimeout,
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:681: //   kAppPack
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:684: //  
kIdleLogoutTimeout
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:685: //  
kIdleLogoutWarningDuration
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:693: //  
kScreenSaverExtensionId
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:694: //  
kScreenSaverTimeout
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/external_provider_impl.cc:47: #include
"components/user_manager/user_manager.h"
Nit: No longer used.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:652: new
PinnedLauncherAppsPolicyHandler()));
This should stay.

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

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:347: return
ash::user::LOGGED_IN_RETAIL_MODE;
Remove this constant from ash/system/user/login_status.h as well.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:414:
builder->Add("demoLoginMessage", IDS_KIOSK_MODE_LOGIN_MESSAGE);
Remove IDS_KIOSK_MODE_LOGIN_MESSAGE from chrome/app/chromeos_strings.grdp as
well.

https://codereview.chromium.org/608283003/diff/60001/chromeos/settings/cros_s...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chromeos/settings/cros_s...
chromeos/settings/cros_settings_names.cc:96: // A list of dictionaries, each
detailing one extension to install as part of
Nit: These are no longer used.

https://codereview.chromium.org/608283003/diff/60001/chromeos/settings/cros_s...
chromeos/settings/cros_settings_names.cc:105: // Values from the ScreenSaver
proto. Defines the extension ID of the screen
Nit: These are no longer used.

https://codereview.chromium.org/608283003/diff/60001/chromeos/settings/cros_s...
chromeos/settings/cros_settings_names.cc:110: // Values from the
ForcedLogoutTimeouts proto. Defines the timeouts before a
Nit: These are no longer used.

https://codereview.chromium.org/608283003/diff/60001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/608283003/diff/60001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.cc:29: return
DEVICE_MODE_NOT_SET;
We will now reach this line on retail mode devices. Did you check how they will
behave? Will they suddenly think they are no longer enterprise owned and allow a
local user to take ownership? That would be bad.

https://codereview.chromium.org/608283003/diff/60001/components/user_manager/...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/608283003/diff/60001/components/user_manager/...
components/user_manager/user_manager_base.cc:591: // Data belonging to the guest
and stub users is always
Nit: The entire comment fits on one line now.

[rkc, 2014-11-20 21:06:30]

rkc@chromium.org changed reviewers:
+ zelidrag@chromium.org

[rkc, 2014-11-20 21:06:35]

Also updated the code so now a device enrolled in legacy retail mode will launch
the offline demo mode.

Nikita, could you look at the changes since patch set 4 also?

Bartosz, please do let me know if it looks like I am doing things wrong with the
policy code. I am leaving the device mode code in, just renaming the device mode
enum to correctly reflect that the device is now in a 'legacy' retail mode.

https://codereview.chromium.org/608283003/diff/60001/ash/shell/window_type_la...
File ash/shell/window_type_launcher.cc (right):

https://codereview.chromium.org/608283003/diff/60001/ash/shell/window_type_la...
ash/shell/window_type_launcher.cc:19: #include "base/bind.h"
On 2014/10/15 09:52:19, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/ash/shell/window_type_la...
ash/shell/window_type_launcher.cc:21: #include "base/time/time.h"
On 2014/10/15 09:52:19, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/ash/shell/window_type_la...
ash/shell/window_type_launcher.cc:22: #include
"content/public/browser/browser_thread.h"
On 2014/10/15 09:52:19, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:559:
retail_mode_power_save_blocker_ = content::PowerSaveBlocker::Create(
On 2014/10/15 09:52:19, bartfab wrote:
> Remove the |retail_mode_power_save_blocker_| member defined in the header file
> as well.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:112: #include
"content/public/browser/power_save_blocker.h"
On 2014/10/15 09:52:20, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/auth/login_performer.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/auth/login_performer.cc:91:
UserMetricsAction("Login_DemoUserLoginSuccess"));
On 2014/10/15 09:52:20, bartfab wrote:
> Should this action be removed from |tools/metrics/actions/actions.xml| as well
> now?

Marked it obsolete.
Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:26: #include
"chromeos/dbus/session_manager_client.h"
On 2014/10/15 09:52:20, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/existing_user_controller.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:491:
l10n_util::GetStringUTF8(IDS_CHROMEOS_ACC_LOGIN_SIGNIN_DEMOUSER));
On 2014/10/15 09:52:20, bartfab wrote:
> Remove IDS_CHROMEOS_ACC_LOGIN_SIGNIN_DEMOUSER from chromeos_strings.grdp as
> well.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:56: #include
"chromeos/settings/cros_settings_names.h"
On 2014/10/15 09:52:20, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.h
(left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.h:81: class
PinnedLauncherAppsPolicyHandler
On 2014/10/02 09:56:47, Mattias Nissler wrote:
> Please keep this, it's also used for regular sessions.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:16: #include
"chrome/browser/chromeos/policy/enterprise_install_attributes.h"
On 2014/10/15 09:52:20, bartfab wrote:
> Nit: No longer needed.

Replied below.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:278:
EnterpriseInstallAttributes* install_attributes) {
On 2014/10/15 09:52:20, bartfab wrote:
> Nit: No longer needed.

Since DEVICE_MODE_RETAIL_KIOSK will still exist (and used to start up the
Offline Demo MOde), should we still leave the code below still in? I don't know
enough about the policy code to know if it is still relevant.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:697:
EnterpriseInstallAttributes* install_attributes) {
On 2014/10/15 09:52:20, bartfab wrote:
> Nit: No longer needed.

As above.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/enterprise_install_attributes.cc:352:
NOTREACHED() << "Unknown device mode string: " << mode;
On 2014/10/02 09:56:47, Mattias Nissler wrote:
> We'll now hit the NOTREACHED() here for devices already enrolled into retail
> mode. The code should handle that in a more graceful way.

Changed this code. We will now handle devices currently in retail mode by
running the offline demo mode instead.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:600: optional
ScreenSaverProto login_screen_saver = 16;
On 2014/10/02 09:56:47, Mattias Nissler wrote:
> Please deprecate these fields properly (to avoid accidental reuse later), see
> OBSOLETE_online_only above for an example.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.cc
(right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/session_state_controller_delegate_chromeos.cc:8:
#include "chrome/browser/lifetime/application_lifetime.h"
On 2014/10/15 09:52:20, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:54: kAppPack,
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:58:
kIdleLogoutTimeout,
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:59:
kIdleLogoutWarningDuration,
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:69:
kScreenSaverExtensionId,
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:70:
kScreenSaverTimeout,
On 2014/10/15 09:52:20, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:681: //   kAppPack
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:684: //  
kIdleLogoutTimeout
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:685: //  
kIdleLogoutWarningDuration
On 2014/10/15 09:52:20, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:693: //  
kScreenSaverExtensionId
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:694: //  
kScreenSaverTimeout
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/external_provider_impl.cc:47: #include
"components/user_manager/user_manager.h"
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:652: new
PinnedLauncherAppsPolicyHandler()));
On 2014/10/15 09:52:21, bartfab wrote:
> This should stay.

Done.

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

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:347: return
ash::user::LOGGED_IN_RETAIL_MODE;
On 2014/10/15 09:52:21, bartfab wrote:
> Remove this constant from ash/system/user/login_status.h as well.

Done.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (left):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:414:
builder->Add("demoLoginMessage", IDS_KIOSK_MODE_LOGIN_MESSAGE);
On 2014/10/15 09:52:21, bartfab wrote:
> Remove IDS_KIOSK_MODE_LOGIN_MESSAGE from chrome/app/chromeos_strings.grdp as
> well.

Done.

https://codereview.chromium.org/608283003/diff/60001/chromeos/settings/cros_s...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chromeos/settings/cros_s...
chromeos/settings/cros_settings_names.cc:96: // A list of dictionaries, each
detailing one extension to install as part of
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: These are no longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chromeos/settings/cros_s...
chromeos/settings/cros_settings_names.cc:105: // Values from the ScreenSaver
proto. Defines the extension ID of the screen
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: These are no longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/chromeos/settings/cros_s...
chromeos/settings/cros_settings_names.cc:110: // Values from the
ForcedLogoutTimeouts proto. Defines the timeouts before a
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: These are no longer used.

Done.

https://codereview.chromium.org/608283003/diff/60001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/608283003/diff/60001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.cc:29: return
DEVICE_MODE_NOT_SET;
On 2014/10/15 09:52:21, bartfab wrote:
> We will now reach this line on retail mode devices. Did you check how they
will
> behave? Will they suddenly think they are no longer enterprise owned and allow
a
> local user to take ownership? That would be bad.

Leaving this code as is. We aren't removing the device mode itself, since we
need to detect that to ensure we launch the offline demo mode instead.

https://codereview.chromium.org/608283003/diff/60001/components/user_manager/...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/608283003/diff/60001/components/user_manager/...
components/user_manager/user_manager_base.cc:591: // Data belonging to the guest
and stub users is always
On 2014/10/15 09:52:21, bartfab wrote:
> Nit: The entire comment fits on one line now.

Done.

[Nikita (slow), 2014-11-21 11:27:48]

On 2014/11/20 21:06:35, Rahul Chaturvedi wrote:
> Also updated the code so now a device enrolled in legacy retail mode will
launch
> the offline demo mode.
> 
> Nikita, could you look at the changes since patch set 4 also?

Rahul, I've opened all diffs between patch sets 4 and 6 and there're a lot of
changes mostly because of rebase.
Can you please point to the specific files I should look into?

[rkc, 2014-11-21 17:22:18]

On 2014/11/21 11:27:48, Nikita wrote:
> On 2014/11/20 21:06:35, Rahul Chaturvedi wrote:
> > Also updated the code so now a device enrolled in legacy retail mode will
> launch
> > the offline demo mode.
> > 
> > Nikita, could you look at the changes since patch set 4 also?
> 
> Rahul, I've opened all diffs between patch sets 4 and 6 and there're a lot of
> changes mostly because of rebase.
> Can you please point to the specific files I should look into?

demo_app_launcher.cc and oobe_ui.cc
The rest are mostly changes for the rebase - a lot of code had changed in the
few weeks that I didn't update the CL.

[Nikita (slow), 2014-11-26 12:49:47]

On 2014/11/21 17:22:18, Rahul Chaturvedi wrote:
> On 2014/11/21 11:27:48, Nikita wrote:
> > On 2014/11/20 21:06:35, Rahul Chaturvedi wrote:
> > > Also updated the code so now a device enrolled in legacy retail mode will
> > launch
> > > the offline demo mode.
> > > 
> > > Nikita, could you look at the changes since patch set 4 also?
> > 
> > Rahul, I've opened all diffs between patch sets 4 and 6 and there're a lot
of
> > changes mostly because of rebase.
> > Can you please point to the specific files I should look into?
> 
> demo_app_launcher.cc and oobe_ui.cc
> The rest are mostly changes for the rebase - a lot of code had changed in the
> few weeks that I didn't update the CL.

lgtm

[bartfab (slow), 2014-11-27 16:25:48]

Awesome to see this picking up steam again.

I think we are almost there now but I strongly object to the introduction of a
new singleton where none is needed.

To make sure this is not missed, not lgtm until LegacyRetailModeDetector is
removed. Once that singleton is gone, I think we can land the CL.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:278:
EnterpriseInstallAttributes* install_attributes) {
On 2014/11/20 21:06:33, Rahul Chaturvedi wrote:
> On 2014/10/15 09:52:20, bartfab wrote:
> > Nit: No longer needed.
> 
> Since DEVICE_MODE_RETAIL_KIOSK will still exist (and used to start up the
> Offline Demo MOde), should we still leave the code below still in? I don't
know
> enough about the policy code to know if it is still relevant.

If I understand "offline demo mode" correctly, it should be entirely offline.
Hence, the proxy is irrelevant and we can remove the special case handling we
had for retail mode.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/legacy_retail_mode_detector.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.cc:52: } else if
(device_mode == policy::DEVICE_MODE_PENDING) {
Handling policy::DEVICE_MODE_PENDING is unnecessary. The
EnterpriseInstallAttributes class was recently rewritten so that it always
synchronously loads the mode on startup. You can rely on the mode being
available straight away.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/legacy_retail_mode_detector.h (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.h:23: // This method checks
if Kiosk Mode is enabled or not.
What kiosk mode is this talking about? We have a Chrome OS kiosk mode but that
is configured through policy and is used to run a single app in full screen
mode. I presume you are talking about something different? If so, it should have
a different name.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.h:29: virtual
~LegacyRetailModeDetector();
Nit: Use |override|, not |virtual|.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.h:35: // Makes sure the
browser will switch to kiosk mode if cryptohome was not
As above - "kiosk mode" is not the right term here.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.h:39: bool is_kiosk_mode_;
As above - "kiosk mode" is not the right term here.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:44:
command_line->HasSwitch(switches::kDemoModeAppPath)
Why is this switch being introduced as part of the retail mode removal?

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:422: // For a guest/demo
user, screen_locker_ would have never been initialized.
Nit: s:/demo::

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1017: // Skip the
default first-run behavior for public accounts.
Nit: This comment should not be removed.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enterprise_install_attributes.cc:332: return
EnterpriseInstallAttributes::kRetailKioskDeviceMode;
Nit: The |kRetailKioskDeviceMode| constant should also be renamed to include
|Legacy| in its name.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enterprise_install_attributes_unittest.cc
(right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enterprise_install_attributes_unittest.cc:174:
TEST_F(EnterpriseInstallAttributesTest, GetMode) {
Nit: Can you switch this test to another, non-deprecated, mode?

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:111: optional
string extension_id = 1;
Nit: Add OBSOLETE_ and deprecated = true.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:112: optional
string update_url = 2;
Nit: Add OBSOLETE_ and deprecated = true.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:129: repeated
string app_id = 1;
Nit: Add OBSOLETE_ and deprecated = true.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:137: optional
int64 idle_logout_timeout = 1;
Nit: Add OBSOLETE_ and deprecated = true.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:142: optional
int64 idle_logout_warning_duration = 2;
Nit: Add OBSOLETE_ and deprecated = true.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:149: optional
string screen_saver_extension_id = 1;
Nit: Add OBSOLETE_ and deprecated = true.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:154: optional
int64 screen_saver_timeout = 2;
Nit: Add OBSOLETE_ and deprecated = true.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:232: repeated
string start_up_urls = 1;
Nit: Add OBSOLETE_ and deprecated = true.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:554: if
(chromeos::LegacyRetailModeDetector::Get()->IsKioskModeEnabled()) {
There is no need to introduce a singleton here. Just query the device mode at
this point.

https://codereview.chromium.org/608283003/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_constants.h (right):

https://codereview.chromium.org/608283003/diff/100001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_constants.h:121: // mode so
that if a device has still been
Nit: s/a device has still been left/an existing device is still enrolled/

[rkc, 2014-12-01 19:15:05]

The only reason I added the legacy retail mode detector was because I thought we
still needed to fetch the device policy async. If we don't need to do that, I
completely agree, the whole class is completely unneeded, hence removed.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:278:
EnterpriseInstallAttributes* install_attributes) {
On 2014/11/27 16:25:47, bartfab wrote:
> On 2014/11/20 21:06:33, Rahul Chaturvedi wrote:
> > On 2014/10/15 09:52:20, bartfab wrote:
> > > Nit: No longer needed.
> > 
> > Since DEVICE_MODE_RETAIL_KIOSK will still exist (and used to start up the
> > Offline Demo MOde), should we still leave the code below still in? I don't
> know
> > enough about the policy code to know if it is still relevant.
> 
> If I understand "offline demo mode" correctly, it should be entirely offline.
> Hence, the proxy is irrelevant and we can remove the special case handling we
> had for retail mode.

Ah - I honestly don't completely understand this code, hence am relying on you
to correct me :)
Changed this to no longer look at install attributes and check for the device
mode when setting the proxy settings policy (I am presuming that with the retail
mode code gone, we'll if the proxy settings are empty, we'll just set the policy
level to POLICY_LEVEL_RECOMMENDED?).

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/legacy_retail_mode_detector.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.cc:52: } else if
(device_mode == policy::DEVICE_MODE_PENDING) {
On 2014/11/27 16:25:47, bartfab wrote:
> Handling policy::DEVICE_MODE_PENDING is unnecessary. The
> EnterpriseInstallAttributes class was recently rewritten so that it always
> synchronously loads the mode on startup. You can rely on the mode being
> available straight away.

Acknowledged.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/legacy_retail_mode_detector.h (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.h:23: // This method checks
if Kiosk Mode is enabled or not.
On 2014/11/27 16:25:47, bartfab wrote:
> What kiosk mode is this talking about? We have a Chrome OS kiosk mode but that
> is configured through policy and is used to run a single app in full screen
> mode. I presume you are talking about something different? If so, it should
have
> a different name.

This entire file is gone.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.h:29: virtual
~LegacyRetailModeDetector();
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: Use |override|, not |virtual|.

Acknowledged.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.h:35: // Makes sure the
browser will switch to kiosk mode if cryptohome was not
On 2014/11/27 16:25:47, bartfab wrote:
> As above - "kiosk mode" is not the right term here.

Acknowledged.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/legacy_retail_mode_detector.h:39: bool is_kiosk_mode_;
On 2014/11/27 16:25:47, bartfab wrote:
> As above - "kiosk mode" is not the right term here.

Acknowledged.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:44:
command_line->HasSwitch(switches::kDemoModeAppPath)
On 2014/11/27 16:25:47, bartfab wrote:
> Why is this switch being introduced as part of the retail mode removal?

It helps in testing this code. It doesn't need to be part of this CL though.
Removed.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:422: // For a guest/demo
user, screen_locker_ would have never been initialized.
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: s:/demo::

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enterprise_install_attributes.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enterprise_install_attributes.cc:332: return
EnterpriseInstallAttributes::kRetailKioskDeviceMode;
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: The |kRetailKioskDeviceMode| constant should also be renamed to include
> |Legacy| in its name.

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enterprise_install_attributes_unittest.cc
(right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enterprise_install_attributes_unittest.cc:174:
TEST_F(EnterpriseInstallAttributesTest, GetMode) {
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: Can you switch this test to another, non-deprecated, mode?

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:111: optional
string extension_id = 1;
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: Add OBSOLETE_ and deprecated = true.

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:112: optional
string update_url = 2;
On 2014/11/27 16:25:48, bartfab wrote:
> Nit: Add OBSOLETE_ and deprecated = true.

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:129: repeated
string app_id = 1;
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: Add OBSOLETE_ and deprecated = true.

I thought that Mattias mentioned that this is still used elsewhere? That is why
I left this one alone.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:137: optional
int64 idle_logout_timeout = 1;
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: Add OBSOLETE_ and deprecated = true.

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:142: optional
int64 idle_logout_warning_duration = 2;
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: Add OBSOLETE_ and deprecated = true.

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:149: optional
string screen_saver_extension_id = 1;
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: Add OBSOLETE_ and deprecated = true.

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:154: optional
int64 screen_saver_timeout = 2;
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: Add OBSOLETE_ and deprecated = true.

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:232: repeated
string start_up_urls = 1;
On 2014/11/27 16:25:47, bartfab wrote:
> Nit: Add OBSOLETE_ and deprecated = true.

Done.

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

https://codereview.chromium.org/608283003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:554: if
(chromeos::LegacyRetailModeDetector::Get()->IsKioskModeEnabled()) {
On 2014/11/27 16:25:48, bartfab wrote:
> There is no need to introduce a singleton here. Just query the device mode at
> this point.

Done.

https://codereview.chromium.org/608283003/diff/100001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_constants.h (right):

https://codereview.chromium.org/608283003/diff/100001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_constants.h:121: // mode so
that if a device has still been
On 2014/11/27 16:25:48, bartfab wrote:
> Nit: s/a device has still been left/an existing device is still enrolled/

Done.

[bartfab (slow), 2014-12-02 21:25:27]

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:278:
EnterpriseInstallAttributes* install_attributes) {
On 2014/12/01 19:15:04, Rahul Chaturvedi wrote:
> On 2014/11/27 16:25:47, bartfab wrote:
> > On 2014/11/20 21:06:33, Rahul Chaturvedi wrote:
> > > On 2014/10/15 09:52:20, bartfab wrote:
> > > > Nit: No longer needed.
> > > 
> > > Since DEVICE_MODE_RETAIL_KIOSK will still exist (and used to start up the
> > > Offline Demo MOde), should we still leave the code below still in? I don't
> > know
> > > enough about the policy code to know if it is still relevant.
> > 
> > If I understand "offline demo mode" correctly, it should be entirely
offline.
> > Hence, the proxy is irrelevant and we can remove the special case handling
we
> > had for retail mode.
> 
> Ah - I honestly don't completely understand this code, hence am relying on you
> to correct me :)
> Changed this to no longer look at install attributes and check for the device
> mode when setting the proxy settings policy (I am presuming that with the
retail
> mode code gone, we'll if the proxy settings are empty, we'll just set the
policy
> level to POLICY_LEVEL_RECOMMENDED?).

Proxy settings are documented as a mandatory user policy only. I do not
understand why we decode it as a recommended device policy here. I started a
thread to discuss this and CC'd you on it.

https://codereview.chromium.org/608283003/diff/120001/ash/system/user/user_ca...
File ash/system/user/user_card_view.cc (right):

https://codereview.chromium.org/608283003/diff/120001/ash/system/user/user_ca...
ash/system/user/user_card_view.cc:342: switch (login_status) {
Nit: This can become a simple if/else now.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (left):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:603:
extension_system_event_observer_.reset(new ExtensionSystemEventObserver());
Why are you removing the |extension_system_event_observer_|?

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:741:
ScreenLocker::ShutDownClass();
Why is this no longer necessary?

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1020: // Skip the
default first-run behavior for public accounts.
Nit: Leave this comment in place please.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:299:
POLICY_SCOPE_MACHINE, proxy_settings.release(), NULL);
Nit: Now that you are touching this line, s/NULL/nullptr/

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:127: message
PinnedAppsProto {
Nit: Add OBSOLETE_.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:129: repeated
string app_id = 1;
Nit: Add OBSOLETE_ and deprecated = true.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:607: optional
PinnedAppsProto pinned_apps = 19 [deprecated = true];
Nit: Add OBSOLETE_.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:575: if (g_browser_process) {
When do we run into |g_browser_process == nullptr|? In some unit tests? If so,
those tests should have a TestingBrowserProcess.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:578: if
(connector->GetDeviceMode() == policy::DEVICE_MODE_LEGACY_RETAIL_MODE) {
Nit: #include "components/policy/core/common/cloud/cloud_policy_constants.h"

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:581: LoginDisplayHost* host =
LoginDisplayHostImpl::default_host();
Nit 1: #include "chrome/browser/chromeos/login/ui/login_display_host.h"
Nit 2: #include "chrome/browser/chromeos/login/ui/login_display_host_impl.h"

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:583: }
Return.

https://codereview.chromium.org/608283003/diff/120001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/608283003/diff/120001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:29: const char kDemoModeAppPath[] =
"demo-mode-app-path";
Nit: This belongs into a separate CL.

https://codereview.chromium.org/608283003/diff/120001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_constants.h (right):

https://codereview.chromium.org/608283003/diff/120001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_constants.h:122:
DEVICE_MODE_LEGACY_RETAIL_MODE,  // The device is enrolled as retail kiosk
Nit: s/as/as a/

[rkc, 2014-12-04 19:50:04]

rkc@chromium.org changed reviewers:
+ isherman@chromium.org

[rkc, 2014-12-04 19:50:07]

Also adding Ilya for tools/metrics/actions/actions.xml owners review.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:278:
EnterpriseInstallAttributes* install_attributes) {
On 2014/12/02 21:25:26, bartfab wrote:
> On 2014/12/01 19:15:04, Rahul Chaturvedi wrote:
> > On 2014/11/27 16:25:47, bartfab wrote:
> > > On 2014/11/20 21:06:33, Rahul Chaturvedi wrote:
> > > > On 2014/10/15 09:52:20, bartfab wrote:
> > > > > Nit: No longer needed.
> > > > 
> > > > Since DEVICE_MODE_RETAIL_KIOSK will still exist (and used to start up
the
> > > > Offline Demo MOde), should we still leave the code below still in? I
don't
> > > know
> > > > enough about the policy code to know if it is still relevant.
> > > 
> > > If I understand "offline demo mode" correctly, it should be entirely
> offline.
> > > Hence, the proxy is irrelevant and we can remove the special case handling
> we
> > > had for retail mode.
> > 
> > Ah - I honestly don't completely understand this code, hence am relying on
you
> > to correct me :)
> > Changed this to no longer look at install attributes and check for the
device
> > mode when setting the proxy settings policy (I am presuming that with the
> retail
> > mode code gone, we'll if the proxy settings are empty, we'll just set the
> policy
> > level to POLICY_LEVEL_RECOMMENDED?).
> 
> Proxy settings are documented as a mandatory user policy only. I do not
> understand why we decode it as a recommended device policy here. I started a
> thread to discuss this and CC'd you on it.

I'll leave this behavior as is for now? We can always create another CL to
remove this once we have a definitive answer on the thread.

https://codereview.chromium.org/608283003/diff/120001/ash/system/user/user_ca...
File ash/system/user/user_card_view.cc (right):

https://codereview.chromium.org/608283003/diff/120001/ash/system/user/user_ca...
ash/system/user/user_card_view.cc:342: switch (login_status) {
On 2014/12/02 21:25:26, bartfab wrote:
> Nit: This can become a simple if/else now.

Done.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (left):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:603:
extension_system_event_observer_.reset(new ExtensionSystemEventObserver());
On 2014/12/02 21:25:26, bartfab wrote:
> Why are you removing the |extension_system_event_observer_|?

It doesn't seem to be used for anything else except the retail mode power save
blocker.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:741:
ScreenLocker::ShutDownClass();
On 2014/12/02 21:25:27, bartfab wrote:
> Why is this no longer necessary?

Whoops, I missed the !
Done.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1020: // Skip the
default first-run behavior for public accounts.
On 2014/12/02 21:25:27, bartfab wrote:
> Nit: Leave this comment in place please.

Done.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:299:
POLICY_SCOPE_MACHINE, proxy_settings.release(), NULL);
On 2014/12/02 21:25:27, bartfab wrote:
> Nit: Now that you are touching this line, s/NULL/nullptr/

Done.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:127: message
PinnedAppsProto {
On 2014/12/02 21:25:27, bartfab wrote:
> Nit: Add OBSOLETE_.

According to Mattias, this is still in use. Something I am missing?

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:129: repeated
string app_id = 1;
On 2014/12/02 21:25:27, bartfab wrote:
> Nit: Add OBSOLETE_ and deprecated = true.

Ditto.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:607: optional
PinnedAppsProto pinned_apps = 19 [deprecated = true];
On 2014/12/02 21:25:27, bartfab wrote:
> Nit: Add OBSOLETE_.

Ditto, and removed the [deprecated = true].

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:575: if (g_browser_process) {
On 2014/12/02 21:25:27, bartfab wrote:
> When do we run into |g_browser_process == nullptr|? In some unit tests? If so,
> those tests should have a TestingBrowserProcess.

Done.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:578: if
(connector->GetDeviceMode() == policy::DEVICE_MODE_LEGACY_RETAIL_MODE) {
On 2014/12/02 21:25:27, bartfab wrote:
> Nit: #include "components/policy/core/common/cloud/cloud_policy_constants.h"

Done.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:581: LoginDisplayHost* host =
LoginDisplayHostImpl::default_host();
On 2014/12/02 21:25:27, bartfab wrote:
> Nit 1: #include "chrome/browser/chromeos/login/ui/login_display_host.h"
> Nit 2: #include "chrome/browser/chromeos/login/ui/login_display_host_impl.h"

Already included.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:583: }
On 2014/12/02 21:25:27, bartfab wrote:
> Return.

Done.

https://codereview.chromium.org/608283003/diff/120001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/608283003/diff/120001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:29: const char kDemoModeAppPath[] =
"demo-mode-app-path";
On 2014/12/02 21:25:27, bartfab wrote:
> Nit: This belongs into a separate CL.

Removed.
Done.

[Ilya Sherman, 2014-12-04 22:39:11]

actions.xml lgtm

[bartfab (slow), 2014-12-10 23:24:39]

Sorry for the lack of replies. I got sick. I will be able to do another
(hopefully final :) review on Monday.

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:278:
EnterpriseInstallAttributes* install_attributes) {
On 2014/12/04 19:50:06, Rahul Chaturvedi wrote:
> On 2014/12/02 21:25:26, bartfab wrote:
> > On 2014/12/01 19:15:04, Rahul Chaturvedi wrote:
> > > On 2014/11/27 16:25:47, bartfab wrote:
> > > > On 2014/11/20 21:06:33, Rahul Chaturvedi wrote:
> > > > > On 2014/10/15 09:52:20, bartfab wrote:
> > > > > > Nit: No longer needed.
> > > > > 
> > > > > Since DEVICE_MODE_RETAIL_KIOSK will still exist (and used to start up
> the
> > > > > Offline Demo MOde), should we still leave the code below still in? I
> don't
> > > > know
> > > > > enough about the policy code to know if it is still relevant.
> > > > 
> > > > If I understand "offline demo mode" correctly, it should be entirely
> > offline.
> > > > Hence, the proxy is irrelevant and we can remove the special case
handling
> > we
> > > > had for retail mode.
> > > 
> > > Ah - I honestly don't completely understand this code, hence am relying on
> you
> > > to correct me :)
> > > Changed this to no longer look at install attributes and check for the
> device
> > > mode when setting the proxy settings policy (I am presuming that with the
> > retail
> > > mode code gone, we'll if the proxy settings are empty, we'll just set the
> > policy
> > > level to POLICY_LEVEL_RECOMMENDED?).
> > 
> > Proxy settings are documented as a mandatory user policy only. I do not
> > understand why we decode it as a recommended device policy here. I started a
> > thread to discuss this and CC'd you on it.
> 
> I'll leave this behavior as is for now? We can always create another CL to
> remove this once we have a definitive answer on the thread.

Please add a TODO and file an M41 bug against me to kill this policy. I verified
that it was used by retail mode domains only. I will remove it once your CL has
landed.

[bartfab (slow), 2014-12-15 16:08:40]

Almost there. Just a couple more nits. Please remember to add a TOTO and file a
bug to remove the device-wide proxy policy.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:127: message
PinnedAppsProto {
On 2014/12/04 19:50:07, Rahul Chaturvedi wrote:
> On 2014/12/02 21:25:27, bartfab wrote:
> > Nit: Add OBSOLETE_.
> 
> According to Mattias, this is still in use. Something I am missing?

You had accidentally removed the implementation of pinned apps used in regular
sessions in an earlier revision of this CL. That is what Mattias was pointing
out. This here is the retail-mode-specific implementation of pinned apps, which
should be marked as obsolete.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:129: repeated
string app_id = 1;
On 2014/12/04 19:50:07, Rahul Chaturvedi wrote:
> On 2014/12/02 21:25:27, bartfab wrote:
> > Nit: Add OBSOLETE_ and deprecated = true.
> 
> Ditto.

As above, this is used for retail mode only.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:607: optional
PinnedAppsProto pinned_apps = 19 [deprecated = true];
On 2014/12/04 19:50:07, Rahul Chaturvedi wrote:
> On 2014/12/02 21:25:27, bartfab wrote:
> > Nit: Add OBSOLETE_.
> 
> Ditto, and removed the [deprecated = true].

As above, this is used for retail mode only.

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (left):

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:19: #include
"chrome/common/chrome_switches.h"
Nit: Yes, this include is unused. But I cannot see how the clean-up is related
to retail mode removal.

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (left):

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:56: #include
"chromeos/settings/cros_settings_names.h"
Why are you removing this?

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc
(right):

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:20: #include
"chrome/browser/chromeos/policy/consumer_enrollment_handler.h"
Nit: Why are you adding this?

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right):

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:27: class
ConsumerEnrollmentHandler;
Nit: Why are you adding this?

[rkc, 2014-12-15 20:45:58]

Should be complete now :)
Thanks for the detailed review!

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/608283003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:278:
EnterpriseInstallAttributes* install_attributes) {
On 2014/12/10 23:24:39, bartfab wrote:
> On 2014/12/04 19:50:06, Rahul Chaturvedi wrote:
> > On 2014/12/02 21:25:26, bartfab wrote:
> > > On 2014/12/01 19:15:04, Rahul Chaturvedi wrote:
> > > > On 2014/11/27 16:25:47, bartfab wrote:
> > > > > On 2014/11/20 21:06:33, Rahul Chaturvedi wrote:
> > > > > > On 2014/10/15 09:52:20, bartfab wrote:
> > > > > > > Nit: No longer needed.
> > > > > > 
> > > > > > Since DEVICE_MODE_RETAIL_KIOSK will still exist (and used to start
up
> > the
> > > > > > Offline Demo MOde), should we still leave the code below still in? I
> > don't
> > > > > know
> > > > > > enough about the policy code to know if it is still relevant.
> > > > > 
> > > > > If I understand "offline demo mode" correctly, it should be entirely
> > > offline.
> > > > > Hence, the proxy is irrelevant and we can remove the special case
> handling
> > > we
> > > > > had for retail mode.
> > > > 
> > > > Ah - I honestly don't completely understand this code, hence am relying
on
> > you
> > > > to correct me :)
> > > > Changed this to no longer look at install attributes and check for the
> > device
> > > > mode when setting the proxy settings policy (I am presuming that with
the
> > > retail
> > > > mode code gone, we'll if the proxy settings are empty, we'll just set
the
> > > policy
> > > > level to POLICY_LEVEL_RECOMMENDED?).
> > > 
> > > Proxy settings are documented as a mandatory user policy only. I do not
> > > understand why we decode it as a recommended device policy here. I started
a
> > > thread to discuss this and CC'd you on it.
> > 
> > I'll leave this behavior as is for now? We can always create another CL to
> > remove this once we have a definitive answer on the thread.
> 
> Please add a TODO and file an M41 bug against me to kill this policy. I
verified
> that it was used by retail mode domains only. I will remove it once your CL
has
> landed.

Done.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:127: message
PinnedAppsProto {
On 2014/12/15 16:08:40, bartfab wrote:
> On 2014/12/04 19:50:07, Rahul Chaturvedi wrote:
> > On 2014/12/02 21:25:27, bartfab wrote:
> > > Nit: Add OBSOLETE_.
> > 
> > According to Mattias, this is still in use. Something I am missing?
> 
> You had accidentally removed the implementation of pinned apps used in regular
> sessions in an earlier revision of this CL. That is what Mattias was pointing
> out. This here is the retail-mode-specific implementation of pinned apps,
which
> should be marked as obsolete.

Ah - okay, marked this obsolete.
Done.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:129: repeated
string app_id = 1;
On 2014/12/15 16:08:40, bartfab wrote:
> On 2014/12/04 19:50:07, Rahul Chaturvedi wrote:
> > On 2014/12/02 21:25:27, bartfab wrote:
> > > Nit: Add OBSOLETE_ and deprecated = true.
> > 
> > Ditto.
> 
> As above, this is used for retail mode only.

Done.

https://codereview.chromium.org/608283003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:607: optional
PinnedAppsProto pinned_apps = 19 [deprecated = true];
On 2014/12/15 16:08:40, bartfab wrote:
> On 2014/12/04 19:50:07, Rahul Chaturvedi wrote:
> > On 2014/12/02 21:25:27, bartfab wrote:
> > > Nit: Add OBSOLETE_.
> > 
> > Ditto, and removed the [deprecated = true].
> 
> As above, this is used for retail mode only.

Done.

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc (left):

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc:19: #include
"chrome/common/chrome_switches.h"
On 2014/12/15 16:08:40, bartfab wrote:
> Nit: Yes, this include is unused. But I cannot see how the clean-up is related
> to retail mode removal.

Done.

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (left):

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:56: #include
"chromeos/settings/cros_settings_names.h"
On 2014/12/15 16:08:40, bartfab wrote:
> Why are you removing this?

Not sure, might be another merge fail. I put it back.
Done.

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc
(right):

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:20: #include
"chrome/browser/chromeos/policy/consumer_enrollment_handler.h"
On 2014/12/15 16:08:40, bartfab wrote:
> Nit: Why are you adding this?

Merge screwup. Fixed.
Done.

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right):

https://codereview.chromium.org/608283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:27: class
ConsumerEnrollmentHandler;
On 2014/12/15 16:08:40, bartfab wrote:
> Nit: Why are you adding this?

Merge screwup. Fixed.
Done.

[bartfab (slow), 2014-12-16 10:14:26]

Woohoo! LGTM

[rkc, 2014-12-16 11:11:15]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-12-16 11:11:56]

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

[commit-bot: I haz the power, 2014-12-16 11:16:59]

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

[commit-bot: I haz the power, 2014-12-16 11:17:01]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rkc, 2014-12-16 23:08:41]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-12-16 23:09:53]

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

[commit-bot: I haz the power, 2014-12-16 23:56:07]

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

[commit-bot: I haz the power, 2014-12-16 23:56:09]

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

[rkc, 2014-12-18 05:15:17]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-12-18 05:15:28]

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

[commit-bot: I haz the power, 2014-12-18 06:31:50]

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

[commit-bot: I haz the power, 2014-12-18 06:31:53]

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

[rkc, 2014-12-18 06:37:21]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-12-18 06:38:10]

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

[commit-bot: I haz the power, 2014-12-18 07:47:11]

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

[commit-bot: I haz the power, 2014-12-18 07:47:13]

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

[rkc, 2014-12-18 08:37:03]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-12-18 08:37:57]

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

[commit-bot: I haz the power, 2014-12-18 09:50:26]

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

[commit-bot: I haz the power, 2014-12-18 09:50:30]

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

[rkc, 2014-12-18 14:29:33]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-12-18 14:30:49]

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

[commit-bot: I haz the power, 2014-12-18 15:47:48]

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

[commit-bot: I haz the power, 2014-12-18 15:47:51]

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

[rkc, 2014-12-18 16:03:12]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-12-18 16:03:43]

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

[commit-bot: I haz the power, 2014-12-18 16:54:46]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-12-18 18:05:33]

Patchset 12 (id:??) landed as
https://crrev.com/b526cd6b9e2a79f8b7f1b8e2ceeb10e5f46cf787
Cr-Commit-Position: refs/heads/master@{#309022}