Do not clear tokens from db when the user enters new credentials.

When the user enters new credentials to sync we clear the data from db as well as in memory. However if at this state the user restarts the browser sync will not initialize the next time thinking that user has not setup sync.

This fix prevents the data from being cleared from db. So on restart we can still initialize sync and display auth error.

BUG=103861
TEST=manual by running the repro, sync_integration_tests.exe and unit_tests.exe


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109744

[lipalani1, 2011-11-11 02:46:49]

Please review.
Also with an eye towards whether this could break any other service that depends
on token service.

[tim (not reviewing), 2011-11-11 05:38:01]

On 2011/11/11 02:46:49, lipalani1 wrote:
> Please review.
> Also with an eye towards whether this could break any other service that
depends
> on token service.

Hm.  The only thing that should control "thinking the user has / has not set up
sync" is the SyncHasSetupCompleted pref.  That's always been the expectation of
the code... if some part isn't like that now, then that part seems like a bug.

[lipalani, 2011-11-11 05:41:37]

this again is deliberate as the username is missing. We can change the
other piece but we should not clear user name. So changing the other piece
seems unnecessary.
On Nov 10, 2011 9:38 PM, <tim@chromium.org> wrote:

> On 2011/11/11 02:46:49, lipalani1 wrote:
>
>> Please review.
>> Also with an eye towards whether this could break any other service that
>>
> depends
>
>> on token service.
>>
>
> Hm.  The only thing that should control "thinking the user has / has not
> set up
> sync" is the SyncHasSetupCompleted pref.  That's always been the
> expectation of
> the code... if some part isn't like that now, then that part seems like a
> bug.
>
>
http://codereview.chromium.**org/8515030/<http://codereview.chromium.org/8515...
>

[tim (not reviewing), 2011-11-11 05:53:03]

We store the username in the syncdb "share_info" table.  This was
originally used so that things could init themselves regardless of auth
state...

On Thu, Nov 10, 2011 at 9:41 PM, Lingeshwaran Palaniappan <
lipalani@google.com> wrote:

> this again is deliberate as the username is missing. We can change the
> other piece but we should not clear user name. So changing the other piece
> seems unnecessary.
>  On Nov 10, 2011 9:38 PM, <tim@chromium.org> wrote:
>
>> On 2011/11/11 02:46:49, lipalani1 wrote:
>>
>>> Please review.
>>> Also with an eye towards whether this could break any other service that
>>>
>> depends
>>
>>> on token service.
>>>
>>
>> Hm.  The only thing that should control "thinking the user has / has not
>> set up
>> sync" is the SyncHasSetupCompleted pref.  That's always been the
>> expectation of
>> the code... if some part isn't like that now, then that part seems like a
>> bug.
>>
>>
http://codereview.chromium.**org/8515030/<http://codereview.chromium.org/8515...
>>
>

[tim (not reviewing), 2011-11-11 18:46:47]

Ok, chatted with Lingesh and this LGTM with a few fallout bugs we can follow up
on.

1) If we don't need things like GetAuthenticatedUsername in sync_manager, lets
remove it.
2) Let's add back a switch like the one from
http://codereview.chromium.org/500120, not sure why we lost it
3) Look at feasibility of adding a test here, even if it's only for a part of
the chain.

On 2011/11/11 05:53:03, timsteele wrote:
> We store the username in the syncdb "share_info" table.  This was
> originally used so that things could init themselves regardless of auth
> state...
> 
> On Thu, Nov 10, 2011 at 9:41 PM, Lingeshwaran Palaniappan <
> mailto:lipalani@google.com> wrote:
> 
> > this again is deliberate as the username is missing. We can change the
> > other piece but we should not clear user name. So changing the other piece
> > seems unnecessary.
> >  On Nov 10, 2011 9:38 PM, <mailto:tim@chromium.org> wrote:
> >
> >> On 2011/11/11 02:46:49, lipalani1 wrote:
> >>
> >>> Please review.
> >>> Also with an eye towards whether this could break any other service that
> >>>
> >> depends
> >>
> >>> on token service.
> >>>
> >>
> >> Hm.  The only thing that should control "thinking the user has / has not
> >> set up
> >> sync" is the SyncHasSetupCompleted pref.  That's always been the
> >> expectation of
> >> the code... if some part isn't like that now, then that part seems like a
> >> bug.
> >>
> >>
>
http://codereview.chromium.**org/8515030/%3Chttp://codereview.chromium.org/85...>
> >>
> >

[Nicolas Zea, 2011-11-11 19:24:47]

drive by comment since I was curious about this change (no need to wait for my
lgtm)

http://codereview.chromium.org/8515030/diff/1/chrome/browser/sync/signin_mana...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/8515030/diff/1/chrome/browser/sync/signin_mana...
chrome/browser/sync/signin_manager.cc:174: CleanupNotificationRegistration();
you can get rid of this call.

[lipalani1, 2011-11-11 19:38:31]

Answering Tim's questions:
1. it turns out GetAuthenticatedUserName is used too :) The code seems to use
the above half the time and the other half time seems to get it from
signin_manager. I think there are adv and disadv to getting it from either
place(db is accessible from all threads, sign in manager only from UI.) We just
need to zero in on one and stick with it.
2. Unit test is written.
3. I will add the flag for cred invalidation as a seperate CL.

http://codereview.chromium.org/8515030/diff/1/chrome/browser/sync/signin_mana...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/8515030/diff/1/chrome/browser/sync/signin_mana...
chrome/browser/sync/signin_manager.cc:174: CleanupNotificationRegistration();
On 2011/11/11 19:24:47, nzea wrote:
> you can get rid of this call.

Done.

[tim (not reviewing), 2011-11-11 20:04:01]

LGTM

[Rick Campbell, 2011-11-11 21:01:58]

lgtm as well

[commit-bot: I haz the power, 2011-11-11 23:29:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/8515030/8003

[commit-bot: I haz the power, 2011-11-12 00:43:21]

Change committed as 109744