cros: Replace WizardController string constants with OobeScreen values.

cros: Replace WizardController string constants with OobeScreen values.

Constants which have changed:

  kArcTermsOfServiceScreenName from "arc_tos" to "arc-tos"
  kEnrollmentScreenName from "enroll" to "oauth-enrollment"
  kNetworkScreenName from "network" to "connect"
  kSupervisedUserCreationScreenName from "supervised-user-creation-flow" to "supervised-user-creation"
  kTermsOfServiceScreenName from "tos" to "terms-of-service"
  kUserImageScreenName from "image" to "user-image"

BUG=672142
Review-Url: https://codereview.chromium.org/2566443005
Cr-Commit-Position: refs/heads/master@{#442334}
Committed: https://chromium.googlesource.com/chromium/src/+/9e47a74b21cb8b32e79eb8f41945afcc35d8a825

[jdufault, 2016-12-08 23:41:25]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 23:41:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2016-12-08 23:50:48]

Description was changed from

==========
ignore: wip

BUG=
==========

to

==========
ignore: wip

Constants which have changed:

  kArcTermsOfServiceScreenName from "arc_tos" to "arc-tos"
  kEnrollmentScreenName from "enroll" to "oauth-enrollment"
  kNetworkScreenName from "network" to "connect"
  kSupervisedUserCreationScreenName from "supervised-user-creation-flow" to
"supervised-user-creation"
  kTermsOfServiceScreenName from "tos" to "terms-of-service"
  kUserImageScreenName from "image" to "user-image"

BUG=
==========

[commit-bot: I haz the power, 2016-12-08 23:53:40]

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

[commit-bot: I haz the power, 2016-12-08 23:53:41]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[jdufault, 2016-12-09 18:16:24]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 18:16:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2016-12-09 20:11:54]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 20:13:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2016-12-09 20:43:41]

Description was changed from

==========
ignore: wip

Constants which have changed:

  kArcTermsOfServiceScreenName from "arc_tos" to "arc-tos"
  kEnrollmentScreenName from "enroll" to "oauth-enrollment"
  kNetworkScreenName from "network" to "connect"
  kSupervisedUserCreationScreenName from "supervised-user-creation-flow" to
"supervised-user-creation"
  kTermsOfServiceScreenName from "tos" to "terms-of-service"
  kUserImageScreenName from "image" to "user-image"

BUG=
==========

to

==========
cros: Replace WizardController string constants with OobeScreen values.

Constants which have changed:

  kArcTermsOfServiceScreenName from "arc_tos" to "arc-tos"
  kEnrollmentScreenName from "enroll" to "oauth-enrollment"
  kNetworkScreenName from "network" to "connect"
  kSupervisedUserCreationScreenName from "supervised-user-creation-flow" to
"supervised-user-creation"
  kTermsOfServiceScreenName from "tos" to "terms-of-service"
  kUserImageScreenName from "image" to "user-image"

BUG=672142
==========

[jdufault, 2016-12-09 20:43:41]

jdufault@chromium.org changed reviewers:
+ alemate@chromium.org

[jdufault, 2016-12-09 20:43:52]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-12-09 23:05:28]

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

[commit-bot: I haz the power, 2016-12-09 23:05:29]

Dry run: This issue passed the CQ dry run.

[jdufault, 2016-12-09 23:07:26]

alemate@ PTAL. Thanks!

[Alexander Alekseev, 2016-12-13 03:08:05]

I am OK with replacing screen names with enum.
But I don't understand why you need:

1) to rename screens? I used to think that the first screen is called "network",
because all the handlers have names like "Network screen name".

2) Why UMA lacks support for most of screens? Why not to continue reporting the
same data.

3) Why do you need to move
"chrome/browser/ui/webui/chromeos/login/oobe_screen.h"  to
"chrome/browser/chromeos/login/oobe_screen.h" ?
I think that screen names are mostly a part of UI and should be in the same
place as screen handlers.

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_screen_waiter.cc (right):

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_screen_waiter.cc:7: #include
"base/debug/stack_trace.h"
nit: Remove debug.

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host.h (right):

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host.h:72: virtual void
StartWizard(OobeScreen first_screen_name) = 0;
nit: as this is no longer name, replace it with "first_screen_id" or something
like that (and everywhere else for first_screen_name).

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:146: 
Why is the full set of names not supported now?

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.h (left):

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.h:92: static bool
IsOOBEStepToTrack(const std::string& screen_id);
Why not "screen_id" ?

[jdufault, 2016-12-13 22:16:28]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 22:17:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2016-12-13 22:30:27]

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_screen_waiter.cc (right):

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_screen_waiter.cc:7: #include
"base/debug/stack_trace.h"
On 2016/12/13 03:08:05, Alexander Alekseev wrote:
> nit: Remove debug.

Done.

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host.h (right):

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host.h:72: virtual void
StartWizard(OobeScreen first_screen_name) = 0;
On 2016/12/13 03:08:05, Alexander Alekseev wrote:
> nit: as this is no longer name, replace it with "first_screen_id" or something
> like that (and everywhere else for first_screen_name).

Done.

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:146: 
On 2016/12/13 03:08:05, Alexander Alekseev wrote:
> Why is the full set of names not supported now?

The full set of names is being used; these are just the string constants whose
names have changed (but we want to continue reporting under the same UMA key).

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.h (left):

https://codereview.chromium.org/2566443005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.h:92: static bool
IsOOBEStepToTrack(const std::string& screen_id);
On 2016/12/13 03:08:05, Alexander Alekseev wrote:
> Why not "screen_id" ?

