Factory reset screen is added

Factory reset screen is added

BUG=122768
TEST=manual

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

[glotov, 2012-09-12 12:49:08]

Hi! Please take a look.

Left to do:
1. wizard_controller_browsertest
2. about_flags

[Nikita (slow), 2012-09-12 13:22:21]

Mostly nits

http://codereview.chromium.org/10928088/diff/5001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13939: +      <message
name="IDS_RESET_SCREEN_TITLE" desc="Title of the OOBE reset screen">
nit: Remove "OOBE".
This screen is a normal screen and available not only during OOBE flow.

http://codereview.chromium.org/10928088/diff/5001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15038: +      <message
name="IDS_RESET_SCREEN_WARNING_TEXT_1" desc="Warning text shown on reset screen
above the exclamation icon.">
nit: rename to IDS_RESET_SCREEN_WARNING_MSG

http://codereview.chromium.org/10928088/diff/5001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15041: +      <message
name="IDS_RESET_SCREEN_WARNING_TEXT_2" desc="Warning text shown on reset screen
below the exclamation icon.">
nit: Rename to IDS_RESET_SCREEN_WARNING_DETAILS

http://codereview.chromium.org/10928088/diff/5001/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources.grd (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources.grd:631: <structure type="chrome_scaled_image"
name="IDR_RESET_WARNING" file="reset_warning.png" />
Please submit resources in a separate CL.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/reset_screen.h (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/reset_screen.h:17: // Representation independent
class that controls OOBE screen showing RESET
nit: remove OOBE.
nit: RESET > Reset screen.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/reset_screen_actor.h (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/reset_screen_actor.h:12: // Interface between eula
screen and its representation, either WebUI or
eula > reset

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/reset_screen_actor.h:13: // Views one. Note, do
not forget to call OnActorDestroyed in the dtor.
Drop "either WebUI or Views one"

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/reset_screen_actor.h:16: // Allows us to get info
from eula screen that we need.
eula > reset

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:791:
OnEnterpriseEnrollmentDone();
You should not rely on OnEnterpriseEnrollmentDone().

Create separate method OnResetCancelled() which:
1. Returns to any previous wizard screen that was shown before that (you have to
save it).
2. Otherwise returns to sign in screen.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/display_manager.js:122: if
(currentStepId == SCREEN_GAIA_SIGNIN ||
Please add and allow accelerator for
var SCREEN_NETWORK_ERROR = 'error-message';

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/login.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/login.js:39:
oobe.ResetScreen.register();
Please place into login.* namespace

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:463: padding: 40px 17px 80px
17px;
padding: 40px 17px 80px;

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:468: html[oobe=new] #reset
#reset-warning-text-1 {
reset-warning-text-1 > reset-warning-msg

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:471: padding: 5px 45px 0 45px;
padding: 5px 45px 0;

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:474: html[oobe=new] #reset
#reset-warning-text-2 {
reset-warning-text-2 > reset-warning-details

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:477: padding: 25px 40px 80px
45px;
Why do we have separate padding from left and right?

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.html (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.html:1: <div
class="step hidden no-logo" id="reset">
Please rename oobe_screen_reset.html to screen_factory_reset.html

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.html:3: <div
id="reset-warning-text-1" i18n-content="warningText1"></div>
resetWarningMsg

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.html:4: <br/>
You don't need <br> here.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.html:7: <div
id="reset-warning-text-2" i18n-content="warningText2"></div>
resetWarningDetails

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (left):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:15: var EulaScreen
= cr.ui.define('div');
You should probably delete this file completely locally and copy it from
somewhere else on you local fs so that it is not considered as a copy of eula
screen resources.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Please rename to screen_factory_reset.js

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:6: * @fileoverview
Oobe eula screen implementation.
eula > reset

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:9:
cr.define('oobe', function() {
Please place this screen into login.* namespace

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:11: * Creates a new
oobe screen div.
new oobe screen div > new screen div

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:48: var backButton
= this.ownerDocument.createElement('button');
backButton > cancelButton

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:57: var
acceptButton = this.ownerDocument.createElement('button');
acceptButton > resetButton

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc:60:
localized_strings->SetString("warningText1",
resetWarningText

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc:62:
localized_strings->SetString("warningText2",
resetWarningDetails

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h (left):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
Same for this file.
1. Copy it somewhere else on fs.
2. Delete from src tree.
3. Commit.
4. Copy it back.
5. Commit

It should not track history from eula handler.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h:20: class
HelpAppLauncher;
Please remove.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h:22: // WebUI
implementation of ResetScreenActor. It is used to interact
You don't need second comment

[glotov, 2012-09-12 22:40:25]

http://codereview.chromium.org/10928088/diff/5001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13939: +      <message
name="IDS_RESET_SCREEN_TITLE" desc="Title of the OOBE reset screen">
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> nit: Remove "OOBE".
> This screen is a normal screen and available not only during OOBE flow.

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15038: +      <message
name="IDS_RESET_SCREEN_WARNING_TEXT_1" desc="Warning text shown on reset screen
above the exclamation icon.">
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> nit: rename to IDS_RESET_SCREEN_WARNING_MSG

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15041: +      <message
name="IDS_RESET_SCREEN_WARNING_TEXT_2" desc="Warning text shown on reset screen
below the exclamation icon.">
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> nit: Rename to IDS_RESET_SCREEN_WARNING_DETAILS

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources.grd (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources.grd:631: <structure type="chrome_scaled_image"
name="IDR_RESET_WARNING" file="reset_warning.png" />
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> Please submit resources in a separate CL.

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/reset_screen.h (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/reset_screen.h:17: // Representation independent
class that controls OOBE screen showing RESET
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> nit: remove OOBE.
> nit: RESET > Reset screen.

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/reset_screen_actor.h (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/reset_screen_actor.h:12: // Interface between eula
screen and its representation, either WebUI or
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> eula > reset

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/reset_screen_actor.h:13: // Views one. Note, do
not forget to call OnActorDestroyed in the dtor.
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> Drop "either WebUI or Views one"

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/reset_screen_actor.h:16: // Allows us to get info
from eula screen that we need.
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> eula > reset

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:791:
OnEnterpriseEnrollmentDone();
But we are not supposed to run from other screen except the login one. So
currently no need to save previous screen and restore it.

On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> You should not rely on OnEnterpriseEnrollmentDone().
> 
> Create separate method OnResetCancelled() which:
> 1. Returns to any previous wizard screen that was shown before that (you have
to
> save it).
> 2. Otherwise returns to sign in screen.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/display_manager.js:122: if
(currentStepId == SCREEN_GAIA_SIGNIN ||
What is it?

On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> Please add and allow accelerator for
> var SCREEN_NETWORK_ERROR = 'error-message';

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/login.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/login.js:39:
oobe.ResetScreen.register();
It is still managed by WizardController. Just like oobe.OAuthEnrollmentScreen.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:463: padding: 40px 17px 80px
17px;
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> padding: 40px 17px 80px;

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:468: html[oobe=new] #reset
#reset-warning-text-1 {
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> reset-warning-text-1 > reset-warning-msg

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:471: padding: 5px 45px 0 45px;
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> padding: 5px 45px 0;

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:474: html[oobe=new] #reset
#reset-warning-text-2 {
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> reset-warning-text-2 > reset-warning-details

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:477: padding: 25px 40px 80px
45px;
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> Why do we have separate padding from left and right?

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.html (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.html:3: <div
id="reset-warning-text-1" i18n-content="warningText1"></div>
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> resetWarningMsg

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.html:4: <br/>
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> You don't need <br> here.

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.html:7: <div
id="reset-warning-text-2" i18n-content="warningText2"></div>
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> resetWarningDetails

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (left):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:15: var EulaScreen
= cr.ui.define('div');
But it is nicer this way, no?

On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> You should probably delete this file completely locally and copy it from
> somewhere else on you local fs so that it is not considered as a copy of eula
> screen resources.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:6: * @fileoverview
Oobe eula screen implementation.
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> eula > reset

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:9:
cr.define('oobe', function() {
Discussed above the comments.

On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> Please place this screen into login.* namespace

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:11: * Creates a new
oobe screen div.
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> new oobe screen div > new screen div

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:48: var backButton
= this.ownerDocument.createElement('button');
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> backButton > cancelButton

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:57: var
acceptButton = this.ownerDocument.createElement('button');
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> acceptButton > resetButton

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc:60:
localized_strings->SetString("warningText1",
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> resetWarningText

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc:62:
localized_strings->SetString("warningText2",
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> resetWarningDetails

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h (left):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
And it does not track. Why?

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h:20: class
HelpAppLauncher;
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> Please remove.

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h:22: // WebUI
implementation of ResetScreenActor. It is used to interact
On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> You don't need second comment

Done.

[Nikita (slow), 2012-09-13 07:35:58]

waiting for tests + flag to disable this feature before approving it

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:791:
OnEnterpriseEnrollmentDone();
On 2012/09/12 22:40:25, glotov wrote:
> But we are not supposed to run from other screen except the login one. So
> currently no need to save previous screen and restore it.
> 
> On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> > You should not rely on OnEnterpriseEnrollmentDone().
> > 
> > Create separate method OnResetCancelled() which:
> > 1. Returns to any previous wizard screen that was shown before that (you
have
> to
> > save it).
> > 2. Otherwise returns to sign in screen.
> 

That may change in future so your new code should be able to handle that. Please
add this handling in this CL since it is not that hard.

On the using OnEnterpriseEnrollmentDone: as you could see in this switch no
other exit code is reusing  another handler. Some code could be added into
OnEnterpriseEnrollmentDone which will break your feature.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/display_manager.js:122: if
(currentStepId == SCREEN_GAIA_SIGNIN ||
On 2012/09/12 22:40:25, glotov wrote:
> What is it?
> 
> On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> > Please add and allow accelerator for
> > var SCREEN_NETWORK_ERROR = 'error-message';
> 

That is screen that is shown on GAIA login screen when network is not connected.
We want to have reset working there too.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/login.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/login.js:39:
oobe.ResetScreen.register();
On 2012/09/12 22:40:25, glotov wrote:
> It is still managed by WizardController. Just like oobe.OAuthEnrollmentScreen.

Ok, but my point was regarding logical definition of OOBE which is out of box
experience.
Technically enrollment belongs here. In some sense UserImageScreen belongs there
too as it is launched in context of OOBE but than as a context of new user signs
in.
Reset usually is launched as a part of normal login screen i.e. not part of
OOBE.

But let's leave it as is.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (left):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:15: var EulaScreen
= cr.ui.define('div');
On 2012/09/12 22:40:25, glotov wrote:
> But it is nicer this way, no?
> 
> On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> > You should probably delete this file completely locally and copy it from
> > somewhere else on you local fs so that it is not considered as a copy of
eula
> > screen resources.
> 

No. This file should be treated as a new one even though you've based it on
existing screen.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h (left):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2012/09/12 22:40:25, glotov wrote:
> And it does not track. Why?

It is tracking (A+) and you could see diff with EULA handler which should not be
the case even though if you initially copied that implementation.

[Nikita (slow), 2012-09-14 14:59:46]

lgtm

[glotov, 2012-09-14 16:37:02]

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_controller.cc:791:
OnEnterpriseEnrollmentDone();
On 2012/09/13 07:35:59, Nikita Kostylev wrote:
> On 2012/09/12 22:40:25, glotov wrote:
> > But we are not supposed to run from other screen except the login one. So
> > currently no need to save previous screen and restore it.
> > 
> > On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> > > You should not rely on OnEnterpriseEnrollmentDone().
> > > 
> > > Create separate method OnResetCancelled() which:
> > > 1. Returns to any previous wizard screen that was shown before that (you
> have
> > to
> > > save it).
> > > 2. Otherwise returns to sign in screen.
> > 
> 
> That may change in future so your new code should be able to handle that.
Please
> add this handling in this CL since it is not that hard.
> 
> On the using OnEnterpriseEnrollmentDone: as you could see in this switch no
> other exit code is reusing  another handler. Some code could be added into
> OnEnterpriseEnrollmentDone which will break your feature.

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/display_manager.js:122: if
(currentStepId == SCREEN_GAIA_SIGNIN ||
Discussed. Done.

On 2012/09/13 07:35:59, Nikita Kostylev wrote:
> On 2012/09/12 22:40:25, glotov wrote:
> > What is it?
> > 
> > On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> > > Please add and allow accelerator for
> > > var SCREEN_NETWORK_ERROR = 'error-message';
> > 
> 
> That is screen that is shown on GAIA login screen when network is not
connected.
> We want to have reset working there too.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/login.js (right):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/login.js:39:
oobe.ResetScreen.register();
On 2012/09/13 07:35:59, Nikita Kostylev wrote:
> On 2012/09/12 22:40:25, glotov wrote:
> > It is still managed by WizardController. Just like
oobe.OAuthEnrollmentScreen.
> 
> Ok, but my point was regarding logical definition of OOBE which is out of box
> experience.
> Technically enrollment belongs here. In some sense UserImageScreen belongs
there
> too as it is launched in context of OOBE but than as a context of new user
signs
> in.
> Reset usually is launched as a part of normal login screen i.e. not part of
> OOBE.
> 
> But let's leave it as is.

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (left):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:15: var EulaScreen
= cr.ui.define('div');
On 2012/09/13 07:35:59, Nikita Kostylev wrote:
> On 2012/09/12 22:40:25, glotov wrote:
> > But it is nicer this way, no?
> > 
> > On 2012/09/12 13:22:21, Nikita Kostylev wrote:
> > > You should probably delete this file completely locally and copy it from
> > > somewhere else on you local fs so that it is not considered as a copy of
> eula
> > > screen resources.
> > 
> 
> No. This file should be treated as a new one even though you've based it on
> existing screen.

Done.

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h (left):

http://codereview.chromium.org/10928088/diff/5001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2012/09/13 07:35:59, Nikita Kostylev wrote:
> On 2012/09/12 22:40:25, glotov wrote:
> > And it does not track. Why?
> 
> It is tracking (A+) and you could see diff with EULA handler which should not
be
> the case even though if you initially copied that implementation.

Done.

[glotov, 2012-09-14 16:40:40]

Hi Satoru, James! Please put your OWNER approvals to the chrome/chromeos parts
of this change. Nikita approved the chromeos/login logic.

[James Hawkins, 2012-09-14 16:42:25]

On 2012/09/14 16:40:40, glotov wrote:
> Hi Satoru, James! Please put your OWNER approvals to the chrome/chromeos parts
> of this change. Nikita approved the chromeos/login logic.

Can this CL not be split up?  It's huge.  For one you're going to have to add
the resources in a separate CL in order to run a try job on this.

[Nikita (slow), 2012-09-14 16:45:21]

lgtm

http://codereview.chromium.org/10928088/diff/7017/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10928088/diff/7017/chrome/app/generated_resour...
chrome/app/generated_resources.grd:9994: +	  Reset your Chrome device to its
original factory state.
I think you'll need change Chrome to placeholder here and place
IDS_SHORT_PRODUCT_NAME there (Chrome / Chromium).

http://codereview.chromium.org/10928088/diff/7017/chrome/app/generated_resour...
chrome/app/generated_resources.grd:10000: +	  A restart is required in order to
reset your Chrome device to its original factory state.
Same here.

http://codereview.chromium.org/10928088/diff/7017/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15198: +        Factory Reset your Chrome
device
Same here.

http://codereview.chromium.org/10928088/diff/7017/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

http://codereview.chromium.org/10928088/diff/7017/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:70: * Cancels
enrollment and drops the user back to the login screen.
Enrollment > reset

[glotov, 2012-09-14 17:14:42]

James, this change is not big :) It just spans a lot of files with minimum
changes in each. It almost does not have logic code, just infrastructure
wrappers.
Resources has been committed already and I started trybots. Actually see Patch
Set 4 to see trybot result.

http://codereview.chromium.org/10928088/diff/7017/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10928088/diff/7017/chrome/app/generated_resour...
chrome/app/generated_resources.grd:9994: +	  Reset your Chrome device to its
original factory state.
On 2012/09/14 16:45:21, Nikita Kostylev wrote:
> I think you'll need change Chrome to placeholder here and place
> IDS_SHORT_PRODUCT_NAME there (Chrome / Chromium).

Done.

http://codereview.chromium.org/10928088/diff/7017/chrome/app/generated_resour...
chrome/app/generated_resources.grd:10000: +	  A restart is required in order to
reset your Chrome device to its original factory state.
On 2012/09/14 16:45:21, Nikita Kostylev wrote:
> Same here.

Done.

http://codereview.chromium.org/10928088/diff/7017/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15198: +        Factory Reset your Chrome
device
On 2012/09/14 16:45:21, Nikita Kostylev wrote:
> Same here.

Done.

http://codereview.chromium.org/10928088/diff/7017/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

http://codereview.chromium.org/10928088/diff/7017/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:70: * Cancels
enrollment and drops the user back to the login screen.
On 2012/09/14 16:45:21, Nikita Kostylev wrote:
> Enrollment > reset

Done.

[glotov, 2012-09-14 20:31:32]

Hi Lei, James, Satoru! 
Please put your OWNER approval to the appropriate parts of this change.
The change is not as big as it looks. This is mostly a service code to show and
handle "factory reset" screen written on html, no logic part to be separated
into another CL.

[Lei Zhang, 2012-09-14 20:37:42]

The top level / common files lgtm.

http://codereview.chromium.org/10928088/diff/4019/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/10928088/diff/4019/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:375: extern const char kDisableFactoryReset[];
nit: alphabetical order, ditto in .cc file

[glotov, 2012-09-15 13:14:04]

http://codereview.chromium.org/10928088/diff/4019/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/10928088/diff/4019/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:375: extern const char kDisableFactoryReset[];
On 2012/09/14 20:37:43, Lei Zhang wrote:
> nit: alphabetical order, ditto in .cc file

Done.

[commit-bot: I haz the power, 2012-09-15 13:14:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glotov@chromium.org/10928088/54

[commit-bot: I haz the power, 2012-09-15 13:14:34]

Presubmit check for 10928088-54 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/settings/device_settings_test_helper.cc:25
    content::BrowserThread::GetBlockingPool()->FlushForTesting();

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chromeos

Presubmit checks took 3.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[glotov, 2012-09-15 13:26:02]

Hi Oshima, Satoru!

Could you please put your OWNER lgtm to this list. 3 files from chromeos/ needs
your power to be committed.

[oshima, 2012-09-17 16:46:26]

chromeos LGTM

[commit-bot: I haz the power, 2012-09-17 16:46:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glotov@chromium.org/10928088/14051

[commit-bot: I haz the power, 2012-09-17 17:11:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glotov@chromium.org/10928088/14051

[commit-bot: I haz the power, 2012-09-17 17:29:16]

Retried try job too often for step(s) build

[commit-bot: I haz the power, 2012-09-17 18:38:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glotov@chromium.org/10928088/14051

[commit-bot: I haz the power, 2012-09-17 18:54:16]

Retried try job too often for step(s) build

[commit-bot: I haz the power, 2012-09-17 19:22:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glotov@chromium.org/10928088/14051

[commit-bot: I haz the power, 2012-09-17 19:22:40]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for dinner.
Ping the relevant dude on a on-needed basis.

[commit-bot: I haz the power, 2012-09-17 19:33:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glotov@chromium.org/10928088/14051

[commit-bot: I haz the power, 2012-09-17 19:33:33]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for dinner.
Ping the relevant dude on a on-needed basis.