UI flow rework for device reset options. Reset made available from gaia screen. Tests adapted.

UI flow rework for device reset options. Reset made available from gaia screen. Tests adapted.

BUG=399186
Committed: https://crrev.com/fc6d9e5869b861a5cb82018a934c44b59f58687f
Cr-Commit-Position: refs/heads/master@{#293478}

[merkulova, 2014-08-21 11:10:25]

FYI final flow and UI (strings, views) are under discussion.

dpolukhin@ for metrics.h 
BTW any ideas on proper OWNERS set for metrics.h?

[Dmitry Polukhin, 2014-08-22 07:58:24]

https://codereview.chromium.org/491263002/diff/1/chrome/browser/chromeos/rese...
File chrome/browser/chromeos/reset/metrics.h (right):

https://codereview.chromium.org/491263002/diff/1/chrome/browser/chromeos/rese...
chrome/browser/chromeos/reset/metrics.h:17: DIALOG_SHORTCUT_RESTART_REQUIRED,
It seems that it is structure that is used in UMA metric so you need to add new
members only to the end and also you need to update
tools/metrics/histograms/histograms.xml accordingly and add histograms owners
for review.

https://codereview.chromium.org/491263002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc (right):

https://codereview.chromium.org/491263002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc:84: 
Nit, remove duplicating empty strings.

[merkulova, 2014-08-22 09:46:46]

https://codereview.chromium.org/491263002/diff/1/chrome/browser/chromeos/rese...
File chrome/browser/chromeos/reset/metrics.h (right):

https://codereview.chromium.org/491263002/diff/1/chrome/browser/chromeos/rese...
chrome/browser/chromeos/reset/metrics.h:17: DIALOG_SHORTCUT_RESTART_REQUIRED,
On 2014/08/22 07:58:23, Dmitry Polukhin wrote:
> It seems that it is structure that is used in UMA metric so you need to add
new
> members only to the end and also you need to update
> tools/metrics/histograms/histograms.xml accordingly and add histograms owners
> for review.

Done.

https://codereview.chromium.org/491263002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc (right):

https://codereview.chromium.org/491263002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc:84: 
On 2014/08/22 07:58:23, Dmitry Polukhin wrote:
> Nit, remove duplicating empty strings.

Done.

[merkulova, 2014-08-25 11:13:07]

ping)

[Dmitry Polukhin, 2014-08-25 11:42:06]

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/reset/metrics.h (right):

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/reset/metrics.h:29: DIALOG_SHORTCUT_RESTART_REQUIRED,
Please add new value to tools/metrics/histograms/histograms.xml.

[dzhioev (left Google), 2014-08-26 12:08:18]

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:128: else {
Put 'else' on the same line as '}'

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:151: else if (state
== 'powerwashConfirmationalDialog')
I think it will be better if you create constants for state values.

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:155: else  //
default view is powerwashProposal
Check that it is really "powerwashProposal"

[merkulova, 2014-08-27 08:46:41]

Patchset #3 (id:40001) has been deleted

[merkulova, 2014-08-27 08:54:48]

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/reset/metrics.h (right):

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/reset/metrics.h:29: DIALOG_SHORTCUT_RESTART_REQUIRED,
On 2014/08/25 11:42:06, Dmitry Polukhin wrote:
> Please add new value to tools/metrics/histograms/histograms.xml.

Done.

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:128: else {
On 2014/08/26 12:08:18, dzhioev wrote:
> Put 'else' on the same line as '}'

Done.

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:151: else if (state
== 'powerwashConfirmationalDialog')
On 2014/08/26 12:08:18, dzhioev wrote:
> I think it will be better if you create constants for state values.

Done.

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:155: else  //
default view is powerwashProposal
On 2014/08/26 12:08:18, dzhioev wrote:
> Check that it is really "powerwashProposal"

Not sure what you mean?

[Dmitry Polukhin, 2014-08-27 09:12:54]

lgtm

[dzhioev (left Google), 2014-08-28 15:22:49]

LGTM after addressing last comment.

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:155: else  //
default view is powerwashProposal
On 2014/08/27 08:54:48, merkulova wrote:
> On 2014/08/26 12:08:18, dzhioev wrote:
> > Check that it is really "powerwashProposal"
> 
> Not sure what you mean?
Check that in the last "else" |state == 'powerwashProposal'|. Or it is not true?

[merkulova, 2014-08-29 07:26:54]

Let's clarify the last issue.

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:155: else  //
default view is powerwashProposal
On 2014/08/28 15:22:49, dzhioev wrote:
> On 2014/08/27 08:54:48, merkulova wrote:
> > On 2014/08/26 12:08:18, dzhioev wrote:
> > > Check that it is really "powerwashProposal"
> > 
> > Not sure what you mean?
> Check that in the last "else" |state == 'powerwashProposal'|. Or it is not
true?
Not like that. There's already a special case procession for
'powerwashProposal'. This one case is a simple fallback. That means we can set
dialogView to 'powerwashProposal' in 2 ways. One is explicitly set that state
(and it's how it's being used)  and another just run set-function without state
param.
Does it fit?

[dzhioev (left Google), 2014-08-29 15:44:32]

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:155: else  //
default view is powerwashProposal
On 2014/08/29 07:26:54, merkulova wrote:
> On 2014/08/28 15:22:49, dzhioev wrote:
> > On 2014/08/27 08:54:48, merkulova wrote:
> > > On 2014/08/26 12:08:18, dzhioev wrote:
> > > > Check that it is really "powerwashProposal"
> > > 
> > > Not sure what you mean?
> > Check that in the last "else" |state == 'powerwashProposal'|. Or it is not
> true?
> Not like that. There's already a special case procession for
> 'powerwashProposal'. This one case is a simple fallback. That means we can set
> dialogView to 'powerwashProposal' in 2 ways. One is explicitly set that state
> (and it's how it's being used)  and another just run set-function without
state
> param.
> Does it fit?
I don't see a place where you call setDialogView_ with a |state| that is not
covered by if-branches. Why do you need this else-branch at all? I think we
should threat the situation when |state| has unexpected value as an error.

[merkulova, 2014-09-01 07:40:09]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-01 07:40:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/merkulova@chromium.org/491263002/80001

[commit-bot: I haz the power, 2014-09-01 07:52:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48714)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/54040)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  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 on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-09-01 07:53:48]

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

