[sync] Separate sign in from sync on Desktop Chrome

[sync] Separate sign in from sync on Desktop Chrome

As of today, when a user disables sync on desktop chrome, we
automatically sign out too. With recent changes that have gone in under
the covers, this is no longer necessary. It is possible for the user to
choose not to sync any datatypes, and still be signed in to chrome.

This patch does multiple things:
- Adds a "Sync nothing" option to the drop-down menu in the advanced
  sync setup dialog.
- Detaches sign in from sync by no longer calling SignOut when the user
  disables sync.
- Updates the Sign in / Users section of the chrome settings page and the
  advanced sync settings dialog on Desktop Chrome and Chrome OS.
- Eliminates the re-authentication step when the user enables sync while
  being already signed in.
- Dismisses the advanced sync settings dialog if it is still visible when the
  user gets signed out in the background.
- Adds new strings and modifies existing strings to account for the new
  UI.

BUG=229860, 237051, 230348, 238938, 240436, 128692, 94814
TEST=Exercise the advanced sync settings dialog to enable and disable sync
R=atwilson@chromium.org, estade@chromium.org

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

[Raghu Simha, 2013-05-08 07:06:44]

Drew: Overall review.
Tim: Owners approval for chrome/browser/sync.
Evan: Owners approval for chrome/browser/resources.

Evan, I've added myself as an owner of sync_setup_overlay, since the file is
specific to sync, and has lacked clear ownership for a while. Let me know if
you're fine with this.

[Raghu Simha, 2013-05-11 02:37:10]

Ping. This is now ready for a full review.

[Evan Stade, 2013-05-13 19:07:50]

OWNERS change lgtm

[Andrew T Wilson (Slow), 2013-05-14 16:48:27]

Sorry, still working on this - I'm swamped with review requests, but I got
through the JS part! Woo!

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:46: var dataTypeBoxes_ = {};
Can you describe what the dictionary holds (what are the keys, what are the
values, at least examples + types)? Also, how is this initialized (lets say that
sync is already set to CHOOSE_WHAT_TO_SYNC when you bring up the dialog, then
you set the dropdown to SYNC_EVERYTHING, then you go back to CHOOSE_WHAT_TO_SYNC
-- this should restore the checkboxes based on what's in this data structure. At
what point does this data structure get initialized)?

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:195: if ('bookmarksSynced' in
dataTypeBoxes_)
What does it mean to not have 'bookmarksSynced' in this dictionary?

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:342: 'syncNothing': syncNothing,
nit: I'm slightly nervous about this because it's redundant with
syncAllDataTypes (i.e. what does it mean to have syncNothing = true and
syncAllDataTypes=true at the same time?) - this is a sign that perhaps these
variables should be combined into one?

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:424:
dataTypeBoxes_['bookmarksSynced'] = $('bookmarks-checkbox').checked;
I'm OK with this, but I'm wondering if there's some way to eliminate some of
this boilerplate, maybe by defining a "prefName" attribute on the DOM element.
This would let you do this:

$('bookmarks-checkbox').onclick = handleDataTypeClick;

function handleDataTypeClick() {
  dataTypeBoxes_[this.prefName] = this.checked;
}

That way every checkbox could use the same onclick handler.

Anyhow, you don't have to do this, but if you're interested in removing some
boilerplate you could really simplify this code.

Alternatively, you could just have a single onclick handler that just rebuilt
dataTypeBoxes_ entirely by querying the various DOM elements rather than setting
only a single attribute on each click.

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:646:
$('sync-select-datatypes').selectedIndex = 2;
Consider using something other than '2' here.

[Raghu Simha, 2013-05-15 02:09:54]

Drew, PTAL.

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:46: var dataTypeBoxes_ = {};
On 2013/05/14 16:48:27, Andrew T Wilson wrote:
> Can you describe what the dictionary holds (what are the keys, what are the
> values, at least examples + types)? Also, how is this initialized (lets say
that
> sync is already set to CHOOSE_WHAT_TO_SYNC when you bring up the dialog, then
> you set the dropdown to SYNC_EVERYTHING, then you go back to
CHOOSE_WHAT_TO_SYNC
> -- this should restore the checkboxes based on what's in this data structure.
At
> what point does this data structure get initialized)?

