Make the change in ProfileSyncService such that it declares success

Make the change in ProfileSyncService such that it declares success
for new users only when the oauth login token is successfully generated.
Make sure for existing logged in users, it continues to setup sync
properly on startup.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112714

[Munjal (Google), 2011-12-01 01:56:44]

Hey Tim/Lingesh/Rick,

Can you give your opinion on this approach of making PSS check OAuth2 login
token also for new users?

[Rick Campbell, 2011-12-01 18:43:47]

LGTM

[Andrew T Wilson (Slow), 2011-12-01 22:07:01]

LGTM after resolving my comments below.

http://codereview.chromium.org/8760019/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/8760019/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service.cc:1453: case
chrome::NOTIFICATION_TOKEN_REQUEST_FAILED: {
Can you either check the token type here (and below in TOKEN_AVAILABLE) or just
add a TODO() that we should check the token type? We should ignore
failure/available notifications for tokens we don't care about.

http://codereview.chromium.org/8760019/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service.cc:1470: if (AreCredentialsAvailable())
{
Can you add a note here about why we are intentionally allowing sync startup to
happen even though we may not have an oauth token?

[Munjal (Google), 2011-12-01 22:52:58]

http://codereview.chromium.org/8760019/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/8760019/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service.cc:1453: case
chrome::NOTIFICATION_TOKEN_REQUEST_FAILED: {
On 2011/12/01 22:07:01, Andrew T Wilson wrote:
> Can you either check the token type here (and below in TOKEN_AVAILABLE) or
just
> add a TODO() that we should check the token type? We should ignore
> failure/available notifications for tokens we don't care about.

Done.

http://codereview.chromium.org/8760019/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service.cc:1470: if (AreCredentialsAvailable())
{
On 2011/12/01 22:07:01, Andrew T Wilson wrote:
> Can you add a note here about why we are intentionally allowing sync startup
to
> happen even though we may not have an oauth token?

Done. The comment is a bit long but this is an important thing to explain.

[Andrew T Wilson (Slow), 2011-12-02 00:52:35]

LGTM still with changes below.

http://codereview.chromium.org/8760019/diff/5003/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/8760019/diff/5003/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1468:
TokenService::TokenRequestFailedDetails token_details =
I'd change this to const TokenService::TokenRequestFailedDetails&, so we don't
have to copy the structure.

http://codereview.chromium.org/8760019/diff/5003/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1479:
TokenService::TokenAvailableDetails token_details =
See previous comment.

http://codereview.chromium.org/8760019/diff/5003/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1493: // This notification gets
fired when TokenService loads  the tokens
nit: double-space between "loads  the".

[Munjal (Google), 2011-12-02 02:14:01]

Note that I needed a fix for unit tests also. Did that.

http://codereview.chromium.org/8760019/diff/5003/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/8760019/diff/5003/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1468:
TokenService::TokenRequestFailedDetails token_details =
On 2011/12/02 00:52:35, Andrew T Wilson wrote:
> I'd change this to const TokenService::TokenRequestFailedDetails&, so we don't
> have to copy the structure.

Done.

http://codereview.chromium.org/8760019/diff/5003/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1479:
TokenService::TokenAvailableDetails token_details =
On 2011/12/02 00:52:35, Andrew T Wilson wrote:
> See previous comment.

Done.

http://codereview.chromium.org/8760019/diff/5003/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1493: // This notification gets
fired when TokenService loads  the tokens
On 2011/12/02 00:52:35, Andrew T Wilson wrote:
> nit: double-space between "loads  the".

Done.

[Andrew T Wilson (Slow), 2011-12-02 02:34:58]

Still LGTM.