Perform /ListAccounts check before session merge to see if there is a need for session merge at all.

Perform /ListAccounts check before session merge to see if there is a need for session merge at all.

BUG=338543
TEST=manual, added OAuth2Test.PRE_PRE_MergeSession to test existing user with fresh cookies, modified OAuth2Test.PRE_MergeSession to test existing user with stale GAIA cookies

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247862

[zel, 2014-01-28 01:46:10]

+isherman for histograms part

[xiyuan, 2014-01-28 03:51:31]

https://codereview.chromium.org/148463004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/148463004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/oauth2_login_manager.cc:338: bool is_pre_merge =
(state_ == SESSION_RESTORE_PREPARING);
nit: Move this before RecordCookiesCheckOutcome and use it as arg to call
RecordCookiesCheckOutcome.

https://codereview.chromium.org/148463004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/oauth2_login_manager.cc:339: if (is_pre_merge &&
!connection_error) {
Should we schedule retry for connection failure? Otherwise, we could end up in
SESSION_RESTORE_PREPARING indefinitely.

[zel, 2014-01-29 02:15:34]

https://codereview.chromium.org/148463004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/148463004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/oauth2_login_manager.cc:338: bool is_pre_merge =
(state_ == SESSION_RESTORE_PREPARING);
On 2014/01/28 03:51:32, xiyuan wrote:
> nit: Move this before RecordCookiesCheckOutcome and use it as arg to call
> RecordCookiesCheckOutcome.

Done.

https://codereview.chromium.org/148463004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/oauth2_login_manager.cc:339: if (is_pre_merge &&
!connection_error) {
On 2014/01/28 03:51:32, xiyuan wrote:
> Should we schedule retry for connection failure? Otherwise, we could end up in
> SESSION_RESTORE_PREPARING indefinitely. 

Fixed - good catch!

No need to retrying since that logic for this call is already driven by
OAuth2LoginVerifier::DelayNetworkCall().

[zel, 2014-01-29 02:20:34]

+asvitkine for histograms part

[xiyuan, 2014-01-29 03:26:47]

lgtm

[Alexei Svitkine (slow), 2014-01-29 14:54:11]

https://codereview.chromium.org/148463004/diff/110001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/148463004/diff/110001/chrome/browser/chromeos...
chrome/browser/chromeos/login/oauth2_login_manager.cc:368:
"OAuth2Login.PostMergeVerification",
This is not valid to do. The UMA histogram macros cache the histogram object, so
using a variable name from the same callsites will result in incorrect behaviour
when it gets hit with a different name value.

Instead, use an if/else block.

[zel, 2014-01-29 17:21:42]

https://codereview.chromium.org/148463004/diff/110001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/148463004/diff/110001/chrome/browser/chromeos...
chrome/browser/chromeos/login/oauth2_login_manager.cc:368:
"OAuth2Login.PostMergeVerification",
On 2014/01/29 14:54:12, Alexei Svitkine wrote:
> This is not valid to do. The UMA histogram macros cache the histogram object,
so
> using a variable name from the same callsites will result in incorrect
behaviour
> when it gets hit with a different name value.
> 
> Instead, use an if/else block.

Done.

[Alexei Svitkine (slow), 2014-01-29 17:41:22]

lgtm with optional style nit

https://codereview.chromium.org/148463004/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/148463004/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/oauth2_login_manager.cc:368:
"OAuth2Login.PreMergeVerification",
Nit: Move this on the line above and align the other args. Same below.

[zel, 2014-01-29 17:51:45]

https://codereview.chromium.org/148463004/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/oauth2_login_manager.cc (right):

https://codereview.chromium.org/148463004/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/oauth2_login_manager.cc:368:
"OAuth2Login.PreMergeVerification",
On 2014/01/29 17:41:22, Alexei Svitkine wrote:
> Nit: Move this on the line above and align the other args. Same below.

Done.

[commit-bot: I haz the power, 2014-01-29 17:53:03]

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

[commit-bot: I haz the power, 2014-01-30 07:05:05]

Change committed as 247862