Dont retry sync in case of auth errors.

Dont retry sync in case of auth errors.

BUG=
TEST=

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

[lipalani1, 2011-08-03 23:48:40]

Please review.

[tim (not reviewing), 2011-08-04 00:24:39]

Some nits, and one remaining question in addition to offline discussion. 

Please also add a TODO for tests, and reference 90944 in the BUG=.

LGTM

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:196: if
(auth_token.empty()) {
could we put all this in syncapi_server_connection_manager, since it already
examines auth_token and SYNC_AUTH_ERROR?

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
File chrome/browser/sync/engine/net/server_connection_manager.h (right):

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.h:292: void
reset_auth_token() {
lets call this InvalidateAndClearAuthToken()  (or just InvalidateAuthToken()).

reset sounds more innocent than it is here, since we imply "invalidation" (esp
in unix_hacker_style which implies a simple, basic clear).

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.h:296:
previous_invalid_token_.assign(auth_token_);
With the method change above, lets tweak this slightly to be
previously_invalidated_token_

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.h:356: // The previous
auth token that is invalid now.
rename the var as suggested, and comment could be "The last token to have been
invalidated."

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_scheduler.cc:287: return CONTINUE;
so we always continue with canary_jobs?  don't we set that in more places than
purely the exp. backoff callback?

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_scheduler.cc:813: if (IsBackingOff() &&
wait_interval_->timer.IsRunning() &&
comment

[lipalani1, 2011-08-04 01:14:33]

taken care of your feedback.

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:196: if
(auth_token.empty()) {
CheckTime uses that code and we probably dont want to mess with that.
On 2011/08/04 00:24:40, timsteele wrote:
> could we put all this in syncapi_server_connection_manager, since it already
> examines auth_token and SYNC_AUTH_ERROR?

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
File chrome/browser/sync/engine/net/server_connection_manager.h (right):

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.h:292: void
reset_auth_token() {
On 2011/08/04 00:24:40, timsteele wrote:
> lets call this InvalidateAndClearAuthToken()  (or just InvalidateAuthToken()).
> 
> reset sounds more innocent than it is here, since we imply "invalidation" (esp
> in unix_hacker_style which implies a simple, basic clear).

Done.

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.h:296:
previous_invalid_token_.assign(auth_token_);
On 2011/08/04 00:24:40, timsteele wrote:
> With the method change above, lets tweak this slightly to be
> previously_invalidated_token_

Done.

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.h:356: // The previous
auth token that is invalid now.
On 2011/08/04 00:24:40, timsteele wrote:
> rename the var as suggested, and comment could be "The last token to have been
> invalidated."

Done.

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_scheduler.cc:287: return CONTINUE;
We call that when network connection changes. We still want to continue in that
case as well. No other places set that.
On 2011/08/04 00:24:40, timsteele wrote:
> so we always continue with canary_jobs?  don't we set that in more places than
> purely the exp. backoff callback?

http://codereview.chromium.org/7563017/diff/7/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_scheduler.cc:813: if (IsBackingOff() &&
wait_interval_->timer.IsRunning() &&
On 2011/08/04 00:24:40, timsteele wrote:
> comment

Done.