Pass errors generated during limited-user registration up to the UI for display.

Pass errors generated during limited-user registration up to the UI for display.

Introduces a new Profile::CREATE_STATUS_REMOTE_FAIL type, and renames
CREATE_STATUS_FAIL to CREATE_STATUS_LOCAL_FAIL. Adds callbacks and reorganizes
profile creation to allow registration errors to propagate back up to the UI
layer, and displays them in the profile creation dialog. Adds a spinner to the
creation dialog. Adds UMA metrics.

BUG=234991
TEST=manual

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

[Pam (message me for reviews), 2013-05-31 21:06:25]

sail - profiles and ui/cocoa
jhawkins - resources and webui
bauerb - managed_mode
atwilson - sync

Properly allowing and handling cancellation is a known missing piece, which I'll
do next if Bernhard hasn't already.

Thanks!
- Pam

[Bernhard Bauer, 2013-06-03 10:52:47]

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_registration_service.cc (right):

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_registration_service.cc:111: const
ProfileManager::CreateCallback& callback) {
I wonder if we still need this now that our client is guaranteed not to outlive
us?

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:152: void
OnProfileCreatedCallCallbacks(
Naming nit: Name this either based on what it does (RunProfileCreationCallbacks)
or on when it's called (OnProfileCreated), but not both? Hm, seeing as there are
too many OnProfileCreated events anyway, probably the latter?

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:166: if (status ==
Profile::CREATE_STATUS_INITIALIZED) {
Early return on other status?

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1216: void
BrowserOptionsHandler::DeleteProfile(const ListValue* args) {
Is there a chance we could keep this in ManageProfileHandler and instead call
that method there if needed? BrowserOptionsHandler is quite big already.

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1234: if
(service->ProfileIsManaged())
Use the static version? That prevents us from creating the MUService if it isn't
needed.

[Nikita (slow), 2013-06-03 13:09:40]

cc: antrim@

[Pam (message me for reviews), 2013-06-03 13:39:28]

New pacth up. PTAL, everyone.

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_registration_service.cc (right):

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_registration_service.cc:111: const
ProfileManager::CreateCallback& callback) {
On 2013/06/03 10:52:47, Bernhard Bauer wrote:
> I wonder if we still need this now that our client is guaranteed not to
outlive
> us?

Cleaned up following our live discussion. Thanks, I like the new way better,
letting ManagedUserService handle interactions with
ManagedUserRegistrationService and making what's going on more apparent in the
BrowserOptionsHandler.

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1216: void
BrowserOptionsHandler::DeleteProfile(const ListValue* args) {
On 2013/06/03 10:52:47, Bernhard Bauer wrote:
> Is there a chance we could keep this in ManageProfileHandler and instead call
> that method there if needed? BrowserOptionsHandler is quite big already.

There's quite a bit in BrowserOptionsHandler that I'm not sure belongs there
anymore -- largely historical, I suppose. But I moved this here to match
CreateProfile (or rather, mainly because the CreateProfile flow now calls it
from a few places); it feels strange to me to have them in different classes.

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1234: if
(service->ProfileIsManaged())
On 2013/06/03 10:52:47, Bernhard Bauer wrote:
> Use the static version? That prevents us from creating the MUService if it
isn't
> needed.

Changed throughout.

[Bernhard Bauer, 2013-06-03 14:25:01]

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1216: void
BrowserOptionsHandler::DeleteProfile(const ListValue* args) {
On 2013/06/03 13:39:28, Pam - also send IM for reviews wrote:
> On 2013/06/03 10:52:47, Bernhard Bauer wrote:
> > Is there a chance we could keep this in ManageProfileHandler and instead
call
> > that method there if needed? BrowserOptionsHandler is quite big already.
> 
> There's quite a bit in BrowserOptionsHandler that I'm not sure belongs there
> anymore -- largely historical, I suppose. But I moved this here to match
> CreateProfile (or rather, mainly because the CreateProfile flow now calls it
> from a few places); it feels strange to me to have them in different classes.

Hm. TBH, I'd rather have profile creation and deletion both in
ManageProfileHandler than here, because I figure BrowserOptionsHandler should be
more general options than profile-specific stuff. I won't block this CL on that
particular yak though.

[Bernhard Bauer, 2013-06-03 14:25:18]

LGTM, I meant :)

[Pam (message me for reviews), 2013-06-03 14:27:10]

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/15774008/diff/40007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1216: void
BrowserOptionsHandler::DeleteProfile(const ListValue* args) {
On 2013/06/03 14:25:01, Bernhard Bauer wrote:
> On 2013/06/03 13:39:28, Pam - also send IM for reviews wrote:
> > On 2013/06/03 10:52:47, Bernhard Bauer wrote:
> > > Is there a chance we could keep this in ManageProfileHandler and instead
> call
> > > that method there if needed? BrowserOptionsHandler is quite big already.
> > 
> > There's quite a bit in BrowserOptionsHandler that I'm not sure belongs there
> > anymore -- largely historical, I suppose. But I moved this here to match
> > CreateProfile (or rather, mainly because the CreateProfile flow now calls it
> > from a few places); it feels strange to me to have them in different
classes.
> 
> Hm. TBH, I'd rather have profile creation and deletion both in
> ManageProfileHandler than here, because I figure BrowserOptionsHandler should
be
> more general options than profile-specific stuff. I won't block this CL on
that
> particular yak though.

Yeah, so would I, but I didn't want to get involved in that cleanup in this
patch. Thanks.

[James Hawkins, 2013-06-03 18:43:44]

LGTM with nits.

https://codereview.chromium.org/15774008/diff/66003/chrome/browser/resources/...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/15774008/diff/66003/chrome/browser/resources/...
chrome/browser/resources/options/manage_profile_overlay.js:461: * @param
{boolean} inProgress Whether the step is newly in progress, or
nit: Remove comma.

https://codereview.chromium.org/15774008/diff/66003/chrome/browser/resources/...
chrome/browser/resources/options/manage_profile_overlay.js:461: * @param
{boolean} inProgress Whether the step is newly in progress, or
nit: This is confusing when reading.  Is it true when 'the step is newly in
progress || has just completed' or true when 'the step is newly in progress' and
false when  the steps 'has just completed'?

https://codereview.chromium.org/15774008/diff/66003/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/15774008/diff/66003/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1161: 
nit: Remove blank line.

[sail, 2013-06-03 21:07:41]

profiles/* LGTM

[Andrew T Wilson (Slow), 2013-06-04 07:01:00]

LGTM with one nit which you can address in a separate CL if you want.

https://codereview.chromium.org/15774008/diff/74001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/15774008/diff/74001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:206: case
Profile::CREATE_STATUS_REMOTE_FAIL: {
Per our chat discussion, STATUS_REMOTE_FAIL should move down to the invalid
profile creation status case since it should never happen.

[commit-bot: I haz the power, 2013-06-04 07:04:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/15774008/85004

[Pam (message me for reviews), 2013-06-04 11:36:07]

On 2013/06/04 07:04:55, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/pam%40chromium.org/15774008/85004

CQ is stuck on android_dbg_triggered_tests: slave_steps, no progress in hours.
Let's try again.

[commit-bot: I haz the power, 2013-06-04 11:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/15774008/85004

[commit-bot: I haz the power, 2013-06-04 13:18:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/15774008/85004

[commit-bot: I haz the power, 2013-06-04 15:26:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/15774008/85004

[commit-bot: I haz the power, 2013-06-04 16:38:37]

Change committed as 203976