Made OAuth token verification and user seession cookie retrieval process robust on transient netw...

Build break fix.

BUG=none
TEST=none
TBR=xiyuan

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

[xiyuan, 2011-11-17 22:34:58]

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:1: // Use of this source code is
governed by a BSD-style license that can be
Restore the license?

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:169: const int
kOAuthVerificationRestartDelay = 5000;    // ms
nit: two-spaces before line end comment

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:251: }
Maybe we should check oauth_fetcher_->HasPendingFetch() and
gaia_fetcher_->HasPendingFetch() here so that we will not fire two requests at
the same time?

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:302: //   
OnOAuthLoginFailure(GoogleServiceAuthError::FromConnectionError(400));
nit: remove this?

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:1295: std::string());
nit: align with args above

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:1306:
oauth_login_verifier_->ContinueVerification();
What if we already have a pending OAuthLogin request? See  comments in
ContinueVerification

[zel, 2011-11-18 03:53:39]

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:1: // Use of this source code is
governed by a BSD-style license that can be
On 2011/11/17 22:34:58, xiyuan wrote:
> Restore the license?

Done.

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:169: const int
kOAuthVerificationRestartDelay = 5000;    // ms
On 2011/11/17 22:34:58, xiyuan wrote:
> nit: two-spaces before line end comment

Done.

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:251: }
On 2011/11/17 22:34:58, xiyuan wrote:
> Maybe we should check oauth_fetcher_->HasPendingFetch() and
> gaia_fetcher_->HasPendingFetch() here so that we will not fire two requests at
> the same time?

Good point. Done.

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:302: //   
OnOAuthLoginFailure(GoogleServiceAuthError::FromConnectionError(400));
On 2011/11/17 22:34:58, xiyuan wrote:
> nit: remove this?

Done.

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:1295: std::string());
On 2011/11/17 22:34:58, xiyuan wrote:
> nit: align with args above

Done.

http://codereview.chromium.org/8586007/diff/1006/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:1306:
oauth_login_verifier_->ContinueVerification();
On 2011/11/17 22:34:58, xiyuan wrote:
> What if we already have a pending OAuthLogin request? See  comments in
> ContinueVerification

ContinueVerification will now check for pending requests.

[xiyuan, 2011-11-18 04:05:05]

LGTM

[commit-bot: I haz the power, 2011-11-18 05:19:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/8586007/12001

[commit-bot: I haz the power, 2011-11-18 06:24:36]

Change committed as 110667

[Nikita (slow), 2011-11-22 09:14:39]

LGTM

http://codereview.chromium.org/8586007/diff/12001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8586007/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:256:
kOAuthVerificationRestartDelay);
verification_count_ is increased below this block.
If user is constantly offline, we'll be scheduling ContinueVerification() each
10s.

Since we also have handler for OnOnlineStateChanged(), verification_count_++
should be moved before this block.

http://codereview.chromium.org/8586007/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:327: void OnCookueFetchFailed(const
GoogleServiceAuthError& error) {
nit: fix spelling OnCookieFetchFailed

[Nikita (slow), 2011-11-22 09:21:56]

http://codereview.chromium.org/8586007/diff/12001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8586007/diff/12001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:256:
kOAuthVerificationRestartDelay);
On 2011/11/22 09:14:39, Nikita Kostylev wrote:
> verification_count_ is increased below this block.
> If user is constantly offline, we'll be scheduling ContinueVerification() each
> 10s.
> 
> Since we also have handler for OnOnlineStateChanged(), verification_count_++
> should be moved before this block.

Reading it differently, I think that initial idea was to ignore any verification
errors when network is not connected / in captive portal.

I think scheduling ContinueVerification() in block 254-256 might be omitted and
rely on OnOnlineStateChanged() handler. From there on verification_count_ will
be increased since online == true already.