Done.

[commit-bot: I haz the power, 2016-12-13 23:37:42]

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

[commit-bot: I haz the power, 2016-12-13 23:37:43]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2016-12-14 02:58:56]

lgtm

[achuithb, 2017-01-05 17:09:08]

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

[achuithb, 2017-01-05 17:09:09]

https://codereview.chromium.org/2566443005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/oobe_screen.h (right):

https://codereview.chromium.org/2566443005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/oobe_screen.h:12: // TODO(jdufault): Rename to
LoginScreen or similar since this is not directly
Is this a short-lived TODO? Otherwise please also reference a bug.

https://codereview.chromium.org/2566443005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2566443005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:134: if (screen ==
chromeos::OobeScreen::SCREEN_ARC_TERMS_OF_SERVICE)
Can you use a static map (legacy_uma?) instead of this if-else statement?

[jdufault, 2017-01-05 22:23:10]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 22:23:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 23:43:56]

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

[commit-bot: I haz the power, 2017-01-05 23:43:57]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-01-05 23:51:43]

https://codereview.chromium.org/2566443005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/oobe_screen.h (right):

https://codereview.chromium.org/2566443005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/oobe_screen.h:12: // TODO(jdufault): Rename to
LoginScreen or similar since this is not directly
On 2017/01/05 17:09:09, achuithb wrote:
> Is this a short-lived TODO? Otherwise please also reference a bug.

Added bug reference.

https://codereview.chromium.org/2566443005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2566443005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:134: if (screen ==
chromeos::OobeScreen::SCREEN_ARC_TERMS_OF_SERVICE)
On 2017/01/05 17:09:09, achuithb wrote:
> Can you use a static map (legacy_uma?) instead of this if-else statement?

Done.

[achuithb, 2017-01-06 00:13:24]

lgtm

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_manager.cc (right):

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_manager.cc:16: auto iter =
screens_.find(screen);
Does const auto& not work here? Because the type is const_iterator?

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:144: // Fetch screen name.
Some of the screens had multiple different names in the
Some version of this comment should accompany the definition of
kLegacyUmaOobeScreenNames

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.h (right):

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.h:341: OobeScreen
first_screen_id_;
I prefer first_screen_ to first_screen_id_, here and everywhere.

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:309: OobeScreen
screen = GetOobeScreenFromName(screen_name);
nit: const

[jdufault, 2017-01-06 01:07:48]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 01:09:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 02:38:58]

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

[commit-bot: I haz the power, 2017-01-06 02:38:59]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-01-09 19:13:35]

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_manager.cc (right):

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_manager.cc:16: auto iter =
screens_.find(screen);
On 2017/01/06 00:13:24, achuithb wrote:
> Does const auto& not work here? Because the type is const_iterator? 

Is that safe? It seems like std::map::find could return a stack value when the
element is not found.

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:144: // Fetch screen name.
Some of the screens had multiple different names in the
On 2017/01/06 00:13:24, achuithb wrote:
> Some version of this comment should accompany the definition of
> kLegacyUmaOobeScreenNames

Done.

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.h (right):

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.h:341: OobeScreen
first_screen_id_;
On 2017/01/06 00:13:24, achuithb wrote:
> I prefer first_screen_ to first_screen_id_, here and everywhere.

Done.

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):

https://codereview.chromium.org/2566443005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:309: OobeScreen
screen = GetOobeScreenFromName(screen_name);
On 2017/01/06 00:13:24, achuithb wrote:
> nit: const

Done.

[jdufault, 2017-01-09 19:13:44]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2017-01-09 19:13:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
alemate@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2566443005/#ps100001
(title: "Address comments")

[commit-bot: I haz the power, 2017-01-09 19:14:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 20:47:01]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483989224493210,
"parent_rev": "66edd71ca9b4e2691ab0d0926b7b77945c92556f", "commit_rev":
"9e47a74b21cb8b32e79eb8f41945afcc35d8a825"}

[commit-bot: I haz the power, 2017-01-09 20:47:39]

Description was changed from

==========
cros: Replace WizardController string constants with OobeScreen values.

Constants which have changed:

  kArcTermsOfServiceScreenName from "arc_tos" to "arc-tos"
  kEnrollmentScreenName from "enroll" to "oauth-enrollment"
  kNetworkScreenName from "network" to "connect"
  kSupervisedUserCreationScreenName from "supervised-user-creation-flow" to
"supervised-user-creation"
  kTermsOfServiceScreenName from "tos" to "terms-of-service"
  kUserImageScreenName from "image" to "user-image"

BUG=672142
==========

to

==========
cros: Replace WizardController string constants with OobeScreen values.

Constants which have changed:

  kArcTermsOfServiceScreenName from "arc_tos" to "arc-tos"
  kEnrollmentScreenName from "enroll" to "oauth-enrollment"
  kNetworkScreenName from "network" to "connect"
  kSupervisedUserCreationScreenName from "supervised-user-creation-flow" to
"supervised-user-creation"
  kTermsOfServiceScreenName from "tos" to "terms-of-service"
  kUserImageScreenName from "image" to "user-image"

BUG=672142

Review-Url: https://codereview.chromium.org/2566443005
Cr-Commit-Position: refs/heads/master@{#442334}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e47a74b21cb8b32e79eb8f41945...
==========

[commit-bot: I haz the power, 2017-01-09 20:47:40]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9e47a74b21cb8b32e79eb8f41945...