Done. I've beefed up this comment to include what you suggest here. There's also
a detailed description of how cache is used at the top of showConfigure_().

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:195: if ('bookmarksSynced' in
dataTypeBoxes_)
On 2013/05/14 16:48:27, Andrew T Wilson wrote:
> What does it mean to not have 'bookmarksSynced' in this dictionary?

This is to prevent us from trying to modify any hidden checkboxes, which aren't
cached in the first place. Some data types are not registered under all
circumstances, and we don't want to mess with their values here. I've updated
the comment to mention this.

I've also replaced the implementation with a simpler version that uses the
existing "id" attribute instead of brute-forcing the list of checkboxes.

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:342: 'syncNothing': syncNothing,
On 2013/05/14 16:48:27, Andrew T Wilson wrote:
> nit: I'm slightly nervous about this because it's redundant with
> syncAllDataTypes (i.e. what does it mean to have syncNothing = true and
> syncAllDataTypes=true at the same time?) - this is a sign that perhaps these
> variables should be combined into one?

It's impossible for both values to be true, since we initialize them using
$('sync-select-datatypes').selectedIndex, which can make one or the other true,
but not both.

The reason the values are not redundant is that the drop-down can have a third
value, which is CHOOSE_WHAT_TO_SYNC, in which case both booleans are false.

I did consider eliminating the two booleans and passing in an enum to the C++
layer, but it involved unnecessarily rewriting a bunch of code in
sync_setup_handler and other layers. The value of syncAll is used all over the
place in sync setup code, while syncNothing is used in just one spot, in order
to disable sync. I figured it would be less disruptive to send in two booleans
than to refactor all the set up code to use an enum.

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:424:
dataTypeBoxes_['bookmarksSynced'] = $('bookmarks-checkbox').checked;
On 2013/05/14 16:48:27, Andrew T Wilson wrote:
> I'm OK with this, but I'm wondering if there's some way to eliminate some of
> this boilerplate, maybe by defining a "prefName" attribute on the DOM element.

The DOM elements already have an "id" attribute that I can use.

> This would let you do this:
> 
> $('bookmarks-checkbox').onclick = handleDataTypeClick;
> 
> function handleDataTypeClick() {
>   dataTypeBoxes_[this.prefName] = this.checked;
> }
> 
> That way every checkbox could use the same onclick handler.

Good idea. Done.

> Anyhow, you don't have to do this, but if you're interested in removing some
> boilerplate you could really simplify this code.
> 
> Alternatively, you could just have a single onclick handler that just rebuilt
> dataTypeBoxes_ entirely by querying the various DOM elements rather than
setting
> only a single attribute on each click.

This is possible, but I don't believe we'll gain much by adding a single onclick
handler, because it will still involve enumerating the list of checkboxes and
checking if they are hidden or not. Also, Content Security Policy forbids event
handlers from being defined inline in html, so we'll have to do it in
JavaScript.

Having said that, the latest patch has considerably less boilerplate. Let me
know what you think.

https://codereview.chromium.org/14691004/diff/73002/chrome/browser/resources/...
chrome/browser/resources/sync_setup_overlay.js:646:
$('sync-select-datatypes').selectedIndex = 2;
On 2013/05/14 16:48:27, Andrew T Wilson wrote:
> Consider using something other than '2' here.

Forgot to use the new enum at this spot. Done.

[Andrew T Wilson (Slow), 2013-05-15 15:01:24]

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:1920: if (!sync_initialized() &&
GetAuthError().state() != AuthError::NONE) {
This doesn't seem like the right change - we want is_auth_in_progress to be true
during initial signin, but with this change it won't be (because GetAuthError()
starts out as NONE).

