Handle account removal correctly on all platforms.

Handle account removal correctly on all platforms.
Don't validate refresh token before usage to reduce network traffic.

BUG=359700, 416612
Committed: https://crrev.com/6128c200e138d3d7c52aae5e01d0af36a48b1706
Cr-Commit-Position: refs/heads/master@{#296715}

[Roger Tawa OOO till Jul 10th, 2014-09-22 20:31:52]

Patchset #3 (id:40001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-09-22 20:36:43]

rogerta@chromium.org changed reviewers:
+ mlerman@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-09-22 20:36:44]

Hi Mike,

Can you please take a look?  Thanks.

[Mike Lerman, 2014-09-23 14:27:47]

Comments :)

https://codereview.chromium.org/590113004/diff/60001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/590113004/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:386:
"Signin.Reconciler.AddedToChrome.FirstRun", 0, 1);
All of these references to the AddedToChrome metric will need to be changed.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:19: #include
"components/signin/core/browser/signin_oauth_helper.h"
We no longer have any references to the signin_oauth_helper, I think.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:364:
chrome_accounts_.end(),
nit: 1 space less here and the line below

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:474: VLOG(1) <<
"AccountReconcilor::MergeSessionCompleted: account_id="
We should add a human readable representation of the error into this VLOG
statement, so we can determine if/why MergeSessions might be failing.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.h (right):

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.h:30: class SigninOAuthHelper;
I think we can remove this declaration?

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:31: // We want to include
zeroes in the counts of added accounts to easily
Nit: Change the comment to something like "added or removed accounts"

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:56: //                          
   in the cookie jar.
Nit: add a space to left-align the comments

[Roger Tawa OOO till Jul 10th, 2014-09-23 21:11:04]

Patchset #4 (id:80001) has been deleted

[Roger Tawa OOO till Jul 10th, 2014-09-23 21:25:59]

Thanks Mike.  Please take another look.

https://codereview.chromium.org/590113004/diff/60001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/590113004/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:386:
"Signin.Reconciler.AddedToChrome.FirstRun", 0, 1);
On 2014/09/23 14:27:47, Mike Lerman wrote:
> All of these references to the AddedToChrome metric will need to be changed.

Doh!  I meant to fix this before submitting and forgot.  Made me realized I did
not have a test for the remove from cookie case.

Good catch.  Done.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:19: #include
"components/signin/core/browser/signin_oauth_helper.h"
On 2014/09/23 14:27:47, Mike Lerman wrote:
> We no longer have any references to the signin_oauth_helper, I think.

Done.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:364:
chrome_accounts_.end(),
On 2014/09/23 14:27:47, Mike Lerman wrote:
> nit: 1 space less here and the line below

Done.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:474: VLOG(1) <<
"AccountReconcilor::MergeSessionCompleted: account_id="
On 2014/09/23 14:27:47, Mike Lerman wrote:
> We should add a human readable representation of the error into this VLOG
> statement, so we can determine if/why MergeSessions might be failing.

Done.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.h (right):

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.h:30: class SigninOAuthHelper;
On 2014/09/23 14:27:47, Mike Lerman wrote:
> I think we can remove this declaration?

Done.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.h:30: class SigninOAuthHelper;
On 2014/09/23 14:27:47, Mike Lerman wrote:
> I think we can remove this declaration?

Done.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:31: // We want to include
zeroes in the counts of added accounts to easily
On 2014/09/23 14:27:47, Mike Lerman wrote:
> Nit: Change the comment to something like "added or removed accounts"

Done.

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/590113004/diff/60001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:56: //                          
   in the cookie jar.
On 2014/09/23 14:27:47, Mike Lerman wrote:
> Nit: add a space to left-align the comments

Done.

[Mike Lerman, 2014-09-24 13:55:54]

lgtm

https://codereview.chromium.org/590113004/diff/100001/components/signin/core/...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/590113004/diff/100001/components/signin/core/...
components/signin/core/browser/account_reconcilor.cc:407: added_to_cookie++;
We're changing the meaning of AddedToCookie here, and not in a useful way. With
the old reconcilor, added_to_cookie would be incremented: Whenever there was a
token absent from the cookie jar, or whenever the entire cookie jar was being
blown away (which I hadn't realized). Now, it will be also be incremented
whenever a cookie isn't present as a token, as we will count all the other
cookies as well.

This should be more explicit in the description of the histogram. Can you
mention that this metric tracks the number of times MergeSession is called? This
will make it clear that we're not merely tracking the logical gap between the
pre-reconcile token/cookie states.

[Roger Tawa OOO till Jul 10th, 2014-09-24 14:48:27]

rogerta@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-09-24 14:48:28]

Hi Alexei,

Can you owner review histograms.xml please.  Thanks,

https://codereview.chromium.org/590113004/diff/100001/components/signin/core/...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/590113004/diff/100001/components/signin/core/...
components/signin/core/browser/account_reconcilor.cc:407: added_to_cookie++;
On 2014/09/24 13:55:54, Mike Lerman wrote:
> We're changing the meaning of AddedToCookie here, and not in a useful way.
With
> the old reconcilor, added_to_cookie would be incremented: Whenever there was a
> token absent from the cookie jar, or whenever the entire cookie jar was being
> blown away (which I hadn't realized). Now, it will be also be incremented
> whenever a cookie isn't present as a token, as we will count all the other
> cookies as well.
> 
> This should be more explicit in the description of the histogram. Can you
> mention that this metric tracks the number of times MergeSession is called?
This
> will make it clear that we're not merely tracking the logical gap between the
> pre-reconcile token/cookie states.

Let's not change the description and have the added-to-cookie reflect what we
really want.  I'll move just the counting part to above line 369 when I get to
my computer.

[Alexei Svitkine (slow), 2014-09-24 14:50:07]

asvitkine@chromium.org changed reviewers:
+ jwd@chromium.org

[Alexei Svitkine (slow), 2014-09-24 14:50:07]

Over to Jesse, who's now an owner of metrics files too.

[jwd, 2014-09-24 15:08:21]

lgtm

[Roger Tawa OOO till Jul 10th, 2014-09-24 23:26:11]

Thanks Jesse.

Mike: I fixed the added-to-cookie calculation.

[Mike Lerman, 2014-09-25 15:32:03]

On 2014/09/24 23:26:11, Roger Tawa wrote:
> Thanks Jesse.
> 
> Mike: I fixed the added-to-cookie calculation.

still lgtm

[Roger Tawa OOO till Jul 10th, 2014-09-25 15:58:27]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-09-25 15:59:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/590113004/160001

[commit-bot: I haz the power, 2014-09-25 16:04:37]

Committed patchset #7 (id:160001) as 5655fae4a4bc997302c2d1cc065830e623997b0c

[commit-bot: I haz the power, 2014-09-25 16:05:40]

Patchset 7 (id:??) landed as
https://crrev.com/6128c200e138d3d7c52aae5e01d0af36a48b1706
Cr-Commit-Position: refs/heads/master@{#296715}