[commit-bot: I haz the power, 2014-09-01 07:53:49]

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 on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[merkulova, 2014-09-03 08:48:20]

Could you update lgtm?

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_reset.js (right):

https://codereview.chromium.org/491263002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_reset.js:155: else  //
default view is powerwashProposal
On 2014/08/29 15:44:31, dzhioev wrote:
> On 2014/08/29 07:26:54, merkulova wrote:
> > On 2014/08/28 15:22:49, dzhioev wrote:
> > > On 2014/08/27 08:54:48, merkulova wrote:
> > > > On 2014/08/26 12:08:18, dzhioev wrote:
> > > > > Check that it is really "powerwashProposal"
> > > > 
> > > > Not sure what you mean?
> > > Check that in the last "else" |state == 'powerwashProposal'|. Or it is not
> > true?
> > Not like that. There's already a special case procession for
> > 'powerwashProposal'. This one case is a simple fallback. That means we can
set
> > dialogView to 'powerwashProposal' in 2 ways. One is explicitly set that
state
> > (and it's how it's being used)  and another just run set-function without
> state
> > param.
> > Does it fit?
> I don't see a place where you call setDialogView_ with a |state| that is not
> covered by if-branches. Why do you need this else-branch at all? I think we
> should threat the situation when |state| has unexpected value as an error.

Done.

[Dmitry Polukhin, 2014-09-03 09:32:18]

lgtm

[dzhioev (left Google), 2014-09-03 12:11:12]

lgtm

[merkulova, 2014-09-03 12:11:25]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-03 12:12:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/merkulova@chromium.org/491263002/100001

[commit-bot: I haz the power, 2014-09-03 13:09:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-09-03 13:12:54]

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

[commit-bot: I haz the power, 2014-09-03 13:12:55]

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

[merkulova, 2014-09-03 13:14:56]

merkulova@chromium.org changed reviewers:
+ asvitkine@chromium.org

[merkulova, 2014-09-03 13:14:56]

+asvitkine@ as histograms.xml owner

[Alexei Svitkine (slow), 2014-09-03 13:35:13]

lgtm

[merkulova, 2014-09-03 13:35:35]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-03 13:36:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/merkulova@chromium.org/491263002/100001

[commit-bot: I haz the power, 2014-09-03 13:57:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-03 14:47:24]

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

[commit-bot: I haz the power, 2014-09-03 14:47:25]

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

[merkulova, 2014-09-04 08:15:29]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-04 08:16:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/merkulova@chromium.org/491263002/120001

[commit-bot: I haz the power, 2014-09-04 09:00:18]

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

[commit-bot: I haz the power, 2014-09-04 09:00:19]

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

[merkulova, 2014-09-04 09:30:24]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-04 09:31:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/merkulova@chromium.org/491263002/140001

[commit-bot: I haz the power, 2014-09-04 16:26:10]

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

[commit-bot: I haz the power, 2014-09-04 16:26:11]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[merkulova, 2014-09-05 07:26:22]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-05 07:27:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/merkulova@chromium.org/491263002/140001

[commit-bot: I haz the power, 2014-09-05 08:27:58]

Committed patchset #7 (id:140001) as 554eb81bbde739d4364b21bb70ba28daddf68c23

[commit-bot: I haz the power, 2014-09-10 03:38:04]

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