Also, the comment below seems wrong. If the user signs in and re-enables sync
while last_auth_error != NONE, auth is *not* complete until
UpdateAuthErrorState() is called (we want to wait to see if the backend is able
to contact the sync server).

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/sync...
File chrome/browser/sync/sync_ui_util.cc (right):

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/sync...
chrome/browser/sync/sync_ui_util.cc:60: if
(status.sync_protocol_error.error_type == syncer::NOT_MY_BIRTHDAY) {
If you clear the sync dashboard, then restart chrome, will the error_type still
be set to NOT_MY_BIRTHDAY? I'm just surprised that we would persist this across
restarts.

If it's not persisted, then there's not really any point in differentiating
these two cases - we might as well just display the same string in both cases,
since that's what we'll display on the next restart anyway.

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/sync...
chrome/browser/sync/sync_ui_util.cc:236: if (status_label && link_label) {
Are we sure this is OK? Aren't there callers that pass in only the status_label
(no link_label)? Doesn't this break those callers by not setting the status
label?

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/sync...
chrome/browser/sync/sync_ui_util.cc:257: // The user is signed in, but sync is
disabled.
Hmmm. Do you need the !service->HasSyncSetupCompleted() here? Isn't the
service->HasSyncSetupCompleted() == true already handled by the if clause
starting on line 136?

Also, peer bonus for the first person to break up this routine - a 150-line if
statement is basically unmaintainable.

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:154: 
Add a DCHECK(!(sync_nothing && sync_everything)) since it's not valid to set
both?

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:907: error.state() ==
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS) {
Why would we only do this on INVALID_GAIA_CREDENTIALS but not other errors
(like, account_disabled, etc)?

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:1165: configuring_sync_ &&
You can't really rely on configuring_sync_ - it's just there to allow UMA
tracking, but it's not correctly set in all cases IIRC. Perhaps you can omit
relying on it and just always disable sync if sync setup has not yet completed?

[Raghu Simha, 2013-05-16 02:13:20]

Drew, thanks for the review. PTAL.

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:1920: if (!sync_initialized() &&
GetAuthError().state() != AuthError::NONE) {
On 2013/05/15 15:01:24, Andrew T Wilson wrote:
> This doesn't seem like the right change - we want is_auth_in_progress to be
true
> during initial signin, but with this change it won't be (because
GetAuthError()
> starts out as NONE).
> 
> Also, the comment below seems wrong. If the user signs in and re-enables sync
> while last_auth_error != NONE, auth is *not* complete until
> UpdateAuthErrorState() is called (we want to wait to see if the backend is
able
> to contact the sync server).

I had orignally changed this logic to handle the case where the user hits an
auth error while re-setting up sync, and the backend was already initialized by
the time the user finished entering their newest gaia passphrase.
SigninTracker::GetSigninState was silently dropping the OnStateChanged callback
resulting from the GOOGLE_SIGNIN_SUCCESSFUL notification in that case, and was
never getting to the SIGNIN_COMPLETE state.

Now that I've switched to the flow where the user sees a link on the settings
page during an auth error, this change is no longer required. I've reverted this
section to what it was originally. I've run this by Rachel, and she's fine with
the string change.

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/sync...
File chrome/browser/sync/sync_ui_util.cc (right):

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/sync...
chrome/browser/sync/sync_ui_util.cc:60: if
(status.sync_protocol_error.error_type == syncer::NOT_MY_BIRTHDAY) {
On 2013/05/15 15:01:24, Andrew T Wilson wrote:
> If you clear the sync dashboard, then restart chrome, will the error_type
still
> be set to NOT_MY_BIRTHDAY? I'm just surprised that we would persist this
across
> restarts.

I just checked -- birthday errors are not persisted across restarts on desktop
Chrome or Chrome OS.

> If it's not persisted, then there's not really any point in differentiating
> these two cases - we might as well just display the same string in both cases,
> since that's what we'll display on the next restart anyway.

I originally kept the two messages around so that if a user cleared their data
via the dashboard while the settings page was visible, they would see a message
that indicated exactly what happened.

I agree with your point though. I've gone ahead and replaced the old "cleared
via dashboard" message with a single "stopped on this device" message.

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/sync...
chrome/browser/sync/sync_ui_util.cc:236: if (status_label && link_label) {
On 2013/05/15 15:01:24, Andrew T Wilson wrote:
> Are we sure this is OK? Aren't there callers that pass in only the
status_label
> (no link_label)? Doesn't this break those callers by not setting the status
> label?

Yes, this is okay. There's a check on line 129 that makes sure that if one label
is available, the other one is too.

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/sync/sync...
chrome/browser/sync/sync_ui_util.cc:257: // The user is signed in, but sync is
disabled.
On 2013/05/15 15:01:24, Andrew T Wilson wrote:
> Hmmm. Do you need the !service->HasSyncSetupCompleted() here? Isn't the
> service->HasSyncSetupCompleted() == true already handled by the if clause
> starting on line 136?

You're right. I've removed the redundant check.

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:154: 
On 2013/05/15 15:01:24, Andrew T Wilson wrote:
> Add a DCHECK(!(sync_nothing && sync_everything)) since it's not valid to set
> both?

Done.

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:907: error.state() ==
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS) {
On 2013/05/15 15:01:24, Andrew T Wilson wrote:
> Why would we only do this on INVALID_GAIA_CREDENTIALS but not other errors
> (like, account_disabled, etc)?

Good catch. I've added those cases, and we now match the auth failure cases in
signin_ui_util.cc.

https://codereview.chromium.org/14691004/diff/116001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:1165: configuring_sync_ &&
On 2013/05/15 15:01:24, Andrew T Wilson wrote:
> You can't really rely on configuring_sync_ - it's just there to allow UMA
> tracking, but it's not correctly set in all cases IIRC. Perhaps you can omit
> relying on it and just always disable sync if sync setup has not yet
completed?

If the user clicks cancel, we want to disable sync if setup hasn't been
completed. However, if the user tried to re-enable sync and saw an auth error
(say, due to a password that was changed in the interim), we want to close the
setup dialog and show the user an error message on the settings page. In this
case, we don't want to disable sync. Instead, we'd like to leave the backend
initialized (about:sync shows that it's in a deferred state), and give the user
the chance to re-auth and continue enabling sync.

I was using |configuring_sync_| to differentiate between the two cases, but what
you say makes sense. I've moved this block to higher up in CloseSyncSetup, and
we now disable sync only if the user actually canceled sync.

The other case where we want to disable sync is when the user selected "Sync
nothing". That's already taken care of by the handler for syncNothing.

[Andrew T Wilson (Slow), 2013-05-16 08:39:30]

I'm going to LGTM this so you're not blocked, assuming that you are able to
resolve my concern about calling CloseOverlay() (i.e. if you can change this to
a call to CloseSyncSetup() instead).

Finally, it would probably be valuable to test this with a chromeos build. You
can do this like so:

GYP_DEFINES="chromeos=1" gclient runhooks
ninja -C out/Debug chrome
out/Debug/chrome --login-manager --login-profile=rsimha@chromium.org
(or whatever acct you want to use)

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:574:
service->SetSetupInProgress(false);
Where was this done previously (i.e. why is this necessary now)?

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:918: CloseOverlay();
It seems dangerous to just call CloseOverlay() - CloseSyncSetup() does lots of
good cleanup like freeing the SigninTracker. I think the code below that calls
CloseOverlay() is probably incorrect. In fact, I think there's lots of code that
calls CloseOverlay() (like DisplayGaiaSuccessAndClose(), etc that are doing the
wrong thing here too - but that's for another CL :)

Can we just fall through to use the existing cases below? Especially since roger
is driving a CL to remove all of the non-web-based-signin-flow code, so all of
this could just turn into a single call to CloseSyncSetup().

If CloseSyncSetup() does something you don't want to happen here, then we should
change it to do the right thing.

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:963: std::string json;
Can you please add a DCHECK(!signin_tracker.get()) here - it's an error to ever
get here with a SigninTracker active.

[Raghu Simha, 2013-05-17 05:14:17]

Drew, your comments have been addressed by the latest patch. Let me know if you
still have concerns about CloseOverlay() calling CloseSyncSetup().

I'll send you a separate CL to clean up the logic once Roger eliminates the
non-webui flows.

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:574:
service->SetSetupInProgress(false);
On 2013/05/16 08:39:30, Andrew T Wilson wrote:
> Where was this done previously (i.e. why is this necessary now)?

Earlier, this was being called in CloseSyncSetup as part of the
sign-out-if-not-fully-setup block, and was being run irrespective of whether the
user canceled sign in or not. The above call is necessary now that the
CloseSyncSetup has been refactored out to disable sync only in the cancel case,
since we want to leave the backend initialized in the user-needs-to-re-auth
case.

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:918: CloseOverlay();
On 2013/05/16 08:39:30, Andrew T Wilson wrote:
> It seems dangerous to just call CloseOverlay() - CloseSyncSetup() does lots of
> good cleanup like freeing the SigninTracker. I think the code below that calls
> CloseOverlay() is probably incorrect. In fact, I think there's lots of code
that
> calls CloseOverlay() (like DisplayGaiaSuccessAndClose(), etc that are doing
the
> wrong thing here too - but that's for another CL :)
> 
> Can we just fall through to use the existing cases below? Especially since
roger
> is driving a CL to remove all of the non-web-based-signin-flow code, so all of
> this could just turn into a single call to CloseSyncSetup().
> 
> If CloseSyncSetup() does something you don't want to happen here, then we
should
> change it to do the right thing.

As a matter of fact, CloseOverlay() calls CloseSyncSetup(), and therefore, does
all the good cleanup you mentioned. In addition to calling CloseSyncSetup(), it
also closes the overlay window. This has been true since this CL:
https://chromiumcodereview.appspot.com/9295044/diff/38001/chrome/browser/ui/w...

The case I am trying to handle here is when sync has been stopped, and the user
tries to re-enable it after the gaia password has changed. If we merely call
CloseSyncSetup without calling CloseOverlay, all the cleanup is done, but the
spinner window remains visible, and hides the "sign in again" link that shows up
on the main settings page.

I've collapsed the if block I originally added into one of the existing cases.
I've also retained the call to CloseOverlay(), which explicitly calls
CloseSyncSetup(). I've updated the description of CloseOverlay() in the header
file. If you believe CloseOverlay() shouldn't be calling CloseSyncSetup(), let
me know, and I'll change it in another CL.

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:963: std::string json;
On 2013/05/16 08:39:30, Andrew T Wilson wrote:
> Can you please add a DCHECK(!signin_tracker.get()) here - it's an error to
ever
> get here with a SigninTracker active.

Done.

[commit-bot: I haz the power, 2013-05-17 05:14:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsimha@chromium.org/14691004/139004

[commit-bot: I haz the power, 2013-05-17 07:20:05]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Andrew T Wilson (Slow), 2013-05-17 07:26:10]

lgtm

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/14691004/diff/152001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:918: CloseOverlay();
On 2013/05/17 05:14:17, rsimha wrote:
> On 2013/05/16 08:39:30, Andrew T Wilson wrote:
> > It seems dangerous to just call CloseOverlay() - CloseSyncSetup() does lots
of
> > good cleanup like freeing the SigninTracker. I think the code below that
calls
> > CloseOverlay() is probably incorrect. In fact, I think there's lots of code
> that
> > calls CloseOverlay() (like DisplayGaiaSuccessAndClose(), etc that are doing
> the
> > wrong thing here too - but that's for another CL :)
> > 
> > Can we just fall through to use the existing cases below? Especially since
> roger
> > is driving a CL to remove all of the non-web-based-signin-flow code, so all
of
> > this could just turn into a single call to CloseSyncSetup().
> > 
> > If CloseSyncSetup() does something you don't want to happen here, then we
> should
> > change it to do the right thing.
> 
> As a matter of fact, CloseOverlay() calls CloseSyncSetup(), and therefore,
does
> all the good cleanup you mentioned. In addition to calling CloseSyncSetup(),
it
> also closes the overlay window. This has been true since this CL:
>
https://chromiumcodereview.appspot.com/9295044/diff/38001/chrome/browser/ui/w...
> 
> The case I am trying to handle here is when sync has been stopped, and the
user
> tries to re-enable it after the gaia password has changed. If we merely call
> CloseSyncSetup without calling CloseOverlay, all the cleanup is done, but the
> spinner window remains visible, and hides the "sign in again" link that shows
up
> on the main settings page.
> 
> I've collapsed the if block I originally added into one of the existing cases.
> I've also retained the call to CloseOverlay(), which explicitly calls
> CloseSyncSetup(). I've updated the description of CloseOverlay() in the header
> file. If you believe CloseOverlay() shouldn't be calling CloseSyncSetup(), let
> me know, and I'll change it in another CL.

Hah, apparently I forgot about my own refactorings :)

[Raghu Simha, 2013-05-17 19:17:59]

Committed patchset #6 manually as r200867 (presubmit successful).