Sync: Remove pthreads from auth_watcher.

Sync: Remove pthreads from auth_watcher.

BUG=19895
TEST=AuthWatcherTest

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

[tim (not reviewing), 2009-10-06 00:48:20]

Apologies for all the patch sets, just style fixups.

[chron_chromium.org, 2009-10-06 02:12:34]

http://codereview.chromium.org/246098/diff/7003/7005
File chrome/browser/sync/engine/auth_watcher.cc (right):

http://codereview.chromium.org/246098/diff/7003/7005#newcode75
Line 75: void AuthWatcher::ProcessGaiaAuthSuccess() {
Can we rename this to like, PersistCredentials, and then move
DoAuthenticateWithToken to the caller?

http://codereview.chromium.org/246098/diff/7003/7005#newcode97
Line 97: DoAuthenticateWithToken(results.email, gaia_->auth_token());
Can we move this to the caller? I think it's more clear.

http://codereview.chromium.org/246098/diff/7003/7005#newcode102
Line 102: void AuthWatcher::AuthenticateWithToken(const std::string& email,
Comment in .h file about which of these is supposed to be used when. Also
s/email to match gaia_email below.

http://codereview.chromium.org/246098/diff/7003/7005#newcode224
Line 224: int attempt = 0;
Unused var

http://codereview.chromium.org/246098/diff/7003/7005#newcode261
Line 261: message_loop()->PostTask(FROM_HERE, NewRunnableMethod(this,
Can we DCHECK here for which thread we're on? (if we know?) Seems like this is
safe on scm_'s thread but no other?

http://codereview.chromium.org/246098/diff/7003/7005#newcode267
Line 267: const ServerConnectionEvent& event, const std::string& auth_token) {
Can you name this param so it indicates it came from the time that the scm sent
the event? Like event_auth_token or something.

http://codereview.chromium.org/246098/diff/7003/7005#newcode319
Line 319: string mail = email;
functionize this

http://codereview.chromium.org/246098/diff/7003/7008
File chrome/browser/sync/engine/auth_watcher.h (right):

http://codereview.chromium.org/246098/diff/7003/7008#newcode69
Line 69: class AuthWatcher : public base::RefCountedThreadSafe<AuthWatcher> {
About time for a class comment.

http://codereview.chromium.org/246098/diff/7003/7004
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/246098/diff/7003/7004#newcode1193
Line 1193: auth_watcher_ = NULL;
Is this OK to do with a refptr? Will it leak a ref?

[tim (not reviewing), 2009-10-06 03:12:03]

http://codereview.chromium.org/246098/diff/7003/7005
File chrome/browser/sync/engine/auth_watcher.cc (right):

http://codereview.chromium.org/246098/diff/7003/7005#newcode75
Line 75: void AuthWatcher::ProcessGaiaAuthSuccess() {
On 2009/10/06 02:12:34, chron_chromium.org wrote:
> Can we rename this to like, PersistCredentials, and then move
> DoAuthenticateWithToken to the caller?

Done.

http://codereview.chromium.org/246098/diff/7003/7005#newcode97
Line 97: DoAuthenticateWithToken(results.email, gaia_->auth_token());
On 2009/10/06 02:12:34, chron_chromium.org wrote:
> Can we move this to the caller? I think it's more clear.

Done.

http://codereview.chromium.org/246098/diff/7003/7005#newcode102
Line 102: void AuthWatcher::AuthenticateWithToken(const std::string& email,
On 2009/10/06 02:12:34, chron_chromium.org wrote:
> Comment in .h file about which of these is supposed to be used when. Also
> s/email to match gaia_email below.

Done.

http://codereview.chromium.org/246098/diff/7003/7005#newcode224
Line 224: int attempt = 0;
On 2009/10/06 02:12:34, chron_chromium.org wrote:
> Unused var

Done.

http://codereview.chromium.org/246098/diff/7003/7005#newcode261
Line 261: message_loop()->PostTask(FROM_HERE, NewRunnableMethod(this,
On 2009/10/06 02:12:34, chron_chromium.org wrote:
> Can we DCHECK here for which thread we're on? (if we know?) Seems like this is
> safe on scm_'s thread but no other?

We could be on the syncer thread or the auth thread at the moment (Authenticator
calls in to SCM).  We have no easy way to get access to the syncer thread
either, at the moment :\

http://codereview.chromium.org/246098/diff/7003/7005#newcode267
Line 267: const ServerConnectionEvent& event, const std::string& auth_token) {
On 2009/10/06 02:12:34, chron_chromium.org wrote:
> Can you name this param so it indicates it came from the time that the scm
sent
> the event? Like event_auth_token or something.

went with auth_token_snapshot

http://codereview.chromium.org/246098/diff/7003/7005#newcode319
Line 319: string mail = email;
On 2009/10/06 02:12:34, chron_chromium.org wrote:
> functionize this

Done.

http://codereview.chromium.org/246098/diff/7003/7008
File chrome/browser/sync/engine/auth_watcher.h (right):

http://codereview.chromium.org/246098/diff/7003/7008#newcode69
Line 69: class AuthWatcher : public base::RefCountedThreadSafe<AuthWatcher> {
On 2009/10/06 02:12:34, chron_chromium.org wrote:
> About time for a class comment.

Done.

http://codereview.chromium.org/246098/diff/7003/7004
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/246098/diff/7003/7004#newcode1193
Line 1193: auth_watcher_ = NULL;
On 2009/10/06 02:12:34, chron_chromium.org wrote:
> Is this OK to do with a refptr? Will it leak a ref?

Actually scoped_refptr overrides operator= to deref, and as such it's the most
common way to clean up.

[chron_chromium.org, 2009-10-06 20:04:28]

LGTM, but need a unit test. Next CL is fine by me.

[tim (not reviewing), 2009-10-07 03:00:17]

Added AuthWatcherTest.  Please have a look!

[chron_chromium.org, 2009-10-07 20:30:04]

Looks good! LGTM