ListAccounts will return the Gaia ID as well as the email of the account.

ListAccounts will return the Gaia ID as well as the email of the account.

Part two of this CL is: https://codereview.chromium.org/1166373006

BUG=492787
Committed: https://crrev.com/4f2a3fd855bc5c1843fb68ea9f75294a2ff85b53
Cr-Commit-Position: refs/heads/master@{#332877}

[Mike Lerman, 2015-06-01 18:16:33]

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

[Mike Lerman, 2015-06-01 18:16:34]

Hi Roger,

Please take a look. As discussed, changing the Reconcilor to make use of Gaia
IDs or Emails for comparison will come in another CL.

Thanks,
Mike

[Roger Tawa OOO till Jul 10th, 2015-06-02 21:58:42]

Thanks Mike, CL a little bigger than I thought :-)

I there are places where account_id should be used instead of email, to make the
code work after the switch to gaiaid.  Note that after the switch, code should
not be checking email to determine if two accounts are the same, only
account_id.

https://codereview.chromium.org/1162103003/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/1162103003/diff/120001/chrome/browser/signin/...
chrome/browser/signin/account_reconcilor_unittest.cc:336: ASSERT_EQ(account_id,
accounts[0].email);
Shouldn't this be: accounts[0].account_id ?

https://codereview.chromium.org/1162103003/diff/120001/components/signin/core...
File components/signin/core/browser/about_signin_internals.cc (right):

https://codereview.chromium.org/1162103003/diff/120001/components/signin/core...
components/signin/core/browser/about_signin_internals.cc:359:
gaia_accounts[i].email,
Could we show the raw email instead?

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

https://codereview.chromium.org/1162103003/diff/120001/components/signin/core...
components/signin/core/browser/account_reconcilor.cc:35: return p1.valid ==
p2.valid && gaia::AreEmailsSame(p1.email, p2.email);
Instead of:

   gaia::AreEmailsSame(p1.email, p2.email)

shouldn't this be:

   p1.account_id == p2.account_id

Same for line 47 below.  Doing it this way means the code will work whether we
use gaiaid or email as the account id, and that after we switch to gaiaids, we
will not look at the email at all.

https://codereview.chromium.org/1162103003/diff/120001/components/signin/core...
components/signin/core/browser/account_reconcilor.cc:56: }
You probably want a function called AccountFromAccountId() which does pretty
much the same thing except with the account_id.

About renaming, probably want to rename the two classes above:

EmailEqualToFunc --> AccountEqualToFunc
AreEmailsSameFunc --> AreAccountsSameFunc

https://codereview.chromium.org/1162103003/diff/120001/components/signin/core...
components/signin/core/browser/account_reconcilor.cc:343:
gaia::AreEmailsSame(primary_account_, gaia_accounts_[0].email);
This should be:

   primary_account_id_ == gaia_accounts_[0].account_id

https://codereview.chromium.org/1162103003/diff/120001/components/signin/core...
File components/signin/core/browser/gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/1162103003/diff/120001/components/signin/core...
components/signin/core/browser/gaia_cookie_manager_service.cc:564: account.id =
AccountTrackerService::PickAccountIdForAccount(
Before assigning, maybe DCHECK(account.id.empty()) ?

https://codereview.chromium.org/1162103003/diff/120001/google_apis/gaia/gaia_...
File google_apis/gaia/gaia_auth_util.cc (right):

https://codereview.chromium.org/1162103003/diff/120001/google_apis/gaia/gaia_...
google_apis/gaia/gaia_auth_util.cc:55: raw_email == other.raw_email;
Do we need to check all fields?  Should be enough to check only account_id.

[Roger Tawa OOO till Jul 10th, 2015-06-03 19:52:23]

lgtm

OK to address my comments in a follow up CL.  But could you show the raw email
in signin internals as part of this CL?  Thanks.

[Mike Lerman, 2015-06-04 14:08:12]

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

[Mike Lerman, 2015-06-04 14:08:13]

Thanks, Roger.

Hi Xiyuan,

Could I get a review, please, for the chromeos and ui/webui code please? Thanks!

https://codereview.chromium.org/1162103003/diff/120001/components/signin/core...
File components/signin/core/browser/about_signin_internals.cc (right):

https://codereview.chromium.org/1162103003/diff/120001/components/signin/core...
components/signin/core/browser/about_signin_internals.cc:359:
gaia_accounts[i].email,
On 2015/06/02 21:58:41, Roger Tawa wrote:
> Could we show the raw email instead?

Done.

[xiyuan, 2015-06-04 15:59:24]

chromeos/* and webui/* LGTM

https://codereview.chromium.org/1162103003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/signin/oauth2_login_manager.cc (right):

https://codereview.chromium.org/1162103003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:320: if
(gaia::CanonicalizeEmail(iter->email) == user_email) {
nit: |email| is canonicalized so we probably can remove CanonicalizeEmail call
here.

[Mike Lerman, 2015-06-04 16:52:41]

Thanks Xiyuan.

https://codereview.chromium.org/1162103003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/signin/oauth2_login_manager.cc (right):

https://codereview.chromium.org/1162103003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:320: if
(gaia::CanonicalizeEmail(iter->email) == user_email) {
On 2015/06/04 15:59:24, xiyuan wrote:
> nit: |email| is canonicalized so we probably can remove CanonicalizeEmail call
> here.

Done.

[Mike Lerman, 2015-06-04 18:02:05]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2015-06-04 18:02:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1162103003/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-04 18:03:01]

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

[commit-bot: I haz the power, 2015-06-04 19:28:57]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-04 19:30:35]

Patchset 10 (id:??) landed as
https://crrev.com/4f2a3fd855bc5c1843fb68ea9f75294a2ff85b53
Cr-Commit-Position: refs/heads/master@{#332877}